6cebac598e5e85eadd60eb1274d7f33d63ce1108 test: MiniWallet: introduce enum type for output mode (Sebastian Falbesoner)
Pull request description:
This is a follow-up PR to #21945 which lifted the number of MiniWallet's tx output modes from 2 to 3 (by adding P2PK Support).
Since the current way of specifying the mode on the ctor via two booleans is ugly and error-prone (see table in comment https://github.com/bitcoin/bitcoin/pull/21945#issuecomment-842526575), a new Enum type `MiniWalletMode` is introduced that can hold the following values:
- ADDRESS_OP_TRUE
- RAW_OP_TRUE
- RAW_P2PK
Also adds documentation that should guide the user on which mode is useful for what etc. with a summary table. (Can also be split up in a separate commit or shortened if that is desired, maybe it's considered to be too verbose).
ACKs for top commit:
MarcoFalke:
cr ACK 6cebac598e5e85eadd60eb1274d7f33d63ce1108
Tree-SHA512: cbbc10806d9d9e62829548094415e9f1a281cd247b9a9fc7f7f33b923c723aa03e7bc3024623a77fb1f7da4d73455fa8244840f746980d32acdad97ee12100da
4bea30169218e2f21e0c93a059966b41c8edd205 test: use P2PK-MiniWallet for feature_csv_activation.py (Sebastian Falbesoner)
dc7eb64e83f5b8e63f12729d5f77b1c920b136e4 test: MiniWallet: add P2PK support (Sebastian Falbesoner)
Pull request description:
This PR adds support for creating and spending transactions with raw pubkey (P2PK) outputs to MiniWallet, [as suggested by MarcoFalke](https://github.com/bitcoin/bitcoin/pull/21900#discussion_r629524841). Using that mode in the test `feature_csv_activation.py`, all txs submitted to the mempool follow the standard policy, i.e. `-acceptnonstdtxn=1` can be removed.
Possible follow-ups:
* Improve MiniWallet constructor Interface; an enum-like parameter instead of two booleans would probably be better
* Look at other tests that could benefit from P2PK (e.g. feature_cltv.py?)
* Check vsize also for P2PK txs (vsize varies due to signature, i.e. a range has to be asserted)
ACKs for top commit:
laanwj:
Code review ACK 4bea30169218e2f21e0c93a059966b41c8edd205
Tree-SHA512: 9b428e6b7cfde59a8c7955d5096cea88af1384a5f49723f00052e9884d819d952d20a5ab39bb02f9d8b6073769c44462aa265d84a33e33da33c2d21670c488a6
bd7f27d16dacf6f7de3b4f6bd052def41d9601be refactor: feature_csv_activation.py: move tx helper functions to methods (Sebastian Falbesoner)
2eca46b0aa0ecf4738500b53523d7013985b387d test: use MiniWallet for feature_csv_activation.py (Sebastian Falbesoner)
Pull request description:
This PR enables one more of the non-wallet functional tests (feature_csv_activation.py) to be run even with the Bitcoin Core wallet disabled by using the new MiniWallet instead, as proposed in #20078.
Short reviewers guideline:
- Since we exclusively work with anyone-can-spend outputs here (raw scriptPubKey = OP_TRUE), signing is not needed anymore. The function `sign_transaction` and its calls are removed, after changing a tx (e.g. its scriptSig or nVersion) a simple `.rehash()` call is sufficient. Also, generating an address `self.nodeaddress` (and with that, passing it to the the various test tx creation/sending helper methods) is not needed anymore and removed.
- The test repeatedly uses the same input for creating different txs (e.g. with different txversions 1 and 2). To let `MiniWallet` create a tx with a specific input, we have to call `.get_utxo()` before which also marks the UTXO as spent. The method is changed to also support keeping the UTXO in its internal list (`mark_as_spent=False`). With the behaviour on master, the second call to `.get_utxo()` with the same input would fail.
- To keep the diff in the first commit short, the `miniwallet` is set as a global variable, to avoid passing it on every tx creation/spending helper. The global is eliminated in the second (refactoring) commit, where all the helpers are moved to the test class as methods. By that, we can use `self.nodes[0]` directly in the helpers and don't have to pass it again and again. I think there could still be a lot of improvements/refactoring done in the test, but that should hopefully serve as a good basis.
ACKs for top commit:
laanwj:
Code review ACK bd7f27d16dacf6f7de3b4f6bd052def41d9601be
MarcoFalke:
review ACK bd7f27d16dacf6f7de3b4f6bd052def41d9601be 🐕
Tree-SHA512: 24fb6a0f7702bae40d5271d197119827067d4b597e954d182e4c1aa5d0fa870368eb3ffed469b26713fa8ff8eb3ecc06abc80b2449cd68156d5559e7ae8a2b11
2db69d7b81 chore: add release notes for "quorum platformsign" (Konstantin Akimov)
283c5f89a2 feat: create new composite command "quorum platformsign" (Konstantin Akimov)
Pull request description:
## Issue being fixed or feature implemented
It splits from #6100
With just whitelist it is impossible to limit the RPC `quorum sign` to use only one specific quorum type, this PR aim to provide ability for quorum signing for platform quorum only.
## What was done?
Implemented a new composite command "quorum platformsign"
This composite command let to limit quorum type for signing for case of whitelist.
After that old way to limit platform commands can be deprecated - #6105
## How Has This Been Tested?
Updated a functional tests to use platform signing for Asset Unlocks feature.
## Breaking Changes
N/A
## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [x] I have added or updated relevant unit/integration/functional/e2e tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone
ACKs for top commit:
UdjinM6:
utACK 2db69d7b81
PastaPastaPasta:
utACK 2db69d7b81
Tree-SHA512: b0dff9934137c4faa85664058e1e77f85067cc8d931e6d76ee5b9e610164ac8b0609736d5f09475256cb78d65bf92466624d784f0b13d20136df7e75613662cb
createwallet has changed list of arguments: createwallet "wallet_name" ( disable_private_keys blank "passphrase" avoid_reuse descriptors load_on_startup )
load_on_startup used to be an argument 5 but now has a number 6.
Both arguments 5 and 6 are boolean and it can confuse an user.
To prevent confusion if user is not aware about this breaking changes,
the RPC createwallet throws an exception if user trying to create descriptor wallet but has not mentioned load_on_startup.
This requirement can be removed when major amount of users updated to v21
faa137eb9eac5554504b062a6dc865ca87fd572b test: Speed up rpc_blockchain.py by removing miniwallet.generate() (MarcoFalke)
fa1fe80c757df0adcbfaf41b5c5c8a468bc07b6f test: Change address type from P2PKH to P2WSH in rpc_blockchain (MarcoFalke)
fa4d8f3169e38cbdbae20258efebe7070c49f522 test: Cache 25 mature coins for ADDRESS_BCRT1_P2WSH_OP_TRUE (MarcoFalke)
fad25153f5c8e88f72cf666b16b0b0dbdc45d3b1 test: Remove unused bug workaround (MarcoFalke)
faabce7d07c5776e4116b1a7ad1f6c408a4a4e46 test: Start only the number of nodes that are needed (MarcoFalke)
Pull request description:
Speed up various tests:
* Remove unused nodes, which only consume time on start/stop
* Remove unused "bug workarounds"
* Remove the need for `miniwallet.generate()` by adding `miniwallet.scan_blocks()`. (On my system, with valgrind, generating 105 blocks takes 3.31 seconds. Rescanning 5 blocks takes 0.11 seconds.)
ACKs for top commit:
laanwj:
Code review ACK faa137eb9eac5554504b062a6dc865ca87fd572b
Tree-SHA512: ead1988d5aaa748ef9f8520af1e0bf812cf1d72e281ad22fbd172b7306d850053040526f8adbcec0b9a971c697a0ee7ee8962684644d65b791663eedd505a025
c9095b738fd4257ae5bdbb2ae38d3e7f41f51b64 test: remove unnecessary assignment in bdb (Bruno Garcia)
Pull request description:
This PR removes the unnecessary assignment to page_info['entries'] on line 54 since there is another assignment for it in line 59.
I think a lint (#21096) would detect cases like this one.
ACKs for top commit:
achow101:
ACK c9095b738fd4257ae5bdbb2ae38d3e7f41f51b64
theStack:
Code Review ACK c9095b738fd4257ae5bdbb2ae38d3e7f41f51b64
Tree-SHA512: 23377077c015b04361fd416b41bf6806ad0bdd4d264be6760f0fd3bc88d694d2cd52cae250519925c5d3b3c70715772714c3863f8fa181a2eb4883204ccdbf9d
5730a43703f7e5a5ca26245ba3b55fbdd027d0b6 test: Add functional test for AddrFetch connections (Martin Zumsande)
c34ad3309f93979b274a37de013502b05d25fad8 net, rpc: Enable AddrFetch connections for functional testing (Martin Zumsande)
533500d9072b7d5a36a6491784bdeb9247e91fb0 p2p: Add timeout for AddrFetch peers (Martin Zumsande)
b6c5d1e450dde6a54bd785504c923adfb45c7060 p2p: AddrFetch - don't disconnect on self-announcements (Martin Zumsande)
Pull request description:
AddrFetch connections (old name: oneshots) are intended to be short-lived connections on which we ask a peer for addresses via `getaddr` and disconnect after receiving them.
This is done by disconnecting after receiving the first `addr`. However, it is no longer working as intended, because nowadays, the first `addr` a typical bitcoin core node sends is its self-announcement.
So we'll disconnect before the peer gets a chance to answer our `getaddr`.
I checked that this affects both `-seednode` peers specified manually, and DNS seeds when AddrFetch is used as a fallback if DNS doesn't work for us.
The current behavior of getting peers via AddrFetch when starting with an empty addrman would be to connect to the peer, receive its self-announcement and add it to addrman, disconnect, reconnect to the same peer again as a full outbound (no other addresses in addrman) and then receive more `addr`. This is silly and not in line with AddrFetch peer being intended to be short-lived peers.
Fix this by only disconnecting after receiving an `addr` message of size > 1.
[Edit] As per review discussion, this PR now also adds a timeout after which we disconnect if we haven't received any suitable `addr`, and a functional test.
ACKs for top commit:
amitiuttarwar:
reACK 5730a43703f7e5a5ca26245ba3b55fbdd027d0b6
naumenkogs:
ACK 5730a43703f7e5a5ca26245ba3b55fbdd027d0b6
jnewbery:
ACK 5730a43703
Tree-SHA512: 8a81234f37e827705138eb254223f7f3b3bf44a06cb02126fc7990b0d231b9bd8f07d38d185cc30d55bf35548a6fdc286b69602498d875b937e7c58332158bf9
5f9c0b6360215636cfa62a70d3a70f1feb3977ab wallet: Remove -upgradewallet from dummywallet (MarcoFalke)
a314271f08215feba53ead27096ac7fda34acb3c test: Remove unused wallet.dat (MarcoFalke)
bf7635963c03203e7189ddaa56c6b086a0108cbf tests: Test specific upgradewallet scenarios and that upgrades work (Andrew Chow)
4b418a9decc3e855ee4b0bbf9e61121c8e9904e5 test: Add test_framework/bdb.py module for inspecting bdb files (Andrew Chow)
092fc434854f881330771a93a1280ac67b1d3549 tests: Add a sha256sum_file function to util (Andrew Chow)
0bd995aa19be65b0dd23df1df571c71428c2bc32 wallet: upgrade the CHDChain version number when upgrading to split hd (Andrew Chow)
8e32e1c41c995e832e643f605d35a7aa112837e6 wallet: remove nWalletMaxVersion (Andrew Chow)
bd7398cc6258c258e9f4411c50630ec4a552341b wallet: have ScriptPubKeyMan::Upgrade check against the new version (Andrew Chow)
5f720544f34dedf75b063b962845fa8eca604514 wallet: Add GetClosestWalletFeature function (Andrew Chow)
842ae3842df489f1b8d68e67a234788966218184 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 5f9c0b6360215636cfa62a70d3a70f1feb3977ab
jonatack:
ACK 5f9c0b6360215636cfa62a70d3a70f1feb3977ab, 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
10a006e626 test: disable ipv6 tests for now (UdjinM6)
Pull request description:
## Issue being fixed or feature implemented
Our CI nodes on Amazon have issues running tests using ipv6.
## What was done?
Pretend we don't have ipv6 when we run tests for now
## How Has This Been Tested?
See CI results for this PR.
## Breaking Changes
n/a but we'll have ipv6 not being tested for some time.
## Checklist:
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
ACKs for top commit:
kwvg:
ACK 10a006e626
PastaPastaPasta:
utACK 10a006e
Tree-SHA512: 75da180b916a5a99b1363f152494cbdd1031c7daf2d5eb370ca74547dee694ad04db3e9c677f0f738b9dbb15760d685a6b43b032dde642a4c1d49fff671e2aac
68ace23fa3bc01baa734ddf2c0963acae1c75ed1 test: convert docs into type annotations in test_framework/test_node.py (fanquake)
8bfcba36db974326d258c610456bd55cf5818b1e test: convert docs into type annotations in test_framework/wallet.py (fanquake)
b043ca8e8b65199061ebe4bbed2200504dfc6ce9 test: convert docs into type annotations in test_framework/util.py (fanquake)
Pull request description:
Rather than having function types exist as documentation, make them type annotations, which enables more `mypy` checking.
ACKs for top commit:
instagibbs:
utACK 68ace23fa3
Tree-SHA512: b705f26b48baabde07b9b2c0a8d547b4dcca291b16eaf5ac70462bb3a1f9e9c2783d93a9d4290889d0cbb3f7db3671446754411a1f498b265d336f6ff33478df
ceefab5226 fix: feature_backwards compatible works now with as expected if no bdb compiled (Konstantin Akimov)
b20f812674 fix: follow-up fixes for functional tests used protx (Konstantin Akimov)
655146d5e7 Merge #21302: wallet: createwallet examples for descriptor wallets (W. J. van der Laan)
99a8b60393 Merge #21063: wallet, rpc: update listdescriptors response format (fanquake)
6ee2c7cc59 Merge #21277: wallet: listdescriptors uses normalized descriptor form (Wladimir J. van der Laan)
8bacdbf71f Merge #19136: wallet: add parent_desc to getaddressinfo (Samuel Dobson)
f567de007a chore: release notes for 5965 with wallet tool improvements (Konstantin Akimov)
0daf360edf chore: add TODO to implement mnemonic for descriptor wallets (Konstantin Akimov)
5016294307 chore: move functional test wallet_multiwallet from category "slow 5 minutes" to "fast test" (Konstantin Akimov)
ef7ce87c1b fix: remove workarounds introduced due to missing bitcoin#20267 (bdb is not compiled) (Konstantin Akimov)
06b2d85bb4 partial Merge #20267: Disable and fix tests for when BDB is not compiled (Wladimir J. van der Laan)
Pull request description:
## Issue being fixed or feature implemented
https://github.com/dashpay/dash-issues/issues/59
## Extra notes
This commit `chore: move functional test wallet_multiwallet from category "slow 5 minutes" to "fast test"` is not directly connected to descriptor wallets, but added to this PR due to conflicts with 20267
## What was done?
It steadily improves support of descriptor wallets in Dash core.
Done backports and related fixes:
- partial bitcoin/bitcoin#20267
- bitcoin/bitcoin#19136
- bitcoin/bitcoin#21277
- bitcoin/bitcoin#21063
- bitcoin/bitcoin#21302
Beside backports and related fixes, this PR includes release notes for previous batch of backports for descriptor wallets support #5965
## How Has This Been Tested?
Run unit functional tests
## Breaking Changes
N/A
## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [x] I have added or updated relevant unit/integration/functional/e2e tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone
Top commit has no ACKs.
Tree-SHA512: f4b2033f8c4fa1d0f72cfc31378909703b3ae8f44748989ff00c3e71311ac80ac37837137133c7e4a166823a941ed7df10efa09c89f5b213f3c8ede7d3d6e8f4
1fedf470cd test: add type annotation for `ADDRS` in `p2p_addrv2_relay` (Kittywhiskers Van Gogh)
022b76f20b merge bitcoin#23218: Use mocktime for ping timeout (Kittywhiskers Van Gogh)
45d9e58023 merge bitcoin#22960: Set peertimeout in write_config (Kittywhiskers Van Gogh)
06e909b737 merge bitcoin#22604: address rate-limiting follow-ups (Kittywhiskers Van Gogh)
60b3e08ed1 merge bitcoin#22616: address relay fixups (Kittywhiskers Van Gogh)
8b8fbc5226 merge bitcoin#22618: Small follow-ups to 21528 (Kittywhiskers Van Gogh)
18fe765988 merge bitcoin#21528: Reduce addr blackholes (Kittywhiskers Van Gogh)
c1874c6615 net_processing: gate `m_tx_relay` access behind `!IsBlockOnlyConn()` (Kittywhiskers Van Gogh)
602d13d2a2 merge bitcoin#22387: Rate limit the processing of rumoured addresses (Kittywhiskers Van Gogh)
fe66202c05 merge bitcoin#22211: relay I2P addresses even if not reachable (by us) (Kittywhiskers Van Gogh)
7e08db55fe merge bitcoin#22306: Improvements to p2p_addr_relay.py (Kittywhiskers Van Gogh)
ff3497c18b merge bitcoin#21843: enable GetAddr, GetAddresses, and getnodeaddresses by network (Kittywhiskers Van Gogh)
51edeb082c merge bitcoin#21594: add network field to getnodeaddresses (Kittywhiskers Van Gogh)
Pull request description:
## Additional Information
* Dependency for https://github.com/dashpay/dash/pull/5982
* Population of `ADDRS` in `p2p_addr`(`v2`)`_relay` in Dash is done in the test object ([source](0a62b9f985/test/functional/p2p_addrv2_relay.py (L42-L49))) as opposed to upstream, where it is done in the global state ([source](d930c7f5b0/test/functional/p2p_addrv2_relay.py (L23-L35))). This is because Dash specifically relies on `self.mocktime` instead of Bitcoin, which will work with simply sampling current time (`time.time()`).
* [bitcoin#22211](https://github.com/bitcoin/bitcoin/pull/22211) adds changes ([source](https://github.com/bitcoin/bitcoin/pull/22211/files#diff-d3d7b1bb23f25a96c9c7444a79159ad1799895565f99efebf1618e41e886bd53R44-R46)) that add usage of `ADDRS` outside the test object. That, alongside with other considerations, resulted in [dash#5967](https://github.com/dashpay/dash/pull/5967) and a discussion ([source](https://github.com/dashpay/dash/pull/5967/files#r1548101561))
* Eventually, following the footsteps of [dash#5967](https://github.com/dashpay/dash/pull/5967), `ADDRS` was defined outside but setup within the test object. This worked just fine ([build](https://gitlab.com/dashpay/dash/-/jobs/6594036014)) but displeased the linter ([build](https://gitlab.com/dashpay/dash/-/jobs/6594035886)) because `ADDRS` type could not be implicitly determined solely on usage in the global scope.
* An attempt to correct this was done by realignment with upstream ([commit](262d00682c)), which pleased the linter ([build](https://gitlab.com/dashpay/dash/-/jobs/6597322521)) but broken the test ([build](https://gitlab.com/dashpay/dash/-/jobs/6597322548)) for the reasons as mentioned above.
* Therefore, to keep the linter happy, `ADDRS` has been annotated as a `List[CAddress]` (which involved importing `List` but that's fine) ([commit](cb6d36df7d))
* Working on [bitcoin#21528](https://github.com/bitcoin/bitcoin/pull/21528) proved challenging due to differences in Dash's and Bitcoin's approach to relaying and the workarounds used to accommodate for that.
* Bitcoin conditionally initializes `m_tx_relay` ([source](3f7250b328/src/net.cpp (L2989-L2991))) and can always check if transaction relaying is permitted by checking if it's initialized ([source](3f7250b328/src/net_processing.cpp (L1820-L1826))).
* Dash unconditionally initializes it ([source](0a62b9f985/src/net.h (L605-L607))). Earlier, Dash used to check if it's _appropriate_ to relay transactions by checking if it can relay addresses ([source](dc6f52ac99/src/net_processing.cpp (L2134-L2140))), which at the time, simply meant, it wasn't a block-only connection ([source](dc6f52ac99/src/net.h (L568-L572))).
* This mutual exclusivity no longer held true in [dash#5964](https://github.com/dashpay/dash/pull/5964) and therefore, some transaction relay decisions were bound to **not** being a block-only connection ([commit](26c39f5b92)) but some were left behind, adopting `RelayAddrsWithPeer()` ([source](0a62b9f985/src/net_processing.cpp (L2215-L2221))), which, to be noted, is determined by the initialization status of `Peer::m_addr_known` ([source](0a62b9f985/src/net_processing.cpp (L839-L842))), which, so far, was pegged to **not** block-relay connection status ([source](0a62b9f985/src/net_processing.cpp (L1319))).
* [bitcoin#21528](https://github.com/bitcoin/bitcoin/pull/21528) got rid of `RelayAddrsWithPeer()` and replaced it with `Peer::m_addr_relay_enabled` ([source](3f7250b328/src/net_processing.cpp (L237-L251))), which is setup using `Peer::SetupAddressRelay()` ([source](3f7250b328/src/net_processing.cpp (L637-L643))). This means, rather than defining the address relay status during construction, it is setup during the first address-related message (i.e. `ADDR`, `ADDRV2`, `GETADDR`) ([source](3f7250b328/src/net_processing.cpp (L227-L236))).
* Meaning, until the first addr-related message happens, the state is has not been determined and defaults to `false`. Because some `m_tx_relay` usage still piggybacked on addr-relay permission to determine tx-relay, if a transaction message is processed before an address message is processed, there will be a false-negative condition.
The transaction relay logic won't run since it's expecting that if transactions can be relayed, so can addresses and checks for address relaying but believes that it cannot do address relaying, borrowing that state for transaction relaying, despite address relaying permissions actually being indeterminate since it hasn't had a chance to validate its eligibility.
* There were two approaches, run `SetupAddressRelay()` as early in the connection as possible to substitute for the "determine at construction" behaviour and change no other conditional statements... and break address-related tests _or_ move the remaining conditional transaction relay logic to use **not** block-only connection checks instead.
* We've gone with the latter, resulting in some changes where the condition only changes form but is the same (`RelayAddrsWithPeer()` > `Peer::m_addr_relay_enabled`) ([source](109c5a9383 (diff-6875de769e90cec84d2e8a9c1b962cdbcda44d870d42e4215827e599e11e90e3L2131-L2134))) but other changes where the condition itself has been changed (`RelayAddrsWithPeer()` > `!CNode::IsBlockOnlyConn()`) ([source](109c5a9383 (diff-6875de769e90cec84d2e8a9c1b962cdbcda44d870d42e4215827e599e11e90e3R2256-R2259)))
* This does mean that in [dash#5982](https://github.com/dashpay/dash/pull/5982), `Peer::m_block_relay_only` is introduced to be the counterpart to `Peer::m_addr_relay_enabled` ([source](45b48dae0a/src/net_processing.cpp (L321-L322))) to account for some `CConnman` logic being moved into `PeerManager` ([source](45b48dae0a/src/net_processing.cpp (L2186-L2195))), which, in a way, reverts [dash#5339](https://github.com/dashpay/dash/pull/5339) but also, doesn't, since it moves the information into `Peer` instead of reinstating it into `CNode`.
* This was eventual since the underlying presumption that `CNode::IsAddrRelayPeer() == !CNode::IsBlockOnlyConn()` no longer holds true (also because `CNode::IsAddrRelayPeer()` doesn't exist anymore).
Special thanks to @UdjinM6 for help with understanding Dash-specifics with respect to functional tests through help on [dash#5964](https://github.com/dashpay/dash/pull/5964) and [dash#5967](https://github.com/dashpay/dash/pull/5967)
## Breaking Changes
None expected.
RPC changes have been introduced in `getnodeaddresses`, where a new input `network`, can filter addresses based on desired network and a new output, also `network`, will associate the address with the origin network. This change is expected to be backwards-compatible.
## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [x] I have added or updated relevant unit/integration/functional/e2e tests
- [x] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
ACKs for top commit:
PastaPastaPasta:
utACK 1fedf470cd
Tree-SHA512: 533d33f79a0d9fd730073b3b9a58baf1dd3b0c95823e765c88a43cc974970ed3609bf1863c63ac7fc5586d1437e5250b0a2d3005468da09e407110a412bd0264
21ad71c578 Merge #21676: test: Use mocktime to avoid intermittent failure in rpc_tests (MarcoFalke)
76a41eb245 Merge #21602: rpc: add additional ban time fields to listbanned (MarcoFalke)
adea52a5fe Merge bitcoin-core/gui#260: Handle exceptions instead of crash (W. J. van der Laan)
7e023c394f Merge #17934: doc: Use CONFIG_SITE variable instead of --prefix option (fanquake)
bc6e3ed6e4 Merge #21606: fuzz: Extend psbt fuzz target a bit (MarcoFalke)
233fb245f7 Merge #21445: cirrus: Use SSD cluster for speedup (fanquake)
a224b800e4 Merge #21609: ci: increase CPU count of sanitizer job to increase memory limit (MarcoFalke)
ad947099a0 test: remove exception for util::Ref which doesn't exist more (Konstantin Akimov)
6674ee85ab Merge #21390: test: Test improvements for UTXO set hash tests (MarcoFalke)
e10eec249b Merge #21338: test: add functional test for anchors.dat (MarcoFalke)
d9c31d6817 Merge #21411: test: add logging, reduce blocks, move sync_all in wallet_ groups (MarcoFalke)
Pull request description:
## Issue being fixed or feature implemented
Regular backports from bitcoin v22
## Note for reviewers:
PRs bitcoin#17934 and bitcoin#21606 have been backported partially in past.
## What was done?
Removed unused sanitizer rules (see bitcoin#21366 and dashpay/dash#5055)
- bitcoin/bitcoin#21338
- bitcoin/bitcoin#21390
- bitcoin/bitcoin#21609
- bitcoin/bitcoin#21445
- bitcoin/bitcoin#21606
- bitcoin/bitcoin#17934
- bitcoin-core/gui#260
- bitcoin/bitcoin#21602
- bitcoin/bitcoin#21676
## How Has This Been Tested?
Run unit/functional tests
## Breaking Changes
N/A
## Checklist:
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone
ACKs for top commit:
PastaPastaPasta:
utACK 21ad71c578
Tree-SHA512: 94276d56255300d7d8c056d15b468720ba028d83cc585b16396e8bad90157e9e010490be239e09cccd4362f575f8df2d56dde3fb505745376c7790d70e3635cd
005a6b104a fix: format string in llmq/commitment - mismatched arguments (Konstantin Akimov)
4774e1e8f6 Merge #19809: log: Prefix log messages with function name and source code location if -logsourcelocations is set (Wladimir J. van der Laan)
43a94f0580 fix: adjust functional tests due to dash's support of thread name after v0.12 (Konstantin Akimov)
085120d9f9 Merge #20993: test: store subversion (user agent) as string in msg_version (MarcoFalke)
e866b43160 Merge #21542: ci: Bump macOS VM image to the latest version (fanquake)
a3702534e5 Merge #21354: build, doc: Drop no longer required packages from macOS cross-compiling dependencies (fanquake)
6bcc86ad3b Merge #21221: [tools] Allow argument/parameter bin packing in clang-format (MarcoFalke)
318c7263d0 Merge #19522: build: fix building libconsensus with reduced exports for Darwin targets (Wladimir J. van der Laan)
88a45d4a9a Merge #21138: ci: Re-run wine tests once if they fail (fanquake)
4abb768456 Merge #21126: ci: Properly bump to focal for win cross build (fanquake)
f254f77d75 Merge #21075: doc: Fix markdown formatting (MarcoFalke)
Pull request description:
## Issue being fixed or feature implemented
Regular backports from bitcoin v22 and related fixes
## What was done?
Follow-up fixes for bitcoin#19809
Backports:
- bitcoin/bitcoin#21075
- bitcoin/bitcoin#21126
- bitcoin/bitcoin#21138
- bitcoin/bitcoin#19522
- bitcoin/bitcoin#21221
- bitcoin/bitcoin#21354
- bitcoin/bitcoin#21542
- bitcoin/bitcoin#19809
- bitcoin/bitcoin#20993
## How Has This Been Tested?
Run unit/functional tests
## Breaking Changes
N/A
Notice, that function name is included now by default to logs with `-logfunctionnames` and many logs have function name twice now such as:
```
node0 2024-04-06T20:13:56.564123Z (mocktime: 2014-12-04T17:15:38Z) [httpworker.3] [masternode/sync.cpp:331] [NotifyHeaderTip] CMasternodeSync::NotifyHeaderTip -- pindexNew->nHeight: 5 fInitialDownload=0
```
For further development need to take it in account and do not use more direct calls `__func__` from code as well as reduce usages in codebase.
## Checklist:
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone
ACKs for top commit:
PastaPastaPasta:
utACK 005a6b104a
Tree-SHA512: f98949c4605dda7d6dfe790554e1d31a8c8178b57520578fcd58178c68fe7af33c0d66a79865683c1357de9a73fa4f484eb828b28e11ca110b8e1915c1fcf9b3
a2f190deff Merge bitcoin-core/gui#115: Replace "Hide tray icon" option with positive "Show tray icon" one (Jonas Schnelli)
65b80e76b7 Merge #21531: test: remove qt byteswap compattests (MarcoFalke)
ba883c5da2 Merge bitcoin-core/gui#139: doc: Improve gui/src/qt README.md (MarcoFalke)
368c65d050 Merge bitcoin-core/gui#72: util: Log static plugins meta data and used style (Jonas Schnelli)
317777e7d7 Merge bitcoin-core/gui#171: Use layout manager for Create Wallet dialog (MarcoFalke)
83313a5603 Merge bitcoin#20789: Rework strong and weak net enum fuzzing (MarcoFalke)
4a3e3af6e7 Merge #20813: scripted-diff: Bump copyright headers (MarcoFalke)
e36eacd868 Merge #18772: rpc: calculate fees in getblock using BlockUndo data (MarcoFalke)
41a1e10954 Merge #20690: Clean up logging of outbound connection type (MarcoFalke)
648d6f04fb Merge bitcoin-core/gui#13: Hide peer detail view if multiple are selected (Jonas Schnelli)
Pull request description:
## Issue being fixed or feature implemented
Regular backports from bitcoin v22
## What was done?
- bitcoin-core/gui#13
- bitcoin-core/gui#115
- bitcoin/bitcoin#20690
- bitcoin/bitcoin#18772
- bitcoin/bitcoin#20813
- bitcoin/bitcoin#20789
- bitcoin-core/gui#171
- bitcoin-core/gui#72
- bitcoin-core/gui#139
- bitcoin/bitcoin#21531
## How Has This Been Tested?
Run unit/functional tests
## Breaking Changes
N/A
## Checklist:
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone
ACKs for top commit:
PastaPastaPasta:
utACK a2f190deff
Tree-SHA512: 29421e7ca38583c47f49c2605775f34b64ae2fb6aeb45ac42941fbc78598fc26e7f7a248b40fcc2c9fd21154b0a6f2aed64287a8b7cca43de1b99ae3dccd990f
Backport notice: changes in feature_notification.py are missing due to #18878 is not done yet
49797c3ccfbb9f7ac9c1fbb574d35b315c103805 tests: Disable bdb dump test when no bdb (Andrew Chow)
1194cf9269e6c4bf67b09c4766f42bf173d12c0a Fix wallet_send.py wallet setup to work with descriptors (Andrew Chow)
fbaea7bfe44822710a36601c6b0febbd5e33dfbd Require legacy wallet for wallet_upgradewallet.py (Andrew Chow)
b1b679e0ab7a9981e3e78424fe8836edd59abf6f Explicitly mark legacy wallet tests as such (Andrew Chow)
09514e1bef46444a67cde9ff13e76bd4b9f8c7ac Setup wallets for interface_zmq.py (Andrew Chow)
4d03ef9a73ceb5ef4e9d184a135bca6bdeb8c311 Use MiniWallet in rpc_net.py (Andrew Chow)
4de23824b0c711ece68f9fc007ffac12126710aa Setup wallets for interface_bitcoin_cli.py (Andrew Chow)
7c71c627d28f0cddaf2349a55336278a681c27c2 Setup wallets with descriptors for feature_notifications (Andrew Chow)
1f1bef8dbab7225884d769a45477ee11d0ebf654 Have feature_filelock.py test both bdb and sqlite, depending on compiled (Andrew Chow)
c77975abc0123b29b0eb3481b8916e7c025b7c4c Disable upgrades tests that require BDB if BDB is not compiled (Andrew Chow)
1f20cac9d41e507901a2811d6db7147d7ab0321b Disable wallet_descriptor.py bdb format check if BDB is not compiled (Andrew Chow)
3641597d7ef6f5097a9e93cab3ef7e0f9c820296 tests: Don't make any wallets unless wallet is required (Andrew Chow)
b9b88f57a9b9a28e0f0614c12ae3012cf5050b10 Skip legacy wallet reliant tests if BDB is not compiled (Andrew Chow)
6f36242389bd3e7eacf594ce90491e8ccca70f3a tests: Set descriptors default based on compilation (Andrew Chow)
Pull request description:
This PR fixes tests for when BDB is not compiled. Tests which rely on or test legacy wallet behavior are disabled and skipped when BDB is not compiled. For the components of some tests that are for legacy wallet things, those parts of the tests are skipped.
For the majority of tests, changes are made so that they can be run with either legacy wallets or descriptor wallets without materially effecting the test. Most tests only need the wallet for balance and transactions, so the type of wallet is not an important part of those tests. Additionally, some tests are wallet agnostic and modified to instead use the test framework's MiniWallet.
ACKs for top commit:
laanwj:
ACK 49797c3ccfbb9f7ac9c1fbb574d35b315c103805
ryanofsky:
Code review ACK 49797c3ccfbb9f7ac9c1fbb574d35b315c103805. Only change since last review is dropping last commit. Previous review w/ suggestions for future followup is https://github.com/bitcoin/bitcoin/pull/20267#pullrequestreview-581508843
Tree-SHA512: 69659f8a81fb437ecbca962f4082c12835282dbf1fba7d9952f727a49e01981d749af9b09feda1c8ca737516c7d7a08ef17e782795df3fa69892d5021b41c1ed
4f2653a89018fa4d24bd2a551832a7410b682600 test: Use deterministic chain in utxo set hash test (Fabian Jahr)
4973c5175c5fd1f4791ea26e8ddefd6fb11ac1c3 test: Remove wallet dependency of utxo set hash test (Fabian Jahr)
1a27af1d7b5ec18b4248ead1eaf0f381047b4b24 rpc: Improve gettxoutsetinfo help (Fabian Jahr)
Pull request description:
Follow-ups to #19145:
- Small improvement on the help text of RPC gettxoutsetinfo
- Using deterministic blockchain in the test `functional/feature_utxo_set_hash.py`
- Removing wallet dependency in the test `functional/feature_utxo_set_hash.py`
Split out of #19521.
ACKs for top commit:
MarcoFalke:
review ACK 4f2653a89018fa4d24bd2a551832a7410b682600 👲
Tree-SHA512: 92927b3aa22b6324eb4fc9d346755313dec44d973aa69a0ebf80a8569b5f3a7cf3539721ebdba183737534b9e29b3e33f412515890f0d0b819878032a3bba8f9
b4511e2e2ed1a6077ae6826a9ee6b7a311293d08 log: Prefix log messages with function name if -logsourcelocations is set (practicalswift)
Pull request description:
Prefix log messages with function name if `-logfunctionnames` is set.
Yes, exactly like `-logthreadnames` but for function names instead of thread names :)
This is a small developer ergonomics improvement: I've found this to be a cheap/simple way to correlate log output and originating function.
For me it beats the ordinary cycle of 1.) try to figure out a regexp matching the static part of the dynamic log message, 2.) `git grep -E 'Using .* MiB out of .* requested for signature cache'`, 3.) `mcedit filename.cpp` (`openemacs filename.cpp` works too!) and 4.) search for log message and scroll up to find the function name :)
Without any logging parameters:
```
$ src/bitcoind -regtest
2020-08-25T03:29:04Z Using RdRand as an additional entropy source
2020-08-25T03:29:04Z Using 16 MiB out of 32/2 requested for signature cache, able to store 524288 elements
2020-08-25T03:29:04Z Using 16 MiB out of 32/2 requested for script execution cache, able to store 524288 elements
2020-08-25T03:29:04Z Loaded best chain: hashBestChain=0fff88f13cb7b2c71f2a335e3a4fc328bf5beb436012afca590b1a11466e22ff height=0 date=2011-02-02T23:16:42Z progress=1.000000
2020-08-25T03:29:04Z block tree size = 1
2020-08-25T03:29:04Z nBestHeight = 0
2020-08-25T03:29:04Z Imported mempool transactions from disk: 0 succeeded, 0 failed, 0 expired, 0 already there, 0 waiting for initial broadcast
2020-08-25T03:29:04Z 0 addresses found from DNS seeds
```
With `-logthreadnames` and `-logfunctionnames`:
```
$ src/bitcoind -regtest -logthreadnames -logfunctionnames
2020-08-25T03:29:04Z [init] [ReportHardwareRand] Using RdRand as an additional entropy source
2020-08-25T03:29:04Z [init] [InitSignatureCache] Using 16 MiB out of 32/2 requested for signature cache, able to store 524288 elements
2020-08-25T03:29:04Z [init] [InitScriptExecutionCache] Using 16 MiB out of 32/2 requested for script execution cache, able to store 524288 elements
2020-08-25T03:29:04Z [init] [LoadChainTip] Loaded best chain: hashBestChain=0fff88f13cb7b2c71f2a335e3a4fc328bf5beb436012afca590b1a11466e22ff height=0 date=2011-02-02T23:16:42Z progress=1.000000
2020-08-25T03:29:04Z [init] [AppInitMain] block tree size = 1
2020-08-25T03:29:04Z [init] [AppInitMain] nBestHeight = 0
2020-08-25T03:29:04Z [loadblk] [LoadMempool] Imported mempool transactions from disk: 0 succeeded, 0 failed, 0 expired, 0 already there, 0 waiting for initial broadcast
2020-08-25T03:29:04Z [dnsseed] [ThreadDNSAddressSeed] 0 addresses found from DNS seeds
```
ACKs for top commit:
laanwj:
Code review ACK b4511e2e2ed1a6077ae6826a9ee6b7a311293d08
MarcoFalke:
review ACK b4511e2e2ed1a6077ae6826a9ee6b7a311293d08 🌃
Tree-SHA512: d100f5364630c323f31d275259864c597f7725e462d5f4bdedcc7033ea616d7fc0d16ef1b2af557e692f4deea73c6773ccfc681589e7bf6ba970b9ec169040c7
de85af5cce727981383ac0fe81f635451b331f23 test: store subversion (user agent) as string in msg_version (Sebastian Falbesoner)
Pull request description:
It seems more natural to treat the "subversion" field (=user agent string, see [BIP 14](https://github.com/bitcoin/bips/blob/master/bip-0014.mediawiki#Proposal)) of a node as pure string rather than a bytestring within the test framework. This is also suggested with the naming prefix in `msg_version.strSubVer`: one probably wouldn't expect a field starting with "str" to be a bytestring that needs further decoding to be useful. This PR moves the encoding/decoding parts to the serialization/deserialization routines so that the user doesn't have to bother with that anymore.
Note that currently, in the master branch the `msg_version.strSubVer` is never read (only in `msg_version.__repr__`); However, one issue that is solved by this PR came up while testing #19509 (not merged yet): A decoding script for binary message capture files takes use of the functional test framework convert it into JSON format. Bytestrings will be convered to hexstrings, while pure strings will (surprise surprise) end up without modification in the file.
So without this patch, we get:
```
$ jq . out.json | grep -m5 strSubVer
"strSubVer": "2f5361746f7368693a32312e39392e302f"
"strSubVer": "2f5361746f7368693a302e32302e312f"
"strSubVer": "2f5361746f7368693a32312e39392e302f"
"strSubVer": "2f5361746f7368693a302e32302e312f"
"strSubVer": "2f5361746f7368693a32312e39392e302f"
```
After this patch:
```
$ jq . out2.json | grep -m5 strSubVer
"strSubVer": "/Satoshi:21.99.0/"
"strSubVer": "/Satoshi:0.20.1/"
"strSubVer": "/Satoshi:21.99.0/"
"strSubVer": "/Satoshi:0.20.1/"
"strSubVer": "/Satoshi:21.99.0/"
```
ACKs for top commit:
jnewbery:
utACK de85af5cce727981383ac0fe81f635451b331f23
Tree-SHA512: ff23642705c858e8387a625537dfec82e6b8a15da6d99b8d12152560e52d243ba17431b602b26f60996d897e00e3f37dcf8dc8a303ffb1d544df29a5937080f9
fa0074e2d82928016a43ca408717154a1c70a4db scripted-diff: Bump copyright headers (MarcoFalke)
Pull request description:
Needs to be done because no one has removed the years yet
ACKs for top commit:
practicalswift:
ACK fa0074e2d82928016a43ca408717154a1c70a4db
Tree-SHA512: 210e92acd7d400b556cf8259c3ec9967797420cfd19f0c2a4fa54cb2b3d32ad9ae27e771269201e7d554c0f4cd73a8b1c1a42c9f65d8685ca4d52e5134b071a3
Backport notice:
- data have real blocks from testnet
- due to big gap in blocks before checkpoints (half-year) for sack of this test is added one more checkpoint, otherwise blocks are not accepted due to non-mockable time for other chains except regtest
- data for 2 blocks in split chain are generated locally for testnet
--------------
333317ce6b67aa92f7363d48cd750712190b4b6b test: Test that low difficulty chain fork is rejected (MarcoFalke)
fa31dc1bf4ee471c4641eef8de02702ba0619ae7 test: Pass down correct chain name in tests (MarcoFalke)
Pull request description:
To prevent OOM, Bitcoin Core will reject chain forks at low difficulty by default. This is the only use-case of checkpoints, so add a test for it to make sure the feature works as expected. If it didn't work, checkpoints would have no use-case and we might as well remove them
ACKs for top commit:
Sjors:
Thanks for adding the node 1 example. Code review ACK 333317c
Tree-SHA512: 90dffa540d0904f3cffb61d2382b1a26f84fe9560b7013e4461546383add31a8757b350616a6d43217c59ef7b8b2a1b62bb3bab582c679cbb2c660a782ce7be1
d5d1a714fb Merge bitcoin/bitcoin#24390: test: Remove suppression no longer needed with headers-only Boost.Test (fanquake)
51630d2e5e Merge bitcoin/bitcoin#22824: refactor: remove RecursiveMutex cs_nBlockSequenceId (MarcoFalke)
a9b1575fe8 Merge bitcoin/bitcoin#22781: wallet: fix the behavior of IsHDEnabled, return false in case of a blank hd wallet. (Samuel Dobson)
0505229c89 Merge bitcoin/bitcoin#22327: cli: Avoid truncating -rpcwaittimeout (MarcoFalke)
1dc97c7679 Merge bitcoin/bitcoin#22149: test: Add temporary logging to debug #20975 (W. J. van der Laan)
44f91cbc9a Merge #21597: test: Document race:validation_chainstatemanager_tests suppression (fanquake)
c326830f48 Merge bitcoin-core/gui#243: fix issue when disabling the auto-enabled blank wallet checkbox (MarcoFalke)
267f42fd6a Merge #21382: build: Clean remnants of QTBUG-34748 fix (fanquake)
1fcc5f1101 Merge #20540: test: Fix wallet_multiwallet issue on windows (MarcoFalke)
4afbaf2ea1 Merge #20322: test: Fix intermittent issue in wallet_listsinceblock (MarcoFalke)
Pull request description:
## Issue being fixed or feature implemented
Batch of backports
## What was done?
Trivial batch of backports
## How Has This Been Tested?
CI looks good
## Breaking Changes
None
## Checklist:
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
Top commit has no ACKs.
Tree-SHA512: 8eeac54f011eb1111888c745dd56184ac9601de290f2b0f7b7ad02240e8dc1cab5a47fed26bfed2bd6f1066e0710827a3e5b2426f0bf66821cf1cd09099d5160
faa94961d6e38392ba068381726ed4e033367b03 test: Add temporary logging to debug #20975 (MarcoFalke)
Pull request description:
to be reverted after a fix
ACKs for top commit:
laanwj:
Code review ACK faa94961d6
Tree-SHA512: 1f3103fcf4cad0af54e26c4d257bd824b128b5f2d2b81c302e861a829fd55d6a099fa476b79b30a71fe98975ae604b9e3ff31fd48a51d442389a9bd515e60ba0
bdb8b9a347e68f80a2e8d44ce5590a2e8214b6bb test: doc: improve doc for `from_hex` helper (mention `to_hex` alternative) (Sebastian Falbesoner)
191405420815d49ab50184513717a303fc2744d6 scripted-diff: test: rename `FromHex` to `from_hex` (Sebastian Falbesoner)
a79396fe5f8f81c78cf84117a87074c6ff6c9d95 test: remove `ToHex` helper, use .serialize().hex() instead (Sebastian Falbesoner)
2ce7b47958c4a10ba20dc86c011d71cda4b070a5 test: introduce `tx_from_hex` helper for tx deserialization (Sebastian Falbesoner)
Pull request description:
There are still many functional tests that perform conversions from a hex-string to a message object (deserialization) manually. This PR identifies all those instances and replaces them with a newly introduced helper `tx_from_hex`.
Instances were found via
* `git grep "deserialize.*BytesIO"`
and some of them manually, when it were not one-liners.
Further, the helper `ToHex` was removed and simply replaced by `.serialize().hex()`, since now both variants are in use (sometimes even within the same test) and using the helper doesn't really have an advantage in readability. (see discussion https://github.com/bitcoin/bitcoin/pull/22257#discussion_r652404782)
ACKs for top commit:
MarcoFalke:
review re-ACK bdb8b9a347e68f80a2e8d44ce5590a2e8214b6bb 😁
Tree-SHA512: e25d7dc85918de1d6755a5cea65471b07a743204c20ad1c2f71ff07ef48cc1b9ad3fe5f515c1efaba2b2e3d89384e7980380c5d81895f9826e2046808cd3266e
c7b7e0a69265946aecc885be911c7650911ba2e3 tests: Make only desc wallets for wallet_multwallet.py --descriptors (Andrew Chow)
d4b67ad214ada7645c4ce2d5ec336fe5c3f7f7ca Avoid creating legacy wallets in wallet_importdescriptors.py (Andrew Chow)
6c9c12bf87f95066acc28ea2270a00196eb77703 Update feature_backwards_compatibility for descriptor wallets (Andrew Chow)
9a4c631e1c00eb1661c000978b133d7aa0226290 Update wallet_labels.py to not require descriptors=False (Andrew Chow)
242aed7cc1d003e8fed574bbebd19c7e54e23402 tests: Add a --legacy-wallet that is mutually exclusive with --descriptors (Andrew Chow)
388053e1722632c2e485c56a444bc75cf0152188 Disable some tests for tool_wallet when descriptors (Andrew Chow)
47d3243160fdec7e464cfb8f869be7f5d4ee25fe Make raw multisig tests legacy wallet only in rpc_rawtransaction.py (Andrew Chow)
59d3da5bce4ebd9c2291d8f201a53ee087938b21 Do addmultisigaddress tests in legacy wallet mode in wallet_address_types.py (Andrew Chow)
25bc5dccbfd52691adca6edd418dd54290300c28 Use importdescriptors when in descriptor wallet mode in wallet_createwallet.py (Andrew Chow)
0bd1860300b13b12a25d330ba3a93ff2d13aa379 Avoid dumpprivkey and watchonly behavior in rpc_signrawtransaction.py (Andrew Chow)
08067aebfd7e838e6ce6b030c31a69422260fc6f Add script equivalent of functions in address.py (Andrew Chow)
86968882a8a26312a7af29c572313c4aff488c11 Add descriptor wallet output to tool_wallet.py (Andrew Chow)
3457679870e8eff2a7d14fe59a479692738c48b6 Use separate watchonly wallet for multisig in feature_nulldummy.py (Andrew Chow)
a42652ec10c733a5bf37e418e45d4841f54331b4 Move import and watchonly tests to be legacy wallet only in wallet_balance.py (Andrew Chow)
4b871909d6e4a51888e062d322bf53263deda15e Use importdescriptors for descriptor wallets in wallet_bumpfee.py (Andrew Chow)
c2711e4230d9a423ead24f6609691fb338b1d26b Avoid dumpprivkey in wallet_listsinceblock.py (Andrew Chow)
553dbf9af4dea96e6a3e79bba9607003342029bd Make import tests in wallet_listtransactions.py legacy wallet only (Andrew Chow)
dc81418fd01021070f3f66bab5fee1484456691a Use a separate watchonly wallet in rpc_fundrawtransaction.py (Andrew Chow)
a357111047411f18c156cd34a002a38430f2901c Update wallet_importprunedfunds to avoid dumpprivkey (Andrew Chow)
Pull request description:
I went through all the tests and checked whether they passed with descriptor wallets. This partially informed some changes in #16528. Some tests needed changes to work with descriptor wallets. These were primarily due to import and watchonly behavior. There are some tests and test cases that only test legacy wallet behavior so those tests won't be run with descriptor wallets.
This PR updates more tests to have to the `--descriptors` switch in `test_runner.py`. Additionally a mutually exclusive `--legacy-wallet` option has been added to force legacy wallets. This does nothing currently but will be useful in the future when descriptor wallets are the default. For the tests that rely on legacy wallet behavior, this option is being set so that we don't forget in the future. Those tests are `feature_segwit.py`, `wallet_watchonly.py`, `wallet_implicitsegwit.py`, `wallet_import_with_label.py`, and `wallet_import_with_label.py`.
If you invert the `--descriptors`/`--legacy-wallet` default so that descriptor wallets are the default, all tests (besides the legacy wallet specific ones) will pass.
ACKs for top commit:
MarcoFalke:
review ACK c7b7e0a69265946aecc885be911c7650911ba2e3 🎿
laanwj:
ACK c7b7e0a69265946aecc885be911c7650911ba2e3
Tree-SHA512: 2f4e87815005d1d0a2543ea7947f7cd7593d8cf5312228ef85f8e096f19739b225769961943049cb44f6f07a35b8de988e2246ab9aca5bb5a0b2e62694d5637d
3a83a01694160f2e722e1bc90a328bd569b8e109 [tests] move generate_wif_key to wallet_util.py (John Newbery)
b216b0b71f7853d747af8b712fc250c699f3c320 [tests] sort imports in rpc_createmultisig.py (John Newbery)
e38081846d889fcbbbc6ca4a4d3bca26807fde2f Revert "[TESTS] Move base58 to own module to break circular dependency" (John Newbery)
Pull request description:
generate_wif_key is a wallet utility function. Move it from the EC key module to the wallet util module.
This fixes the circular dependency issue in #17977
ACKs for top commit:
MarcoFalke:
ACK 3a83a01694160f2e722e1bc90a328bd569b8e109 🍪
Tree-SHA512: 24985dffb75202721ccc0c6c5b52f1fa5d1ce7963bccde24389feb913cab4dad0c265274ca67892c46c8b64e6a065a0f23263a89be4fb9134dfefbdbe5c7238a
c75de5da5fdd3a304f9da3d8a2e0370d1723ddd0 [TESTS] Move base58 to own module to break circular dependency (Pieter Wuille)
Pull request description:
I encountered difficulties with the test framework in #17977. This fixes them, and I think the change is generally useful.
ACKs for top commit:
laanwj:
Code review ACK c75de5da5fdd3a304f9da3d8a2e0370d1723ddd0
MarcoFalke:
ACK c75de5da5fdd3a304f9da3d8a2e0370d1723ddd0 according to --color-moved=dimmed-zebra this is a move-only apart from the imports 👒
Tree-SHA512: 9e0493de3e279074f0c70e92c959b73ae30479ad6f2083a3c6bbf4b0191d65ef94854559a5b7c904f5dadc5e93129ed00f6dc0a8ccce6ba7921cd45f7119f74b
Skipped tests:
- feature_filelock
- wallet_descriptors
- interface_bitcoin_cli
And all functional tests which explitely specify they are using non-descriptor wallets
c4a29d0a90b821c443c10891d9326c534d15cf97 Update wallet_multiwallet.py for descriptor and sqlite wallets (Russell Yanofsky)
310b0fde04639b7446efd5c1d2701caa4b991b86 Run dumpwallet for legacy wallets only in wallet_backup.py (Andrew Chow)
6c6639ac9f6e1677da066cf809f9e3fa4d2e7c32 Include sqlite3 in documentation (Andrew Chow)
f023b7cac0eb16d3c1bf40f1f7898b290de4cc73 wallet: Enforce sqlite serialized threading mode (Andrew Chow)
6173269866306058fcb1cc825b9eb681838678ca Set and check the sqlite user version (Andrew Chow)
9d3d2d263c331e3c77b8f0d01ecc9fea0407dd17 Use network magic as sqlite wallet application ID (Andrew Chow)
9af5de3798c49f86f27bb79396e075fb8c1b2381 Use SQLite for descriptor wallets (Andrew Chow)
9b78f3ce8ed1867c37f6b9fff98f74582d44b789 walletutil: Wallets can also be sqlite (Andrew Chow)
ac38a87225be0f1103ff9629d63980550d2f372b Determine wallet file type based on file magic (Andrew Chow)
6045f77003f167bee9a85e2d53f8fc6ff2e297d8 Implement SQLiteDatabase::MakeBatch (Andrew Chow)
727e6b2a4ee5abb7f2dcbc9f7778291908dc28ad Implement SQLiteDatabase::Verify (Andrew Chow)
b4df8fdb19fcded7e6d491ecf0b705cac0ec76a1 Implement SQLiteDatabase::Rewrite (Andrew Chow)
010e3659069e6f97dd7b24483f50ed71042b84b0 Implement SQLiteDatabase::TxnBegin, TxnCommit, and TxnAbort (Andrew Chow)
ac5c1617e7f4273daf24c24da1f6bc5ef5ab2d2b Implement SQLiteDatabase::Backup (Andrew Chow)
f6f9cd6a64842ef23777312f2465e826ca04b886 Implement SQLiteBatch::StartCursor, ReadAtCursor, and CloseCursor (Andrew Chow)
bf90e033f4fe86cfb90492c7e0962278ea3a146d Implement SQLiteBatch::ReadKey, WriteKey, EraseKey, and HasKey (Andrew Chow)
7aa45620e2f2178145a2eca58ccbab3cecff08fb Add SetupSQLStatements (Andrew Chow)
6636a2608a4e5906ee8092d5731595542261e0ad Implement SQLiteBatch::Close (Andrew Chow)
93825352a36456283bf87e39b5888363ee242f21 Implement SQLiteDatabase::Close (Andrew Chow)
a0de83372be83f59015cd3d61af2303b74fb64b5 Implement SQLiteDatabase::Open (Andrew Chow)
3bfa0fe1259280f8c32b41a798c9453b73f89b02 Initialize and Shutdown sqlite3 globals (Andrew Chow)
5a488b3d77326a0d957c1233493061da1b6ec207 Constructors, destructors, and relevant private fields for SQLiteDatabase/Batch (Andrew Chow)
ca8b7e04ab89f99075b093fa248919fd10acbdf7 Implement SQLiteDatabaseVersion (Andrew Chow)
7577b6e1c88a1a7b45ecf5c7f1735bae6f5a82bf Add SQLiteDatabase and SQLiteBatch dummy classes (Andrew Chow)
e87df8258090138d5c22ac46b8602b618620e8a1 Add sqlite to travis and depends (Andrew Chow)
54729f3f4e6765dfded590af5fb28c88331685f8 Add libsqlite3 (Andrew Chow)
Pull request description:
This PR adds a new class `SQLiteDatabase` which is a subclass of `WalletDatabase`. This provides access to a SQLite database that is used to store the wallet records. To keep compatibility with BDB and to complexity of the change down, we don't make use of many SQLite's features. We use it strictly as a key-value store. We create a table `main` which has two columns, `key` and `value` both with the type `blob`.
For new descriptor wallets, we will create a `SQLiteDatabase` instead of a `BerkeleyDatabase`. There is no requirement that all SQLite wallets are descriptor wallets, nor is there a requirement that all descriptor wallets be SQLite wallets. This allows for existing descriptor wallets to work as well as keeping open the option to migrate existing wallets to SQLite.
We keep the name `wallet.dat` for SQLite wallets. We are able to determine which database type to use by searching for specific magic bytes in the `wallet.dat` file. SQLite begins it's files with a null terminated string `SQLite format 3`. BDB has `0x00053162` at byte 12 (note that the byte order of this integer depends on the system endianness). So when we see that there is a `wallet.dat` file that we want to open, we check for the magic bytes to determine which database system to use.
I decided to keep the `wallet.dat` naming to keep things like backup script to continue to function as they won't need to be modified to look for a different file name. It also simplifies a couple of things in the implementation and the tests as `wallet.dat` is something that is specifically being looked for. If we don't want this behavior, then I do have another branch which creates `wallet.sqlite` files instead, but I find that this direction is easier.
ACKs for top commit:
Sjors:
re-utACK c4a29d0a90b821c443c10891d9326c534d15cf97
promag:
Tested ACK c4a29d0a90b821c443c10891d9326c534d15cf97.
fjahr:
reACK c4a29d0a90b821c443c10891d9326c534d15cf97
S3RK:
Re-review ACK c4a29d0a90b821c443c10891d9326c534d15cf97
meshcollider:
re-utACK c4a29d0a90b821c443c10891d9326c534d15cf97
hebasto:
re-ACK c4a29d0a90b821c443c10891d9326c534d15cf97, only rebased since my [previous](https://github.com/bitcoin/bitcoin/pull/19077#pullrequestreview-507743699) review, verified with `git range-diff master d18892dcc c4a29d0a9`.
ryanofsky:
Code review ACK c4a29d0a90b821c443c10891d9326c534d15cf97. I am honestly confused about reasons for locking into `wallet.dat` again when it's so easy now to use a clean format. I assume I'm just very dense, or there's some unstated reason, because the only thing that's been brought up are unrealistic compatibility scenarios (all require actively creating a wallet with non-default descriptor+sqlite option, then trying to using the descriptor+sqlite wallets with old software or scripts and ignoring the results) that we didn't pay attention to with previous PRs like #11687, which did not require any active interfaction.
jonatack:
ACK c4a29d0a90b821c443c10891d9326c534d15cf97, debug builds and test runs after rebase to latest master @ c2c4dbaebd9, some manual testing creating, using, unloading and reloading a few different new sqlite descriptor wallets over several node restarts/shutdowns.
Tree-SHA512: 19145732e5001484947352d3175a660b5102bc6e833f227a55bd41b9b2f4d92737bbed7cead64b75b509decf9e1408cd81c185ab1fb4b90561aee427c4f9751c
b79dbe86a9886b3539512f6c35205f4c5454a952 test: add feature_rbf.py --descriptors to test_runner.py (Sebastian Falbesoner)
166f8ec28e48aa4c0cd94544909c488df553d8ef test: always rescan after importing private keys in `init_wallet` helper (Sebastian Falbesoner)
Pull request description:
The functional test feature_rbf.py currently fails on master branch, if descriptor wallets are used (argument `--descriptors`). This is due to the fact that in this case, a call to the helper `init_wallet`
111c3e06b3/test/functional/test_framework/test_framework.py (L428-L434)
creates a wallet without rescanning the blockchain; the test framework maps the importprivkey RPC calls to the importdescriptors RPC without rescanning by default (timestamp='now'). Fix this by always calling with `rescan=True`, which calls importdescriptors with timestamp=0. Also add `feature_rbf.py --descriptors` to the list of the test runner's calls.
Fixes#23563.
ACKs for top commit:
mjdietzx:
ACK b79dbe86a9886b3539512f6c35205f4c5454a952
Tree-SHA512: a3f3f7a4077066e3c910919d3b5e04bc6b580c1e0a06e9a2fc258950eaea5e59c0f805c8f00432aea722609f2f7e41eebfab653471b76729c5a316825a3d8c86
