30a94b1ab9ae850d55cb9eb606a06890437bc75e test, wallet: Remove concurrent writes test (Ava Chow)
b44b7c03fef01e0b5db704e50762b3d16b3da69e wallet: Write best block record on unload (Ava Chow)
876a2585a8b69e12ac171a0d9ff5aab864067c42 wallet: Remove unnecessary database Close step on shutdown (Ava Chow)
98a1a5275c8c395fe47ff7f10109d75b06bc391d wallet: Remove chainStateFlushed (Ava Chow)
7fd3e1cf0c88553e0722048ce488f265883558e7 wallet, bench: Write a bestblock record in WalletMigration (Ava Chow)
6d3a8b195a826448c021dd189255ca41ba70cc5a wallet: Replace chainStateFlushed in loading with SetLastBlockProcessed (Ava Chow)
7bacabb204b6c34f9545f0b37e2c66296ad2c0de wallet: Update best block record after block dis/connect (Ava Chow)
Pull request description:
Implements the idea discussed in https://github.com/bitcoin/bitcoin/pull/29652#issuecomment-2010579484
Currently, `m_last_block_processed` and `m_last_block_processed_height` are not guaranteed to match the block locator stored in the wallet, nor do either of those fields actually represent the last block that the wallet is synced up to. This is confusing and unintuitive.
This PR changes those last block fields to be updated whenever the wallet makes a change to the db for new transaction state found in new blocks. Whenever a block is received that contains a transaction relevant to the wallet, the last block locator will now be written to disk. Furthermore, every block disconnection will now write an updated locator.
To ensure that the locator is relatively recent and loading rescans are fairly quick in the event of unplanned shutdown, it is also now written every 144 blocks (~1 day). Additionally it is now written when the wallet is unloaded so that it is accurate when the wallet is loaded again.
Lastly, the `chainstateFlushed` notification in the wallet is changed to be a no-op. The best block locator record is no longer written when `chainstateFlushed` is received from the node since it should already be mostly up to date.
ACKs for top commit:
rkrux:
ACK 30a94b1ab9ae850d55cb9eb606a06890437bc75e
mzumsande:
Code Review ACK 30a94b1ab9ae850d55cb9eb606a06890437bc75e
ryanofsky:
Code review ACK 30a94b1ab9ae850d55cb9eb606a06890437bc75e. Only changes since last review are using WriteBestBlock method more places and updating comments.
Tree-SHA512: 46117541f8aaf13dde57430e813b4bbbd5e146e2632769675803c8e65a82f149a7cc6026489a127d32684b90124bd2b7c28216dbcfa6a47447300e8f3814e029
Descriptor wallets are already created by default since v23.0, but
since the recent legacy wallet removal this parameter *must* be True
(see commit 9f04e02ffaee0fe64027dc56c7bea3885254321a), i.e. still
passing it wouldn't contain any information for test readers
anymore. So simply drop them in the functional tests in order to
reduce code bloat.
-BEGIN VERIFY SCRIPT-
sed -i 's/, descriptors=True//g' $(git ls-files -- 'test/functional' ':(exclude)test/functional/wallet_backwards_compatibility.py')
sed -i '/descriptors=True,/d' ./test/functional/mempool_persist.py
-END VERIFY SCRIPT-
b104d442277090337ce405d92f1398b7cc9bcdb7 test: Remove RPCOverloadWrapper (Ava Chow)
4d32c19516fdb65b364638fa088b8d7167b438b6 test: Replace importpubkey (Ava Chow)
fe838dd391be669ccd0765b95f81c25fecfd3636 test: Replace usage of addmultisigaddress (Ava Chow)
d3142077794f4e910d3cdc749020d725e30feb24 test: Replace usage of importaddress (Ava Chow)
fcc457573f9b39e6f173a4f51c45d7dbb47e7ab0 test: Replace importprivkey with wallet_importprivkey (Ava Chow)
94c87bbbd06eb9a57930b9f59315533cfbe8f460 test: Remove unnecessary importprivkey from wallet_createwallet (Ava Chow)
Pull request description:
`RPCOverloadWrapper` implemented overloads for legacy wallet only RPCs so that the same function call could be used within tests for both legacy wallets and descriptor wallets. With legacy wallets now removed, there is no need to continue to have these overloads.
For `importaddress`, `addmultisigaddress`, and `importpubkey`, the uses of these are converted to `importdescriptors`.
For `importprivkey`, a new helper function `wallet_imporprivkey` is introduced that does what the overload did. This is mainly to reduce verbosity as `importprivkey` was more widely used throughout the tests.
Some tests that used these RPCs are now also no longer relevant and have been removed.
ACKs for top commit:
Sjors:
ACK b104d442277090337ce405d92f1398b7cc9bcdb7
pablomartin4btc:
cr ACK b104d442277090337ce405d92f1398b7cc9bcdb7
rkrux:
ACK b104d442277090337ce405d92f1398b7cc9bcdb7
w0xlt:
ACK b104d44227
Tree-SHA512: ded2f73829e2ce28466d4a9738eb382783ad990daee5d1859dbc4d354e6f8eec0c483ed5ecb1287fe0dd24ac332065b733a30d71b126b841bd7cd49e9a094b6d
75a185ea3db3177e8e479ee61a39bcb51e08d9a6 test: add skip_if_running_under_valgrind() (fanquake)
Pull request description:
Enable it in the USDT tests. The context (from 0xB10C):
> every time the tracepoint is reached a SIGTRAP is fired.
> No matter the tracepoint contents, even with an empty one.
> Valgrind intercepts SIGTRAP and aborts.
See discussion in #32374.
ACKs for top commit:
maflcko:
lgtm ACK 75a185ea3db3177e8e479ee61a39bcb51e08d9a6
willcl-ark:
ACK 75a185ea3db3177e8e479ee61a39bcb51e08d9a6
Tree-SHA512: 7f45c3049ab39cc514024067bd6ac26598e99202c114b48459834c26c2e1273fa58af693878298e628a10c561b954850e49e76b39567b771bb0c0534a063a524
4b241867567203b204823a4558c2aa5767acf028 test: add test for decoding PSBT with MuSig2 PSBT key types (BIP 373) (Sebastian Falbesoner)
8ba245cb8318c9be375ac2fb2a3e7ded875b4408 test: add constants for MuSig2 PSBT key types (BIP 373) (Sebastian Falbesoner)
Pull request description:
This PR is a follow-up to #31247 (see https://github.com/bitcoin/bitcoin/pull/31247#pullrequestreview-2427834909) and adds a functional test for decoding PSBTs (using the `decodepsbt` RPC) with MuSig2 per-input and per-output types. The first commit adds the new MuSig2 key types to the test frameworks and extends the PSBT serialization to cope with lists of bytestrings.
ACKs for top commit:
achow101:
ACK 4b241867567203b204823a4558c2aa5767acf028
rkrux:
re-ACK 4b24186
Tree-SHA512: f12919f71b3fff74df1d7ddaa8db455b1b139f7abd51d7f3fa5d750fc7dd613454b438c4e0dedad679476d414fa1da43ef1121e486b0bdfd97d5ef8bdf37f060
Since CWallet::chainStateFlushed is now no-op, this test no longer tests
the concurrent writes scenario. There are no other cases where multiple
DatabaseBatches are open at the same time.
When a block is connected, if the new block had anything relevant to the
wallet, update the best block record on disk. If not, also sync the best
block record to disk every 144 blocks.
Also reuse the new WriteBestBlock method in BackupWallet.
e7ad86e1ca3b0b2f2795e91c2f9959486c67dd90 test: fix another intermittent failure in wallet_basic.py (Martin Zumsande)
07350e204dedfba20da461d9cdcd469dc95e01c3 test: Fix intermittent failure in wallet_basic.py (Martin Zumsande)
Pull request description:
Fixes two rare failures that happened in the CI:
#27249:
There could be a race with outstanding TxAddedToMempool notifications being applied to the soon-to-be created wallet:
1. importdescriptors during rescan sets status to `TxStateConfirmed`
2. old `transactionAddedToMempool` notification changes status back to `TxStateInMempool`
3. If the listunspent call happens here the test will fail
4. blockConnected notification will change the status back to `TxStateConfirmed` (so it's not a persistent failure)
I could reproduce this by adding a 100 microsecond sleep to `AddToWallet()`, the fix is to add a sync, so `transactionAddedToMempool` notifications won't affect the new wallet anymore.
#32456:
During init, the test framework will start using rpc after the mempool was loaded.
It will not wait for `start()` / `postInitProcess` or outstanding `transactionAddedToMempool` notifications (which would both set the status to `TxStateInMempool`), leading to
a possible race, in which `listunspent` can be called while the tx is still in `Inactive` status.
Can be reproduced by adding two sleeps: To init before calling `start()` for the chain clients, plus to `transactionAddedToMempool` in `wallet.cpp`.
Prevent this by processing outstanding notifications.
Fixes#27249Fixes#32456
ACKs for top commit:
maflcko:
review ACK e7ad86e1ca3b0b2f2795e91c2f9959486c67dd90 🎩
Tree-SHA512: 1f1a11e5c8e1c6d3c39a49401c2c5122befdbbec25c0451953f5bfe8dfb53221ada552a68006e266570addda12bb16c1b9b1e49ad2198c33d91c4b96b764d73e
Enable it in the USDT tests. The context (from 0xB10C):
> every time the tracepoint is reached a SIGTRAP is fired.
> No matter the tracepoint contents, even with an empty one.
> Valgrind intercepts SIGTRAP and aborts.
See discussion in #32374.
2aa63d511affdcc9980b58fc4ff18b8ad10b0f8c test: Use uninvolved pruned node in feature_pruning undo test (enoch)
772ba7f9ce09e836a51636524a8a96a23946d658 test: Fix nTimes typo in feature_pruning test (enoch)
Pull request description:
This PR contains two commits:
1. Fixes a typo in feature_pruning.py where 'nTimes' was incorrectly
used instead of 'nTime'. This typo caused the test to always reset
mine_large_blocks.nTime to 0, rather than only on the first run.
2. Fixes the test failure exposed by the typo fix. The
test_pruneheight_undo_presence test was failing because it was using
node 2, which is involved in reorg testing and could be on a
different chain than other nodes. The solution switches to using
node 5, which is also a pruned node but isn't involved in reorg
testing.
Testing:
- Ran test/functional/feature_pruning.py multiple times to verify
consistent passing
- Verified that the test now passes with the correct nTime variable name
- Confirmed the test behavior matches the intended functionality of
verifying pruned block availability
- Ran the full test suite to ensure the changes did not introduce any
regressions or affect other tests
Thanks to fjahr for his assistance in diagnosing the issue and
suggesting the solution.
This fixes the test failure reported in #32249
ACKs for top commit:
fjahr:
tACK 2aa63d511affdcc9980b58fc4ff18b8ad10b0f8c
maflcko:
lgtm ACK 2aa63d511affdcc9980b58fc4ff18b8ad10b0f8c
naiyoma:
tACK 2aa63d511affdcc9980b58fc4ff18b8ad10b0f8c
stratospher:
tested ACK 2aa63d5. verified that `nTime` is being incremented now.
Tree-SHA512: a543528fd4eeb30e978c0b43cfa109768252edaf1f94679dbbc7fe684122c00da34224e2cc1abd2a265af1b267eef1cd34246207946cf7d8e93d2c0f11aa56d8
During init, the test framework will start using rpc after the
mempool was loaded. It will not wait for postInitProcess or
outstanding transactionAddedToMempool notifications, leading to
a possible race, in which listunspent is being called while the
tx is still in Inactive status. Prevent this by processing
outstanding notifications.
There could be a race with outstanding TxAddedToMempool notifications
being applied to the soon-to-be created wallet.
Fixes an intermittent timeout reproducable by adding a sleep to
AddToWallet.
a58cb3b1c12c8cb75a87375c50f94c4605bb805d qa: sanity check mined block have their coinbase timelocked to height (Antoine Poinsot)
8f2078af6a55448c003b3f7f3021955fbb351caa miner: timelock coinbase transactions (Antoine Poinsot)
788aeebf343526760fa8f3ed969ac3713212a5b6 qa: use prev height as nLockTime for coinbase txs created in unit tests (Antoine Poinsot)
c76dbe9b8b6f03b761a0ef97e1b8cd133b934714 qa: timelock coinbase transactions created in fuzz targets (Antoine Poinsot)
9c94069d8b6cf67a24eb03c51230a4f2b2bf2d64 contrib: timelock coinbase transactions in signet miner (Antoine Poinsot)
a5f52cfcc400ad0adb41a78c65b8abb971e0d622 qa: timelock coinbase transactions created in functional tests (Antoine Poinsot)
Pull request description:
The Consensus Cleanup soft fork proposal includes enforcing that coinbase transactions set their
nLockTime field to the block height minus 1, as well as their nSequence such as to not disable the
timelock. If such a fork were to be activated by Bitcoin users, miners need to be ready to produce
compliant blocks at the risk of losing substantial amounts mining would-be invalid blocks. As miners
are unfamously slow to upgrade, it's good to make this change as early as possible.
Although Bitcoin Core's GBT implementation does not provide the `coinbasetxn` field, and mining
pool software crafts the coinbase on its own, updating the Bitcoin Core mining code is a first step
toward convincing pools to update their (often closed source) code. A possible followup is also to
introduce new fields to GBT. In addition, this first step also makes it possible to test future
Consensus Cleanup changes.
The commit making the change also updates a bunch of seemingly-unrelated tests. This is because those tests were asserting error messages based on the txid of transactions involved, and changing the coinbase transaction structure necessarily changes the txid of all tests' transactions.
ACKs for top commit:
Sjors:
Code review ACK a58cb3b1c12c8cb75a87375c50f94c4605bb805d
achow101:
ACK a58cb3b1c12c8cb75a87375c50f94c4605bb805d
TheCharlatan:
Re-ACK a58cb3b1c12c8cb75a87375c50f94c4605bb805d
Tree-SHA512: a2aae009a187eb760d34435f518a895ee76c6b02a667eb030ddf6bd584da6e8eae2737d974dbf81a928d60c07bcb4820f055adc067e18d8819640db0240bb513
importprivkey was a legacy wallet only RPC which had a helper for
descriptor wallets in tests. Add wallet_importprivkey helper and use it
wherever importprivkey is used (other than backward compatibility tests)
This test was testing importprivkey behavior in a legacy wallet without
private keys. As legacy wallets no longer exist, this test case is no
longer relevant.
10845cd7cc89fc83b4ee844dc577b391720bba9c qa: Add feature_framework_startup_failures.py (Hodlinator)
28e282ef9ae94ede4aace6b97ff18c66cb72a001 qa: assert_raises_message() - Stop assuming certain structure for exceptions (Hodlinator)
1f639efca5e71a0ff208415d94e408a74778d4db qa: Work around Python socket timeout issue (Hodlinator)
9b24a403fae4b896ff7705519bd48c877b4e621b qa: Only allow calling TestNode.stop() after connecting (Hodlinator)
6ad21b4c0114029d16d334bf8d437834708a295e qa: Include ignored errors in RPC connection timeout (Hodlinator)
879243e81fd55f54739fbdae4d5b8666f5acb9a9 qa refactor: wait_for_rpc_connection - Treat OSErrors the same (Hodlinator)
Pull request description:
Improves handling of startup errors in functional tests and puts tests in place to ensure knock-on errors don't creep in.
- `wait_for_rpc_connection()` now appends specific failures leading up to the `Unable to connect to bitcoind` error to that error message:
`[node 0] Unable to connect to bitcoind after 60s (ignored errors: {'missing_credentials': 1, 'OSError.ECONNREFUSED': 239}, latest error: ConnectionRefusedError(111, 'Connection refused'))`
- Fixes Windows Python issue where `socket.timeout` exceptions end up with unset `errno`-fields.
- Also adds comments, refactors code, improves logging.
The underlying purpose is to ensure developer efficiency in finding root causes of test failures.
Prior iterations of the PR partially focused on fixing the same issue as #31620.
Originally inspired by #30390.
### Testing
Can be tested by reverting either faf2f2c654d9aa18b2f49a157956f9ab0fce302a or fae3bf6b870eb0f9cddd1adac82ba72890806ae3 from #31620, or the "qa: Avoid calling stop-RPC if not connected" from this PR, and running *feature_framework_startup_failures.py*.
ACKs for top commit:
l0rinc:
ACK 10845cd7cc89fc83b4ee844dc577b391720bba9c
ryanofsky:
Code review ACK 10845cd7cc89fc83b4ee844dc577b391720bba9c. Only changes since last review were adding a new commit tweaking assert_raises_message(), extending the new test to have a self-check, and to pass through all options to child tests instead of a hardcoded list of options. I left some cleanup suggestions below but they are not important.
Tree-SHA512: f0235c5cbb6d1bb85d8dc5de492a08a34f6edc83499cbf0a5f9a3824809ff84635888c62c9c01101e3cc9ef9f1cdee2c9ab6537fea6feeb005b29f428caf8b22
Also, support serialization of lists of byte-strings as PSBTMap values,
which will be simply concatenated without any compact-size prefixes
(neither for the individual items nor for the size of the list).
de054df6dc32cbd8b127c6761d9c65d95025e08f contrib: Remove legacy wallet RPCs from bash completions (Ava Chow)
5dff04a1bba887043616a24590a8df752620f1c3 legacy spkm: Make IsMine() and CanProvide() private and migration only (Ava Chow)
c0f3f3264ff7f17c39c00c4409a48580f98d4f57 wallet: Remove unused db functions (Ava Chow)
83af1a3cca7e8dabe724f1a3258860c40bc8216d wallet: Delete LegacySPKM (Ava Chow)
8ede6dea0c55bb4afefa790b83dd4da48a2f84da wallet, rpc: Remove legacy wallet only RPCs (Ava Chow)
4de3cec28dfb3186a2263f28ab2f6481820fd550 test: rpcs disabled for descriptor wallets will be removed (Ava Chow)
84f671b01df4e6ce81e2d6284371a4326a04d47e test: Run multisig script limit test (Ava Chow)
810476f31e42aa7ed36a684c45accf4d337b3b24 test: Remove unused options and variables, correct comments (Ava Chow)
04a7a7a28cdca46d977e474436f7447157a52208 build, wallet, doc: Remove BDB (Ava Chow)
Pull request description:
The final step of #20160.
A bare minimum of legacy wallet code is kept in order to perform wallet migration. Migration of legacy wallets uses the independent BDB parser and a minimal `LegacyDataSPKM` that allows the legacy data to be loaded so that the migration can be completed.
BDB has been removed as a dependency and documentation have been updated to reflect that.
ACKs for top commit:
Sjors:
re-ACK de054df6dc32cbd8b127c6761d9c65d95025e08f
maflcko:
re-ACK de054df6dc32cbd8b127c6761d9c65d95025e08 🔗
w0xlt:
reACK de054df6dc
rkrux:
Concept ACK de054df6dc32cbd8b127c6761d9c65d95025e08f
Tree-SHA512: 16a6c265bc1ada5e7a5ef9b95f0ff65015672ca46d9a43b7e10d60e9e085052e9bbfe01ac3e494cc606afb652a1b476b10e434d13e9877b67d2cb0196a9bd190
In order to remove potential confusion, this commit adapts all script
error constant names in the functional tests (currently only in
feature_taproot.py) to the ones used in our C++ codebase. This also
makes checking whether we have test coverage for a certain script error
easier.
-BEGIN VERIFY SCRIPT-
ren() { sed -i "s|$1|$2|g" $( git grep -l "$1" -- "./test" ) ; }
ren ERR_SIG_SIZE ERR_SCHNORR_SIG_SIZE
ren ERR_SIG_HASHTYPE ERR_SCHNORR_SIG_HASHTYPE
ren ERR_SIG_SCHNORR ERR_SCHNORR_SIG
ren ERR_CONTROLBLOCK_SIZE ERR_TAPROOT_WRONG_CONTROL_SIZE
ren ERR_PUSH_LIMIT ERR_PUSH_SIZE
ren ERR_MINIMALIF ERR_TAPSCRIPT_MINIMALIF
ren ERR_UNKNOWN_PUBKEY ERR_PUBKEYTYPE
ren ERR_STACK_EMPTY ERR_INVALID_STACK_OPERATION
ren ERR_SIGOPS_RATIO ERR_TAPSCRIPT_VALIDATION_WEIGHT
ren ERR_UNDECODABLE ERR_BAD_OPCODE
ren ERR_NO_SUCCESS ERR_EVAL_FALSE
ren ERR_EMPTY_WITNESS ERR_WITNESS_PROGRAM_WITNESS_EMPTY
-END VERIFY SCRIPT-
c7e2b9e2644442b147880becb8a659f3d00092d9 tests: Test migration cleans up bad inactive chain derivation path (Ava Chow)
Pull request description:
A bug in 0.21.x and 22.x resulted in some wallets having invalid derivation paths that are the concatenation of two derivation paths. These appear only when inactive hd chains are topped up.
Since key metadata is a legacy wallet only record, migrating legacy wallets to descriptor wallets will fix this issue as all key metadata records are deleted. The derivation path information is derived on-the-fly from the descriptor that is produced for the inactive hd chain.
Thus we only need a test to verify that the derivation paths are good, and that all key metadata records are deleted from the migrated wallet.
ACKs for top commit:
murchandamus:
re-ACK c7e2b9e2644442b147880becb8a659f3d00092d9 via range-diff:
rkrux:
re-ACK c7e2b9e2644442b147880becb8a659f3d00092d9
furszy:
utACK c7e2b9e2644442b147880becb8a659f3d00092d9
Tree-SHA512: 3117c4a43798972109fe2d3539341a8b69db70c6457fcabdd019e6044834dc4b17212abbc006d7b8008f560dce4b7856142b057981b9404f406d58fa0955cbd9
fa58f40b898ba6c57655bf38a241fb10107d4a3a test: Slim down previous releases bdb check (MarcoFalke)
Pull request description:
The check iterates over several previous BDB-only releases to check that descriptor wallets are considered "corrupt" when loading. It is unclear why this needs to be done for more than one release.
Avoid the confusion by removing the unused releases from the test and from the download script.
ACKs for top commit:
achow101:
ACK fa58f40b898ba6c57655bf38a241fb10107d4a3a
rkrux:
ACK fa58f40b898ba6c57655bf38a241fb10107d4a3a
Tree-SHA512: 8084392481bfe1fba9b80bb865ffbdfa454e9e6e14e02c39fa3f61c1a596b1def2c531c5da1c7566e5fddb77ac7e56f19feabaaf9b5af043fa6c230d9e2370b5
The Consensus Cleanup soft fork proposal includes enforcing that coinbase transactions set their
locktime field to the block height, minus 1 (as well as their nSequence such as to not disable the
timelock). If such a fork were to be activated by Bitcoin users, miners need to be ready to produce
compliant blocks at the risk of losing substantial amounts mining would-be invalid blocks. As miners
are unfamously slow to upgrade, it's good to make this change as early as possible.
Although Bitcoin Core's GBT implementation does not provide the "coinbasetxn" field, and mining
pool software crafts the coinbase on its own, updating the Bitcoin Core mining code is a first step
toward convincing pools to update their (often closed source) code. A possible followup is also to
introduce new fields to GBT. In addition, this first step also makes it possible to test future
Consensus Cleanup changes.
The changes to the seemingly-unrelated RBF tests is because these tests assert an error message
which may vary depending on the txid of the transactions used in the test. This commit changes the
coinbase transaction structure and therefore impact the txid of transactions in all tests.
The change to the "Bad snapshot" error message in the assumeutxo functional test is because this
specific test case reads into the txid of the next transaction in the snapshot and asserts the error
message based it gets on deserializing this txid as a coin for the previous transaction. As this
commit changes this txid it impacts the deserialization error raised.
A bug in 0.21.x and 22.x resulted in some wallets having invalid
derivation paths that are the concatenation of two derivation paths.
These appear only when inactive hd chains are topped up.
Since key metadata is a legacy wallet only record, migrating legacy
wallets to descriptor wallets will fix this issue as all key metadata
records are deleted. The derivation path information is derived
on-the-fly from the descriptor that is produced for the inactive hd
chain.
Thus we only need a test to verify that the derivation paths are good,
and that all key metadata records are deleted from the migrated wallet.
Legacy wallets do not have the descriptors flag set. Don't load wallets
without the descriptors flag.
At the same time, we will no longer load BDB databases since they are
only used for legacy wallets.
