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
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
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
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
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`)
fa84df1f03 scripted-diff: wallet: rename AvailableCoinsParams members to snake_case (furszy)
61c2265629 wallet: group AvailableCoins filtering parameters in a single struct (furszy)
f0f6a3577b RPC: listunspent, add "include immature coinbase" flag (furszy)
Pull request description:
Simple PR; adds a "include_immature_coinbase" flag to `listunspent` to include the immature coinbase UTXOs on the response. Requested by #25728.
ACKs for top commit:
danielabrozzoni:
reACK fa84df1f03
achow101:
ACK fa84df1f03
aureleoules:
reACK fa84df1f03
kouloumos:
reACK fa84df1f03
theStack:
Code-review ACK fa84df1f03
Tree-SHA512: 0f3544cb8cfd0378a5c74594480f78e9e919c6cfb73a83e0f3112f8a0132a9147cf846f999eab522cea9ef5bd3ffd60690ea2ca367dde457b0554d7f38aec792
db929893ef Faster -reindex by initially deserializing only headers (Larry Ruane)
c72de9990a util: add CBufferedFile::SkipTo() to move ahead in the stream (Larry Ruane)
48a68908ba Add LoadExternalBlockFile() benchmark (Larry Ruane)
Pull request description:
### Background
During the first part of reindexing, `LoadExternalBlockFile()` sequentially reads raw blocks from the `blocks/blk00nnn.dat` files (rather than receiving them from peers, as with initial block download) and eventually adds all of them to the block index. When an individual block is initially read, it can't be immediately added unless all its ancestors have been added, which is rare (only about 8% of the time), because the blocks are not sorted by height. When the block can't be immediately added to the block index, its disk location is saved in a map so it can be added later. When its parent is later added to the block index, `LoadExternalBlockFile()` reads and deserializes the block from disk a second time and adds it to the block index. Most blocks (92%) get deserialized twice.
### This PR
During the initial read, it's rarely useful to deserialize the entire block; only the header is needed to determine if the block can be added to the block index immediately. This change to `LoadExternalBlockFile()` initially deserializes only a block's header, then deserializes the entire block only if it can be added immediately. This reduces reindex time on mainnet by 7 hours on a Raspberry Pi, which translates to around a 25% reduction in the first part of reindexing (adding blocks to the index), and about a 6% reduction in overall reindex time.
Summary: The performance gain is the result of deserializing each block only once, except its header which is deserialized twice, but the header is only 80 bytes.
ACKs for top commit:
andrewtoth:
ACK db929893ef
achow101:
ACK db929893ef
aureleoules:
ACK db929893ef - minor changes and new benchmark since last review
theStack:
re-ACK db929893ef
stickies-v:
re-ACK db929893e
Tree-SHA512: 5a5377192c11edb5b662e18f511c9beb8f250bc88aeadf2f404c92c3232a7617bade50477ebf16c0602b9bd3b68306d3ee7615de58acfd8cae664d28bb7b0136
2dede9f675 Adjust RPCTypeCheckObj error string (Leonardo Araujo)
Pull request description:
Unifies the JSON type error strings as mentioned in #26214. Also refer to #25737.
ACKs for top commit:
furszy:
ACK 2dede9f6
Tree-SHA512: c918889e347ba32cb6d0e33c0de5956c2077dd40c996151e16741b0c4983ff098c60258206ded76ad7bbec4876c780c6abb494a97e4f1e05717d28a59b9167a6
Review note: The changes are complete, because self.options.descriptors
is set to None in parse_args (test_framework.py).
A value of None implies -disablewallet, see the previous commit.
So if a call to add_wallet_options is missing, it will lead to a test
failure when the wallet is compiled in.
The bool is only used to call a public helper, which some tests already
do. So use the public helper in all tests consistently and make the
confusingly named bool private.
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:
bitcoin-cli -named createwallet wallet_name=mywallet load_on_startup=1
Can be shortened to:
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.
0de30ed509 tests: Test Taproot PSBT signing with keys in other descriptor (Andrew Chow)
6efcdf6b7f tests: Use new wallets for each test in wallet_taproot.py (Andrew Chow)
8781a1b6bb psbt: Include output pubkey in additional pubkeys to sign (Andrew Chow)
323890d0d7 sign: Fill in taproot pubkey info for all script path sigs (Andrew Chow)
Pull request description:
A user reported on [stackexchange](https://bitcoin.stackexchange.com/q/115742/48884) that they were unable to sign for a `multi_a` script using a wallet that only had the corresponding keys (i.e. it did not have the `multi_a()` descriptor). This PR fixes this issue.
Additionally, `wallet_taproot.py` is modified to test for this scenario by having another wallet in `do_test_psbt` which contains descriptors that only have the keys involved in the descriptor being tested. `wallet_taproot.py` was also modified to create new wallets for each test case rather than sharing wallets throughout as the sharing could result in the signing wallet having the keys in a different descriptor and accidentally result in failing to detect a test failure.
The changes to the test also revealed a similar issue with `rawtr()` descriptors, which has also been fixed by checking if a descriptor can produce a `SigningProvider` for the Taproot output pubkey.
ACKs for top commit:
instagibbs:
crACK 0de30ed509
darosior:
ACK 0de30ed509
Tree-SHA512: 12e131dd8afd93da7b1288c9054de2415a228d4477b97102da3ee4e82ce9de20b186260c3085a4b7b067bd8b74400751dcadf153f113db83abc59e7466e69f14
Using disconnect_p2ps instead of peer_disconnect makes
the node wait for the disconnect to complete. As a result,
we can reuse p2p_idx=0 in the add_outbound_p2p_connection calls.
201b9a02fd test: fix intermittent failure in feature_index_prune.py (Martin Zumsande)
Pull request description:
I can't reproduce the error from #26630 locally, but from analying the logs I think the problem is the following:
After calling `sync_blocks`, we didn't check that the indexes have caught up to the tip before performing the manual pruning. This could possibly lead to prune blockers with a lower height than the expected 2489, which do appear in the logs of the failed CI runs, e.g.
- `2022-10-27T21:14:17.703920Z [C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\src\validation.cpp:2395] [FlushStateToDisk] [prune] coinstatsindex limited pruning to height 2488` ([Cirrus](https://cirrus-ci.com/task/5443742333665280?logs=functional_tests#L2506))
So, this should be fixed by a call to `sync_index`.
Fixes#26330
ACKs for top commit:
brunoerg:
crACK 201b9a02fd
Tree-SHA512: fb7023c9eb2ba6d0e69e059a401453cbdf63abc6804543dffcf36ba9f93c9cd13209e57aa5536d94b2e420c9d4cd0b1a7eff1adadd19aa7b3c33f592502e1bc0
fa24239a1c net: Avoid SetTxRelay for feeler connections (MacroFake)
Pull request description:
Seems odd to reserve memory for the struct (the heaviest member being `m_tx_inventory_known_filter`) when it is never used.
This also avoids sending out `msg_sendtxrcncl` before disconnecting. This shouldn't matter, as other messages, such as `msg_wtxidrelay`, `msg_sendaddrv2`, `msg_verack` or `msg_getaddr` are still sent. Though, it allows to test the changes here as a side-effect.
ACKs for top commit:
naumenkogs:
ACK fa24239a1c
vasild:
ACK fa24239a1c
jonatack:
ACK fa24239a1c
mzumsande:
ACK fa24239a1c
Tree-SHA512: d7604c7eb4df8f2de811e600bdd312440ee03e508d3a0f09ae79f7f2d3eeec663bfd47a2d079fa50b756d61e35dfa998de068a7b9afaf35378fa0e62a538263d
8a9f1e4d18 test: fix intermittent failure in rpc_getblockfrompeer.py (Martin Zumsande)
Pull request description:
Fixes an intermittent failure in `rpc_getblockfrompeer.py` observed in https://cirrus-ci.com/task/6610115527704576 by adding a sync to make sure the node has processed the header we sent it before we query it for the corresponding block.
Fixes#26412
ACKs for top commit:
jonatack:
ACK 8a9f1e4d18
Tree-SHA512: f6188ab3cfd863034e44e9806d0d99a8781462bec94141501aefc71589153481ffb144e126326ab81807c2b2c93de7f4aac5d09dbcf26c4512a176e06fc6e5f8
To avoid a wallet potentially being able to sign a transaction using
keys from descriptors imported in previous tests, make new wallets for
each test case rather than sharing them.
After syncing the blocks, we didn't check that the
indexes have caught up to the tip before manually pruning.
This could lead to prune blockers lower thatn the expected height.
Currently, debug.log is spammed with messages from random.cpp
when functional tests are run. These logs are not useful for
debugging, and decrease the signal to noise ratio of the logs.
3fcb545ab2 bench: benchmark transaction creation process (furszy)
a8a75346d7 wallet: SelectCoins, return early if target is covered by preset-inputs (furszy)
f41712a734 wallet: simplify preset inputs selection target check (furszy)
5baedc3351 wallet: remove fetch pre-selected-inputs responsibility from SelectCoins (furszy)
295852f619 wallet: encapsulate pre-selected-inputs lookup into its own function (furszy)
37e7887cb4 wallet: skip manually selected coins from 'AvailableCoins' result (furszy)
94c0766b0c wallet: skip available coins fetch if "other inputs" are disallowed (furszy)
Pull request description:
#### # Context (Current Flow on Master)
In the transaction creation process, in order to select which coins the new transaction will spend,
we first obtain all the available coins known by the wallet, which means walking-through the
wallet txes map, gathering the ones that fulfill certain spendability requirements in a vector.
This coins vector is then provided to the Coin Selection process, which first checks if the user
has manually selected any input (which could be internal, aka known by the wallet, or external),
and if it does, it fetches them by searching each of them inside the wallet and/or inside the
Coin Control external tx data.
Then, after finding the pre-selected-inputs and gathering them in a vector, the Coin Selection
process walks-through the entire available coins vector once more just to erase coins that are
in both vectors. So the Coin Selection process doesn’t pick them twice (duplicate inputs inside
the same transaction).
#### # Process Workflow Changes
Now, a new method, `FetchCoins` will be responsible for:
1) Lookup the user pre-selected-inputs (which can be internal or external).
2) And, fetch the available coins in the wallet (excluding the already fetched ones).
Which will occur prior to the Coin Selection process. Which allows us to never include the
pre-selected-inputs inside the available coins vector in the first place, as well as doing other
nice improvements (written below).
So, Coin Selection can perform its main responsibility without mixing it with having to fetch
internal/external coins nor any slow and unneeded duplicate coins verification.
#### # Summarizing the Improvements:
1) If any pre-selected-input lookup fail, the process will return the error right away.
(before, the wallet was fetching all the wallet available coins, walking through the
entire txes map, and then failing for an invalid pre-selected-input inside SelectCoins)
2) The pre-selected-inputs lookup failure causes are properly described on the return error.
(before, we were returning an "Insufficient Funds" error for everything, even if the failure
was due a not solvable external input)
3) **Faster Coin Selection**: no longer need to "remove the pre-set inputs from the available coins
vector so that Coin Selection doesn't pick them" (which meant to loop-over the entire
available coins vector at Coin Selection time, erasing duplicate coins that were pre-selected).
Now, the available coins vector, which is built after the pre-selected-inputs fetching,
doesn’t include the already selected inputs in the first place.
4) **Faster transaction creation** for transactions that only use manually selected inputs.
We now will return early, as soon as we finish fetching the pre-selected-inputs and
not perform the resources expensive calculation of walking-through the entire wallet
txes map to obtain the available coins (coins that we will not use).
---------------------------
Added a new bench (f6d0bb2) measuring the transaction creation process, for a wallet with ~250k UTXO, only using the pre-selected-inputs inside coin control. Setting `m_allow_other_inputs=false` to disallow the wallet to include coins automatically.
#### Result on this PR (tip f6d0bb2d):
| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 1,048,675.00 | 953.58 | 0.3% | 0.06 | `WalletCreateTransaction`
vs
#### Result on master (tip 4a4289e2):
| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 96,373,458.20 | 10.38 | 0.2% | 5.30 | `WalletCreateTransaction`
The benchmark took to run in master: **96.37 milliseconds**, while in this PR: **1 millisecond** 🚀 .
ACKs for top commit:
S3RK:
Code Review ACK 3fcb545ab2
achow101:
ACK 3fcb545ab2
aureleoules:
reACK 3fcb545ab2
Tree-SHA512: 42f833e92f40c348007ca565a4c98039e6f1ff25d8322bc2b27115824744779baf0b0a38452e4e2cdcba45076473f1028079bbd0f670020481ec5d3db42e4731
eb679a7896 rpc: make `address` field optional (w0xlt)
Pull request description:
Close https://github.com/bitcoin/bitcoin/issues/26338.
This PR makes optional the `address` field in the response of `listtransactions` and `listsinceblock` RPC.
And adds two tests that fail on master, but not on this branch.
ACKs for top commit:
achow101:
ACK eb679a7896
aureleoules:
ACK eb679a7896
Tree-SHA512: b267439626e2ec3134ae790c849949a4c40ef0cebd20092e8187be3db0a61941b2da10bbbba92ca880b8369f46c1aaa806d057eaa5159325f65cbec7cb33c52f
