7195fa792f test: Tool wallet test coverage for unexpected writes to wallet (Jon Atack)
3bf2b3a37b test: Split tool_wallet.py test into subtests (Jon Atack)
1eb13f09a9 test: Add log messages to test/functional/tool_wallet.py (Jon Atack)
Pull request description:
This pull request adds test coverage in `test/functional/tool_wallet.py` to reproduce unexpected writes to the wallet as described in https://github.com/bitcoin/bitcoin/issues/15608 and serve as a benchmark for fixing the issue:
- Wallet tool `info` unexpectedly writes to the wallet file if the wallet file permissions are read/write.
- Wallet tool `info` raises with "Error loading . Is wallet being used by another process?" if the wallet file permissions are read-only.
Goals:
1. Reproduce the reported issue, define the current unexpected behavior, and add test coverage to guide a future fix. Add debug-level logging for sanity checking and commented-out assertions to be uncommented when fixing the issue. Add the same coverage to the wallet tool create test and the getwalletinfo test as regression tests while fixing the issue.
2. Add info log messages as there are currently none in the test file.
3. Split the tests out to separate functions as per review feedback.
Thanks to Marco Falke for pointing me in the right direction.
ACKs for top commit:
laanwj:
code review ACK 7195fa792f
Tree-SHA512: 16a41cce989c8f819cf5b02c6cf8ea84653ede2738fb402f6c36cf4dc075b424dff3e2c73a1cfa1ec9c75f614675baecc71e588845a2596db06ba0957db2df7b
This commit adds test coverage in `test/functional/tool_wallet.py` to reproduce unexpected writes to the wallet as described in https://github.com/bitcoin/bitcoin/issues/15608:
- wallet tool `info` unexpectedly writes to the wallet file if the wallet file permissions are read/write.
- wallet tool `info` raises with "Error loading . Is wallet being used by another process?" if the wallet file permissions are read-only.
1. Reproduce the reported issue, define the current unexpected behavior, and add test coverage to guide a future fix in the form of commented-out assertions to be uncommented when testing/fixing.
2. Provisionally extend the same coverage to the wallet tool create test and the getwalletinfo test as regression tests while fixing the issue.
3. Add some logging for sanity checking.
------
Changes after rebase:
5. Make wallet_path an instance method instead of a function in tool_wallet.py as per Marco Falke review suggestion.
6. Assert wallet permissions instead of logging them in tool_wallet.py. This ran into an issue with Appveyor keeping permissions at 666 so allowed for 666 as a workaround.
7. Change the added logging from info to debug level.
8. More helpful assertions order in tool_wallet.py#assert_tool_output. This change makes #assert_tool_output raise "Error loading wallet.dat. Is wallet being used by another process?" rather than a less-helpful message when debugging the read-only wallet permissions issue.
91cc18f602 [docs] Add release notes for PR 15427 (John Newbery)
3b11420b3c [RPC] add new utxoupdatepsbt arguments to the CRPCCommand and CPRCConvertParam tables (John Newbery)
Pull request description:
The new `descriptors` argument was not added to the CRPCCommand and CPRCCvertParam tables, meaning that it couldn't be used with bitcoin-cli or named arguments.
Before this PR:
```
> bitcoin-cli utxoupdatepsbt 'cHNidP8BAFMCAAAAAYCdwVRx2X3o4KHx5tAMsN1ddp51MbfWsietjfMbl5HtAAAAAAD/////AQDh9QUAAAAAF6kUW+rtEOi4nk9rpw2F5XZl1dd8ehGHAAAAAAAAAA==' "[{\"desc\":\"sh(wpkh([bd50871a/0h/0h/0h]03895c66337b38699bfafff1084ad35bc347fac4f4e5e5fe5eb7dd81155280db53))\"}]"
error code: -3
error message:
Expected type array, got string
> bitcoin-cli --named utxoupdatepsbt psbt='cHNidP8BAFMCAAAAAYCdwVRx2X3o4KHx5tAMsN1ddp51MbfWsietjfMbl5HtAAAAAAD/////AQDh9QUAAAAAF6kUW+rtEOi4nk9rpw2F5XZl1dd8ehGHAAAAAAAAAA==' descriptors="[{\"desc\":\"sh(wpkh([bd50871a/0h/0h/0h]03895c66337b38699bfafff1084ad35bc347fac4f4e5e5fe5eb7dd81155280db53))\"}]"
error code: -8
error message:
Unknown named parameter descriptors
```
After this PR:
```
bitcoin-cli utxoupdatepsbt 'cHNidP8BAFMCAAAAAYCdwVRx2X3o4KHx5tAMsN1ddp51MbfWsietjfMbl5HtAAAAAAD/////AQDh9QUAAAAAF6kUW+rtEOi4nk9rpw2F5XZl1dd8ehGHAAAAAAAAAA==' "[{\"desc\":\"sh(wpkh([bd50871a/0h/0h/0h]03895c66337b38699bfafff1084ad35bc347fac4f4e5e5fe5eb7dd81155280db53))\"}]"
cHNidP8BAFMCAAAAAYCdwVRx2X3o4KHx5tAMsN1ddp51MbfWsietjfMbl5HtAAAAAAD/////AQDh9QUAAAAAF6kUW+rtEOi4nk9rpw2F5XZl1dd8ehGHAAAAAAAAAA==
bitcoin-cli --named utxoupdatepsbt psbt='cHNidP8BAFMCAAAAAYCdwVRx2X3o4KHx5tAMsN1ddp51MbfWsietjfMbl5HtAAAAAAD/////AQDh9QUAAAAAF6kUW+rtEOi4nk9rpw2F5XZl1dd8ehGHAAAAAAAAAA==' descriptors="[{\"desc\":\"sh(wpkh([bd50871a/0h/0h/0h]03895c66337b38699bfafff1084ad35bc347fac4f4e5e5fe5eb7dd81155280db53))\"}]"
cHNidP8BAFMCAAAAAYCdwVRx2X3o4KHx5tAMsN1ddp51MbfWsietjfMbl5HtAAAAAAD/////AQDh9QUAAAAAF6kUW+rtEOi4nk9rpw2F5XZl1dd8ehGHAAAAAAAAAA==
```
ACKs for top commit:
promag:
ACK 91cc18f.
fanquake:
re-ACK 91cc18f602
Tree-SHA512: 279b2339a5cac17e363002e4ab743e251d6757c904c89f1970575bdce18d4f63d5e13507e171bf2bdc1bf6dd457db345a4b11b15d4ff71b96c2fedc4ffe52b23
1ac454a384 Enable ShellCheck rules (Hennadii Stepanov)
Pull request description:
Enable some simple ShellCheck rules.
Note for reviewers: `bash` and `shellcheck` on macOS are different from ones on Ubuntu.
For local tests the latest `shellcheck` version 0.6.0 should be used (see #15166).
ACKs for top commit:
practicalswift:
utACK 1ac454a384
dongcarl:
utACK 1ac454a
fanquake:
ACK 1ac454a384
Tree-SHA512: 8d0a3a5c09fe1a0c22120178f5e6b80f81f746f8c3356b7701ff301c117acb2edea8fe08f08fb54ed73f94b1617515fb239fa28e7ab4121f74872e6494b6f20e
The new `descriptors` argument needs to be added to the Command and
ConvertParams tables to by usable as a named argument and by
bitcoin-cli.
Also update the test to use named arguments to test this.
fab0c820fa rpc: Clarify that block count means height excl genesis (MarcoFalke)
Pull request description:
There is a common misconception that the block count returned by the blockchain rpcs includes the genesis block. See for example the discussion in https://github.com/bitcoin/bitcoin/pull/16292#issuecomment-506303256.
However, it really returns the height, which is `0` for the genesis block.
So clarify that and also remove the misleading "longest blockchain" comment.
Finally, fix the wallet test that incorrectly used this rpc.
ACKs for top commit:
instagibbs:
utACK fab0c820fa
promag:
ACK fab0c82, sorry for the misconception.
Tree-SHA512: 0d087cbb628d3866352bca6420402f392e6a997e579941701a408a7fca355d84645045661f39b022e4479cc07f85a6cddaa9095b6fd9911b245692482420a5e4
8a6810d0d2 Add a 'logpath' field to getrpcinfo (darosior)
Pull request description:
as discussed in #15438
ACKs for top commit:
laanwj:
Tested ACK 8a6810d0d2
Tree-SHA512: 752c7d90f670677c8144efb338c5c97c2264f85f1e65e031fd5a44f04230b6eafbabd0f634db263eb42c25642ecc1c4b1b602d4735e3fab07ec00b566134ddab
01174596e6 signrawtransactionwithkey: report error when missing redeemScript/witnessScript param (Anthony Towns)
Pull request description:
Adding support for "witnessScript" as an alternative to "redeemScript" when using "signrawtransactionwithkey" meant that the `RPCTypeCheckObj()` call in `SignTransaction` can't error out just because either parameter is missing -- it's only a problem if both are missing, which isn't a state `RPCTypeCheckObj()` tests for. This results in the regression described in #16249. This patch adds some code to test for this case and give a similar error, namely:
error code: -8
error message:
Missing redeemScript/witnessScript
Fixes: #16249
ACKs for top commit:
meshcollider:
utACK 01174596e6
promag:
ACK 01174596e. Could also write test without `dict`/`del`:
Tree-SHA512: cf51346b7dea551b7f18f2a93c2a336a293b2535c62c03a5263cd2be8c58cf0cc302891da659c167e88ad1a68a756472c3c07e99f71627c61d32886fc5a3a353
26fe9b9909 Add support for descriptors to utxoupdatepsbt (Pieter Wuille)
3135c1a2d2 Abstract out UpdatePSBTOutput from FillPSBT (Pieter Wuille)
fb90ec3c33 Abstract out EvalDescriptorStringOrObject from scantxoutset (Pieter Wuille)
eaf4f88734 Abstract out IsSegWitOutput from utxoupdatepsbt (Pieter Wuille)
Pull request description:
This adds a descriptors argument to the `utxoupdatepsbt` RPC. This means:
* Input and output scripts and keys will be filled in when known.
* P2SH-witness inputs will be filled in from the UTXO set when a descriptor is provided that shows they're spending segwit outputs.
This also moves some (newly) shared code to separate functions: `UpdatePSBTOutput` (an analogue to `SignPSBTInput`), `IsSegWitOutput`, and `EvalDescriptorStringOrObject` (implementing the string or object notation parsing used in `scantxoutset`).
ACKs for top commit:
jnewbery:
utACK 26fe9b9909
laanwj:
utACK 26fe9b9909 (will hold merging until response to promag's comments)
promag:
ACK 26fe9b9, checked refactors and tests look comprehensive. Still missing a release note but can be added later.
Tree-SHA512: 1d833b7351b59d6c5ded6da399ff371a8a2a6ad04c0a8f90e6e46105dc737fa6f2740b1e5340280d59e01f42896c40b720c042f44417e38dfbee6477b894b245
276972cb95 wallet_bumpfee.py: Make sure coin selection produces change (Gregory Sanders)
Pull request description:
I was hitting the case where change-less transactions were being made.
ACKs for top commit:
ryanofsky:
utACK 276972cb95
Tree-SHA512: e2b7a50363daddd3ee749cacfc9d3d685a6c0c7e3e48118bb60131d205bf83ea06cdd66b69dfa3bd4dbb3bbf2b5b673d7225171486ae72fc762e5dabe2c01ef5
806b0052c3 [wallet] abort when attempting to fund a transaction above maxtxfee (Sjors Provoost)
Pull request description:
`FundTransaction` calls `GetMinimumFee` which, when the fee rate is absurdly high, quietly reduces the fee to `-maxtxfee`.
Becaue an absurdly high fee rate is usually the result of a fat finger, aborting seems safer behavior.
Before:
```
bitcoin-cli walletcreatefundedpsbt '[]' '[{"tb1q...": 0.01}]' 0 '{"feeRate": 10}' true
{
"psbt": "cHNidP8...gAA=",
"fee": 0.10000000,
"changepos": 1
}
```
After:
```
bitcoin-cli walletcreatefundedpsbt '[]' '[{"tb1q...": 0.01}]' 0 '{"feeRate": 10}' true
error code: -25
error message:
Fee exceeds maximum configured by -maxtxfee
```
QT still checks the max fee rate as expected:
<img width="566" alt="Schermafbeelding 2019-06-20 om 19 52 00" src="https://user-images.githubusercontent.com/10217/59888424-a2aa7100-9395-11e9-8ae6-8a3c1f7de585.png">
ACKs for top commit:
laanwj:
Code review ACK 806b0052c3
Tree-SHA512: bee95811711cdab100b614d2347921407af3b400aea613ca156953ed3f60b924ad29a1d335bd0e240c0b7c0fbb360226bab03294d226a5560cdf2a3f21e6d406
FundTransaction calls GetMinimumFee which, when the fee rate is absurdly high, quietly reduced the fee to -maxtxfee. Becaue an absurdly high fee rate is usually the result of a fat finger, aborting seems safer behavior.
71d0344cf2 docs: release note wording (Karl-Johan Alm)
3d2ff37913 wallet/rpc: use static help text (Karl-Johan Alm)
53c3c1ea9e wallet/rpc/getbalances: add entry for 'mine.used' balance in results (Karl-Johan Alm)
Pull request description:
This addresses a few remaining issues pointed out in #13756:
* First commit addresses https://github.com/bitcoin/bitcoin/pull/13756#discussion_r284907468
* Second commit addresses https://github.com/bitcoin/bitcoin/pull/13756#discussion_r294868973
Ping jnewbery and achow101 as they pointed out these issues.
ACKs for commit 71d034:
jnewbery:
ACK 71d0344cf2
meshcollider:
re-utACK 71d0344cf2
Tree-SHA512: 5e28822af0574ad07dbbed21aa2fe7866bf5770b4c0a1c150ad0da8af3152bcfb7170330a7497fa500326c594740ecf63733cf58325821e2811d7b911d5783a0
e61de6306f Change ismine to take a CWallet instead of CKeyStore (Andrew Chow)
7c611e2000 Move ismine to wallet module (Andrew Chow)
Pull request description:
`IsMine` isn't used outside of the wallet except for the tests. It also doesn't make sense to be outside of the wallet. This PR moves `IsMine` into the wallet module and for it to take a `CWallet` instead of `CKeyStore`. The test that used `IsMine` is also moved to the wallet tests.
This is first [prerequisites](https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Wallet-Class-Structure-Changes#ismine) for the wallet structure changes.
ACKs for commit e61de6:
MarcoFalke:
re-ACK e61de6306f (only change is rebase with git auto-merge)
meshcollider:
Very light code review ACK e61de6306f
Tree-SHA512: 1cb4ad12652aef7922ab7460c6d413e8b9d1855dca78c0a286ae49d5c0765bc7996c55f262c742001d434eb9bd4215dc2cc7aae1b371ee1a82d46b32c17e6341
a49503402b Make and get the multisig redeemscript and destination in one function instead of two (Andrew Chow)
Pull request description:
`CreateMultisigRedeemscript()` is changed to `AddAndGetMultisigDestination()` so that the process of constructing the redeemScript and then getting the `CTxDestination` are done in the same function. This allows that function to see what the keys in the multisig are so that the correct address type is returned from `AddAndGetDestinationForScript()`.
This only effects the `createmultisig` and `addmultisigaddress` RPCs and does not change signing logic as #16022 does.
Alternative to #16022 and #16012Fixes#16011
ACKs for commit a49503:
Tree-SHA512: 5b0154a714deea3b2cc3a54beb420c95eeeacf4ca30c40ca80940d9d640f8b03611b0fc14c2f0710bfd8a79e8d27ad7d9ae380b4b83d52b40ab201624f2a63f0
fa7dd88b71 test: Add test for unknown args (MarcoFalke)
Pull request description:
Currently uncovered.
Further reading:
* https://marcofalke.github.io/btc_cov/total.coverage/src/util/system.cpp.gcov.html
* Fail on unknown config file options #15021
ACKs for commit fa7dd8:
promag:
ACK fa7dd88b71, tests looks good to me.
hebasto:
ACK fa7dd88b71, I have tested the code.
Tree-SHA512: 86ab370ce8e85925f945a52e81457b5678d71bbabcef01205a97782b780003f363552e0bad1ff678bccc784f82c6b511c3b88de3f8f25f62b0b713c387950564
Instead of creating a redeemScript with CreateMultisigRedeemscript and
then getting the destination with AddAndGetDestinationForScript, do
both in the same function.
CreateMultisigRedeemscript is changed to AddAndGetMultisigDestination.
It creates the redeemScript and returns it via an output parameter. Then
it calls AddAndGetDestinationForScript to add the destination to the
keystore and get the proper destination.
This allows us to inspect the public keys in the redeemScript before creating
the destination so that the correct destination is used when uncompressed
pubkeys are in the multisig.
e91f0a7af2 doc: Remove travis badge from readme (MarcoFalke)
Pull request description:
The readme(s) are shipped in the released source-code archive, in which case the travis badge is useless since it doesn't link to the travis result of the correct commit/tag/branch. GitHub embeds the correct links for each tag or commit that ci ran on, so we don't need this link in the readme.
ACKs for commit e91f0a:
hebasto:
ACK e91f0a7af2
Tree-SHA512: 860435a58b38a9bd0bc62a1e74b3a63c138c9a2f09008a090d5ecc7fd86fa908d2e5eda41d16606507a238d9488fa5323405364a9556b670684a2e4838aead2d
We should eventually request a transaction from all peers that announce
it (assuming we never receive it).
We should prefer requesting from outbound peers over inbound peers.
Enforce the max tx requests in flight, and the eventual expiry of those
requests.
Test author: Suhas Daftuar <sdaftuar@gmail.com>
Adjusted by: MarcoFalke
5ebc6b0eb2 bitcoind: update -avoidpartialspends description to account for auto-enable for avoid_reuse wallets (Karl-Johan Alm)
ada258f8c8 doc: release notes for avoid_reuse (Karl-Johan Alm)
27669551da wallet: enable avoid_partial_spends by default if avoid_reuse is set (Karl-Johan Alm)
8f2e208f7c test: add test for avoidreuse feature (Karl-Johan Alm)
0bdfbd34cf wallet/rpc: add 'avoid_reuse' option to RPC commands (Karl-Johan Alm)
f904723e0d wallet/rpc: add setwalletflag RPC and MUTABLE_WALLET_FLAGS (Karl-Johan Alm)
8247a0da3a wallet: enable avoid_reuse feature (Karl-Johan Alm)
eec15662fa wallet: avoid reuse flags (Karl-Johan Alm)
58928098c2 wallet: make IsWalletFlagSet() const (Karl-Johan Alm)
129a5bafd9 wallet: rename g_known_wallet_flags constant to KNOWN_WALLET_FLAGS (Karl-Johan Alm)
Pull request description:
Add a new wallet flag called `avoid_reuse` which, when enabled, will keep track of when a specific destination has been spent from, and will actively "blacklist" any new UTXOs which send to an already-spent-from destination.
This improves privacy, as a payer could otherwise begin tracking a payee's wallet by regularly peppering a known UTXO with dust outputs, which would then be scooped up and used in payments by the payee, allowing the payer to map out (1) the inputs owned by the payee and (2) the destinations to which the payee is making payments.
This replaces #10386 and together with the (now merged) #12257 it addresses #10065 in full. The concerns raised in https://github.com/bitcoin/bitcoin/pull/10386#issuecomment-302361381 are also addressed due to #12257.
~~Note: this builds on top of #15780.~~ (merged)
ACKs for commit 5ebc6b:
jnewbery:
ACK 5ebc6b0eb
laanwj:
Concept and code-review ACK 5ebc6b0eb2
meshcollider:
Code review ACK 5ebc6b0eb2
achow101:
ACK 5ebc6b0eb2 modulo above nits
Tree-SHA512: fdef45826af544cbbb45634ac367852cc467ec87081d86d08b53ca849e588617e9a0a255b7e7bb28692d15332de58d6c3d274ac003355220e4213d7d9070742e
faa2a47cd7 logging: Add threadsafety comments (MarcoFalke)
0b282f9b00 Log early messages with -printtoconsole (Anthony Towns)
412987430c Replace OpenDebugLog() with StartLogging() (Anthony Towns)
Pull request description:
Early log messages are dropped on the floor and they'd never make it to the console or debug log. This can be tested by running the test included in this pull request without re-compiling the `bitcoind`.
Fix that by buffering early messages and flushing them as soon as all logging options have been initialized and logging has been started.
This pull request is identical to "Log early messages with -printtoconsole" (#13088) by **ajtowns**, with the following changes:
* Rebased
* Added docstrings for `m_buffering` and `StartLogging`
* Switch `CCriticalSection` (aka `RecursiveMutex`) to just `Mutex` in the last commit
* Added tests
Fixes#16098Fixes#13157Closes#13088
ACKs for commit faa2a4:
ajtowns:
utACK faa2a47cd7
hebasto:
ACK faa2a47cd7
kristapsk:
ACK faa2a47cd7 (ran added functional test before / after recompiling, didn't do additional testing)
Tree-SHA512: 685e2882642fe2a43ce171d42862582dadb840d03cda8236a994322c389ca2a1f3f431b179b2726c155c61793543bb340c568a5455d97f8b83bc7d307a85d387
8053e5cdad Remove -mempoolreplacement to prevent needless block prop slowness. (Matt Corallo)
Pull request description:
At this point there is no reasonable excuse to disable opt-in RBF,
and, unlike when this option was added, there are now significant
issues created when disabling it (in the form of compact block
reconstruction failures). Further, it breaks a lot of modern wallet
behavior.
This removes an option that is:
* (a) only useful when a large portion of (other) miners enforce it as well
* (b) is detrimental to everyone (income for miners, RBF notifications for others) who uses it individually otherwise
* (c) is effectively unused
* (d) is often confused with disabling RBF (rather than just remaining stubbornly unaware of it while the rest of the network lets it through)
ACKs for commit 8053e5:
practicalswift:
utACK 8053e5cdad
promag:
Deprecation would save from unlikely rantings, still ACK 8053e5c.
jtimon:
utACK 8053e5cdad
ajtowns:
ACK 8053e5cdad -- quick code review, checked tests work
MarcoFalke:
ACK 8053e5cdad
Tree-SHA512: 01aee8905b2487fc38a3a86649d422d2d2345bc60f878889ebda4b8680783e1f1a97c2000c27ef086719501be2abc2911b2039a259a5e5c04f3b24ff02b0427e
fa499b5f02 rpc: bugfix: Properly use iswitness in converttopsbt (MarcoFalke)
fa5c5cd141 rpc: Switch touched RPCs to IsValidNumArgs (MarcoFalke)
Pull request description:
When a serialized transaction has inputs, there is no risk in only trying to deserialize it with witness allowed. (This is how all transactions from p2p are deserialized.) In fact, it would avoid a common issue where a transaction with inputs can be deserialized in two ways:
* Fixes#12989
* Fixes#15872
* Fixes#15701
* Fixes#13738
* ...
When a serialized transaction has no inputs, there is no risk in only trying to deserialze it with witness disallowed. (A transaction without inputs can't have corresponding witness data)
ACKs for commit fa499b:
meshcollider:
utACK fa499b5f02
ryanofsky:
utACK fa499b5f02. Changes since last review: consolidating commits and making iswitness documentation the same across methods.
PastaPastaPasta:
utACK fa499b5f02
Tree-SHA512: a64423a3131f3f0222a40da557c8b590c9ff01b45bcd40796f77a1a64ae74c6680a6be9d01ece95c492dfbcc7e2810409d2c2b336c2894af00bb213972fc85c6
fa1d766717 tests: Make msg_block a witness block (MarcoFalke)
fa52eb55c9 test: Remove True argument to CBlock::serialize (MarcoFalke)
Pull request description:
Unnamed arguments are confusing as to what they mean without looking up the function signature.
Since segwit is active by default in regtest, and all blocks are serialized with witness (#15664), remove the argument `with_witness=True` from all calls to `CBlock::serialize` and `BlockTransactions::serialize`.
ACKs for commit fa1d76:
laanwj:
code-review ACK fa1d766717
Tree-SHA512: 2c550646f99c9ca86a223ca988c61a730f5e6646807adeaa7174fb2424a32cea3fef8bcd3e0b12e162e7ff192877d0c02fd0654df6ee1a9b821b065707c2dcbc
fa8f195195 Replace remaining fprintf with tfm::format manually (MarcoFalke)
fac03ec43a scripted-diff: Replace fprintf with tfm::format (MarcoFalke)
fa72a64b90 tinyformat: Add doc to Bitcoin Core specific strprintf (MarcoFalke)
Pull request description:
This should be a refactor except in the cases where we use the wrong format specifier [1], in which case this patch is a bug fix.
[1] : e.g. depends: Add libevent compatibility patch for windows #8730
ACKs for commit fa8f19:
promag:
ACK fa8f195195. Ideally this should be rebased before merge.
practicalswift:
utACK fa8f195195
Empact:
ACK fa8f195195
laanwj:
code review and lightly tested ACK fa8f195195
jonatack:
ACK fa8f195195 from light code review, building, and running linter/unit tests/extended functional tests.
Tree-SHA512: 65f648b0bc383e3266a5bdb4ad8c8a1908a719635d49e1cd321b91254be24dbc7e22290370178e29b98ddcb3fec0889de9cbae273c7140abc9793d849534a743
b748bf6f50 Fix spelling errors identified by codespell 1.15.0 (Ben Woosley)
Pull request description:
Note all changes are to comments / documentation.
After this commit, the only remaining output is:
```
$ test/lint/lint-spelling.sh
src/test/base32_tests.cpp:14: fo ==> of, for
src/test/base64_tests.cpp:14: fo ==> of, for
^ Warning: codespell identified likely spelling errors. Any false positives? Add them to the list of ignored words in test/lint/lint-spelling.ignore-words.txt
```
Note:
* I ignore several valid alternative spellings ~, but changed homogenous
to homogeneous as the latter is a more specific term according to the
Google dictionary definitions I found~
* homogenous is present in tinyformat, hence should be addressed upstream
* process' is correct only if there are plural processes
ACKs for commit b748bf:
practicalswift:
ACK b748bf6f50
fanquake:
ACK b748bf6f50
Tree-SHA512: 9add7044643ce015e0a44d8b27a3f300d72c485ffff550fb6491a17f14528085289ec5caddfe02f291ea9b2cded38a0dd3079652a054e2d7fe2ff4f7b53db5d7
After this commit, the only remaining output is:
$ test/lint/lint-spelling.sh
src/test/base32_tests.cpp:14: fo ==> of, for
src/test/base64_tests.cpp:14: fo ==> of, for
^ Warning: codespell identified likely spelling errors. Any false positives? Add them to the list of ignored words in test/lint/lint-spelling.ignore-words.txt
Note:
* I ignore several valid alternative spellings
* homogenous is present in tinyformat, hence should be addressed upstream
* process' is correct only if there are plural processes
At this point there is no reasonable excuse to disable opt-in RBF,
and, unlike when this option was added, there are now significant
issues created when disabling it (in the form of compact block
reconstruction failures). Further, it breaks a lot of modern wallet
behavior.
53b7de629d Add test for dumping the private key imported from descriptor (MeshCollider)
2857bc4a64 Extend importmulti descriptor tests (MeshCollider)
81a884bbd0 Import private keys from descriptor with importmulti if provided (MeshCollider)
a4d1bd1a29 Add private key derivation functions to descriptors (MeshCollider)
Pull request description:
~This is based on #14491, review the last 3 commits only.~
Currently, descriptors have an Expand() function which returns public keys and scripts for a specific index of a ranged descriptor. But the private key for a specific index is not given. This allows private keys for specific indices to be derived. This also allows those keys to be imported through the `importmulti` RPC rather than having to provide them separately.
ACKs for commit 53b7de:
achow101:
ACK 53b7de629d
Tree-SHA512: c060bc01358a1adc76d3d470fefc2bdd39c837027f452e9bc4bd2e726097e1ece4af9d5627efd942a5f8819271e15ba54f010b169b50a9435a1f0f40fd1cebf3
67f4e9c522 Include core_io.h from core_read.cpp (practicalswift)
eca9767673 Make reasoning about dependencies easier by not including unused dependencies (practicalswift)
Pull request description:
Make reasoning about dependencies easier by not including unused dependencies.
Please note that the removed headers are _not_ "transitively included" by other still included headers. Thus the removals are real.
As an added bonus this change means less work for the preprocessor/compiler. At least 51 393 lines of code no longer needs to be processed:
```
$ git diff -u HEAD~1 | grep -E '^\-#include ' | cut -f2 -d"<" | cut -f1 -d">" | \
sed 's%^%src/%g' | xargs cat | wc -l
51393
```
Note that 51 393 is the lower bound: the real number is likely much higher when taking into account transitively included headers :-)
ACKs for commit 67f4e9:
Tree-SHA512: 0c8868aac59813f099ce53d5307eed7962dd6f2ff3546768ef9e5c4508b87f8210f1a22c7e826c3c06bebbf28bdbfcf1628ed354c2d0fdb9a31a42cefb8fdf13
BIP 37 bloom filters have been well-known to be a significant DoS
target for some time. However, in order to provide continuity for
SPV clients relying on it, the NODE_BLOOM service flag was added,
and left as a default, to ensure sufficient nodes exist with such a
flag.
NODE_BLOOM is, at this point, well-established and, as long as
there exist 0.18 nodes with default config (which I'd anticipate
will be true for many years), will be available from some peers. By
that time, the continued slowdown of BIP 37-based filtering will
likely have rendered it useless (though this is already largely the
case). Further, BIP 37 was deliberately never updated to support
witness-based filtering as newer wallets are expected to migrate to
some yet-to-be-network-exposed filters.
fa47330397 test: Speed up cache creation (MarcoFalke)
fa6ad7a5ec test: Bump MAX_NODES to 12 (MarcoFalke)
Pull request description:
When testing a combination of settings that affect the datadir (e.g. prune, blockfilter, ...) we may need a lot of datadirs.
Bump the maximum number of nodes proactively from 8 to 12, so that caches get populated with 12 node dirs, as opposed to 8.
Also, add an assert that the list of deterministic keys is exactly the number of max nodes (and not more than that.
Also, create the cache faster.
ACKs for commit fa4733:
laanwj:
utACK fa47330397
Tree-SHA512: 9803c765ed52d344102f5a3bce57b05d88a7429dcb05ed66ed6c881fda8d87c2834d02d21b95fe9f39c0efe3b8527e13cf94f006588cde22e8c2cd50b2d517a6
0784af16ef remove parameters -addresstype=legacy in rpc_rawtransaction test (LongShao007)
a65dafa8f1 replace tx hash with txid in test rawtransaction (LongShao007)
Pull request description:
The transaction hash is different from txid for witness transactions, so we should use txid instead of hash.
ACKs for commit 0784af:
Tree-SHA512: 98b699eb5f25c3a603b11eb7072efe9bc69c0c0ecc7f996405de31bc45d92105970e09fd8e4f75b42a46498817f596d36d9b28eae7d24e63a4f2f2abfcee0eab
fa2b52af32 Disallow extended encoding for non-witness transactions (take 3) (MarcoFalke)
Pull request description:
(previous title "p2p: Disallow extended encoding for non-witness transactions (take 3)")
Remote peers can send us illegally encoded txs and thus have us write to stderr. Fix that by not writing to stderr.
This is a follow up to the previous (incomplete) attempts at this:
* Disallow extended encoding for non-witness transactions #14039
* Add test for superfluous witness record in deserialization #15893
ACKs for commit fa2b52:
laanwj:
utACK fa2b52af32
ryanofsky:
utACK fa2b52af32. Would change title to something like "Avoid logging transaction decode errors to stderr" instead of "Disallow extended encoding for non-witness transactions." The current title is confusing because this PR isn't really allowing or disallowing anything, just logging the condition differently. "Disallow" also seems to contradict the "Allow exceptions from..." comments in the actual code.
Tree-SHA512: c66990e69b432d00dc1c5510bf976a1188664d0890a32d1e5c6459094e7e27da82a5d227627afcbc203676f5540eec74b7d9b1d71d2c62d3b2069e1781824b4d
fa8ced32a6 doc: Mention blocksonly in reduce-traffic.md, unhide option (MarcoFalke)
fa320de79f test: Add test for p2p_blocksonly (MarcoFalke)
fa3872e7b4 test: Format predicate source as multiline on error (MarcoFalke)
fa1dce7329 net: Rename ::fRelayTxes to ::g_relay_txes (MarcoFalke)
Pull request description:
This is de-facto no longer hidden
ACKs for commit fa8ced:
jamesob:
utACK fa8ced32a6
Tree-SHA512: 474fbdee6cbd035ed9068a066b6056c1f909ec7520be0417820fcd1672ab3069b53f55c5147968978d9258fd3a3933fe1a9ef8e4f6e14fb6ebbd79701a0a1245
662d1171d9 Add option to create an encrypted wallet (Andrew Chow)
Pull request description:
This PR adds a new `passphrase` argument to `createwallet` which will create a wallet that is encrypted with that passphrase.
This is built on #15226 because it needs to first create an empty wallet, then encrypt the empty wallet and generate new keys that have only been stored in an encrypted state.
ACKs for commit 662d11:
laanwj:
utACK 662d1171d9
jnewbery:
Looks great. utACK 662d1171d9
Tree-SHA512: a53fc9a0f341eaec1614eb69abcf2d48eb4394bc89041ab69bfc05a63436ed37c65ad586c07fd37dc258ac7c7d5e4f7f93b4191407f5824bbf063b4c50894c4a
fa7e311e16 [doc] rpcwallet: Only fail rescan when blocks have been pruned (MarcoFalke)
aaaa57c2aa scripted-diff: Bump copyright headers in wallet (MarcoFalke)
faf3729242 wallet: Only fail rescan when blocks have actually been pruned (MarcoFalke)
Pull request description:
This brings the behaviour of the import* calls closer to importmulti. After this change, the difference between importmulti and the other import* calls is
* that in importmulti you can "opt-out" of scanning early blocks by setting a later timestamp.
* that in importmulti the wallet will successfully import the data, but fail to rescan. Whereas in the other calls, the wallet will abort before importing the data.
ACKs for commit fa7e31:
promag:
utACK fa7e311e16.
jnewbery:
utACK fa7e311e16
Tree-SHA512: a57d52ffea94b64e0eb9b5d3a7a63031325833908297dd14eb0c5251ffea3b2113b131003f1db4e9599e014369165a57f107a7150bb65e4c791e5fe742f33cb8
The description reads:
```
# This script runs all contrib/devtools/lint-*.sh files, and fails if any exit
# with a non-zero status code.
```
This runs all scripts and returns with a non-zero exit code if any failed.
a407b6fdf3 [tests] Make random seed logged and settable (John Newbery)
Pull request description:
This allows tests which use randomness to be reproducibly run on failure.
ACKs for commit a407b6:
jonatack:
re-ACK a407b6fdf3
jb55:
great! utACK a407b6fdf3
Tree-SHA512: e1e89e6e76d11ddec71a8f0f077227e4b46303f80461b170900d3f95d4dcc4187b0d1decfd63562ea970aaaf530ef032a3e64ed1669aac29033d95161855fda3
This adds a descriptors argument to the utxoupdatepsbt RPC. This means:
* Input and output scripts and keys will be filled in when known
* P2SH-witness outputs will be filled in from the UTXO set when a descriptor
is provided to show they're segwit outputs.
d20d756752 rpc: faster getblockstats using BlockUndo data (Felix Weis)
Pull request description:
Using undo data for a block (rev?????.dat) we can retrieve value information about prevouts and calculate the final transaction fee (rate). This approach is about 80x faster, drops the requirement for `-txindex`, and works for all non-pruned blocks.
```
# 2018-11-25T16:36:19Z Bitcoin Core version v0.17.99.0-edc715240-dirty (release build)
seq 550100 550200 0.00s user 0.00s system 62% cpu 0.004 total
xargs -n1 src/bitcoin-cli getblockstats 0.21s user 0.19s system 17% cpu 2.302 total
# 2018-11-25T16:39:17Z Bitcoin Core version v0.17.0 (release build)
seq 550100 550200 0.00s user 0.00s system 87% cpu 0.002 total
xargs -n1 src/bitcoin-cli getblockstats 0.24s user 0.22s system 0% cpu 3:19.42 total
```
ACKs for commit d20d75:
MarcoFalke:
re-utACK d20d756752
Tree-SHA512: 5babc3eb8d2fee2cb23dc12f522656b80737a540cbf2b13390a8f388304c46c064cca76f896b46a6e2abae8cc582d28e1ab20dd4bb17ad6142f20630c2d30c54
Using undo data for a block (rev?????.dat) we can retrieve value information about prevouts and calculate the final transaction fee (rate). This approach is about 80x faster, drops the requirement for -txindex, and works for all non-pruned blocks.
510c6532ba Extract ParseDescriptorRange (Ben Woosley)
Pull request description:
So as to be consistently informative when the checks fail, and
to protect against unintentional divergence among the checks.
ACKs for commit 510c65:
meshcollider:
Oh apologies, yes. Thanks :) utACK 510c6532ba
MarcoFalke:
utACK 510c6532ba
sipa:
utACK 510c6532ba
Tree-SHA512: b1f0792bfaa163890a20654a0fc2c4c4a996659916bf5f4a495662436b39326692a1a0c825caafd859e48c05f5dd1865c4f7c28092be5074edda3c94f94f9f8b
This diff has been generated with the following script, but is better
reviewed without looking at the script.
# -BEGIN VERIFY SCRIPT-
echo "Use msg_witness_block everywhere, except for tests that require msg_block"
# This could be a separate commit, but it is combined with the
# following scripts to reduce the overall diff
sed -i -e 's/msg_block/msg_witness_block/g' ./test/functional/{feature_assumevalid,feature_cltv,feature_dersig,feature_versionbits_warning,p2p_fingerprint,p2p_sendheaders,p2p_unrequested_blocks,example_test,rpc_blockchain}.py
echo "Rename msg_block to msg_no_witness_block"
# Rename msg_block to msg_no_witness_block in all tests (not the
# framework)
sed -i -e 's/msg_block/msg_no_witness_block/g' $(git grep -l msg_block ./test/functional/*.py)
# Derive msg_no_witness_block from msg_block
# Make msg_block a witness block in messages.py
patch -p1 --fuzz 0 << EOF
diff --git a/test/functional/test_framework/messages.py b/test/functional/test_framework/messages.py
index 00190e4cbd..e454ed5987 100755
--- a/test/functional/test_framework/messages.py
+++ b/test/functional/test_framework/messages.py
@@ -1133 +1133 @@ class msg_block:
- return self.block.serialize(with_witness=False)
+ return self.block.serialize()
@@ -1155 +1155 @@ class msg_generic:
-class msg_witness_block(msg_block):
+class msg_no_witness_block(msg_block):
@@ -1158,2 +1158 @@ class msg_witness_block(msg_block):
- r = self.block.serialize()
- return r
+ return self.block.serialize(with_witness=False)
@@ -1445 +1444 @@ class msg_blocktxn:
- r += self.block_transactions.serialize(with_witness=False)
+ r += self.block_transactions.serialize()
@@ -1452 +1451 @@ class msg_blocktxn:
-class msg_witness_blocktxn(msg_blocktxn):
+class msg_no_witness_blocktxn(msg_blocktxn):
@@ -1456,3 +1455 @@ class msg_witness_blocktxn(msg_blocktxn):
- r = b""
- r += self.block_transactions.serialize()
- return r
+ return self.block_transactions.serialize(with_witness=False)
EOF
# Conclude rename of msg_block to msg_no_witness_block
sed -i -e 's/msg_witness_block/msg_block/g' $(git grep -l msg_witness_block)
# -END VERIFY SCRIPT-
Unnamed arguments are confusing as to what they mean without looking up
the function signature.
Since segwit is active by default in regtest, and all blocks are
serialized with witness (#15664, c459c5f), remove the argument
`with_witness=True` from all calls to `CBlock::serialize` and
`BlockTransactions::serialize`.
This diff has been created with a script, but is better reviewed without
a scripted diff.
sed -i --regexp-extended -e 's/block(_?[2a-z]*)\.serialize\([a-z_]*=?True/block\1.serialize(/g' $(git grep -l serialize ./test)
7b29ec277b [tests] Comment for why logging config is set as command-line args. (John Newbery)
ba534ccd56 [tests] log thread names by default in functional tests (John Newbery)
Pull request description:
More detailed logs are better
ACKs for commit 7b29ec:
jamesob:
utACK 7b29ec277b
Tree-SHA512: 327cfedb7b7bf32f7ce1e2de5f70c7092041a8e868e14285a79176277c6cf47ebea27027f68787332f8ad21c7f64d2640dd21813eda5b2bd0e5208a65364a879
facfb4111d rpc: Deprecate getunconfirmedbalance and getwalletinfo balances (MarcoFalke)
999931cf8f rpc: Add getbalances RPC (MarcoFalke)
fad13e925e rpcwallet: Make helper methods const on CWallet (MarcoFalke)
fad40ec915 wallet: Use IsValidNumArgs in getwalletinfo rpc (MarcoFalke)
Pull request description:
This exposes the `CWallet::GetBalance()` struct over RPC.
In the future, incorrectly named rpcs such as `getunconfirmedbalance` or rpcs redundant to this such as `getbalance` could be removed.
ACKs for commit facfb4:
jnewbery:
utACK facfb4111d
Tree-SHA512: 1f54fedce55df9a8ea82d2b6265354b39a956072621876ebaee2355aac0e23c7b64340c3279502415598c095858529e18b50789be956250aafda1cd3a8d948a5
0ff1c2a838 Separate reason for premature spends (coinbase/locktime) (Suhas Daftuar)
54470e767b Assert validation reasons are contextually correct (Suhas Daftuar)
2120c31521 [refactor] Update some comments in validation.cpp as we arent doing DoS there (Matt Corallo)
12dbdd7a41 [refactor] Drop unused state.DoS(), state.GetDoS(), state.CorruptionPossible() (Matt Corallo)
aa502b88d1 scripted-diff: Remove DoS calls to CValidationState (Matt Corallo)
7721ad64f4 [refactor] Prep for scripted-diff by removing some \ns which annoy sed. (Matt Corallo)
5e78c5734b Allow use of state.Invalid() for all reasons (Matt Corallo)
6b34bc6b6f Fix handling of invalid headers (Suhas Daftuar)
ef54b486d5 [refactor] Use Reasons directly instead of DoS codes (Matt Corallo)
9ab2a0412e CorruptionPossible -> BLOCK_MUTATED (Matt Corallo)
6e55b292b0 CorruptionPossible -> TX_WITNESS_MUTATED (Matt Corallo)
7df16e70e6 LookupBlockIndex -> CACHED_INVALID (Matt Corallo)
c8b0d22698 [refactor] Drop redundant nDoS, corruptionPossible, SetCorruptionPossible (Matt Corallo)
34477ccd39 [refactor] Add useful-for-dos "reason" field to CValidationState (Matt Corallo)
6a7f8777a0 Ban all peers for all block script failures (Suhas Daftuar)
7b999103e2 Clean up banning levels (Matt Corallo)
b8b4c80146 [refactor] drop IsInvalid(nDoSOut) (Matt Corallo)
8818729013 [refactor] Refactor misbehavior ban decisions to MaybePunishNode() (Matt Corallo)
00e11e61c0 [refactor] rename stateDummy -> orphan_state (Matt Corallo)
f34fa719cf Drop obsolete sigops comment (Matt Corallo)
Pull request description:
This is a rebase of #11639 with some fixes for the last few comments which were not yet addressed.
The original PR text, with some strikethroughs of text that is no longer correct:
> This cleans up an old main-carryover - it made sense that main could decide what DoS scores to assign things because the DoS scores were handled in a different part of main, but now validation is telling net_processing what DoS scores to assign to different things, which is utter nonsense. Instead, we replace CValidationState's nDoS and CorruptionPossible with a general ValidationInvalidReason, which net_processing can handle as it sees fit. I keep the behavior changes here to a minimum, but in the future we can utilize these changes for other smarter behavior, such as disconnecting/preferring to rotate outbound peers based on them providing things which are invalid due to SOFT_FORK because we shouldn't ban for such cases.
>
> This is somewhat complementary with, though obviously conflicts heavily with #11523, which added enums in place of DoS scores, as well as a few other cleanups (which are still relevant).
>
> Compared with previous bans, the following changes are made:
>
> Txn with empty vin/vout or null prevouts move from 10 DoS
> points to 100.
> Loose transactions with a dependency loop now result in a ban
> instead of 10 DoS points.
> ~~BIP68-violation no longer results in a ban as it is SOFT_FORK.~~
> ~~Non-SegWit SigOp violation no longer results in a ban as it
> considers P2SH sigops and is thus SOFT_FORK.~~
> ~~Any script violation in a block no longer results in a ban as
> it may be the result of a SOFT_FORK. This should likely be
> fixed in the future by differentiating between them.~~
> Proof of work failure moves from 50 DoS points to a ban.
> Blocks with timestamps under MTP now result in a ban, blocks
> too far in the future continue to not result in a ban.
> Inclusion of non-final transactions in a block now results in a
> ban instead of 10 DoS points.
Note: The change to ban all peers for consensus violations is actually NOT the change I'd like to make -- I'd prefer to only ban outbound peers in those situations. The current behavior is a bit of a mess, however, and so in the interests of advancing this PR I tried to keep the changes to a minimum. I plan to revisit the behavior in a followup PR.
EDIT: One reviewer suggested I add some additional context for this PR:
> The goal of this work was to make net_processing aware of the actual reasons for validation failures, rather than just deal with opaque numbers instructing it to do something.
>
> In the future, I'd like to make it so that we use more context to decide how to punish a peer. One example is to differentiate inbound and outbound peer misbehaviors. Another potential example is if we'd treat RECENT_CONSENSUS_CHANGE failures differently (ie after the next consensus change is implemented), and perhaps again we'd want to treat some peers differently than others.
ACKs for commit 0ff1c2:
jnewbery:
utACK 0ff1c2a838
ryanofsky:
utACK 0ff1c2a838. Only change is dropping the first commit (f3883a321bf4ab289edcd9754b12cae3a648b175), and dropping the temporary `assert(level == GetDoS())` that was in 35ee77f2832eaffce30042e00785c310c5540cdc (now c8b0d22698)
Tree-SHA512: e915a411100876398af5463d0a885920e44d473467bb6af991ef2e8f2681db6c1209bb60f848bd154be72d460f039b5653df20a6840352c5f7ea5486d9f777a3
This is a first step towards cleaning up our DoS interface - make
validation return *why* something is invalid, and let net_processing
figure out what that implies in terms of banning/disconnection/etc.
Behavior change: peers will now be banned for providing blocks
with premature coinbase spends.
Co-authored-by: Anthony Towns <aj@erisian.com.au>
Suhas Daftuar <sdaftuar@gmail.com>
Compared with previous bans, the following changes are made:
* Txn with empty vin/vout or null prevouts move from 10 DoS
points to 100.
* Loose transactions with a dependency loop now result in a ban
instead of 10 DoS points.
* Many pre-segwit soft-fork errors now result in a ban.
Note: Transactions that violate soft-fork script flags since P2SH do not generally
result in a ban. Also, banning behavior for invalid blocks is dependent on
whether the node is validating with multiple script check threads, due to a long-
standing bug. That inconsistency is still present after this commit.
* Proof of work failure moves from 50 DoS points to a ban.
* Blocks with timestamps under MTP now result in a ban, blocks
too far in the future continue to *not* result in a ban.
* Inclusion of non-final transactions in a block now results in a
ban instead of 10 DoS points.
Co-authored-by: Anthony Towns <aj@erisian.com.au>
effe81f750 Move g_is_mempool_loaded into CTxMemPool::m_is_loaded (Ben Woosley)
bb8ae2c419 rpc: Expose g_is_mempool_loaded via getmempoolinfo and /rest/mempool/info.json (Ben Woosley)
Pull request description:
And use it to fix a race condition in mempool_persist.py:
https://travis-ci.org/Empact/bitcoin/jobs/487577243
Since e.g. getrawmempool returns errors based on this status, this
enables users to test it for readiness.
Fixes#12863
ACKs for commit effe81:
MarcoFalke:
utACK effe81f750
jnewbery:
utACK effe81f750
Tree-SHA512: 74328b0c17a97efb8a000d4ee49b9a673c2b6dde7ea30c43a6a2eff961a233351c9471f9a42344412135786c02bdf2ee1b2526651bb8fed68bd94d2120c4ef86
fa90a89eee [test] combine_logs: append node stderr and stdout if it exists (MarcoFalke)
Pull request description:
See issue:
* tests: bitcoind stdout and error should be passed to the logger #13519
ACKs for commit fa90a8:
laanwj:
utACK fa90a89eee
Tree-SHA512: 39c4596e2e133c9011ab01bc4dc24e884d0a8cce7a67d3765f17c288d3ffbd438e1ff6016d0f817a981b27fce17fa77a1ff56787ddb1ea55123ce9ecffb44c08
fa79a783d6 test: Add reorg test to wallet_balance (MarcoFalke)
fad03cd046 test: Check that wallet txs not in the mempool are untrusted (MarcoFalke)
fa195315e6 test: Add getunconfirmedbalance test with conflicts (MarcoFalke)
fa464e8211 test: Add wallet_balance test for watchonly (MarcoFalke)
Pull request description:
Second commit can be reviewed with `--ignore-all-space`
ACKs for commit fa79a7:
jnewbery:
utACK fa79a783d6
Tree-SHA512: ec4919a3c93b6dcb35d58e7c65bdffe7f4c8cb87b9287f3679631c1823ef5bd72789f233def94e60c1ab332711601751645566f5997ce250af55b328ed60e917
9f9db39041 QA/mininode: Send all headers upfront in send_blocks_and_test to avoid sending an unconnected one (Luke Dashjr)
Pull request description:
While this doesn't currently trigger any problems, the network protocol does expect headers to be sent connectable in normal circumstances, and if too many are sent out of order will disconnect the peer.
ACKs for commit 9f9db3:
Tree-SHA512: 25b88718e4ba3d31aed2de7ece23fab9a0737fd6536c5e618ea8eb5a3a217dab0dffaebc4892df7993bcea7efb7c4fb5085fabebe99535b8f7fdde3c19df54ff
c634b1e20 [POLICY] Make sending to future native witness outputs standard (Pieter Wuille)
Pull request description:
As discussed in the April 18 2019 IRC meeting.
This makes sending to future Segwit versions via native outputs (bech32) standard for relay, mempool acceptance, and mining. The reasons are:
* This may interfere with smooth adoption of future segwit versions, if they're defined (by the sender wallet/node).
* It violates BIP173 ("Version 0 witness addresses are always 42 or 62 characters, but implementations MUST allow the use of any version."), though admittedly this code was written before BIP173.
* It doesn't protect much, as P2SH-embedded segwit cannot be filtered in this way.
* As a general policy, the sender shouldn't care what the receiver likes his outputs to be.
Note that _spending_ such outputs (including P2SH-embedded ones) remains nonstandard, as that is actually required for softfork safety.
ACKs for commit c634b1:
MarcoFalke:
utACK c634b1e207
harding:
Tested ACK c634b1e207
meshcollider:
utACK c634b1e207
Tree-SHA512: e37168a1be9f445a04d4280593f0a92bdae33eee00ecd803d5eb16acb5c9cfc0f1f0a1dfbd5a0cc73da2c9928ec11cbdac7911513a78f85b789ae0d00e1b5962
a014373d81 QA: Avoid re-reading config.ini unnecessarily (Luke Dashjr)
Pull request description:
BitcoinTestFramework.main already loads and stores config.ini on the object itself; just access that instead of re-reading the file to check for features
ACKs for commit a01437:
practicalswift:
utACK a014373d81
hebasto:
utACK a014373d81
Tree-SHA512: 36e3dda36cf4fae243feb1655da2891d1b78c86319be9bd9809c20054fa0cb75749370b05aa9d589a4dcab6322d8cdf4e874c5175144ed23ba63b2ed338538ca
BitcoinTestFramework.main already loads and stores config.ini on the object itself; just access that instead of re-reading the file to check for features
c87fc71f7e Bugfix: test/functional/rpc_psbt: Correct test description comment (Luke Dashjr)
097c4aa379 Bugfix: test/functional/rpc_psbt: Remove check for specific error message that depends on uncertain assumptions (Luke Dashjr)
Pull request description:
When converttopsbt is called with a signed transaction, it either fails with "TX decode failed" if one or more inputs were segwit, or "Inputs must not have scriptSigs and scriptWitnesses" otherwise.
Since no effort is made by the test to ensure the inputs are segwit or not, avoid checking the exact message used.
The error code is still checked to ensure it is of the correct kind of failure.
ACKs for commit c87fc7:
instagibbs:
utACK c87fc71f7e
achow101:
utACK c87fc71f7e
Tree-SHA512: 61312b5d49aa50652902f30ba9693dfba9e5b7e6478f23becda20202d8b328ddb3e040f2199b617a68df133a5f1f8b5d68bc19d4621303f17c1963dca01bd9ef
faca95effd qa: Make swap_magic_bytes in p2p_invalid_messages atomic (MarcoFalke)
Pull request description:
Otherwise, this will lead to errors logged in the network thread:
https://travis-ci.org/MarcoFalke/bitcoin/jobs/513076282#L2765
ACKs for commit faca95:
Tree-SHA512: 8bc8280f9c0e8d40bc68c0ff1cab1c3386cbfef0728dbab5f29c2606dd65a9ac3c695b0c286be707a9f2bd62a6f87ac2032d7749fc2cf8b9fa1eba3c4cf70933
fa1c8e297 Resolve the qt/guiutil <-> qt/optionsmodal CD (251)
Pull request description:
This pull request attempts to resolve the `qt/guiutil` <-> `qt/optionsmodel` circular dependency.
The `Intro` class in `qt/intro` has a static member function `getDefaultDataDirectory` which is used by `qt/optionsmodel` and creates the circular dependency
`qt/guiutil -> qt/walletmodel -> qt/optionsmodel -> qt/intro -> qt/guiutil`.
This circular dependency is resolved by moving `Intro::getDefaultDataDirectory` to `GUIUtil::getDefaultDataDirectory` without modifying the implementation.
ACKs for commit fa1c8e:
MarcoFalke:
utACK fa1c8e2978
promag:
utACK fa1c8e2.
hebasto:
utACK fa1c8e2978
practicalswift:
utACK fa1c8e2978
jonasschnelli:
utACK fa1c8e2978
Tree-SHA512: 58cc4aee937c943d8de9dc97ef1789decfddb0287308f44e7e3a3b497c19e51da184988e17207544fff410168ec98dd49a3e62c47e84ad1f0cf6ef7247a80fb5
This pull request attempts to resolve the `qt/guiutil` <-> `qt/optionsmodel`
circular dependency.
The circular dependency is resolved by moving the `Intro::getDefaultDataDirectory`
member function to `GUIUtil::getDefaultDataDirectory`.
b67978529a Add comments to Python ECDSA implementation (John Newbery)
8c7b9324ca Pure python EC (Pieter Wuille)
Pull request description:
This removes the dependency on OpenSSL for the interaction tests, by providing a pure-Python
toy implementation of secp256k1.
ACKs for commit b67978:
jnewbery:
utACK b67978529a
Tree-SHA512: 181445eb08b316c46937b80dc10aa50d103ab1fdddaf834896c0ea22204889f7b13fd33cbcbd00ddba15f7e4686fe0d9f8e8bb4c0ad0e9587490c90be83966dc
418d3230f8 Resolve the checkpoints <-> validation CD. (251)
Pull request description:
This pull request attempts to resolve the `checkpoints -> validation -> checkpoints` circular dependency.
The circular dependency is resolved by moving the `CheckPoints::GetLastCheckpoint(const CCheckpointData& data)` function to `validation.cpp` where it used exclusively by the private function `ContextualCheckBlockHeader(const CBlockHeader& block, CValidationState& state, const CChainParams& params, const CBlockIndex* pindexPrev, int64_t nAdjustedTime)`.
ACKs for commit 418d32:
promag:
utACK 418d323, only `GetLastCheckpoint` usage is in `validation.cpp` and so makes sense to move it there.
practicalswift:
utACK 418d3230f8
MarcoFalke:
utACK 418d3230f8
sipa:
utACK 418d3230f8
Tree-SHA512: 03c3556bc192e65f5e3fa76fd545d4ee7d63d3fb06b132f7a1fa6131aa21ddd2e5b2d19e2222dfe524f422daaca30efde219bed188db8c74ff4b088876b5bc16
c7efb652f3 blockfilter: Update BIP 158 test vectors. (Jim Posen)
19308c9e21 rpc: Add getblockfilter RPC method. (Jim Posen)
ff35105096 init: Add CLI option to enable block filter index. (Jim Posen)
accc8b8b18 index: Access functions for global block filter indexes. (Jim Posen)
2bc90e4e7b test: Unit test for block filter index reorg handling. (Jim Posen)
6bcf0998c0 test: Unit tests for block index filter. (Jim Posen)
b5e8200db7 index: Implement lookup methods on block filter index. (Jim Posen)
75a76e3619 index: Implement block filter index with write operations. (Jim Posen)
2ad2338ef9 serialize: Serialization support for big-endian 32-bit ints. (Jim Posen)
ba6ff9a6f7 blockfilter: Functions to translate filter types to/from names. (Jim Posen)
62b7a4f094 index: Ensure block locator is not stale after chain reorg. (Jim Posen)
4368384f1d index: Allow atomic commits of index state to be extended. (Jim Posen)
Pull request description:
This introduces a new BlockFilterIndex class, which is required for BIP 157 support.
The index is uses the asynchronous BaseIndex infrastructure driven by the ValidationInterface callbacks. Filters are stored sequentially in flat files and the disk location of each filter is indexed in LevelDB along with the filter hash and header. The index is designed to ensure persistence of filters reorganized out of the main chain to simplify the BIP 157 net implementation.
Stats (block height = 565500):
- Syncing the index from scratch takes 45m
- Total index size is 3.8 GiB
ACKs for commit c7efb6:
MarcoFalke:
utACK c7efb652f3
ryanofsky:
Slightly tested ACK c7efb652f3 (I just rebuilt the index with the updated PR and tested the RPC). Changes since last review: rebase, fixed compile errors in internal commits, new comments, updated error messages, tweaked cache size logic, renamed commit method, renamed constants and globals, fixed whitespace, extra BlockFilterIndex::Init error check.
Tree-SHA512: f8ed7a9b6f76df45933aa5eba92b27b3af83f6df2ccb3728a5c89eec80f654344dc14f055f6f63eb9b3a7649dd8af6553fe14969889e7e2fd2f8461574d18f28
fab6a0a659 test: Add test that addmultisigaddress fails for watchonly addresses (MarcoFalke)
fad81d870a test: Fixup creatmultisig documentation and whitespace (MarcoFalke)
Pull request description:
Just to make sure this is not regressed on accidentally in the future
ACKs for commit fab6a0:
jonatack:
ACK fab6a0a659
Tree-SHA512: bf8dcbc752f8910902a995e55ce486621156aa01f112990344815c4aab980298dfecc108e78245a8986a00c3871338ad16fc818a1bce9dfc6b37b9c88851e39d
aa410c2b17 rpc: Validate maxfeerate with AmountFromValue (João Barbosa)
Pull request description:
With this change `maxfeerate` can also be set as a string, accordingly to the help test:
```
maxfeerate (numeric or string,
```
Beside, there are no tests for the removed errors.
ACKs for commit aa410c:
meshcollider:
utACK aa410c2b17
MarcoFalke:
utACK aa410c2b17 Good catch
Tree-SHA512: f3bfea91dc7daa943729e270585dbf333055aeda805fbd01eaab20a7e0e6147382647c11525334382d198df0d3d45da6102b541efda5a1361f96271c98d5d89d
184f8785f wallet_bumpfee.py: add test for change key preservation (Gregory Sanders)
d08becff8 add functional tests for feerate bumpfee with adding inputs (Gregory Sanders)
0ea47ba7b generalize bumpfee to add inputs when needed (Gregory Sanders)
Pull request description:
When targeting a feerate using `bumpfee`, call a new function that directly uses `CWallet::CreateTransaction` and coin control to get the desired result. This allows us to get a superset of previous behavior, with an arbitrary RBF bump of a transaction provided it passes the preconditional checks and spare confirmed utxos are available.
Note(s):
0) The coin selection will use knapsack solver for the residual selection.
1) This functionality, just like knapsack coin selection in general, will hoover up negative-value inputs when given the chance.
2) Newly added inputs must be confirmed due to current Core policy. See error: `replacement-adds-unconfirmed`
3) Supporting this with `totalFee` is difficult since the "minimum total fee" option in `CreateTransaction` logic was (rightly)taken out in #10390 .
ACKs for commit 184f87:
jnewbery:
utACK 184f8785f7
Tree-SHA512: fb6542bdfb2c6010e328ec475cf9dcbff4eb2b1a1b27f78010214534908987a5635797196fa05edddffcbcf2987335872dc644a99261886d5cbb34a8f262ad3e
fafe5f0d09 test: Remove unused imports (MarcoFalke)
fa16a09215 scripted-diff: use self.sync_* methods (MarcoFalke)
faf77f9b90 test: Pass self to test_simple_bumpfee_succeeds (MarcoFalke)
fa6dc7c5c3 test: Add BitcoinTestFramework::sync_* methods (MarcoFalke)
fafe008cb4 test: Pass at most one node group to sync_all (MarcoFalke)
fa4680ed09 scripted-diff: Rename sync_blocks to send_blocks to avoid name collisions and confusion (MarcoFalke)
Pull request description:
This adds methods to the test framework that can be called by just `self.sync_*()`.
This avoids having to import the underlying util method. Also, in the default case, where all nodes are synced this avoid having to pass `self.nodes` explicitly.
So the effective changes are:
```diff
@@
-from test_framework.util import sync_blocks, sync_mempools
@@
- sync_blocks(self.nodes)
+ self.sync_blocks()
@@
- sync_mempools(self.nodes)
+ self.sync_mempools()
ACKs for commit fafe5f:
promag:
utACK fafe5f0.
jonatack:
ACK fafe5f0d09, nice simplification.
Tree-SHA512: 5c81840edf9fb3c5de2d7bf95ca36a5a8d23567cd1479a0f4044547c2080e9a3c5cf375357bc8eebb5b68829be050a171ab2512cfd47b89feed51fe3bad2cd72
4d074e84a2 [build] Move AnalyzePSBT from psbt.cpp to node/psbt.cpp (Russell Yanofsky)
fd509bd1f7 [docs] Document src subdirectories and different libraries (John Newbery)
9eaeb7fb8d [build] Move wallet load functions to wallet/load unit (John Newbery)
91a25d1e71 [build] Add several util units (John Newbery)
99517866b6 [build] Move several units into common libraries (John Newbery)
0509465542 [build] Move rpc rawtransaction util functions to rpc/rawtransaction_util.cpp (John Newbery)
1acc61f874 [build] Move rpc utility methods to rpc/util (John Newbery)
4a75c9d651 [build] Move policy settings to new src/policy/settings unit (John Newbery)
fdf8888b6f [build] Move CheckTransaction from lib_server to lib_consensus (John Newbery)
Pull request description:
This is a move-only commit. No code is changing and the moves can be easily verified with:
```sh
git log -p -n1 --color-moved=dimmed_zebra
```
This commit moves functions and variables that wallet code depends on out of libbitcoin_server.a, so the bitcoin-wallet tool can be built without libbitcoin_server.a in #15639, and attempting to access server state from wallet code will result in link errors instead of silently broken code.
List of moves:
- `CheckTransaction` moves from `consensus/tx_verify.cpp` to `consensus/tx_check.cpp`
- `urlDecode` moves from `httpserver.cpp` to `util/url.cpp`
- `TransactionErrorString` moves from `node/transaction.cpp` to `util/error.cpp`
- `StringForFeeReason` and `FeeModeFromString` move from `policy/fees.cpp` to `util/fees.cpp`
- `incrementalRelayFee` `dustRelayFee` and `nBytesPerSigOp` move from `policy/policy.cpp` to `policy/settings.cpp`
- `SignalsOptInRBF` moves from `policy/rbf.cpp` to `util/rbf.cpp`
- `fIsBareMultisigStd` moves from `validation.cpp` to `policy/settings.cpp`
- `ConstructTransaction` `TxInErrorToJSON` and `SignTransaction` move from `rpc/rawtransaction.cpp` to `rpc/rawtransaction_util.cpp`
- `RPCTypeCheck` `RPCTypeCheckArgument` `RPCTypeCheckObj` `AmountFromValue` `ParseHashV``ParseHashO` `ParseHexV` `ParseHexO` `HelpExampleCli` and `HelpExampleRpc` move from `rpc/server.cpp` to `rpc/util.cpp`
- `AmountHighWarn` and `AmountErrMsg` move from `ui_interface.cpp` to `util/error.cpp`
- `FormatStateMessage` and `strMessageMagic` move from `validation.cpp` to `util/validation.cpp`
- `VerifyWallets` `LoadWallets` `StartWallets` `FlushWallets` `StopWallets` and `UnloadWallets` move from `wallet/init.cpp` to `wallet/node.cpp`
ACKs for commit 4d074e:
jnewbery:
utACK 4d074e84a2 (checked by doing the rebase myself and verifying no difference between my branch and 4d074e84a2)
Tree-SHA512: 5e1604a9fb06475f2b96da0de0baa8330f4dda834dc20a0183ef11e1e4c27631d1d1bbb9abf0054efc03d56945fdf9920f63366b6a4f200f665b742a479ff75c
fae38c3dc6 doc: Fix all typos reported by codespell (MarcoFalke)
fa9058f0ed doc: Add release notes for 15629 (MarcoFalke)
fa4a922d78 qa: Add test for missing testnet section in conf file (MarcoFalke)
dddd6f0f58 init: Throw error when network specific config is ignored (MarcoFalke)
Pull request description:
This should have no effect on mainnet users, but simplifies testing, where config settings are currently ignored with only a warning. Fix this by making it an error.
Issues:
* bitcoin client 0.17.0 ignores wallet's name (file) #14523
* Can't set custom rpcport on testnet #13777
* ...
ACKs for commit fae38c:
Tree-SHA512: 2e209526898eea6e444c803ec2666989cee4ca137492d32984998733c50a70056cb54657df8dc3027a6a0612738a8afce0bc35824b868c5f22281e00e0188530
833d98ae0 [wallet] Remove unnecessary Chain::Lock parameter from ResendWalletTransactions (John Newbery)
52b760fc6 [wallet] Schedule tx rebroadcasts in wallet (John Newbery)
f463cd107 [wallet] Keep track of the best block time in the wallet (John Newbery)
Pull request description:
Remove the `Broadcast()`/`ResendWalletTransactions()` notification from the Validation interface.
Closes#15619. See that issue for discussion.
ACKs for commit 833d98:
ryanofsky:
utACK 833d98ae07. No changes, just rebase.
Tree-SHA512: 7689f2083608ebad8c95ab6692f7842754e1ebe5508bc926a89cad7105cce41007648f37341ba5feb92b30a7aa87acd3abf264a4f1874e35a7161553f6ff3595
Adds the following util units and adds them to libbitcoin_util:
- `util/url.cpp` takes `urlDecode` from `httpserver.cpp`
- `util/error.cpp` takes `TransactionErrorString` from
`node/transaction.cpp` and `AmountHighWarn` and `AmountErrMsg` from
`ui_interface.cpp`
- `util/fees.cpp` takes `StringForFeeReason` and `FeeModeFromString` from `policy/fees.cpp`
- `util/rbf.cpp` takes `SignalsOptInRBF` from `policy/rbf.cpp`
- 'util/validation.cpp` takes `FormatStateMessage` and `strMessageMagic` from 'validation.cpp`
This moves the following policy settings functions and globals to a new
src/policy/settings unit in lib_server:
- `incrementalRelayFee`
- `dustRelayFee`
- `nBytesPerSigOp`
- `fIsBareMultisigStd`
These settings are only required by the node and should not be accessed
by other libraries.
