18c5b23a0f [test] Test that -blocksonly nodes still serve compact blocks. (Niklas Gögge)
a79ad65fc2 [test] Test that getdata(CMPCT) is still sent on regular low bandwidth connections. (Niklas Gögge)
5e231c116b [test] Test that -blocksonly nodes do not send getdata(CMPCT) on a low bandwidth connection. (Niklas Gögge)
5bf6587457 [test] Test that -blocksonly nodes do not request high bandwidth mode. (Niklas Gögge)
0dc8bf5b92 [net processing] Dont request compact blocks in blocks-only mode (Niklas Gögge)
Pull request description:
A blocks-only node does not participate in transaction relay to reduce its own bandwidth usage and therefore does not have a mempool. The use of compact blocks is not beneficial to such a node since it will always have to download full blocks.
In both high- and low-bandwidth relaying the `cmpctblock` message is sent. This represent a bandwidth overhead for blocks-only nodes because the `cmpctblock` message is several times larger in the average case than the equivalent `headers` or `inv` announcement.
>**Example:**
>A block with 2000 txs results in a `cmpctblock` with 2000*6 bytes in short ids. This is several times larger than the equivalent 82 bytes for a `headers` message or 37 bytes for an `inv`.
## Approach
This PR makes blocks-only nodes always use the legacy relaying to download new blocks.
It does so by making blocks-only nodes never initiate a high-bandwidth block relay connection by disabling the sending of `sendcmpct(1)`. Additionally a blocks-only node will never request a compact block using `getdata(CMPCT)`.
A blocks-only node will continue to serve compact blocks to its peers in both high- and low-bandwidth mode.
ACKs for top commit:
naumenkogs:
ACK 18c5b23a0f
rajarshimaitra:
tACK 18c5b23a0f
jnewbery:
reACK 18c5b23a0f
theStack:
re-ACK 18c5b23a0f🥛
Tree-SHA512: 0c78804aa397513d41f97fe314efb815efcd852d452dd903df9d4749280cd3faaa010fa9b51d7d5168b8a77e08c8ab0a491ecdbdb3202f2e9cd5137cddc74624
240ea294d5 doc: update doxygen documention of ComputeTimeSmart() and AddToWalletIfInvolvingMe() regarding rescanning_old_block parameter (BitcoinTsunami)
d6eb39af21 test: add functional test to check transaction time determination during block rescanning (BitcoinTsunami)
07b44f16e7 wallet: fix ComputeTimeSmart algorithm to use blocktime during old block rescanning (BitcoinTsunami)
Pull request description:
The function ComputeTimeSmart in wallet.cpp assume that transaction are discovered in the right order.
Moreover the 'smarttime' determination algorithm is coded with realtime scenario in mind and not rescanning of old block.
The functional test demonstrate that if the user import a wallet, then rescan only recent history, and then rescan the entire history, the older transaction discovered would have an incorrect time determination.
In the context of rescanning old block, the only time value that as a meaning is the blocktime.
That's why I've fixed the problem with a simple separation between rescanning of old block and realtime time determination. The fix is written to have no impact on every realtime scenario and only impact the behaviour during a rescanning process.
This PR Fixes#20181.
To be fair, I don't think that this bug could be triggered with the wallet GUI, because it always proceed with a proper rescan.
But RPC API provide the possibility to trigger it. I've discovered it, because Specter desktop v0.10.0 was impacted. (https://github.com/cryptoadvance/specter-desktop/issues/680).
ACKs for top commit:
jonatack:
ACK 240ea294d5 per `git diff b92d552 240ea29`, re-verified rebase to latest master + debug build clean + new test passes on the branch and fails on master, only change since my review a few hours ago is incorporation of latest review suggestions
meshcollider:
re-utACK 240ea294d5
Tree-SHA512: 514b02e41d011ddfa325f5e8080b93800e1ea4ed5853fa420670a6ac700e8b463000dbea65f8ced8565cfb950c7f51b69154034dcb111e67aca3b964a0061494
43cd6b8af9 doc: add release notes for removal of the -deprecatedrpc=addresses flag (Michael Dietz)
2b1fdc2c6c refactor: minor styling, prefer snake case and same line if (Michael Dietz)
d64deac7b8 refactor: share logic between ScriptPubKeyToUniv and ScriptToUniv (Michael Dietz)
8721638daa rpc: remove deprecated addresses and reqSigs from rpc outputs (Michael Dietz)
Pull request description:
Resolves#21797 now that we've branched-off to v23 ("addresses" and "reqSigs" deprecated) "ExtractDestinations" should be removed.
`-deprecatedrpc=addresses` was initially added in this PR #20286 (which resolved the original issue #20102).
Some chunks of code and logic are no longer used/necessary with the removal of this, and therefore some minor refactoring is done in this PR as well (separated commits)
ACKs for top commit:
MarcoFalke:
re-ACK 43cd6b8af9🐉
meshcollider:
Code review ACK 43cd6b8af9
jonatack:
ACK 43cd6b8af9 per `git range-diff a9d0cec 92dc5e9 43cd6b8`, also rebased to latest master, debug built + quick re-review of each commit to bring back context, and ran tests locally at the final commit
Tree-SHA512: fba83495e396d3c06f0dcf49292f14f4aa6b68fa758f0503941fade1a6e7271cda8378e2734af1faea550d1b43c85a36c52ebcc9dec0732936f9233b4b97901c
Divided tests in rpc_signmessage.py into 2 files wallet_signmessagewithaddress.py and
rpc_signmessagewithprivkey.py, latter one can run even when wallet is disabled.
82b6f89819 [style] Small style improvements to DNS parameters (Amiti Uttarwar)
4c89e24f64 [test] Test the delay before querying DNS seeds (Amiti Uttarwar)
6395c8ed56 [test] Test the interactions between -forcednsseed and -dnsseed (Amiti Uttarwar)
6f6b7df6bd [init] Disallow starting up with conflicting paramters for -dnsseed and -forcednsseed (Amiti Uttarwar)
26d0ffe4f2 [test] Test -forcednsseed causes querying DNS seeds (Amiti Uttarwar)
35851450a9 [test] Test the interactions between -connect and -dnsseed (Amiti Uttarwar)
75c05af361 [test] Test logic to query DNS seeds with block-relay-only connections (Amiti Uttarwar)
9c08719778 [test] Introduce test logic to query DNS seeds (Amiti Uttarwar)
Pull request description:
This PR adds a DNS seed to the regtest chain params to enable testing the DNS seed querying logic of `CConnman::ThreadDNSAddressSeed` and relevant startup parameters. Adds coverage for the changes in #22013 (and then some).
The main behavioral change to bitcoind is that this PR disallows starting up with conflicting parameters for `-dnsseed` and `-forcednsseed`.
The tests include:
* parameter interactions of different combinations of `-connect`, `-dnsseed` and `-forcednsseed`
* the delay before querying DNS seeds depending on how many addresses are in the addrman
* the behavior of `-forcednsseed`
* skipping DNS querying if we have outbound full relay connections & not block-relay-only connections
Huge props to mzumsande for identifying the timing technique for testing successful connections before running `ThreadDNSAddressSeed` 🙌🏽
ACKs for top commit:
mzumsande:
ACK 82b6f89819
jnewbery:
reACK 82b6f89819
Tree-SHA512: 9f0c29bfbf99426727e79c0a25606ae09deab91a92e3c5cee7f84c3ca7503a8ac9ab85a85c51841d40b164ef8c991326070f0b2f41d075fb7985df26f6e95d6d
This commit introduces a DNS seed to the regest chain params in order to add
coverage to the DNS querying logic.
The first test checks that we do not query DNS seeds if we are able to
succesfully connect to 2 outbound connections. Since we participate in ADDR
relay with those connections, including sending a GETADDR message during the
VERSION handshake, querying the DNS seeds is unnecessary.
Co-authored-by: Martin Zumsande <mzumsande@gmail.com>
a006d7d730 test: add logging to wallet_listtransactions (Sebastian Falbesoner)
47915b1187 test: remove unneeded/redundant code in wallet_listtransactions (Sebastian Falbesoner)
fb6c6a7938 test: speedup wallet_listtransactions by whitelisting peers (immediate tx relay) (Sebastian Falbesoner)
Pull request description:
This PR improves the test `wallet_listtransactions.py` in three ways:
* speeds up runtime by a factor of 2-3x by using the good ol' immediate tx relay trick (`-whitelist=noban@127.0.0.1`)
* removes unneeded/redundant code
* adds log messages, mostly by turning comments into `self.log.info(...)` calls
ACKs for top commit:
jonatack:
ACK a006d7d730
kristapsk:
ACK a006d7d730
Tree-SHA512: a91a19f5ebc4d05f0b96c5419683c4c57ac0ef44b64eeb8dd550bd72296fd3a2857a3ba83f755fe4b0b3bd06439973f226070b5d0ce2dee58344dae78cb50290
a806647d26 [validation] Always include merkle root in coinbase commitment (Dhruv Mehta)
189128c220 [validation] Set witness script flag with p2sh for blocks (Dhruv Mehta)
ac82b99db7 [p2p] remove redundant NODE_WITNESS checks (Dhruv Mehta)
6f8b198b82 [p2p] remove unused segwitheight=-1 option (Dhruv Mehta)
eba5b1cd64 [test] remove or move tests using `-segwitheight=-1` (Dhruv Mehta)
Pull request description:
Builds on #21009 and makes progress on remaining items in #17862
Removing `RewindBlockIndex()` in #21009 allows the following:
- removal of tests using `segwitheight=-1` in `p2p_segwit.py`.
- move `test_upgrade_after_activation()` out of `p2p_segwit.py` reducing runtime
- in turn, that allows us to drop support for `-segwitheight=-1`, which is only supported for that test.
- that allows us to always set `NODE_WITNESS` in our local services. The only reason we don't do that is to support `-segwitheight=-1`.
- that in turn allows us to drop all of the `GetLocalServices() & NODE_WITNESS` checks inside `net_processing.cpp`, since our local services would always include `NODE_WITNESS`
ACKs for top commit:
mzumsande:
Code-Review ACK a806647d26
laanwj:
Code review ACK a806647d26, nice cleanup
jnewbery:
utACK a806647d26
theStack:
ACK a806647d26
Tree-SHA512: 73e1a69d1d7eca1f5c38558ec6672decd0b60b16c2ef6134df6f6af71bb159e6eea160f9bb5ab0eb6723c6632d29509811e29469d0d87abbe9b69a2890fbc73e
5730a43703 test: Add functional test for AddrFetch connections (Martin Zumsande)
c34ad3309f net, rpc: Enable AddrFetch connections for functional testing (Martin Zumsande)
533500d907 p2p: Add timeout for AddrFetch peers (Martin Zumsande)
b6c5d1e450 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 5730a43703
naumenkogs:
ACK 5730a43703
jnewbery:
ACK 5730a43703
Tree-SHA512: 8a81234f37e827705138eb254223f7f3b3bf44a06cb02126fc7990b0d231b9bd8f07d38d185cc30d55bf35548a6fdc286b69602498d875b937e7c58332158bf9
By whitelisting the peers via -whitelist, the inventory is transmissioned
immediately rather than on average every 5 seconds, speeding up the test by at
least a factor of two:
before:
$ time ./wallet_listtransactions.py
...
0m40.25s real 0m01.74s user 0m01.70s system
with this PR:
$ time ./wallet_listtransactions.py
...
0m14.93s real 0m01.68s user 0m01.87s system
This commit also moves the wallet_listtransactions tests into the < 30s group.
fa80e10d94 test: Add feature_taproot.py --previous_release (MarcoFalke)
85ccffa266 test: move releases download incantation to README (Sjors Provoost)
29d6b1da2a test: previous releases: add v0.20.1 (Sjors Provoost)
Pull request description:
Disabling the new consensus code at runtime is fine, but potentially fragile and incomplete. Fix that by giving the option to run with a version that has been compiled without any taproot code.
ACKs for top commit:
Sjors:
tACK fa80e10
NelsonGaldeman:
tACK fa80e10d94
Tree-SHA512: 1a1feef823f08c05268759645a8974e1b2d39a024258f5e6acecbe25097aae3fa9302c27262978b40f1aa8e7b525b60c0047199010f2a5d6017dd6434b4066f0
4101ec9d2e doc: mention that we enforce port=0 in I2P (Vasil Dimov)
e0a2b390c1 addrman: reset I2P ports to 0 when loading from disk (Vasil Dimov)
41cda9d075 test: ensure I2P ports are handled as expected (Vasil Dimov)
4f432bd738 net: do not connect to I2P hosts on port!=0 (Vasil Dimov)
1f096f091e net: distinguish default port per network (Vasil Dimov)
aeac3bce3e net: change I2P seeds' ports to 0 (Vasil Dimov)
38f900290c net: change assumed I2P port to 0 (Vasil Dimov)
Pull request description:
_This is an alternative to https://github.com/bitcoin/bitcoin/pull/21514, inspired by https://github.com/bitcoin/bitcoin/pull/21514#issuecomment-815049933. They are mutually exclusive. Just one of them should be merged._
Change assumed ports for I2P to 0 (instead of the default 8333) as this is closer to what actually happens underneath with SAM 3.1 (https://github.com/bitcoin/bitcoin/pull/21514#issuecomment-812632520, https://github.com/bitcoin/bitcoin/pull/21514#issuecomment-816564719).
Don't connect to I2P peers with advertised port != 0 (we don't specify a port to our SAM 3.1 proxy and it always connects to port = 0).
Note, this change:
* Keeps I2P addresses with port != 0 in addrman and relays them to others via P2P gossip. There may be non-bitcoin-core-22.0 peers using SAM 3.2 and for them such addresses may be useful.
* Silently refuses to connect to I2P hosts with port != 0. This is ok for automatically chosen peers from addrman. Not so ok for peers provided via `-addnode` or `-connect` - a user who specifies `foo.b32.i2p:1234` (non zero port) may wonder why "nothing is happening".
Fixes#21389
ACKs for top commit:
laanwj:
Code review ACK 4101ec9d2e
jonatack:
re-ACK 4101ec9d2e per `git range-diff efff9c3 0b0ee03 4101ec9`, built with DDEBUG_ADDRMAN, did fairly extensive testing on mainnet both with and without a peers.dat / -dnsseeds=0 to test boostrapping.
Tree-SHA512: 0e3c019e1dc05e54f559275859d3450e0c735596d179e30b66811aad9d5b5fabe3dcc44571e8f7b99f9fe16453eee393d6e153454dd873b9ff14907d4e6354fe
2feec3ce31 net: don't bind on 0.0.0.0 if binds are restricted to Tor (Vasil Dimov)
Pull request description:
The semantic of `-bind` is to restrict the binding only to some address.
If not specified, then the user does not care and we bind to `0.0.0.0`.
If specified then we should honor the restriction and bind only to the
specified address.
Before this change, if no `-bind` is given then we would bind to
`0.0.0.0:8333` and to `127.0.0.1:8334` (incoming Tor) which is ok -
the user does not care to restrict the binding.
However, if only `-bind=addr:port=onion` is given (without ordinary
`-bind=`) then we would bind to `addr:port` _and_ to `0.0.0.0:8333` in
addition.
Change the above to not do the additional bind: if only
`-bind=addr:port=onion` is given (without ordinary `-bind=`) then bind
to `addr:port` (only) and consider incoming connections to that as Tor
and do not advertise it. I.e. a Tor-only node.
ACKs for top commit:
laanwj:
Code review ACK 2feec3ce31
jonatack:
utACK 2feec3ce31 per `git diff a004833 2feec3c`
hebasto:
ACK 2feec3ce31, tested on Linux Mint 20.1 (x86_64):
Tree-SHA512: a04483af601706da928958b92dc560f9cfcc78ab0bb9d74414636eed1c6f29ed538ce1fb5a17d41ed82c9c9a45ca94899d0966e7ef93da809c9bcdcdb1d1f040
The semantic of `-bind` is to restrict the binding only to some address.
If not specified, then the user does not care and we bind to `0.0.0.0`.
If specified then we should honor the restriction and bind only to the
specified address.
Before this change, if no `-bind` is given then we would bind to
`0.0.0.0:8333` and to `127.0.0.1:8334` (incoming Tor) which is ok -
the user does not care to restrict the binding.
However, if only `-bind=addr:port=onion` is given (without ordinary
`-bind=`) then we would bind to `addr:port` _and_ to `0.0.0.0:8333` in
addition.
Change the above to not do the additional bind: if only
`-bind=addr:port=onion` is given (without ordinary `-bind=`) then bind
to `addr:port` (only) and consider incoming connections to that as Tor
and do not advertise it. I.e. a Tor-only node.
30aee2dfe6 tests: Add test for compact block HB selection (Pieter Wuille)
6efbcec4de Protect last outbound HB compact block peer (Suhas Daftuar)
Pull request description:
If all our high-bandwidth compact block serving peers (BIP 152) stall block
download, then we can be denied a block for (potentially) a long time. As
inbound connections are much more likely to be adversarial than outbound
connections, mitigate this risk by never removing our last outbound HB peer if
it would be replaced by an inbound.
ACKs for top commit:
achow101:
ACK 30aee2dfe6
ariard:
Code ACK 30aee2dfe
jonatack:
ACK 30aee2dfe6
Tree-SHA512: 5c6c9326e3667b97e0864c371ae2174d2be9054dad479f4366127b9cd3ac60ffa01ec9707b16ef29cac122db6916cf56fd9985733390017134ace483278921d5
e4356f6a6c Testcase for wallet issue with orphaned rewards. (Daniel Kraft)
Pull request description:
This adds a new test case demonstrating the wallet issue when block rewards are orphaned (#14148).
ACKs for top commit:
LarryRuane:
ACK e4356f6a6c
leonardojobim:
reACK e4356f6a6c .
Tree-SHA512: e9a2310ee1b3d52cfa302f431ed3d272bbc1b9195439ff318d9eb1006c0b28968dbe840e1600b6ff185e5d7ea57e4dcc837cef16051b5537445e10bc363b8c22
2667366aaa tests: check derivation of P2TR (Pieter Wuille)
7cedafc541 Add tr() descriptor (derivation only, no signing) (Pieter Wuille)
90fcac365e Add TaprootBuilder class (Pieter Wuille)
5f6cc8daa8 Add XOnlyPubKey::CreateTapTweak (Pieter Wuille)
2fbfb1becb Make consensus checking of tweaks in pubkey.* Taproot-specific (Pieter Wuille)
a4bf84039c Separate WitnessV1Taproot variant in CTxDestination (Pieter Wuille)
41839bdb89 Avoid dependence on CTxDestination index order (Pieter Wuille)
31df02a070 Change Solver() output for WITNESS_V1_TAPROOT (Pieter Wuille)
4b1cc08f9f Make XOnlyPubKey act like byte container (Pieter Wuille)
Pull request description:
This is a subset of #21365, to aide review.
This adds support `tr(KEY)` or `tr(KEY,SCRIPT)` or `tr(KEY,{{S1,{{S2,S3},...}},...})` descriptors, describing Taproot outputs with specified internal key, and optionally any number of scripts, in nested groups of 2 inside `{`/`}` if there are more than one. While it permits importing `tr(KEY)`, anything beyond that is just laying foundations for more features later.
Missing:
* Signing support (see #21365)
* Support for more interesting scripts inside the tree (only `pk(KEY)` is supported for now). In particular, a multisig policy based on the new `OP_CHECKSIGADD` opcode would be very useful.
* Inferring `tr()` descriptors from outputs (given sufficient information).
* `getaddressinfo` support.
* MuSig support. Standardizing that is still an ongoing effort, and is generally kind of useless without corresponding PSBT support.
* Convenient ways of constructing descriptors without spendable internal key (especially ones that arent't trivially recognizable as such).
ACKs for top commit:
Sjors:
utACK 2667366 (based on https://github.com/bitcoin/bitcoin/pull/21365#issuecomment-846945215 review, plus the new functional test)
achow101:
Code Review ACK 2667366aaa
lsilva01:
Tested ACK 2667366aaa
meshcollider:
utACK 2667366aaa
Tree-SHA512: 61046fef22c561228338cb178422f0b782ef6587ec8208d3ce2bd07afcff29a664b54b35c6b01226eb70b6540b43f6dd245043d09aa6cb6db1381b6042667e75
This commit moves the ExternalSigner class and RPC methods out of the wallet module.
The enumeratesigners RPC can be used without a wallet since #21417.
With additional modifications external signers could be used without a wallet in general, e.g. via signrawtransaction.
The signerdisplayaddress RPC is ranamed to walletdisplayaddress because it requires wallet context.
A future displayaddress RPC call without wallet context could take a descriptor argument.
This commit fixes a rpc_help.py failure when configured with --disable-wallet.
90ae3d8ca6 doc: Add release notes for -deprecatedrpc=addresses and bitcoin-tx (Michael Dietz)
085b3a7299 rpc: deprecate `addresses` and `reqSigs` from rpc outputs (Michael Dietz)
Pull request description:
Considering the limited applicability of `reqSigs` and the confusing output of `1` in all cases except bare multisig, the `addresses` and `reqSigs` outputs are removed for all rpc commands.
1) add a new sane "address" field (for outputs that have an identifiable address, which doesn't include bare multisig)
2) with -deprecatedrpc: leave "reqSigs" and "addresses" intact (with all weird/wrong behavior they have now)
3) without -deprecatedrpc: drop "reqSigs" and "addresses" entirely always.
Note: Some light refactoring done to allow us to very easily delete a few chunks of code (marked with TODOs) when we remove this deprecated behavior.
Using `IsDeprecatedRPCEnabled` in core_write.cpp caused some circular dependencies involving core_io
Circular dependencies were caused by rpc/util unnecessarily importing node/coinstats and node/transaction. Really what rpc/util needs are some fundamental type/helper-function definitions. So this was cleaned up to make more sense.
This fixes#20102.
ACKs for top commit:
MarcoFalke:
re-ACK 90ae3d8ca6📢
Tree-SHA512: 8ffb617053b5f4a8b055da17c06711fd19632e0037d71c4c8135e50c8cd7a19163989484e4e0f17a6cc48bd597f04ecbfd609aef54b7d1d1e76a784214fcf72a
1) add a new sane "address" field (for outputs that have an
identifiable address, which doesn't include bare multisig)
2) with -deprecatedrpc: leave "reqSigs" and "addresses" intact
(with all weird/wrong behavior they have now)
3) without -deprecatedrpc: drop "reqSigs" and "addresses" entirely,
always.
84716b134e Add "index/blockfilterindex -> validation -> index/blockfilterindex" to expected circular dependencies (Jonas Schnelli)
ab3a0a2fb9 Add functional test for blockfilterindex in prune-mode (Jonas Schnelli)
c286a22f7b Add debug startup parameter -fastprune for more effective pruning tests (Jonas Schnelli)
5e112269c3 Avoid pruning below the blockfilterindex sync height (Jonas Schnelli)
00d57ff768 Avoid accessing nullpointer in BaseIndex::GetSummary() (Jonas Schnelli)
6abe9f5b11 Allow blockfilter in conjunction with prune (Jonas Schnelli)
Pull request description:
Maintaining the blockfilterindexes in prune mode is possible and may lead to efficient p2p based rescans of wallets (restore backups, import/sweep keys) beyond the prune height (rescans not part of that PR).
This PR allows running the blockfilterindex(es) in conjunction with pruning.
* Bitcoind/Qt will shutdown during startup when missing block data has been detected ([re]enable `-blockfilterindex` when we already have pruned)
* manual block pruning is disabled during blockfilterindex sync
* auto-pruning is delayed during blockfilterindex sync
ToDos:
* [x] Functional tests
ACKs for top commit:
fjahr:
Code review ACK 84716b1
ryanofsky:
Code review ACK 84716b134e. Only changes since last review were suggested new FindFilesToPrune argument and test.
benthecarman:
tACK 84716b134e
Tree-SHA512: 91d832c6c562c463f7ec7655c08956385413a99a896640b9737bda0183607fac530435d03d87c3c0e70c61ccdfe73fe8f3639bc7d26d33ca7e60925ebb97d77a
e987ae5a55 test: Add test for deterministic UTXO set hash results (Fabian Jahr)
6ccc8fc067 test: Add test for gettxoutsetinfo RPC with MuHash (Fabian Jahr)
0d3b2f643d rpc: Add hash_type MUHASH to gettxoutsetinfo (Fabian Jahr)
2474645f3b refactor: Separate hash and stats calculation in coinstats (Fabian Jahr)
a1fcceac69 refactor: Improve encapsulation between MuHash3072 and Num3072 (Fabian Jahr)
Pull request description:
This is another Pr in the series PRs for Coinstatsindex (see overview in #18000). This PR adds the `hash_type` option `muhash` to `gettxoutsetinfo` through which the user can calculate the serialized muhash of the utxo set. This PR does not use the index yet.
ACKs for top commit:
Sjors:
tACK e987ae5
achow101:
ACK e987ae5a55
jonatack:
Tested re-ACK e987ae5a55 per `git diff 3506d90 e987ae5`, reviewed diff, debug built, ran gettxoutsetinfo -signet and help on this branch vs master, at height 23127 both returned `hash_serialized_2` of `2b72d65f3b6efb2311f58374ea2b939abf49684d44f4bafda45faa3b5452a454` and this branch returned `muhash` of `c9f1ff12d345ccf9939c6bbf087e6f7399b6115adee1569287e9c5c43dbb475c`
ryanofsky:
Code review ACK e987ae5a55. Looks very good. I left one suggestion to simplify code, but feel free to ignore it here and maybe consider it for later since PR has already had a lot of review.
Tree-SHA512: 9a739ce375e73749fa69a467262b60d3e5314ef384e2d7150b3bbc8e4125cd9fd1db95306623bb9a632fcbaf5d9d2bf2f5cc43bf717d4ff5e2c9c4b52dd9296c
49797c3ccf tests: Disable bdb dump test when no bdb (Andrew Chow)
1194cf9269 Fix wallet_send.py wallet setup to work with descriptors (Andrew Chow)
fbaea7bfe4 Require legacy wallet for wallet_upgradewallet.py (Andrew Chow)
b1b679e0ab Explicitly mark legacy wallet tests as such (Andrew Chow)
09514e1bef Setup wallets for interface_zmq.py (Andrew Chow)
4d03ef9a73 Use MiniWallet in rpc_net.py (Andrew Chow)
4de23824b0 Setup wallets for interface_bitcoin_cli.py (Andrew Chow)
7c71c627d2 Setup wallets with descriptors for feature_notifications (Andrew Chow)
1f1bef8dba Have feature_filelock.py test both bdb and sqlite, depending on compiled (Andrew Chow)
c77975abc0 Disable upgrades tests that require BDB if BDB is not compiled (Andrew Chow)
1f20cac9d4 Disable wallet_descriptor.py bdb format check if BDB is not compiled (Andrew Chow)
3641597d7e tests: Don't make any wallets unless wallet is required (Andrew Chow)
b9b88f57a9 Skip legacy wallet reliant tests if BDB is not compiled (Andrew Chow)
6f36242389 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 49797c3ccf
ryanofsky:
Code review ACK 49797c3ccf. 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
bff7c66e67 Add documentation to contrib folder (Troy Giorshev)
381f77be85 Add Message Capture Test (Troy Giorshev)
e4f378a505 Add capture parser (Troy Giorshev)
4d1a582549 Call CaptureMessage at appropriate locations (Troy Giorshev)
f2a77ff97b Add CaptureMessage (Troy Giorshev)
dbf779d5de Clean PushMessage and ProcessMessages (Troy Giorshev)
Pull request description:
This PR introduces per-peer message capture into Bitcoin Core. 📓
## Purpose
The purpose and scope of this feature is intentionally limited. It answers a question anyone new to Bitcoin's P2P protocol has had: "Can I see what messages my node is sending and receiving?".
## Functionality
When a new debug-only command line argument `capturemessages` is set, any message that the node receives or sends is captured. The capture occurs in the MessageHandler thread. When receiving a message, it is captured as soon as the MessageHandler thread takes the message off of the vProcessMsg queue. When sending, the message is captured just before the message is pushed onto the vSendMsg queue.
The message capture is as minimal as possible to reduce the performance impact on the node. Messages are captured to a new `message_capture` folder in the datadir. Each node has their own subfolder named with their IP address and port. Inside, received and sent messages are captured into two binary files, msgs_recv.dat and msgs_sent.dat, like so:
```
message_capture/203.0.113.7:56072/msgs_recv.dat
message_capture/203.0.113.7:56072/msgs_sent.dat
```
Because the messages are raw binary dumps, included in this PR is a Python parsing tool to convert the binary files into human-readable JSON. This script has been placed on its own and out of the way in the new `contrib/message-capture` folder. Its usage is simple and easily discovered by the autogenerated `-h` option.
## Future Maintenance
I sympathize greatly with anyone who says "the best code is no code".
The future maintenance of this feature will be minimal. The logic to deserialize the payload of the p2p messages exists in our testing framework. As long as our testing framework works, so will this tool.
Additionally, I hope that the simplicity of this tool will mean that it gets used frequently, so that problems will be discovered and solved when they are small.
## FAQ
"Why not just use Wireshark"
Yes, Wireshark has the ability to filter and decode Bitcoin messages. However, the purpose of the message capture added in this PR is to assist with debugging, primarily for new developers looking to improve their knowledge of the Bitcoin Protocol. This drives the design in a different direction than Wireshark, in two different ways. First, this tool must be convenient and simple to use. Using an external tool, like Wireshark, requires setup and interpretation of the results. To a new user who doesn't necessarily know what to expect, this is unnecessary difficulty. This tool, on the other hand, "just works". Turn on the command line flag, run your node, run the script, read the JSON. Second, because this tool is being used for debugging, we want it to be as close to the true behavior of the node as possible. A lot can happen in the SocketHandler thread that would be missed by Wireshark.
Additionally, if we are to use Wireshark, we are at the mercy of whoever it maintaining the protocol in Wireshark, both as to it being accurate and recent. As can be seen by the **many** previous attempts to include Bitcoin in Wireshark (google "bitcoin dissector") this is easier said than done.
Lastly, I truly believe that this tool will be used significantly more by being included in the codebase. It's just that much more discoverable.
ACKs for top commit:
MarcoFalke:
re-ACK bff7c66e67 only some minor changes: 👚
jnewbery:
utACK bff7c66e67
theStack:
re-ACK bff7c66e67
Tree-SHA512: e59e3160422269221f70f98720b47842775781c247c064071d546c24fa7a35a0e5534e8baa4b4591a750d7eb16de6b4ecf54cbee6d193b261f4f104e28c15f47
647b81b709 wallet, rpc: add listdescriptors command (Ivan Metlushko)
Pull request description:
Looking for concept ACKs
**Rationale**: allow users to inspect the contents of their newly created descriptor wallets.
Currently the command only returns xpubs which is not very useful in itself, but there are multiples ways to extend it:
* add an option to export xprv
* with #19136 it'll be possible to return normalised descriptors suitable for a watch-only purposes
The output is compatible with `importdescriptors` command so it could be easily used for backup/recover purposes.
**Output example:**
```json
[
{
"desc": "wpkh(tpubD6NzVbkrYhZ4WW6E2ZETFyNfq2hfF23SKxqSGFvUpPAY58jmmuBybwqwFihAyQPk9KnwTt5516NDZRJ7k5QPeKjy7wuVd5WvXNxwwAs5tUD/*)#nhavpr5h",
"timestamp": 1296688602,
"active": false,
"range": [
0,
999
],
"next": 0
}
]
```
ACKs for top commit:
jonatack:
re-ACK 647b81b709 rebased to master, debug builds cleanly, reviewed diff since last review, tested with a descriptor wallet (and with a legacy wallet)
achow101:
re-ACK 647b81b
Tree-SHA512: 51a3620bb17c836c52cecb066d4fa9d5ff418af56809046eaee0528c4dc240a4e90fff5711ba96e399c6664e00b9ee8194e33852b1b9e75af18061296e19a8a7
