The descriptor inference logic would previously always use a dummy
signing provider and would never analyze the witness script of a P2WSH
scriptPubKey.
Note even a valid Miniscript might not always be decodable from Script
without more contextual information (for instance the key preimage for a
pk_h).
6d31900e52 wallet: migrate wallet, exit early if no legacy data exist (furszy)
Pull request description:
The process first creates a backup file then return an error,
without removing the recently created file, when notices that
the db is already running sqlite.
ACKs for top commit:
john-moffett:
ACK 6d31900e52
achow101:
ACK 6d31900e52
ishaanam:
crACK 6d31900e52
Tree-SHA512: 9fb52e80de96e129487ab91bef13647bc4570a782003b1e37940e2a00ca26283fd24ad39bdb63a984ae0a56140b518fd0d74aa2fc59ab04405b2c179b7d3c54a
b530d9605d test: refactor: introduce `replace_in_config` helper (Sebastian Falbesoner)
Pull request description:
Currently two functional tests (p2p_permissions.py and wallet_crosschain.py) include quite similar code for substituting strings in a TestNode's bitcoind configuration file, so refactoring that out to a dedicated helper method seems to make sense (probably other tests could need that too in the future).
ACKs for top commit:
kouloumos:
ACK b530d9605d
Tree-SHA512: 5ca65a2ef3292460e5720d5c6acf7326335001858e8f71ab054560117f9479dbadb1dacd8c9235f67f3fcfd08dbc761b62704f379cbf619bba8804f16a25bc7b
Xoroshiro128++ is a fast non-cryptographic random generator.
Reference implementation is available at https://prng.di.unimi.it/
Co-Authored-By: Pieter Wuille <pieter@wuille.net>
b0fa5989e1 test: Check that orphaned coinbase unconf spend is still abandoned (Andrew Chow)
9addbd7890 wallet: Automatically abandon orphaned coinbases and their children (Andrew Chow)
Pull request description:
When a block is reorged out of the main chain, any descendants of the coinbase will no longer be valid. Currently they are only marked as inactive, which means that our balance calculations will still include them. In order to be excluded from the balance calculation, they need to either be abandoned or conflicted. This PR goes with the abandoned method.
Note that even when they are included in balance calculations, coin selection will not select outputs belonging to these transactions because they are not in the mempool.
Fixes#14148
ACKs for top commit:
furszy:
ACK b0fa5989 with a not-blocking nit.
aureleoules:
reACK b0fa5989e1
ishaanam:
ACK b0fa5989e1
Tree-SHA512: 68f12e7aa8df392d8817dc6ac0becce8dbe83837bfa538f47027e6730e5b2e1b1a090cfcea2dc598398fdb66090e02d321d799f087020d7e1badcf96e598c3ac
39b93649c4 test: add functional test for IBD stalling logic (Martin Zumsande)
0565951f34 p2p: Make block stalling timeout adaptive (Martin Zumsande)
Pull request description:
During IBD, there is the following stalling mechanism if we can't proceed with assigning blocks from a 1024 lookahead window because all of these blocks are either already downloaded or in-flight: We'll mark the peer from which we expect the current block that would allow us to advance our tip (and thereby move the 1024 window ahead) as a possible staller. We then give this peer 2 more seconds to deliver a block (`BLOCK_STALLING_TIMEOUT`) and if it doesn't, disconnect it and assign the critical block we need to another peer.
Now the problem is that this second peer is immediately marked as a potential staller using the same mechanism and given 2 seconds as well - if our own connection is so slow that it simply takes us more than 2 seconds to download this block, that peer will also be disconnected (and so on...), leading to repeated disconnections and no progress in IBD. This has been described in #9213, and I have observed this when doing IBD on slower connections or with Tor - sometimes there would be several minutes without progress, where all we did was disconnect peers and find new ones.
The `2s` stalling timeout was introduced in #4468, when blocks weren't full and before Segwit increased the maximum possible physical size of blocks - so I think it made a lot of sense back then.
But it would be good to revisit this timeout now.
This PR makes the timout adaptive (idea by sipa):
If we disconnect a peer for stalling, we now double the timeout for the next peer (up to a maximum of 64s). If we connect a block, we half it again up to the old value of 2 seconds. That way, peers that are comparatively slower will still get disconnected, but long phases of disconnecting all peers shouldn't happen anymore.
Fixes#9213
ACKs for top commit:
achow101:
ACK 39b93649c4
RandyMcMillan:
Strong Concept ACK 39b93649c4
vasild:
ACK 39b93649c4
naumenkogs:
ACK 39b93649c4
Tree-SHA512: 85bc57093b2fb1d28d7409ed8df5a91543909405907bc129de7c6285d0810dd79bc05219e4d5aefcb55c85512b0ad5bed43a4114a17e46c35b9a3f9a983d5754
8609f24be2 test: refactor: simplify p2p_eviction.py by using MiniWallet (Sebastian Falbesoner)
7aa4b32cd4 test: refactor: simplify p2p_tx_download.py by using MiniWallet (Sebastian Falbesoner)
Pull request description:
Similar to #26892, this PR simplies the functional tests p2p_tx_download.py and p2p_eviction.py by using MiniWallet in order to avoid manual low-level tx creation. For the latter, rather than mining 100 blocks manually, the pre-mined chain of the test framework is used.
These instances were found via `$ git grep signrawtransactionwithkey ./test/functional`. AFAICT, there are no other instances where MiniWallet could replace tx creation trivially.
ACKs for top commit:
MarcoFalke:
review ACK 8609f24be2
Tree-SHA512: dfb0103fe7f0625d125e8e4408baed8bfc1ff579954af17d0ead5277e05f933b2c2d98a0093e8109e947635f1718d5c58d837ab825f26077fac0a40575bd3e6f
fafeddfe0e rpc: Throw more user friendly arg type check error (MarcoFalke)
Pull request description:
The arg type check error doesn't list which arg (position or name) failed. Fix that.
ACKs for top commit:
stickies-v:
ACK fafeddfe0e - although I think the functional test isn't in a logical place (but not blocking)
Tree-SHA512: 17425aa145aab5045940ec74fff28f0e3b2b17ae55f91c4bb5cbcdff0ef13732f8e31621d85964dc2c04333ea37dbe528296ac61be27541384b44e37957555c8
b9d5674541 init: Remove sensitive flag from rpcbind (Andrew Chow)
Pull request description:
`-rpcbind` is currently flagged as a sensitive option which means that its value will be masked when the command line args are written to the debug.log file. However this is not useful as if `rpcbind` is actually activated, the bound IP addresses will be written to the log anyways. The test `feature_config_args.py` did not catch this contradiction as the test node was not started with `rpcallowip` and so `rpcbind` was not acted upon.
This also brings `rpcbind` inline with `bind` as that is not flagged as sensitive either.
ACKs for top commit:
Sjors:
re-utACK b9d5674541
willcl-ark:
ACK b9d5674
theStack:
ACK b9d5674541
Tree-SHA512: 50ab5ad2e18ae70649deb1ac429d404b5f5c41f32a4943b2041480580152df22e72d4aae493379d0b23fcb649ab342376a82119760fbf6dfdcda659ffd3e244a
b68e5a7fef lint: specify the right commit range when running locally (James O'Beirne)
dff7ed5732 test: add an easy way to run linters locally (James O'Beirne)
Pull request description:
Adds a Dockerfile configuration ~~(originally written mostly by fanquake)~~ that allows straightforward running of linters with compatible versions locally. This removes a ton of annoyance when trying to appease CI, because many of the linter versions are quite old and difficult to maintain locally.
I realize that people may not be thrilled to add more ancillary tooling to the repo, but I think this makes a lot of sense given the linter versions listed in this container configuration are dictated by this repo (within the CI configuration), so having these things live in two separate places is a recipe for version mismatches.
Eventually we can likely just use this container on CI directly to avoid any chance of inconsistencies between local dev experience and CI.
ACKs for top commit:
aureleoules:
ACK b68e5a7fef
stickies-v:
ACK b68e5a7fe
john-moffett:
ACK b68e5a7fef
Tree-SHA512: 7ef7a5dae023d81fdb6296d5d92dfa074ee321c7993e607c9f014d0f21c91558611aa00fc3ce1edc7b5e68371aea0d27fa1931291a79bb867a6c783bb536775f
d96d97ad30 doc: Add release note for shutdownnotify. (klementtan)
0bd73e2c45 util: Add -shutdownnotify option. (klementtan)
Pull request description:
**Description**: Similar to `-startupnotify`, this PR adds a new option to allow users to specify a command to be executed when Bitcoin Core shuts down.
**Note**: The `shutdownnotify` commands will not be executed if bitcoind shut down due to *unexpected* reasons (ie `killall -9 bitcoind`).
### Testing:
**Normal shutdown commands**
```
# start bitcoind with shutdownnotify optioin
./src/bitcoind -signet -shutdownnotify="touch foo.txt"
# shutdown bitcoind
./src/bitcoin-cli -signet stop
# check that foo.txt has been created
```
**Final RPC call**
Commands:
```
$ ./src/bitcoind -signet -nolisten -noconnect -shutdownnotify="./src/bitcoin-cli -signet getblockchaininfo > tmp.txt"
$ ./src/bitcoin-cli stop
$ cat tmp.txt
```
<details>
<summary>Screen Shot</summary>
</details>
ACKs for top commit:
achow101:
ACK d96d97ad30
1440000bytes:
ACK d96d97ad30
theStack:
re-ACK d96d97ad30
Tree-SHA512: 16f7406fd232e8b97aea5e58854c84755b0c35c88cb3ef9ee123b29a1475a376122b1e100da860cc336d4d657e6046a70e915fdb9b70c9fd097c6eef1b028161
Adds a Dockerfile configuration
that allows straightforward running of linters with compatible versions
locally. This removes a ton of annoyance when trying to appease CI,
because many of the linter versions are quite old and difficult to
maintain locally.
I realize that people may not be thrilled to more ancillary tooling to
the repo, but I think this makes a lot of sense given the linter
versions listed in this container configuration are dictated by this
repo (within the CI configuration), so having these things live in
two separate places is a recipe for version mismatches.
Eventually we can likely just use this container on CI directly to avoid
any chance of inconsistencies between local dev experience and CI.
fa6b402114 test: Run mempool_packages.py with MiniWallet (MarcoFalke)
fa448c27d2 test: Return fee from MiniWallet (MarcoFalke)
faec09f240 test: Return chain of MiniWallet txs from MiniWallet chain method (MarcoFalke)
faa12d4ccd test: Refactor MiniWallet sign_tx (MarcoFalke)
fa2d82103f test: Return wtxid from create_self_transfer_multi (MarcoFalke)
Pull request description:
This allows to run the test even when no wallet is compiled in.
Also, it is a lot nicer to read now.
ACKs for top commit:
glozow:
reACK fa6b402
Tree-SHA512: de0338068fd51db01d64ce270f94fd2982a63a6de597325cd1e1f11127e9075bd4aeacace0ed76d09a2db8b962b27237cf11edb4c1fe1a01134d397f8a11bd05
603d295199 test: wallet: add coverage for `-spendzeroconfchange` setting (Sebastian Falbesoner)
50112034bc test: remove `-spendzeroconfchange` setting from mempool_limit.py (Sebastian Falbesoner)
Pull request description:
This PR adds missing test coverage for the `-spendzeroconfchange` setting (in particular the non-default case `=0`). Note that in contrast to the name, the setting does not only apply to change outputs, but in fact to _all_ unconfirmed outputs that we sent to ourselves, i.e. we can trigger the testing path simply with a single recipient address. The first commit removes the setting from the functional test mempool_limit.py, where it doesn't have any effect, since the test was changed to use MiniWallet in commit dddca3899c.
ACKs for top commit:
brunoerg:
crACK 603d295199
Tree-SHA512: 15d9c8bd5eb37c6b228bf887eb27debee0a391c82356662785da4553ee2558e611834c3936ef7136812b46f877bab7aa5f3088bbd278b81f296bdda96cc8e1c3
fa9f6d7bcd rpc: Run type check against RPCArgs (MarcoFalke)
faf96721a6 test: Fix wrong types passed to RPCs (MarcoFalke)
Pull request description:
It seems brittle to require `RPCTypeCheck` being called inside the code logic. Without compile-time enforcement this will lead to places where it is forgotten and thus to inconsistencies and bugs. Fix this by removing the calls to `RPCTypeCheck` and doing the check internally.
The changes should be reviewed as refactoring changes. However, if they change behavior, it will be a bugfix. For example the changes here happen to also detect/fix bugs like the one fixed in commit 3b5fb6e77a.
ACKs for top commit:
ajtowns:
ACK fa9f6d7bcd
Tree-SHA512: fb2c0981fe6e24da3ca7dbc06898730779ea4e02ea485458505a281cf421015e44dad0221a04023fc547ea2c660d94657909843fc85d92b847611ec097532439
6bd098a838 test: simplify tests by using the pre-mined chain (kouloumos)
42029a7fd4 test: remove redundant blocks generation logic (kouloumos)
0377d6bb42 test: add `rescan_utxos` in MiniWallet's initialization (kouloumos)
Pull request description:
When a pre-mined blockchain is used (default behavior), it [contains coinbase outputs in blocks 76-10](07c54de550/test/functional/test_framework/test_framework.py (L809-L813)) to [the MiniWallet's default address](07c54de550/test/functional/test_framework/wallet.py (L99-L101)). That's why we always* `rescan_utxos()` after initializing the MiniWallet, in order for the MiniWallet to account for those mature UTXOs.
> The tests following this usage pattern can be seen with:
> ```git grep -n "MiniWallet(" $(git grep -le "rescan_utxos()" $(git grep -Le "self.setup_clean_chain = True"))```
**This PR adds `rescan_utxos()` inside MiniWallet's initialization to simplify usage when the MiniWallet is used with a pre-mined chain.**
### secondary changes
- *There are a few tests that use the pre-mined blockchain but do not `rescan_utxos()`, they instead generate new blocks to create mature UTXOs.
> Those were written before the `rescan_utxos()` method was introduced with https://github.com/bitcoin/bitcoin/pull/22955 (fac66d0a39) and can be seen with:
> `git grep -n "MiniWallet(" $(git grep -Le "rescan_utxos()" $(git grep -Le "self.setup_clean_chain = True"))`
>
After including `rescan_utxos()` inside MiniWallets initilization, this blocks generation logic is not needed as the MiniWallet already accounts for enough mature UTXOs to perform the tests. **Therefore the now redundant blocks generation logic is removed from those tests with the second commit.**
- The rest of the MiniWallet tests use a clean chain (`self.setup_clean_chain = True`) and can be seen with
`git grep -n "MiniWallet(" $(git grep -le "self.setup_clean_chain = True")`
From those, there are a few that start from a clean chain and then create enough mature UTXOs for the MiniWallet with this kind of logic:
07c54de550/test/functional/mempool_expiry.py (L36-L40)
**Those tests are simplified in the third commit to instead utilize the mature UTXOs of the pre-mined chain.**
ACKs for top commit:
MarcoFalke:
ACK 6bd098a838 🕷
theStack:
re-ACK 6bd098a838
Tree-SHA512: 7f9361e36910e4000c33a32efdde4449f4a8a763bb42df96de826fcde469f9362f701b8c99e2a2c482d2d5a42a83ae5ae3844fdbed187ed0ff231f386c222493
5ca7a7be76 rpc: Return accurate results for scanblocks (Aurèle Oulès)
Pull request description:
Implements #26322.
Adds a `filter_false_positives` mode to `scanblocks` to accurately verify results from blockfilters.
If the option is enabled, pre-results given by blockfilters will be filtered out again by checking vouts and vins of all transactions of the relevant blocks against the given descriptors.
### Master
```bash
./src/bitcoin-cli -testnet -named scanblocks action=start scanobjects='["addr(tb1qcxf2gv93c26s6mqz7y6etpqdf70zmn67dualgr)"]'
{
"from_height": 0,
"to_height": 2376055,
"relevant_blocks": [
"000000000001bc35077dec4104e0ab1f667ae27059bd907f9a8fac55c802ae36",
"00000000000120a9c50542d73248fb7c37640c252850f0cf273134ad9febaf61",
"0000000000000082f7af3835da8b6146b0bfb243b8842f09c495fa1e74d454ed",
"0000000000000094c32651728193bfbe91f6789683b8d6ac6ae2d22ebd3cb5d3"
]
}
```
### PR (without `filter_false_positives` mode)
Same as master
```bash
./src/bitcoin-cli -testnet -named scanblocks action=start scanobjects='["addr(tb1qcxf2gv93c26s6mqz7y6etpqdf70zmn67dualgr)"]' filter_false_positives=false
{
"from_height": 0,
"to_height": 2376055,
"relevant_blocks": [
"000000000001bc35077dec4104e0ab1f667ae27059bd907f9a8fac55c802ae36",
"00000000000120a9c50542d73248fb7c37640c252850f0cf273134ad9febaf61",
"0000000000000082f7af3835da8b6146b0bfb243b8842f09c495fa1e74d454ed",
"0000000000000094c32651728193bfbe91f6789683b8d6ac6ae2d22ebd3cb5d3"
]
}
```
### PR (with `filter_false_positives` mode)
```bash
./src/bitcoin-cli -testnet -named scanblocks action=start scanobjects='["addr(tb1qcxf2gv93c26s6mqz7y6etpqdf70zmn67dualgr)"]' filter_false_positives=true
{
"from_height": 0,
"to_height": 2376058,
"relevant_blocks": [
"0000000000000082f7af3835da8b6146b0bfb243b8842f09c495fa1e74d454ed",
"0000000000000094c32651728193bfbe91f6789683b8d6ac6ae2d22ebd3cb5d3"
]
}
```
Also adds a test to check that the blockhash of a transaction will be included in the `relevant_blocks` whether the `filter_false_positives` mode is enabled or not.
ACKs for top commit:
achow101:
ACK 5ca7a7be76
theStack:
re-ACK 5ca7a7be76
furszy:
Code review ACK 5ca7a7be
Tree-SHA512: e8f3cceddddd66f59509717b6314d89e2fef241e13cee81b18fd95e8362cbb95cc40f884342ce6cf892a86febd9e2d434afce05d51892240e67f72ae991852e7
cfe5aebc79 rpc: add minconf and maxconf options to sendall (ishaanam)
a07a413466 Wallet/RPC: Allow specifying min & max chain depth for inputs used by fund calls (Juan Pablo Civile)
Pull request description:
This PR adds a "minconf" option to `fundrawtransaction`, `walletcreatefundedpsbt`, and `sendall`.
Alternative implementation of #14641Fixes#14542
Edit: This PR now also adds this option to `send`
ACKs for top commit:
achow101:
ACK cfe5aebc79
Xekyo:
ACK cfe5aebc79
furszy:
diff ACK cfe5aebc, only a non-blocking nit.
Tree-SHA512: 836e610926eec3a62308fba88ddbd6a13d8f4dac37352d0309599f893cde9c1df5e9c298fda6e076493068e4d213e4afa7290a9e3bdb5a95a5d507da3f7b59e8
4159ccd031 test: Run feature_bip68_sequence.py with MiniWallet (Miles Liu)
fc0caaf4aa test: Add "include mempool" flag to MiniWallet rescan_utxos (Miles Liu)
d0a909ae54 test: Add "include immature coinbase" flag to MiniWallet get_utxos (Miles Liu)
e5b9127d9e test: Add signs P2TR and RAWSCRIPT to MiniWallet sign_tx (Miles Liu)
Pull request description:
This PR enables one more of the non-wallet functional tests (feature_bip68_sequence.py) to be run even when no wallet is compiled in by using the MiniWallet instead, as proposed in #20078.
ACKs for top commit:
achow101:
ACK 4159ccd031
MarcoFalke:
review ACK 4159ccd031🤸
Tree-SHA512: e891b189381e961c840b45fc30d058363707fd54c1c4bdc3d29623b03309981f1d3ebfac27e6aecf621bdbcd7e31754a3ef7c53f86346f7dd241c137e64c92bd
d6fc1d6a33 test: add coverage for dust mempool policy (`-dustrelayfee` setting) (Sebastian Falbesoner)
8a5dbe2879 test: add `CScript` method for checking for witness program (Sebastian Falbesoner)
Pull request description:
This PR adds missing test coverage for the `-dustrelayfee` setting, which specifies the fee-rate used to define dust. Output scripts for all common types that are treated as standard by default (P2PK, P2(W)PKH, P2(W)SH, P2TR, bare multisig, null data, unknown witness versions v2+) are created and then checked for dust-mempool-policy each via the `testmempoolaccept` RPC: a tx with an output's nValue equal to the dust threshold should be accepted, one with an nValue of just one 1 satoshi below that should be rejected with reason `dust`. This is repeatedly done for a fixed (but obviously somewhat arbitrary) list of different `-dustrelayfee` settings on a single node, including the default and zero (i.e. no dust limit) settings.
Note that the first commit introduces a necessary `CScript` helper method `IsWitnessProgram` (using PascalCase in Python is likely controversial; in this case the style for the already existing method `GetSigOpCount` was followed, which also refers to a method in the core `CScript` class).
Some historical information about dust, contributed by pablomartin4btc:
"The concept of dust was first introduced in https://github.com/bitcoin/bitcoin/pull/2577. This [commit](eb30d1a5b2) from https://github.com/bitcoin/bitcoin/pull/9380 introduced the -dustrelayfee option. Previous to that PR, the dust feerate was whatever -minrelaytxfee was set to."
ACKs for top commit:
LarryRuane:
ACK d6fc1d6a33
glozow:
ACK d6fc1d6a33
kouloumos:
ACK d6fc1d6a33
Tree-SHA512: 35ea2b2497dfb466395af5665bb217f7250aa7cab9dc43539a5658ab69a454e3623ff58fce7489fcc1105b37f8cb4840a93cec658c5df1de611732bc6439ccad
8cbd926a2c test: refactor: simplify p2p_permissions.py by using MiniWallet (Sebastian Falbesoner)
Pull request description:
This PR simplies the functional test p2p_permissions.py by using MiniWallet in order to avoid manual low-level tx creation. Also, rather than mining 100 blocks manually, the pre-mined chain of the test framework is used, which speeds up the test a little (~2-3 seconds faster on my machine).
ACKs for top commit:
MarcoFalke:
ACK 8cbd926a2c
Tree-SHA512: 36cbc2a0f6fb0251c8696cd017163ed30529690736bafd36e80b53007bd02b9030b68fe93b90dc50323526c8b7d8e0abd8f20890d46ff6015d5c632b64a08535
Since this test was changed to use MiniWallet instead of the Bitcoin
Core wallet (see commit d447ded6ba),
the setting doesn't have any effect and hence can be removed.
fa1bf4e705 test: Fix intermittent timeout in p2p_permissions.py (MarcoFalke)
Pull request description:
The sync is based on `bytesrecv_per_msg["verack"]`. However, the bytes are counted before processing the message, so they are not sufficient to ensure the connection is fully up.
ACKs for top commit:
mzumsande:
ACK fa1bf4e705
aureleoules:
ACK fa1bf4e705
Tree-SHA512: eb1ed537032c76a449b1ed5e42ff062e9b8b3c7e11fde2a5b8183ae0d6fbe31dba39e2c758836160cd8157d9ac5cc1f5d1916415861b8d711b7370c88f5e9790
f9ce0eadf4 For feebump, ignore abandoned descendant spends (John Moffett)
Pull request description:
Closes#26667
To be eligible for fee-bumping, a transaction must not have any of its outputs (eg - change) spent in other unconfirmed transactions in the wallet. This behavior is currently [enforced](9e229a542f/src/wallet/feebumper.cpp (L25-L28)) and [tested](9e229a542f/test/functional/wallet_bumpfee.py (L270-L286)).
However, this check shouldn't apply to spends in abandoned descendant transactions, as explained by #26667.
`CWallet::IsSpent` already carves out an exception for abandoned transactions, so we can just use that.
I've also added a new test to cover this case.
ACKs for top commit:
Sjors:
re-utACK f9ce0eadf4
achow101:
ACK f9ce0eadf4
furszy:
ACK f9ce0ead
Tree-SHA512: 19d957d1cf6747668bb114e27a305027bfca5a9bed2b1d9cc9e1b0bd4666486c7c4b60b045a7fe677eb9734d746f5de76390781fb1e9e0bceb4a46d20acd1749
c467cfffce test: add coverage for `purpose` arg in `listlabels` (brunoerg)
Pull request description:
This PR adds test coverage for `listlabels` command when specifying the `purpose` (send and receive).
dcdfd72861/src/wallet/rpc/addresses.cpp (L698-L704)
ACKs for top commit:
kristapsk:
ACK c467cfffce
Tree-SHA512: 7e7143c1264692f7b22952e7c70dbe9ed3f5dcd2e3b69962a47be9f9c21b3f4a9089ca87962fbc8ff9116e7d2dbeb7f36d6a132c9ac13724a255cfe1b32373a8
264f9ef17f [validation] return MempoolAcceptResult for every tx on PCKG_TX failure (glozow)
dae81e01e8 [refactor] rename variables in AcceptPackage for clarity (glozow)
da484bc738 [doc] release note effective-feerate and effective-includes RPC results (glozow)
5eab397b98 [validation] remove PackageMempoolAcceptResult::m_package_feerate (glozow)
601bac88cb [rpc] return effective-includes in testmempoolaccept and submitpackage (glozow)
1691eaa818 [rpc] return effective-feerate in testmempoolaccept and submitpackage (glozow)
d6c7b78ef2 [validation] return wtxids of other transactions whose fees were used (glozow)
1605886380 [validation] return effective feerate from mempool validation (glozow)
5d35b4a7de [test] package validation quits early due to non-policy, non-missing-inputs failure (glozow)
be2e4d94e5 [validation] when quitting early in AcceptPackage, set package_state and tx result (glozow)
Pull request description:
This PR fixes a bug and improves the mempool accept interface to return information more predictably.
Bug: In package validation, we first try the transactions individually (see doc/policy/packages.md for more explanation) and, if they all failed for missing inputs and policy-related (i.e. fee) reasons, we'll try package validation. Otherwise, we'll just "quit early" since, for example, if a transaction had an invalid signature, adding a child will not help make it valid. Currently, when we quit early, we're not setting the `package_state` to be invalid, so the caller might think it succeeded. Also, we're returning no results - it makes more sense to return the individual transaction failure. Thanks instagibbs for catching https://github.com/bitcoin/bitcoin/pull/25038#discussion_r1013293248!
Also, make the package results interface generally more useful/predictable:
- Always return the feerate at which a transaction was considered for `CheckFeeRate` in `MempoolAcceptResult::m_effective_feerate` when it was successful. This can replace the current `PackageMempoolAcceptResult::m_package_feerate`, which only sometimes exists.
- Always provide an entry for every transaction in `PackageMempoolAcceptResult::m_tx_results` when the error is `PCKG_TX`.
ACKs for top commit:
instagibbs:
reACK 264f9ef17f
achow101:
ACK 264f9ef17f
naumenkogs:
reACK 264f9ef17f
Tree-SHA512: ce7fd9927a80030317cc6157822596e85a540feff5dbf5eea7c62da2eb50c917cdddc9da1e2ff62cc18b98b27d360151811546bd9d498859679a04bbee090837
When an orphaned coinbase is reorged back into the main chain, any
unconfirmed ancestors should still be marked as abandoned due to the
original reorg that orphaned that coinbase.
65e78bda7c test: Invalid label name coverage (Aurèle Oulès)
552b51e682 refactor: Add sanity checks in LabelFromValue (Aurèle Oulès)
67e7ba8e1a rpc: Sanitize label name in various RPCs (Aurèle Oulès)
Pull request description:
The following RPCs did not sanitize the optional label name:
- importprivkey
- importaddress
- importpubkey
- importmulti
- importdescriptors
- listsinceblock
Thus is was possible to import an address with a label `*` which should not be possible.
The wildcard label is used for backwards compatibility in the `listtransactions` rpc.
I added test coverage for these RPCs.
ACKs for top commit:
ajtowns:
ACK 65e78bda7c
achow101:
ACK 65e78bda7c
furszy:
diff ACK 65e78bd
stickies-v:
re-ACK 65e78bda7c
theStack:
re-ACK 65e78bda7c
Tree-SHA512: ad99f2824d4cfae352166b76da4ca0069b7c2eccf81aaa0654be25bbb3c6e5d6b005d93960f3f4154155f80e12be2d0cebd5529922ae3d2a36ee4eed82440b31
This value creates an extremely confusing interface as its existence is
dependent upon implementation details (whether something was submitted
on its own, etc). MempoolAcceptResult::m_effective_feerate is much more
helpful, as it always exists for submitted transactions.
c6119f4788 tests: Use unique port for ZMQ tests (Andrew Chow)
Pull request description:
The ZMQ interface tests should use unique ports as we do for the p2p and rpc ports so that multiple instances of the test can be run at the same time.
Without this, the test may hang until killed, or fail.
ACKs for top commit:
MarcoFalke:
ACK c6119f4788
Tree-SHA512: 2ca3ed2f35e5a83d7ab83740674fed362a8d146dc751156cfe100133a591347cd1ac9d164046f1744d65451a57c52cb22d3bb2161105f421f8f655c4a2512c59
730e14a317 test: wallet: check that labels are migrated to watchonly wallet (Sebastian Falbesoner)
d5f4ae7fac wallet: fully migrate address book entries for watchonly/solvable wallets (Sebastian Falbesoner)
Pull request description:
Currently `migratewallet` migrates the address book (i.e. labels and purposes) for watchonly and solvable wallets only in RAM, but doesn't persist them on disk. Fix this by adding another loop for both of the special wallet types after which writes the corresponding NAME and PURPOSE entries to the database in a single batch. Also adds a corresponding test that checks if labels were migrated correctly for a watchonly wallet.
ACKs for top commit:
achow101:
ACK 730e14a317
furszy:
code ACK 730e14a3, left a non-blocking nit.
aureleoules:
ACK 730e14a317
Tree-SHA512: 159487e11e858924ef762e0190ccaea185bdff239e3d2280c8d63c4ac2649ec71714dc4d53dec644f03488f91c3b4bbbbf3434dad23bc0fcecb6657f353ea766
e6864fa157 contrib: remove builder keys (fanquake)
Pull request description:
This has been superseded by adding a builder-keys/ directory in
guix.sigs, where the presence of keys, and validity of signatures
is checked. Preventing issues like missing keys or invalid signatures.
New (or exisiting) Guix builders can add their key in the next PR
they open adding attestations.
Related to issues like #26566, #26563.
Also follows up with the comment here: https://github.com/bitcoin/bitcoin/pull/26565#issuecomment-1326053939.
ACKs for top commit:
hebasto:
ACK e6864fa157, modulo s/update/remove/ in the PR tittle.
Tree-SHA512: 095b4cf12ed0baeaf0ee7b8edcb3e2647e9c0f812e8fd63915ddb454f81dacc9c2d2b409de2773b7adb5ff643893d614d8aad1bc44c26da648e1bbbe19e11e05
21ad4e26ec test: add coverage for cross-chain wallet restore (Sebastian Falbesoner)
8c7222bda3 wallet: fix GUI crash on cross-chain legacy wallet restore (Sebastian Falbesoner)
Pull request description:
Restoring a wallet backup from another chain should result in a dedicated error message (we have _"Wallet files should not be reused across chains. Restart bitcoind with -walletcrosschain to override."_ for that). Unfortunately this is currently not the case for legacy wallet restores, as in the course of cleaning up the newly created wallet directory a `filesystem_error` exception is thrown due to the directory not being empty; the wallet database did indeed load successfully (otherwise we wouldn't know that the chain doesn't match) and hence BDB-related files and directories are already created in the wallet directory.
For bitcoind, this leads to a very confusing error message:
```
$ ./src/bitcoin-cli restorewallet test123 ~/.bitcoin/regtest/wallets/regtest_wallet/wallet.dat
error code: -1
error message: filesystem error: in remove: Directory not empty ["/home/thestack/.bitcoin/wallets/test123"]
```
Even worse, the GUI crashes in such a scenario:
```
libc++abi: terminating with uncaught exception of type std::__1::__fs::filesystem::filesystem_error: filesystem error: in remove: Directory not empty ["/home/thestack/.bitcoin/wallets/foobar"]
Abort trap (core dumped)
```
Fix this by simply deleting the whole folder via `fs::remove_all`. With this, the expected error message appears both for the `restorewallet` RPC call and in the GUI (as a message-box):
```
$ ./src/bitcoin-cli restorewallet test123 ~/.bitcoin/regtest/wallets/regtest_wallet/wallet.dat
error code: -4
error message:
Wallet loading failed. Wallet files should not be reused across chains. Restart bitcoind with -walletcrosschain to override.
```
ACKs for top commit:
achow101:
ACK 21ad4e26ec
aureleoules:
ACK 21ad4e26ec
furszy:
utACK 21ad4e26
Tree-SHA512: 313f6494c2fbe823bff9b975cb2d9410bb518977a1e59a5159ee9836bc012947fa50b56be0e41b1a2f50d9c0c7f4fddfdf4fbe479d8a59a6ee44bb389c804abc
f6eadaa413 Use same Python executable for subprocesses as for all-lint.py (Kristaps Kaupe)
Pull request description:
Before this all linters were ran by `/usr/bin/env python3`, no matter what was used to run `test/lint/all-lint.py`. This change allows to use non-default Python executable for `test/lint/all-lint.py` and then all subprocesses will also use same Python interpreter (for example, `python3.10 ./test/lint/all-lint.py`). See https://github.com/bitcoin/bitcoin/issues/26792#issuecomment-1369558866 as use case.
ACKs for top commit:
fanquake:
ACK f6eadaa413 - did not test
Tree-SHA512: 4da3b5581a0dd8ab9a6387829495019091a93a7ceaf2135d65d40a1983fd11a0b92b20891ef30d2a132abb0a690cd9b2f7eb5fcc38df06a340394ef449d640af
76dc547ee7 gui: create tx, launch error dialog if backend throws runtime_error (furszy)
f4d79477ff wallet: coin selection, add duplicated inputs checks (furszy)
0aa065b14e wallet: return accurate error messages from Coin Selection (furszy)
7e8340ab1a wallet: make SelectCoins flow return util::Result (furszy)
e5e147fe97 wallet: refactor eight consecutive 'AttemptSelection' calls into a loop (furszy)
Pull request description:
Work decoupled from #25806, which cleanup and improves the Coin Selection flow further.
Adding the capability to propagate specific error messages from the Coin Selection process to the user.
Instead of always returning the general "Insufficient funds" message which is not always accurate to what happened internally.
Letting us instruct the user how to proceed under certain circumstances.
The following error messages were added:
1) If the selection result exceeds the maximum transaction weight,
we now will return:
-> "The inputs size exceeds the maximum weight. Please try sending
a smaller amount or manually consolidating your wallet's UTXOs".
2) If the user pre-selected inputs and disallowed the automatic coin
selection process (no other inputs are allowed), we now will
return:
-> "The preselected coins total amount does not cover the transaction
target. Please allow other inputs to be automatically selected or include
more coins manually".
3) The double-counted preset inputs during Coin Selection error will now
throw an "internal bug detected" message instead of crashing the node.
The essence of this work comes from several comments:
1. https://github.com/bitcoin/bitcoin/pull/26560#discussion_r1037395665
2. https://github.com/bitcoin/bitcoin/pull/25729#discussion_r940619491
3. https://github.com/bitcoin/bitcoin/pull/25269#pullrequestreview-1135240825
4. https://github.com/bitcoin/bitcoin/issues/23144 (which is connected to #24845)
ACKs for top commit:
ishaanam:
crACK 76dc547ee7
achow101:
ACK 76dc547ee7
aureleoules:
ACK 76dc547ee7
theStack:
ACK 76dc547ee7🌇
Tree-SHA512: 9de30792d7a5849cae77747aa978e70390b66ee9d082779a56088a024f82e725b0af050e6603aece0ac8229f6d73bc471ba97b4ab69dc7eddf419f5f56ae89a5
04609284ad rpc: Improve error when wallet is already loaded (Aurèle Oulès)
Pull request description:
Currently, trying to load a descriptor (sqlite) wallet that is already loaded throws the following error:
> error code: -4
> error message:
> Wallet file verification failed. SQLiteDatabase: Unable to obtain an exclusive lock on the database, is it being used by another instance of Bitcoin Core?
I don't think it is very clear what it means for a user.
While a legacy wallet would throw:
> error code: -35
> error message:
> Wallet file verification failed. Refusing to load database. Data file '/home/user/.bitcoin/signet/wallets/test_wallet/wallet.dat' is already loaded.
This PR changes the error message for both types of wallet to:
> error code: -35
> error message:
> Wallet file verification failed. Wallet "test_wallet" is already loaded.
ACKs for top commit:
achow101:
ACK 04609284ad
hernanmarino:
ACK 0460928
theStack:
Tested ACK 04609284ad
Tree-SHA512: a8f3d5133bfaef7417a6c05d160910ea08f32ac62bfdf7f5ec305ff5b62e9113b55f385abab4d5a4ad711aabcb1eb7ef746eb41f841b196e8fb5393ab3ccc01e
and not the general "Insufficient funds" when the wallet
actually have funds.
Two new error messages:
1) If the selection result exceeds the maximum transaction weight,
we now will return: "The inputs size exceeds the maximum weight".
2) If the user preselected inputs and disallowed the automatic coin
selection process (no other inputs are allowed), we now will
return: "The preselected coins total amount does not cover the
transaction target".
b2aa9e8528 Add release note for MIN_STANDARD_TX_NONWITNESS_SIZE relaxation (Greg Sanders)
8c5b3646b5 Relax MIN_STANDARD_TX_NONWITNESS_SIZE to 65 non-witness bytes (Greg Sanders)
Pull request description:
Since the original fix was set to be a "reasonable" transaction to reduce allocations and the true motivation later revealed, it makes sense to relax this check to something more principled.
There are more exotic transaction patterns that could take advantage of a relaxed requirement, such as 1 input, 1 output OP_RETURN to burn a utxo to fees for CPFP purposes when change isn't practical.
Two changes could be accomplished:
1) Anything not 64 bytes could be allowed
2) Anything above 64 bytes could be allowed
In the Great Consensus Cleanup, suggestion (2)
was proposed as a consensus change, and is the simpler of the two suggestions. It would not allow an "empty" OP_RETURN but would reduce the required padding from 22 bytes to 5.
The functional test is also modified to test the actual case
we care about: 64 bytes
Related mailing list discussions here:
https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2022-October/020995.html
And a couple years earlier:
https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2020-May/017883.html
ACKs for top commit:
achow101:
reACK b2aa9e8528
glozow:
reACK b2aa9e8528
pablomartin4btc:
re-ACK b2aa9e8528
jonatack:
ACK b2aa9e8528 with some suggestions
Tree-SHA512: c1ec1af9ddcf31b2272209a4f1ee0c5607399f8172e5a1dfd4604cf98bfb933810dd9369a5917ad122add003327c9fcf6ee26995de3aca41d5c42dba527991ad
dc12f2e212 test: improve error msg on previous release tarball extraction failure (kdmukai)
7121fd8fa7 test: self-sign previous release binaries for arm64 macOS (kdmukai)
Pull request description:
## The Problem
If you run `test/get_previous_releases.py -b` on an M1 or M2 mac, you'll get an unsigned v23.0 binary in the arm64 tarball. macOS [sets stricter requirements on ARM binaries](https://news.ycombinator.com/item?id=26996578) so the unsigned arm64 binary is apparently completely unusable without being signed/notarized(?).
This means that any test that depends on a previous release (e.g. `wallet_backwards_compatibility.py`) will fail because the v23.0 node cannot launch:
```
TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "/Users/kdmukai/dev/bitcoin-core/test/functional/test_framework/test_framework.py", line 563, in start_nodes
node.wait_for_rpc_connection()
File "/Users/kdmukai/dev/bitcoin-core/test/functional/test_framework/test_node.py", line 231, in wait_for_rpc_connection
raise FailedToStartError(self._node_msg(
test_framework.test_node.FailedToStartError: [node 2] bitcoind exited with status -9 during initialization
```
This can also be confirmed by downloading bitcoin-23.0-arm64-apple-darwin.tar.gz (https://bitcoincore.org/bin/bitcoin-core-23.0/) and trying to run any of the binaries manually on an M1 or M2 mac.
## Solution in this PR
(UPDATED) Per @ hebasto, we can self-sign the arm64 binaries. This PR checks each binary in the previous release's "bin/" and verifies if the arm64 binary is signed. If not, attempt to self-sign and confirm success.
(note: an earlier version of this PR downloaded the x86_64 binary as a workaround but this approach has been discarded)
## Longer term solution
If possible, produce signed arm64 binaries in a future v23.x tarball?
Note that this same problem affects the new v24.0.1 arm64 tarball so perhaps a signed v24.x.x tarball would also be ideal?
That being said, this PR will check all current and future arm64 binaries and self-sign as needed, so perhaps we need not worry about pre-signing the tarball binaries. And I did test a version of `get_previous_releases.py` that includes the new v24.0.1 binaries and it successfully self-signed both v23.0 and v24.0.1, as expected.
## Further info:
Somewhat related to: https://github.com/bitcoin/bitcoin/issues/15774#issuecomment-1265164753
And @ fanquake noted on IRC that you can confirm which binaries are or are not signed via:
```
$ codesign -v -d bitcoin-qt
bitcoin-qt: code object is not signed at all
```
ACKs for top commit:
hebasto:
ACK dc12f2e212
Tree-SHA512: 644895f8e97f5ffb3c4754c1db2c48abd77fa100c2058e3c896af04806596fc2b9c807a3f3a2add5be53301ad40ca2b8171585bd254e691f6eb38714d938396b
31fdc54dba test: speed up wallet_fundrawtransaction.py and wallet_sendall.py (kdmukai)
Pull request description:
## Problem
`wallet_fundrawtransaction.py` and `wallet_sendall.py` are the two slowest functional tests *when running without a RAM disk*.
```
# M1 MacBook Pro timings
wallet_fundrawtransaction.py --descriptors | ✓ Passed | 55 s
wallet_fundrawtransaction.py --legacy-wallet | ✓ Passed | 381 s
wallet_sendall.py --descriptors | ✓ Passed | 43 s
wallet_sendall.py --legacy-wallet | ✓ Passed | 327 s
```
In each case, the majority of the time is spent iterating through 1500 to 1600 `getnewaddress()` calls. This is particularly slow in the `--legacy-wallet` runs.
see: https://github.com/bitcoin/bitcoin/blob/master/test/functional/wallet_fundrawtransaction.py#L986-L987
see: https://github.com/bitcoin/bitcoin/blob/master/test/functional/wallet_sendall.py#L324
## Solution
Pre-fill the keypool before iterating through those `getnewaddress()` calls.
With this change, the execution time drops to:
```
wallet_fundrawtransaction.py --descriptors | ✓ Passed | 52 s # -3s diff
wallet_fundrawtransaction.py --legacy-wallet | ✓ Passed | 291 s # -90s diff
wallet_sendall.py --descriptors | ✓ Passed | 27 s # -16s diff
wallet_sendall.py --legacy-wallet | ✓ Passed | 228 s # -99s diff
```
---
Tagging @ Sjors as he had encouraged me to take a look at speeding up the tests.
ACKs for top commit:
achow101:
ACK 31fdc54dba
Tree-SHA512: e8dd89323551779832a407d068977c827c09dff55c1079d3c19aab39fcce6957df22b1da797ed7aa3bc2f6dd22fdf9e6f5e1a9a0200fdb16ed6042fc5f6dd992
a4defcdd57 test, lint: add `crypted` to `ignore-words` (brunoerg)
Pull request description:
Fixes#26719
"Crypted" is used in some comments at `walletload_tests` because it refers to `DBKeys::CRYPTED_KEY`, it's not necessary
a mistake.
Obs: I can change the approach (changing `walletload_tests` comments to use `encrypted` word instead of adding it to the `ignore_words`) if reviewers think it makes more sense.
ACKs for top commit:
achow101:
ACK a4defcdd57
Tree-SHA512: 49f38eed30ffb0fda4e792566591c3455629379619eb9a5c4240c5b00e14cd27ba1faa36337192233752e642f0998373b86fcb8ca586508bbf15900d68b17950
This has been superseded by adding a builder-keys/ directory in
guix.sigs, where the presence of keys, and validity of signatures
is checked. Preventing issues like missing keys or invalid signatures.
New (or exisiting) Guix builders can add their key in the next PR
they open adding attestations.
Since the original fix was set to be a "reasonable" transaction
to reduce allocations and the true motivation later revealed,
it makes sense to relax this check to something more principled.
There are more exotic transaction patterns that could take advantage
of a relaxed requirement, such as 1 input, 1 output OP_RETURN to burn
a utxo to fees for CPFP purposes when change isn't practical.
Two changes could be accomplished:
1) Anything not 64 bytes could be allowed
2) Anything above 64 bytes could be allowed
In the Great Consensus Cleanup, suggestion (2) was the route taken.
It would not allow an "empty" OP_RETURN
but would reduce the required padding from 22 bytes to 5.
The functional test is also modified to test the actual case
we care about: 64 bytes
ec63a4892e test: call `keypoolrefill` with private keys disabled should throw an error (brunoerg)
Pull request description:
This PR adds test coverage for the following error:
cb32328d1b/src/wallet/rpc/addresses.cpp (L332-L334)
ACKs for top commit:
aureleoules:
ACK ec63a4892e
Tree-SHA512: b5deda8981ff472f290e6e16c8723a58e02cbe099afd1f672c099f4add0a1d9b192b11a2c3f0e11b96794671f6b9efa75812b7a174248d7c58d7fd7d3310e7b9
1647a11f39 tests: Reorder longer running tests in test_runner (Andrew Chow)
ff6c9fe027 tests: Whitelist test p2p connection in rpc_packages (Andrew Chow)
8c20796aac tests: Use waitfornewblock for work queue test in interface_rpc (Andrew Chow)
6c872d5e65 tests: Initialize sigops draining script with bytes in feature_taproot (Andrew Chow)
544cbf776c tests: Use batched RPC in feature_fee_estimation (Andrew Chow)
4ad7272f8b tests: reduce number of generated blocks for wallet_import_rescan (Andrew Chow)
Pull request description:
When configured with `--enable-debug`, many tests become dramatically slower. These slow downs are particularly noticed in tests that generate a lot of blocks in separate calls, make a lot of RPC calls, or send a lot of data from the test framework's P2P connection. This PR aims to improve the runtime of some of the slower tests and improve the overall runtime of the test runner. This has improved the runtime of the test runner from ~400s to ~140s on my computer.
The slowest test by far was `wallet_import_rescan.py`. This was taking ~320s. Most of that time was spent waiting for blocks to be mined and then synced to the other nodes. It was generating a new block for every new transaction it was creating in a setup loop. However it is not necessary to have one tx per block. By mining a block only every 10 txs, the runtime is improved to ~61s.
The second slowest test was `feature_fee_estimation.py`. This test spends most of its time waiting for RPCs to respond. I was able to improve its runtime by batching RPC requests. This has improved the runtime from ~201s to ~140s.
In `feature_taproot.py`, the test was constructing a Python `CScript` using a very large list of `OP_CHECKSIG`s. The constructor for the Python implementation of `CScript` was iterating this list in order to create a `bytes` from it even though a `bytes` could be created from it without iterating. By making the `bytes` before passing it into the constructor, we are able to improve this test's runtime from ~131s to ~106s.
Although `interface_rpc.py` was not typically a slow test, I found that it would occasionally have a super long runtime. It typically takes ~7s, but I have observed it taking >400s to run on occasion. This longer runtime occurs more often when `--enable-debug`. This long runtime was caused by the "exceeding work queue" test which is really just trying to trigger a race condition. In this test, it would create a few threads and try an RPC in a loop in the hopes that eventually one of the RPCs would be added to the work queue while another was processing. It used `getrpcinfo` for this, but this function is fairly fast. I believe what was happening was that with `--enable-debug`, all of the code for receiving the RPC would often take longer to run than the RPC itself, so the majority of the requests would succeed, until we got lucky after 10's of thousands of requests. By changing this to use a slow RPC, the race condition can be triggered more reliably, and much sooner as well. I've used `waitfornewblock` with a 500ms timeout. This improves the runtime to ~3s consistently.
The last test I've changed was `rpc_packages.py`. This test was one of the higher runtime variability tests. The main source of this variation appears to be waiting for the test node to relay a transaction to the test framework's P2P connection. By whitelisting that peer, the variability is reduced to nearly 0.
Lastly, I've reordered the tests in `test_runner.py` to account for the slower runtimes when configured with `--enable-debug`. Some of the slow tests I've looked at were listed as being fast which was causing overall `test_runner.py` runtime to be extended. This change makes the test runner's runtime be bounded by the slowest test (currently `feature_fee_estimation.py` with my usual config (`-j 60`).
ACKs for top commit:
willcl-ark:
ACK 1647a11
Tree-SHA512: 529e0da4bc51f12c78a40d6d70b3a492b97723c96a3526148c46943d923c118737b32d2aec23d246392e50ab48013891ef19fe6205bf538b61b70d4f16a203eb
564b580bf0 test: Introduce MIN_BLOCKS_TO_KEEP constant (Aurèle Oulès)
71d9a7c03b test: Wallet imports on pruned nodes (Aurèle Oulès)
e6906fcf9e rpc: Enable wallet import on pruned nodes (Aurèle Oulès)
Pull request description:
Reopens#16037
I have rebased the PR, addressed the comments of the original PR and added a functional test.
> Before this change importwallet fails if any block is pruned. This PR makes it possible to importwallet if all required blocks aren't pruned. This is possible because the dump format includes key timestamps.
For reviewers:
`python test/functional/wallet_pruning.py --nocleanup` will generate a large blockchain (~700MB) that can be used to manually test wallet imports on a pruned node. Node0 is not pruned, while node1 is.
ACKs for top commit:
kouloumos:
ACK 564b580bf0
achow101:
reACK 564b580bf0
furszy:
ACK 564b580
w0xlt:
ACK 564b580bf0
Tree-SHA512: b345a6c455fcb6581cdaa5f7a55d79e763a55cb08c81d66be5b12794985d79cd51b9b39bdcd0f7ba0a2a2643e9b2ddc49310ff03d16b430df2f74e990800eabf
To be eligible for fee-bumping, a transaction must not have any
of its outputs (eg - change) spent in other unconfirmed transactions
in the wallet. However, this check should not apply to abandoned
transactions.
A new test case is added to cover this case.
fa34e5f3d3 test: Avoid intermittent timeout in feature_assumevalid.py (MarcoFalke)
Pull request description:
Currently the test will spin up p2p connections in the beginning, then announce the headers to all nodes, but only send the blocks sequentially. This takes a long time, so when getting to the last node, it will have already timed out, while node1 is busy eating blocks. Example:
```
node2 2022-12-06T19:31:35.419291Z [msghand] [net_processing.cpp:5783] [SendMessages] [net] Requesting block 2cfdb317b3b901f79e2d4f96339d0c0dffd8ef2685d324f62ab0e2fa3402430e (1) peer=0
node2 2022-12-06T19:31:35.424784Z [msghand] [net.cpp:2776] [PushMessage] [net] sending getdata (577 bytes) peer=0
[...]
node2 2022-12-06T19:41:35.423257Z [msghand] [net_processing.cpp:5729] [SendMessages] Timeout downloading block 2cfdb317b3b901f79e2d4f96339d0c0dffd8ef2685d324f62ab0e2fa3402430e from peer=0, disconnecting
node1 2022-12-06T19:41:35.438706Z [msghand] [net_processing.cpp:5783] [SendMessages] [net] Requesting block 6575919043306ed309014d0bd79814b4fab8afaa281e026d8cc3a1c4c2336fbc (1748) peer=0
node2 2022-12-06T19:41:35.521253Z [net] [net.cpp:573] [CloseSocketDisconnect] [net] disconnecting peer=0
node2 2022-12-06T19:41:35.630417Z [net] [net_processing.cpp:1532] [FinalizeNode] [net] Cleared nodestate for peer=0
```
Fix this by only spinning up the p2p connection right before they are needed.
ACKs for top commit:
jamesob:
ACK fa34e5f3d3 ([`jamesob/ackr/26651.1.MarcoFalke.test_avoid_intermittent`](https://github.com/jamesob/bitcoin/tree/ackr/26651.1.MarcoFalke.test_avoid_intermittent))
Tree-SHA512: 7a1b114c07dfa30237c8cd8637dd6646c5c2dc2530c9de61db231738fddc800b620c31dc664237e33d35e951cf161f015fda593162efc9d85c5f68c6e37217d4
bcb7123406 test: add add_wallet_options to TestShell (josibake)
Pull request description:
following 555519d082, `TestShell` now always runs with `-disablewallet`. simple fix is to add `add_wallet_options` to `add_options`; looks like testshell was overlooked when adding in the `add_wallet_options` call to the functional tests in #26480
ACKs for top commit:
amitiuttarwar:
ACK bcb7123406
Tree-SHA512: db554a8b3c8ff5bd10cab9592b316035a92f86a0a0ae8ff914de9687bbbb6fc2235bdf82c4cd40e4071782f8b6edf91aad372e82ed3b826c9d8ae39dbe3dbf57
8c3ff7d52a test: Suggested cleanups for rpc_namedparams test (Ryan Ofsky)
d1ca563825 bitcoin-cli: Make it an error to specify the "args" parameter two different ways (Ryan Ofsky)
6bd1d20b8c rpc: Make it an error server-side to specify same named parameter multiple times (Ryan Ofsky)
e2c3b18e67 test: Add RPC tests for same named parameter specified more than once (Ryan Ofsky)
Pull request description:
Make the JSON-RPC server reject requests with the same named parameter specified multiple times, instead of silently overwriting earlier parameter values with later ones.
Generally JSON keys are supposed to unique, and their order isn't supposed to be significant, so having the server silently discard duplicate keys is error-prone. Most likely if an RPC client is sending a request with duplicate keys it means something is wrong with the request and there should be an error.
After this change, named parameters are still allowed to specified multiple times on the `bitcoin-cli` command line, since `bitcoin-cli` automatically replaces earlier values with later values before sending the JSON-RPC request. This makes sense, since it's not unusual for the order of command line options to be significant or for later command line options to override earlier ones.
ACKs for top commit:
MarcoFalke:
review ACK 8c3ff7d52a 🗂
kristapsk:
ACK 8c3ff7d52a
stickies-v:
ACK 8c3ff7d52
Tree-SHA512: 2d1357dcc2c171da287aeefc7b333ba4e67babfb64fc14d7fa0940256e18010a2a65054f3bf7fa1571b144d2de8b82d53076111b5f97ba29320cfe84b6ed986f
The logest running tests should be at the front of the list in
test_runner.py. Since compiling with --enable-debug can have a
significant effect on test runtime, the order is based on the runtime
with that option configured.
test_submit_child_with_parents creates a p2p connection which waits for
the node to announce transactions to it. By whitelisting this
connection, we can reduce the amount of time spent waiting for this
announcement which improves the test runtime and runtime variance.
The work queue exceeded test in interface_rpc.py would repeatedly call
an RPC until the error was achieved. However hitting this error is
dependent on the processing speed of the computer and the optimization
level of the binary. Configurations that result in slower processing
would result in the RPC used being processed before the error could be
hit, resulting the test's runtime having a high variance.
Switching the RPC to waitfornewblock allows it to run in a much more
consistent time that is still fairly fast. waitfornewblock forces
the RPC server to allocate a thread and wait, occupying a spot in the
work queue. This is perfect for this test because the slower the RPC,
the more likely we will achieve the race condition necessary to pass the
test. Using a timeout of 500 ms appears to work reliably without causing
the test to take too long.
The sigops draining script in feature_taproot's block_submit was
initialized with a list that would end up always being iterated by
CScript's constructor. Since this list is very large, a lot of time
would be wasted. By creating and passing a bytes object initialized from
that list, we can avoid this iteration and dramatically improve the
runtime of feature_taproot.
feature_fee_estimation has a lot of loops that hit the RPC many times in
succession in order to setup scenarios. Using batched requests for these
can reduce the test's runtime without effecting the test's behavior.
Generating blocks is slow, especially when --enable-debug. There is no
need to generate a new block for each transaction, so avoid doing that
to improve this test's runtime.
fabb24cbef test: Use last release in compatibility tests (MarcoFalke)
Pull request description:
In compatibility tests it makes sense to always use the last release without the new feature, as it is likely more in use than any even older previous release.
ACKs for top commit:
Sjors:
utACK fabb24c
Tree-SHA512: beb854f4d28ba313282e1e0303abb0e09377828b138bde5a3e209337210b6b4c24855ab90a68f8789387001e4ca33b15cc37dbc9b7809929f4e7d1b69833a527
4e362c2b72 doc: add release note for 25934 (brunoerg)
fe488b4c4b test: add coverage for `label` in `listsinceblock` (brunoerg)
722e9a418d wallet, rpc: add `label` to `listsinceblock` (brunoerg)
852891ff98 refactor, wallet: use optional for `label` in `ListTransactions` (brunoerg)
Pull request description:
This PR adds `label` parameter to `listsinceblock` to be able to fetch all incoming transactions having the specified label since a specific block.
It's possible to use it in `listtransactions`, however, it's only possible to set the number of transactions to return, not a specific block to fetch from. `getreceivedbylabel` only returns the total amount received, not the txs info. `listreceivedbylabel` doesn't list all the informations about the transactions and it's not possible to fetch since a block.
ACKs for top commit:
achow101:
ACK 4e362c2b72
w0xlt:
ACK 4e362c2b72
aureleoules:
ACK 4e362c2b72
Tree-SHA512: fbde5db8cebf7a27804154fa61997b5155ad512e978cebb78c17acab9efcb624ea5f39d649899d12e5e675f80d4d0064cae8132b864de0d93a8d1e6fbcb9a737
6fb102c9f3 test: Changed small_txpuzzle_randfee to return the virtual size instead of the transaction hex for feerate calculation. (Randall Naar)
Pull request description:
The fee rates used in feature_fee_estimation.py are calculated using the raw transaction size instead of the virtual transaction size (which is used in 'CBlockPolicyEstimator::processBlockTx' and 'CBlockPolicyEstimator::processBlock'). This leads to inconsistencies as the fee rates used in check_raw_estimates are incorrect and can cause assertions to fail.
refs #25179
ACKs for top commit:
MarcoFalke:
ACK 6fb102c9f3
Tree-SHA512: b2bca85fffa605aeb17574f050736d4556506d782dd7fd969e165e48e108fd95ef4c4e2abbae83cce05ca777a00f4459cabfa0932694fa8bb93ddfba09d84357
d885bb2f6e test: Test exclusion of OP_RETURN from getblockstats (Fabian Jahr)
ba9d288b24 test: Fix getblockstats test data generator (Fabian Jahr)
2ca5a496c2 rpc: Improve getblockstats (Fabian Jahr)
cb94db119f validation, index: Add unspendable coinbase helper functions (Fabian Jahr)
Pull request description:
Fixes#19885
The genesis block does not have undo data saved to disk so the RPC errored because of that.
ACKs for top commit:
achow101:
ACK d885bb2f6e
aureleoules:
ACK d885bb2f6e
stickies-v:
ACK d885bb2f6
Tree-SHA512: f37bda736ed605b7a41a81eeb4bfbb5d2b8518f847819e5d6a090548a61caf1455623e15165d72589ab3f4478252b00e7b624f9313ad6708cac06dd5edb62e9a
3198e4239e test: check that loading descriptor wallet with legacy entries throws error (Sebastian Falbesoner)
349ed2a0ee wallet: throw error if legacy entries are present on loading descriptor wallets (Sebastian Falbesoner)
Pull request description:
Loading a descriptor wallet currently leads to a segfault if a legacy key type entry is present that can be deserialized successfully and needs SPKman-interaction. To reproduce with a "cscript" entry (see second commit for details):
```
$ ./src/bitcoin-cli createwallet crashme
$ ./src/bitcoin-cli unloadwallet crashme
$ sqlite3 ~/.bitcoin/wallets/crashme/wallet.dat
SQLite version 3.38.2 2022-03-26 13:51:10
Enter ".help" for usage hints.
sqlite> INSERT INTO main VALUES(x'07637363726970740000000000000000000000000000000000000000', x'00');
$ ./src/bitcoin-cli loadwallet crashme
--- bitcoind output: ---
2022-11-06T13:51:01Z Using SQLite Version 3.38.2
2022-11-06T13:51:01Z Using wallet /home/honey/.bitcoin/wallets/crashme
2022-11-06T13:51:01Z init message: Loading wallet…
2022-11-06T13:51:01Z [crashme] Wallet file version = 10500, last client version = 249900
Segmentation fault (core dumped)
```
Background: In the wallet key-value-loading routine, most legacy type entries require a `LegacyScriptPubKeyMan` instance after successful deserialization. On a descriptor wallet, creating that (via method `GetOrCreateLegacyScriptPubKeyMan`) fails and then leads to a null-pointer dereference crash. E.g. for CSCRIPT: 50422b770a/src/wallet/walletdb.cpp (L589-L594)
~~This PR fixes this by simply ignoring legacy entries if the wallet flags indicate that we have a descriptor wallet. The second commits adds a regression test to the descriptor wallet's functional test (fortunately Python includes sqlite3 support in the standard library).~~
~~Probably it would be even better to throw a warning to the user if unexpected legacy entries are found in descriptor wallets, but I think as a first mitigation everything is obvisouly better than crashing. As far as I'm aware, descriptor wallets created/migrated by Bitcoin Core should never end up in a state containing legacy type entries though.~~
This PR fixes this by throwing an error if legacy entries are found in descriptor wallets on loading.
ACKs for top commit:
achow101:
ACK 3198e4239e
aureleoules:
ACK 3198e4239e
Tree-SHA512: ee43da3f61248e0fde55d9a705869202cb83df678ebf4816f0e77263f0beac0d7bae9490465d1753159efb093ee37182931d76b2e2b6e8c6f8761285700ace1c
fa43f60a0c test: Run mempool_compatibility.py with MiniWallet (MarcoFalke)
Pull request description:
By using the already existing miniwallet, the test can be run even when no wallet is compiled.
ACKs for top commit:
glozow:
ACK fa43f60a0c
achow101:
ACK fa43f60a0c
Tree-SHA512: 6877b3f2f364663f04c28ab9f3d69780de6d1b77cc862379bba8c8242bbcfb0d26eb84c56cf721141407c393f1f3b49f667ae4fb32b3566108d71250e8b5d7bc
7362f8e5e2 refactor: make CoinsResult total amounts members private (furszy)
3282fad599 wallet: add assert to SelectionResult::Merge for safety (S3RK)
c4e3b7d6a1 wallet: SelectCoins, return early if wallet's UTXOs cannot cover the target (furszy)
cac2725fd0 test: bugfix, coinselector_test, use 'CoinsResult::Erase/Add' instead of direct member access (furszy)
cf79384697 test: Coin Selection, duplicated preset inputs selection (furszy)
341ba7ffd8 test: wallet, coverage for CoinsResult::Erase function (furszy)
f930aefff9 wallet: bugfix, 'CoinsResult::Erase' is erasing only one output of the set (furszy)
Pull request description:
This comes with #26559.
Solving few bugs inside the wallet's transaction creation
process and adding test coverage for them.
Plus, making use of the `CoinsResult::total_amount` cached value
inside the Coin Selection process to return early if we don't have
enough funds to cover the target amount.
### Bugs
1) The `CoinsResult::Erase` method removes only one
output from the available coins vector (there is a [loop break](c1061be14a/src/wallet/spend.cpp (L112))
that should have never been there) and not all the preset inputs.
Which on master is not a problem, because since [#25685](https://github.com/bitcoin/bitcoin/pull/25685)
we are no longer using the method. But, it's a bug on v24
(check [#26559](https://github.com/bitcoin/bitcoin/pull/26559)).
This method it's being fixed and not removed because I'm later using it to solve
another bug inside this PR.
2) As we update the total cached amount of the `CoinsResult` object inside
`AvailableCoins` and we don't use such function inside the coin selection
tests (we manually load up the `CoinsResult` object), there is a discrepancy
between the outputs that we add/erase and the total amount cached value.
### Improvements
* This makes use of the `CoinsResult` total amount field to early return
with an "Insufficient funds" error inside Coin Selection if the tx target
amount is greater than the sum of all the wallet available coins plus the
preset inputs amounts (we don't need to perform the entire coin selection
process if we already know that there aren't enough funds inside our wallet).
### Test Coverage
1) Adds test coverage for the duplicated preset input selection bug that we have in v24.
Where the wallet invalidly selects the preset inputs twice during the Coin Selection
process. Which ends up with a "good" Coin Selection result that does not cover the
total tx target amount. Which, alone, crashes the wallet due an insane fee.
But.. to make it worst, adding the subtract fee from output functionality
to this mix ends up with the wallet by-passing the "insane" fee assertion,
decreasing the output amount to fulfill the insane fee, and.. sadly,
broadcasting the tx to the network.
2) Adds test coverage for the `CoinsResult::Erase` method.
------------------------------------
TO DO:
* [ ] Update [#26559 ](https://github.com/bitcoin/bitcoin/pull/26559) description.
ACKs for top commit:
achow101:
ACK 7362f8e5e2
glozow:
ACK 7362f8e5e2, I assume there will be a followup PR to add coin selection sanity checks and we can discuss the best way to do that there.
josibake:
ACK [7362f8e](7362f8e5e2)
Tree-SHA512: 37a6828ea10d8d36c8d5873ceede7c8bef72ae4c34bef21721fa9dad83ad6dba93711c3170a26ab6e05bdbc267bb17433da08ccb83b82956d05fb16090328cba
0b78110f73 test: Move tx creation to create_self_transfer_multi (kouloumos)
Pull request description:
Two birds with one stone: replacement of https://github.com/bitcoin/bitcoin/pull/26278 with simplification of the MiniWallet's transaction creation logic.
Currently the MiniWallet creates simple txns (1 input, 1 output) with `create_self_transfer`. https://github.com/bitcoin/bitcoin/pull/24637 introduced `create_self_transfer_multi` **which uses** `create_self_transfer` to create a "transaction template" which then adjusts (copy and mutate inputs and outputs) in order to create more complex multi-input multi-output transactions.
This can more easily lead to issues such as https://github.com/bitcoin/bitcoin/pull/26278 and is more of a maintenance burden.
This PR simplifies the logic by going the other way around. Now `create_self_transfer` **uses** `create_self_transfer_multi`.
The transaction creation logic has been moved to `create_self_transfer_multi` which is being called by `create_self_transfer` to construct the simple case of 1 input 1 output transaction.
ACKs for top commit:
MarcoFalke:
ACK 0b78110f73👒
Tree-SHA512: 147e577ed5444bee57865bd375b37c9b49d6539e9875c30c2667e70fcba27fe80bcb4552a4e6efb42760d34b40d5dad826883b778eaeefe29425ec081787b4bd
cb44c5923a test: Add sendall test for min-fee setting (Aurèle Oulès)
Pull request description:
While experimenting with mutation testing it appeared that the minimum fee-rate check was not tested for the `sendall` RPC.
https://bcm-ui.aureleoules.com/mutations/3581479318544ea6b97f788cec6e6ef1
ACKs for top commit:
0xB10C:
ACK cb44c5923a
ishaanam:
ACK cb44c5923a
stickies-v:
re-ACK [cb44c59](cb44c5923a)
Tree-SHA512: 31978436e1f01cc6abf44addc62b6887e65611e9a7ae7dc72e6a73cdfdb3a6a4f0a6c53043b47ecd1b10fc902385a172921e68818a7f5061c96e5e1ef5280b48
MarcoFalke reported the case of positional arguments silently overwriting the
named "args" parameter in bitcoin-cli
https://github.com/bitcoin/bitcoin/pull/19762#discussion_r1035761471 and this
behavior is confusing and was not intended when support for "args" parameters
was added to bitcoin-cli in #19762.
Instead of letting one "args" value overwrite the other in the client, just
pass the values to the server verbatim, and let the error be handled server
side.
Current behavior isn't ideal and will be changed in upcoming commits, but it's
useful to have test coverage regardless.
MarcoFalke reported the case of bitcoin-cli positional arguments overwriting
the named "args" parameter in
https://github.com/bitcoin/bitcoin/pull/19762#discussion_r1035761471
This exercises the bug inside CoinsResult::Erase that
ends up on (1) a wallet crash or (2) a created and
broadcasted tx that contains a reduced recipient's amount.
This is covered by making the wallet selects the preset
inputs twice during the coin selection process.
Making the wallet think that the selection process result covers
the entire tx target when it does not. It's actually creating
a tx that sends more coins than what inputs are covering for.
Which, combined with the SFFO option, makes the wallet
incorrectly reduce the recipient's amount by the difference
between the original target and the wrongly counted inputs.
Which means, a created and relayed tx sending less coins to
the destination than what the user inputted.
8f2dac5409 [test] Add p2p_tx_privacy.py (dergoegge)
ce63fca13e [net processing] Assume that TxRelay::m_tx_inventory_to_send is empty pre-verack (dergoegge)
845e3a34c4 [net processing] Ensure transaction announcements are only queued for fully connected peers (dergoegge)
Pull request description:
`TxRelay::m_next_inv_send_time` is initialized to 0, which means that any txids in `TxRelay::m_tx_inventory_to_send` will be announced on the first call to `PeerManagerImpl::SendMessages` for a fully connected peer (i.e. it completed the version handshake).
Prior to #21160, `TxRelay::m_tx_inventory_to_send` was guaranteed to be empty on the first `SendMessages` call, as transaction announcements were only queued for fully connected peers. #21160 replaced a `CConnman::ForEachNode` call with a loop over `PeerManagerImpl::m_peer_map`, in which the txid for a transaction to be relayed is added to `TxRelay::m_tx_inventory_to_send` for all peers. Even for those peers that have not completed the version handshake. Prior to the PR this was not the case as `ForEachNode` has a "fully connected check" before calling a function for each node.
ACKs for top commit:
MarcoFalke:
ACK 8f2dac5409🔝
jnewbery:
utACK 8f2dac5409
Tree-SHA512: e9eaccf7e00633ee0806fff1068b0e413a69a5e389d96c9659f68079915a6381ad5040c61f716cfcde77931d1b563b1049da97a232a95c6cd8355bd3d13404b9
fa15c671f7 test: Remove unused blocktools imports from wallet_bumpfee (MarcoFalke)
Pull request description:
Seems bloaty and confusing to use "tools" when a single RPC can already achieve the same.
ACKs for top commit:
theStack:
ACK fa15c671f7
Tree-SHA512: 87f9c31bbb286fee5e479ae54a1f9131f4d4294d665a985df8b14a0cc837a2a2e145ccd3660612768d88cfa0827a3eef392f85519b6cb7df365ba9fadafb0a41
dbed28968a test: refactor: eliminate genesis block timestamp magic numbers (Sebastian Falbesoner)
Pull request description:
This tiny PR replaces all occurences of the regtest/testnet genesis block timestamp (found via `git grep 1296688602`) with the constant `TIME_GENESIS_BLOCK` to increase the readability.
ACKs for top commit:
aureleoules:
ACK dbed28968a
Tree-SHA512: be39d5c2631ad20eb775c2a077b1b1f056a1a4930aa44e6fdec73b974fd4bdf8da0103a3a38e3514b68fcf6a6316e007a371c523da5076a315545c9bf3091aee
150340aeac test: remove unneeded extra_args code (josibake)
989a52e0a5 test: add extra_args to BTF class (josibake)
Pull request description:
## problem
If you try to add `extra_args` when using `TestShell`, you will get the following error:
```python
>>> import sys
>>>
>>> sys.path.insert(0, "/home/josibake/bitcoin/test/functional")
>>>
>>> from test_framework.test_shell import TestShell
>>> test = TestShell().setup(num_nodes=2, extra_args=[[],['-fallbackfee=0.0002']])
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/home/josibake/bitcoin/test/functional/test_framework/test_shell.py", line 41, in setup
raise KeyError(key + " not a valid parameter key!")
KeyError: 'extra_args not a valid parameter key!'
>>>
```
## solution
add `self.extra_args = None` so that `extra_args` is recognized as a valid parameter to be passed to `BitcoinTestFramework`
```python
>>> import sys
>>>
>>> sys.path.insert(0, "/home/josibake/bitcoin/test/functional")
>>>
>>> from test_framework.test_shell import TestShell
>>> test = TestShell().setup(num_nodes=2, extra_args=[[],['-fallbackfee=0.0002']])
2022-12-01T11:23:23.765000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_sbwthbb_
```
ACKs for top commit:
willcl-ark:
re-ACK 150340aeac
Tree-SHA512: e6fa2a780a8f2d3472c322e8cdb00ec35cb220c3b4d6ca02291eb8b41c0d8676a635fbc79c6be80e3bb71d700a2501a4b73f762478f533ae453d492d449307bb
5e65a216d1 wallet: Explicitly say migratewallet on encrypted wallets is unsupported (Andrew Chow)
88afc73ae0 tests: Test for migrating encrypted wallets (Andrew Chow)
86ef7b3c7b wallet: Avoid null pointer deref when cleaning up migratewallet (Andrew Chow)
Pull request description:
When `migratewallet` fails, we do an automatic cleanup in order to reset everything so that the user does not experience any interruptions. However, this apparently has a segfault in it, caused by the the pointers to the watchonly and solvables wallets being nullptr. If those wallets are not created (either not needed, or failed early on), we will accidentally attempt to dereference these nullptrs, which causes a segfault.
This failure can be easily reached by trying to migrate an encrypted wallet. Currently, we can't migrate encrypted wallets because of how we unload wallets before migrating, and therefore forget the encryption key if the wallet was unlocked. So any encrypted wallets will fail, entering the cleanup, and because watchonly and solvables wallets don't exist yet, the segfault is reached.
This PR fixes this by not putting those nullptrs in a place that we will end up dereferencing them later. It also adds a test that uses the encrypted wallet issue.
ACKs for top commit:
S3RK:
reACK 5e65a216d1
stickies-v:
ACK [5e65a21](5e65a216d1)
furszy:
diff ACK 5e65a21
Tree-SHA512: f75643797220d4232ad3ab8cb4b46d0f3667f00486e910ca748c9b6d174d446968f1ec4dd7f907da1be9566088849da7edcd8cd8f12de671c3241b513deb8e80
46339d29b1 test, refactor: Reorder sendtxrcncl tests for better readability (Gleb Naumenko)
14263c13f1 p2p, refactor: Extend logs for unexpected sendtxrcncl (Gleb Naumenko)
87493e112e p2p, test, refactor: Minor code improvements (Gleb Naumenko)
00c5dec818 p2p: Clarify sendtxrcncl policies (Gleb Naumenko)
ac6ee5ba21 test: Expand unit and functional tests for txreconciliation (Gleb Naumenko)
bc84e24a4f p2p, refactor: Switch to enum class for ReconciliationRegisterResult (Gleb Naumenko)
a60f729e29 p2p: Drop roles from sendtxrcncl (Gleb Naumenko)
6772cbf69c tests: stabilize sendtxrcncl test (Gleb Naumenko)
Pull request description:
Non-trivial changes include:
- Getting rid of roles in `sendtxrcncl` message (summarized in the [BIP PR](https://github.com/bitcoin/bips/pull/1376));
- Disconnect the peer if it send `sendtxrcncl` although we are in `blocksonly` and notified the peer with `fRelay=0`;
- Don't send `sendtxrcncl` to feeler connections.
ACKs for top commit:
vasild:
ACK 46339d29b1
ariard:
ACK 46339d2
mzumsande:
Code Review ACK 46339d29b1
Tree-SHA512: b5cc6934b4670c12b7dbb3189e739ef747ee542ec56678bf4e4355bfb481b746d32363c173635685b71969b3fe4bd52b1c8ebd3ea3b35c82044bba69220f6417
fadb8696dd test: Set wallet type in test_runner when only one type is allowed (MarcoFalke)
Pull request description:
Currently devs are free to set or not set the wallet type in the test_runner when only one type is allowed to be set.
This is inconsistent and causes review comments such as:
* https://github.com/bitcoin/bitcoin/pull/24865#discussion_r1009752111
ACKs for top commit:
achow101:
ACK fadb8696dd
Tree-SHA512: 1ca0946df07b5bf6778fea957d74393757781c324d554fec2f7d03bf1915033e644d9a4c3d77e0b24090ab593d7ed3cb3c9169666bc39fff423706fceaa1af80
Due to an oversight, we cannot currently migrate encrypted wallets,
regardless of whether they are unlocked. Migrating such wallets will
trigger an error, and result in the cleanup being run. This conveniently
allows us to check some parts of the cleanup code.
d8b12a75db rpc: Allow named and positional arguments to be used together (Ryan Ofsky)
Pull request description:
It's nice to be able to use named options and positional arguments together.
Most shell tools accept both, and python functions combine options and arguments allowing them to be passed with even more flexibility. This change adds support for python's approach so as a motivating example:
```sh
bitcoin-cli -named createwallet wallet_name=mywallet load_on_startup=1
```
Can be shortened to:
```sh
bitcoin-cli -named createwallet mywallet load_on_startup=1
```
JSON-RPC standard doesn't have a convention for passing named and positional parameters together, so this implementation makes one up and interprets any unused `"args"` named parameter as a positional parameter array.
This change is backwards compatible. It doesn't change the interpretation of any previously valid calls, just treats some previously invalid calls as valid.
Another use case even if you only occasionally use named arguments is that you can define an alias:
```
alias bcli='bitcoin-cli -named'
```
And now use both named named and unnamed arguments from the same alias without having to manually add `-named` option for named arguments or see annoying error "No '=' in named argument... this needs to be present for every argument (even if it is empty)`" for unnamed arguments
ACKs for top commit:
achow101:
ACK d8b12a75db
stickies-v:
re-ACK d8b12a75d
aureleoules:
re-ACK d8b12a75db
Tree-SHA512: 0cff8b50f584bcbbd376624adccf40536566ed8d1bcd6c88ad565dbc208f19d5e7a48c994efd6329d42b560149340d330397278f08a2912af5f3418d8c8837a9
fad1c55301 lint: Skip COMMIT_RANGE if no CIRRUS_PR (MarcoFalke)
Pull request description:
It doesn't make sense to run this for non-PRs, because:
* There are known whitespace "violations" in previous commits, so the lint may fail
* Once the changes are merged, it is too late to fix them up (force pushes are illegal)
* It isn't possible to determine which commits to run on if there is no reference branch (target branch of the pull request)
Moreover, the test fails on non-master:
* https://github.com/bitcoin/bitcoin/runs/8664441400
Fix all issues by skipping it.
ACKs for top commit:
hebasto:
ACK fad1c55301, also tested in my personal Cirrus account.
Tree-SHA512: be15f00e2b2a9069583833545883e0e5968a33d2455dad59e6fb47c1102b4dd16ef932e9ba945e29e9d941e6c17bd531a02c66b0491097801be6bda476875537
fa10f193b5 test: Set default in add_wallet_options if only one type can be chosen (MacroFake)
555519d082 test: Remove wallet option from non-wallet tests (MacroFake)
fac8d59d31 test: Set -disablewallet when no wallet has been compiled (MacroFake)
fa68937b89 test: Make requires_wallet private (MacroFake)
Pull request description:
The tests have several issues:
* Some tests that are wallet-type specific offer the option to run the test with the incompatible type
For example, `wallet_dump.py` offers `--descriptors` and on current master fails with `JSONRPCException: Invalid public key`. After the changes here, it fails with a clear error: `unrecognized arguments: --descriptors`.
* Tests that don't use the wallet at all offer the option to run it with a wallet type. This is confusing and wastes developers time if they are "tricked" into running the test for both wallet types, even though no wallet code is executed at all.
For example, `feature_addrman.py` will happily accept and run with `--descriptors` or `--legacy-wallet`. After the changes here, it no longer silently ignores the flag, but reports a clear error: `unrecognized arguments`.
ACKs for top commit:
achow101:
ACK fa10f193b5
Tree-SHA512: a5784da7305f4ec58c0013f433289000d94fc3d434b00fc329ffa37b812e2cd1da0071e34c3462bf79d904808564f2ae6d3d582f6b86b26215f9b07391b58460
9b5feb76bc script: small fixups/improvements for get_previous_releases.py (Sebastian Falbesoner)
Pull request description:
This is a small follow-up to #25650 (commit 614d4682ba) with three fixes/improvements:
- fix "Checksum did not match" detection, which was not adapted to the new `SHA256_SUMS` structure and hence never executed (the list of tarball names isn't directly in the dictionary's values anymore, but has to be extracted from the `'tarball'` field of each value)
- make both help text and default tag download order deterministic by sorting default tags
- `--tags` argument help text: add missing space between "for" and "backwards"
ACKs for top commit:
Sjors:
tACK 9b5feb76bc. Tested that if I change a checksum, or remove a release, it catches that.
josibake:
tested ACK 9b5feb76bc
Tree-SHA512: 791fa693477eebbda7fd41f3f5ec78fe7eab57df06979aa907ab258a6945534bdc3b931ddfce0fb440c9666b98c88ce5e1b6dc353ed39e129e87d3634855165c
17cad44851 test: refactor `RPCPackagesTest` to use `MiniWallet` (w0xlt)
Pull request description:
This PR refactors `RPCPackagesTest` to use `MiniWallet` and removes `create_child_with_parents`, `make_chain`, and `create_raw_chain` from `test_framework/wallet`, as requested in https://github.com/bitcoin/bitcoin/issues/25965.
Close https://github.com/bitcoin/bitcoin/issues/25965.
ACKs for top commit:
glozow:
ACK 17cad44851
pablomartin4btc:
tested ACK 17cad44; went thru all changes and recommendations from @kouloumos & @glozow; also went up to #20833 to get a bit of background of the origin and purpose of these tests.
kouloumos:
ACK 17cad44851
Tree-SHA512: 9228c532afaecedd577019dbc56f8749046d66f904dd69eb23e7ca3d7806e2132d90af29be276c7635fefb37ef348ae781eb3b225cd6741b20300e6f381041c3
This is a small follow-up to #25650 (commit
614d4682ba) with three fixes/improvements:
- fix "Checksum did not match" detection, which was not adapted to the new
SHA256_SUMS structure and hence never executed (the list of tarball
names isn't directly in the dictionary's values anymore, but has to be
extracted from the 'tarball' field of each value)
- make both help text and default tag download order deterministic by
sorting default tags
- "--tags" argument help text: add missing space between "for" and
"backwards"
5d413c8e79 Add feature_taproot case involved invalid internal pubkey (Pieter Wuille)
Pull request description:
Add a test case to feature_taproot which involves an output that is (incorrectly) constructed, using an invalid internal public key and valid script tree. It is designed to detect cases where the script path spending validation logic does not detect this case, and instead treats the internal public key as the point at infinity.
Equivalent unit test case added in https://github.com/bitcoin-core/qa-assets/pull/98.
ACKs for top commit:
instagibbs:
ACK 5d413c8e79
aureleoules:
reACK 5d413c8e79
Tree-SHA512: dfa014e383cd2743f3c9a996e1f2a2fceb9e244edf4b05dc0c110c4ba32a87684482222907805a4ca998aebcb42a197bb3e7967bfb5f0554fe9f1e5aa5463603
31d0067f8b doc: test: update/fix TestShell example instructions (Sebastian Falbesoner)
Pull request description:
This PR tackles two issues in the TestShell documentation:
- add missing instruction for creating a wallet prior to the `getnewaddress` call (needed as there is no default wallet created anymore since v0.21)
- fix `generatetoaddress` call syntax (the scripted-diff in commit fa0b916971 only worked for tests using `BitcoinTestFramework`)
ACKs for top commit:
fanquake:
ACK 31d0067f8b - current instructions don't work. These do.
Tree-SHA512: d2b7808a06892ad16728cb2b6d4a72b255ad711d27fe98b1de562f80444e7bb25d73296abdde4308162fe3be702864e2f7b7dbbbb000fe54c709951c09e6c730
c8dc0e3eaa refactor: Inline `CTxMemPoolEntry` class's functions (Hennadii Stepanov)
75bbe594e5 refactor: Move `CTxMemPoolEntry` class to its own module (Hennadii Stepanov)
Pull request description:
This PR:
- gets rid of the `policy/fees` -> `txmempool` -> `policy/fees` circular dependency
- is an alternative to #13949, which nukes only one circular dependency
ACKs for top commit:
ryanofsky:
Code review ACK c8dc0e3eaa. Just include and whitespace changes since last review, and there's a moveonly commit now so it's very easy to review
theStack:
Code-review ACK c8dc0e3eaa
glozow:
utACK c8dc0e3eaa, agree these changes are an improvement.
Tree-SHA512: 36ece824e6ed3ab1a1e198b30a906c8ac12de24545f840eb046958a17315ac9260c7de26e11e2fbab7208adc3d74918db7a7e389444130f8810548ca2e81af41
6630a1e844 Add warning on first startup if free disk space is less than necessary (Ben Woosley)
Pull request description:
This reworks/revives https://github.com/bitcoin/bitcoin/pull/15848 to add a check for low disk space on first startup and issue a warning if disk space is below the expected space required to accommodate the blocks.
This PR was fashioned by a team of developers at the [bitcoin++](https://www.btcplusplus.dev/) conference workshop: "[Let's contribute to Bitcoin Core](https://sched.co/12P6Z)"
Fixes#15813
ACKs for top commit:
achow101:
ACK 6630a1e844
willcl-ark:
tACK 6630a1e844 rebased on master. Warning shows on first start but not on restart after some blocks have been downloaded.
aureleoules:
ACK 6630a1e844
pablomartin4btc:
re-ACK 6630a1e844
hernanmarino:
ReACK 6630a1e844
Tree-SHA512: 0f18acabdf2b514e96e2eea8f304960b952226b83dc91334cf7d1f6355ea2f257aaec0ee38d43ac36435385ecd918333d20657c35a8a7407e7cf2680ccb643bb
fa68d086f3 test: Add getpeerinfo test for missing version message (MacroFake)
Pull request description:
There seems to be a lot of discussion about behaviour/code that is completely untested.
Fix this by adding a test. The test documents the current behaviour and helps to detect when the behaviour changes in the future.
ACKs for top commit:
jonatack:
ACK fa68d086f3
mzumsande:
Code Review ACK fa68d086f3
Tree-SHA512: d092b30d5bdb46712c91a7c5bd2d0c82a0da281f1460967aa4e32c648b15d8d97870ded9565a90af34874eb468aad8b99694a2485af6807994e7cfc05482aa8c
Tackles two issues in the TestShell documentation:
- add missing instruction for creating a wallet prior to the
`getnewaddress` call (needed as there is no default wallet created
anymore since v0.21)
- fix `generatetoaddress` call syntax (the scripted-diff in commit
fa0b916971 only worked for tests using
`BitcoinTestFramework`)
