For reasons I do not really grok, this PR changes the timing behavior of the
functional tests to reliably expose a deadlock in the claimpegin RPC that has
existed since the 0.17 rebase.
The mechanism is: in `claimpegin` in src/wallet/rpcwallet.cpp:5873, we call
`AcceptToMemoryPoolWorker`. This requires cs_main to be locked, which it is
not (contrast Core's `testmempoolaccept` RPC, which similarly calls
`AcceptToMemoryPoolWorker` from the RPC thread, and locks cs_main immediately
before).
We do *say* that it is locked, in the `LockAssertion` one the line above, but
this was added in ad3d496d78 during the 0.17
rebase (PR #620), apparently to shut up some linter on OSX, and as near as I
can tell it was never true.
Anyway, `AcceptToMemoryPoolWorker` calls through a couple layers which assume
cs_main is locked, to `AcceptSingleTransaction`, which locks m_pool.cs on line
src/validation.cpp:1101. It then calls `PreChecks`, which on line 784 calls
::ChainActive(), which _actually_ locks cs_main, though only briefly. This
line is the deadlock, because we locked m_pool.cs followed by cs_main...
...meanwhile, in src/net_processing.cpp, we lock cs_main at the top of the
`PeerLogicValidation::SendMessages` loop (src/net_processing.cpp:3628). In the
same loop, in the `feefilter` message processing, we call CTxMemPool::GetMinFee
(src/net_processing.cpp:4137), which locks m_pool.cs. Deadlock.
Anyway, that explains the change to locking behavior that I added to an
otherwise test-only PR.
5f9c0b6360 wallet: Remove -upgradewallet from dummywallet (MarcoFalke)
a314271f08 test: Remove unused wallet.dat (MarcoFalke)
bf7635963c tests: Test specific upgradewallet scenarios and that upgrades work (Andrew Chow)
4b418a9dec test: Add test_framework/bdb.py module for inspecting bdb files (Andrew Chow)
092fc43485 tests: Add a sha256sum_file function to util (Andrew Chow)
0bd995aa19 wallet: upgrade the CHDChain version number when upgrading to split hd (Andrew Chow)
8e32e1c41c wallet: remove nWalletMaxVersion (Andrew Chow)
bd7398cc62 wallet: have ScriptPubKeyMan::Upgrade check against the new version (Andrew Chow)
5f720544f3 wallet: Add GetClosestWalletFeature function (Andrew Chow)
842ae3842d wallet: Add utility method for CanSupportFeature (Andrew Chow)
Pull request description:
This PR cleans up the wallet upgrade mechanism a bit, fixes some probably bugs, and adds more test cases.
The `nWalletMaxVersion` member variable has been removed as it made `CanSupportFeature` unintuitive and was causing a couple of bugs. The reason this was introduced originally was to allow a wallet upgrade to only occur when the new feature is first used. While this makes sense for the old `-upgradewallet` option, for an RPC, this does not quite make sense. It's more intuitive for an upgrade to occur if possible if the `upgradewallet` RPC is used as that's an explicit request to upgrade a particular wallet to a newer version. `nWalletMaxVersion` was only relevant for upgrades to `FEATURE_WALLETCRYPT` and `FEATURE_COMPRPUBKEY` both of which are incredibly old features. So for such wallets, the behavior of `upgradewallet` will be that the feature is enabled immediately without the wallet needing to be encrypted at that time (note that `FEATURE_WALLETCRYPT` indicates support for encryption, not that the wallet is encrypted) or for a new key to be generated.
`CanSupportFeature` would previously indicate whether we could upgrade to `nWalletMaxVersion` not just whether the current wallet version supported a feature. While this property was being used to determine whether we should upgrade to HD and HD chain split, it was also causing a few bugs. Determining whether we should upgrade to HD or HD chain split is resolved by passing into `ScriptPubKeyMan::Upgrade` the version we are upgrading to and checking against that. By removing `nWalletMaxVersion` we also fix a bug where you could upgrade to HD chain split without the pre-split keypool.
`nWalletMaxVersion` was also the version that was being reported by `getwalletinfo` which meant that the version reported was not always consistent across restarts as it depended on whether `upgradewallet` was used. Additionally to make the wallet versions consistent with actually supported versions, instead of just setting the wallet version to whatever is given to `upgradewallet`, we normalize the version number to the closest supported version number. For example, if given 150000, we would store and report 139900.
Another bug where CHDChain was not being upgraded to the version supporting HD chain split is also fixed by this PR.
Lastly several more tests have been added. Some refactoring to the test was made to make these tests easier. These tests check specific upgrading scenarios, such as from non-HD (version 60000) to HD to pre-split keypool. Although not specifically related to `upgradewallet`, `UpgradeKeyMetadata` is now being tested too.
Part of the new tests is checking that the wallet files are identical before and after failed upgrades. To facilitate this, a utility function `sha256sum_file` has been added. Another part of the tests is to examine the wallet file itself to ensure that the records in the wallet.dat file have been correctly modified. So a new `bdb.py` module has been added to deserialize the BDB db of the wallet.dat file. This format isn't explicitly documented anywhere, but the code and comments in BDB's source code in file `dbinc/db_page.h` describe it. This module just dumps all of the fields into a dict.
ACKs for top commit:
MarcoFalke:
approach ACK 5f9c0b6360
laanwj:
Code review ACK 5f9c0b6360
jonatack:
ACK 5f9c0b6360, approach seems fine, code review, only skimmed the test changes but they look well done, rebased on current master, debug built and verified the `wallet_upgradewallet.py` test runs green both before and after running `test/get_previous_releases.py -b v0.19.1 v0.18.1 v0.17.2 v0.16.3 v0.15.2`
Tree-SHA512: 7c4ebf420850d596a586cb6dd7f2ef39c6477847d12d105fcd362abb07f2a8aa4f7afc5bfd36cbc8b8c72fcdd1de8d2d3f16ad8e8ba736b6f4f31f133fe5feba
To resolve the `ConstructTransaction` conflict I reverted to the existing code
then manually applied the diff (it adds a parameter then adds a giant pile of
code to the input loop).
This PR reduced the minrelayfee for Elements, but also brought in some
changes from upstream which I mostly just didn't take (since we've
already brought them in, in earlier parts of this rebase).
FIXME: we disable standardness checks for the non-PAK node in the PAK tests.
This is because of a bug in Elements which causes non-PAK nodes to reject
pegouts for standardness reasons. Need to fix it after the rebase.
Also uncommented a bunch of PSBT functional tests (had to add a fee output
to one transaction, update `find_output` to skip CT outputs, and change two
constant checks at the end of the commented-out section).
fa1cd9e1dd test: Remove unused lock arg from BitcoinTestFramework.wait_until (MarcoFalke)
fad2794e93 test: Rename wait until helper to wait_until_helper (MarcoFalke)
facb41bf1d test: Remove unused p2p_lock in VersionBitsWarningTest (MarcoFalke)
Pull request description:
This avoids confusion with the `wait_until` member functions, which should be preferred because they take the appropriate locks and scale the timeout appropriately on their own.
ACKs for top commit:
laanwj:
Code review ACK fa1cd9e1dd
hebasto:
ACK fa1cd9e1dd, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: 319d400085606a4c738e314824037f72998e6657d8622b363726842aba968744f23c56d27275dfe506b8cbbb6e97fc39ca1d325db05d4d67df0e8b35f2244d5c
36ec9801a4 test: Add chacha20 test vectors in muhash (Fabian Jahr)
0e2b400fea test: Add basic Python/C++ Muhash implementation parity unit test (Fabian Jahr)
b85543cb73 test: Add Python MuHash3072 implementation to test framework (Pieter Wuille)
ab30cece0e test: Move modinv to util and add unit test (Fabian Jahr)
Pull request description:
This is the second in a [series of pull requests](https://github.com/bitcoin/bitcoin/pull/18000) to implement an Index for UTXO set statistics.
This pull request adds a Python implementation of Muhash3072, a homomorphic hashing algorithm to be used for hashing the UTXO set. The Python implementation can then be used to compare behavior with the C++ version.
ACKs for top commit:
jnewbery:
utACK 36ec9801a
laanwj:
Code review ACK 36ec9801a4
Tree-SHA512: a3519c6e11031174f1ae71ecd8bcc7f3be42d7fc9c84c77f2fbea7cfc5ad54fcbe10b55116ad8d9a52ac5d675640eefed3bf260c58a02f2bf3bc0d8ec208baa6
