Messy merge conflicts because this PR backported some ad-hoc stuff from
upstream while keeping a few things that upstream deleted. Hopefully
reviewing is easier than doing this in the first place, since ultimately
all I did was delete code from one side or another of the conflicts.
(Ok, I also changed some boost optional stuff to std::optional, and had
to patch up a test file for test framework changes.)
When reviewing the detailed crypto, bear in mind that taptweaks, like all
hashes are the kind of crypto that cannot be subtly wrong -- it will either
fail very hard or be correct. And we have independent implementations in
C++ and Python that cross-check each other, si it's unlikely to be the former.
This forward-ports the new Taproot sighash but does not fix a couple
22-blocked TODOs related to the MissingDataBehavior enum. Should be fixed
in a followup commit.
One nontrivial change I had to make was feeding the genesis hash to SignTransaction
(the "global" one in script/sign.cpp) so that it could correctly compute
the sighash at signing time.
Surprisingly easy to do. Almost all of the diff resolution was mechanically
* replacing boost::variant with std::variant
* replacing Optional with std::optional
* then replacing `nullopt` with `std::nullopt`
* updating the RPC functions for the new RPCArg::Default type
* update the tests/ directory to make new (since 22) tests use arrays for
createrawtransaction outputs
* other ad-hoc changes to function parameters etc (not too many of these)
I had to "really" change the code in PrecomputePSBTData, which was introduced
in 22.0 and affected by PSET, but this function was like 8 lines long so it
was easy.
Reviewing the diff may be a bit difficult because of the mix of mechanical
changes and ad-hoc things. Probably the most straightforward thing to do
is to redo the merge, `sed -i` to fix the boost::variant and Optional stuff,
then diff the remaining conflicts against this commit.
TODO: grep for `blindpsbt` and you will see that this RPC is still referenced
in documentation and help text even though it was deleted. Need to fix this
in 0.21 in a separate PR.
5a1bef60a0 test: refactor: remove binascii from test_framework (Zero-1729)
Pull request description:
This PR continues the work started in PR #22593, regarding using the `bytes` built-in module. In this PR specifically, instances of `binascii`'s methods `hexlify`, `unhexlify`, and `a2b_hex` have been replaced with the build-in `bytes` module's `hex` and `fromhex` methods where appropriate to make bytes <-> hex-string conversions consistent across the functional test files and test_framework.
Additionally, certain changes made are based on the following assumption:
```
bytes.hex(data) == binascii.hexlify(data).decode()
bytes.hex(data).encode() == binascii.hexlify(data)
```
Ran the functional tests to ensure behaviour is still consistent and changes didn't break existing tests.
closes#22605
ACKs for top commit:
theStack:
Code-review ACK 5a1bef60a0🔢
Tree-SHA512: 8f28076cf0580a0d02a156f3e1e94c9badd3d41c3fbdfb2b87cd8a761dde2c94faa5f4c448d6747b1ccc9111c3ef1a1d7b42a11c806b241fa0410b7529e2445f
222290f543 test: Set BIP34Height = 2 for regtest (MarcoFalke)
fac90c55be test: Create all blocks with version 4 or higher (MarcoFalke)
Pull request description:
BIP34 is active on the current tip of mainnet, so all miners must obey it. It would be nice if it also was active in fresh regtest instances from the earliest time possible.
I changed the BIP34 height to `2`, so that the block at height=1 may be used to mine a duplicate coinbase. (Needed to test mainnet behaviour)
This pull is done in two commits:
* test: Create all blocks with version 4 or higher:
Now that BIP34 is activated earlier, we need to create blocks with a higher version number. Just bump it to 4 instead of 2 to avoid having to bump it again later.
* test: Set BIP34Height = 2 for regtest:
This fixes the BIP34 implementation in the tests (to match the one of the Core codebase) and updates the tests where needed
ACKs for top commit:
ajtowns:
ACK 222290f543
jonatack:
ACK 222290f543 tested and reviewed rebased to current master 5e213822f8
theStack:
Tested ACK 222290f543
Tree-SHA512: d69c637a62a64b8e87de8c7f0b305823d8f4d115c1852514b923625dbbcf9a4854b5bb3771ff41702ebf47c4c182a4442c6d7c0b9f282c95a34b83e56a73939b
3f7250b328 [test] Use the new endpoint to improve tests (Amiti Uttarwar)
3893da06db [RPC] Add field to getpeerinfo to indicate if addr relay is enabled (Amiti Uttarwar)
0980ca78cd [test] Test that we intentionally select addr relay peers. (Amiti Uttarwar)
c061599e40 [net_processing] Remove RelayAddrsWithPeer function (Amiti Uttarwar)
201e496481 [net_processing] Introduce new field to indicate if addr relay is enabled (Amiti Uttarwar)
1d1ef2db7e [net_processing] Defer initializing m_addr_known (Amiti Uttarwar)
6653fa3328 [test] Update p2p_addr_relay test to prepare (Amiti Uttarwar)
2fcaec7bbb [net_processing] Introduce SetupAddressRelay (Amiti Uttarwar)
Pull request description:
This PR builds on the test refactors extracted into #22306 (first 5 commits).
This PR aims to reduce addr blackholes. When we receive an `addr` message that contains 10 or less addresses, we forward them to 1-2 peers. This is the main technique we use for self advertisements, so sending to peers that wouldn't relay would effectively "blackhole" the trickle. Although we cannot prevent this in a malicious case, we can improve it for the normal, honest cases, and reduce the overall likelihood of occurrence. Two known cases where peers would not participate in addr relay are if they have connected to you as a block-relay-only connection, or if they are a light client.
This implementation defers initialization of `m_addr_known` until it is needed, then uses its presence to decide if the peer is participating in addr relay. For outbound (not block-relay-only) peers, we initialize the filter before sending the initial self announcement when processing their `version` message. For inbound peers, we initialize the filter if/when we get an addr related message (`ADDR`, `ADDRV2`, `GETADDR`). We do NOT initialize the filter based on a `SENDADDRV2` message.
To communicate about these changes beyond bitcoin core & to (try to) ensure that no other software would be disrupted, I have:
- Posted to the [mailing list](https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2021-April/018784.html)
- Researched other open source clients to confirm compatibility, opened issues in all the projects & documented in https://github.com/bitcoin/bitcoin/pull/21528#issuecomment-809906430. Many have confirmed that this change would not be problematic.
- Raised as topic during [bitcoin-core-dev meeting](https://www.erisian.com.au/bitcoin-core-dev/log-2021-03-25.html#l-954)
- Raised as topic during [bitcoin p2p meeting](https://www.erisian.com.au/bitcoin-core-dev/log-2021-04-20.html#l-439)
ACKs for top commit:
jnewbery:
reACK 3f7250b328
glozow:
ACK 3f7250b328
ajtowns:
utACK 3f7250b328
Tree-SHA512: 29069282af684c1cd37d107c395fdd432dcccb11626f3c2dabfe92fdc4c85e74c7c4056fbdfa88017fec240506639b72ac6c311f8ce7c583112eb15f47e421af
607076d01b test: remove confusing `MAX_BLOCK_BASE_SIZE` (Sebastian Falbesoner)
4af97c74ed test: introduce `get_weight()` helper for CBlock (Sebastian Falbesoner)
a084ebe133 test: introduce `get_weight()` helper for CTransaction (Sebastian Falbesoner)
Pull request description:
This is a very late follow-up PR to #10618, which removed the constant `MAX_BLOCK_BASE_SIZE` from the core implementation about four years ago (see also #10608 in why it was considered confusing and superfluous).
Since there is also no point in still keeping it in the functional test framework, the PR switches to weight-based accounting on the relevant test code parts and use `MAX_BLOCK_WEIGHT` instead for the block limit checks. To prepare that, the first two commits introduce `get_weight()` helpers for the classes CTransaction and CBlock, respectively.
ACKs for top commit:
MarcoFalke:
review ACK 607076d01b🚴
Tree-SHA512: d59aa0b6b3dfd0a849b8063e66de275d252f705f99e25cd3bf6daec028b47d946777ee5b42a060f5283cb18e917ac073119c2c0e11bbc21211f69ef0a6ed335a
8a2b58db9e test: fix segwit terminology (s/witness_program/witness_script/) (Sebastian Falbesoner)
Pull request description:
This PR fixes wrong uses of the term "witness program", which according to [BIP141](https://github.com/bitcoin/bips/blob/master/bip-0141.mediawiki#Witness_program) is defined as follows:
> A scriptPubKey (or redeemScript as defined in BIP16/P2SH) that consists of a 1-byte push opcode (for 0 to 16) followed by a data push between 2 and 40 bytes gets a new special meaning. The value of the first push is called the "version byte". **The following byte vector pushed is called the "witness program".**
In most cases where "witness program" is used in tests (concerns comments, variable names and in one instance even a function name) what we really want to denote is the "witness script". Thanks to [MarcoFalke for pointing this out in a review comment](https://github.com/bitcoin/bitcoin/pull/22363#discussion_r666794261)!
Some historical background: At the time when the P2P segwit tests were first introduced (commit 330b0f31ee, PR #8149), the term "witness program" was not used consistently in BIP141: https://bitcoin.stackexchange.com/questions/46451/what-is-the-precise-definition-of-witness-program
This was fixed in PR https://github.com/bitcoin/bips/pull/416 later.
So in some way, this PR can be seen as a very late follow-up to the BIP141 fix that also reflects these changes in the tests.
ACKs for top commit:
josibake:
tACK https://github.com/bitcoin/bitcoin/pull/22429/commits/8a2b58db9ee6a14d36b5d8e430b35f18e7c7b0c5
Tree-SHA512: f36bb9e53d1b54b86bfa87ec12f33e3ebca64b5f59d97e9662fe35ba12c25e1c9a4f93a5425d0eaa3879dce9e50368d345555b927bfab76945511f873396892b
8ca51af1ec test: Disable automatic connections by default (Martin Zumsande)
Pull request description:
A node normally doesn't make automatic connections to peers in the functional tests because neither DNS seeds nor hardcoded peers are available on regtest. However, when random entries are inserted into addrman as part of a functional test (e.g. while testing addr relay), `ThreadOpenConnections` will periodically try to connect to them, resulting in log entries such as:
`[opencon] [net.cpp:400] [ConnectNode] trying connection 18.166.1.1:8333 lastseen=0.0hrs`
I don't think it's desirable that functional tests try to connect to random computers on the internet, aside from the possibility that at some point in time someone out there might actually answer in a way to ruin a test.
This PR fixes this problem by disabling `ThreadOpenConnections` by adding `-connect=0` to the default args, and adding exceptions only when needed for the test to pass.
ACKs for top commit:
tryphe:
Concept ACK, light code review ACK 8ca51af1ec
Tree-SHA512: bcfb2de610e6c35a97a2bd7ad6267e968b1ac7529638d99276993cd5bc93ce9919d54e22d6dc84e1b02ecd626ab6554e201693552ea065c29794eece38c43f7d
Use SetupAddressRelay to only initialize `m_addr_known` as needed. For outbound
peers, we initialize the filter before sending our self announcement (not
applicable for block-relay-only connections). For inbound peers, we initialize
the filter when we get an addr related message (ADDR, ADDRV2, GETADDR).
These changes intend to mitigate address blackholes. Since an inbound peer has
to send us an addr related message to become eligible as a candidate for addr
relay, this should reduce our likelihood of sending them self-announcements.
This prevents the node from trying to connect to random IPs on the internet
while running the functional tests. Exceptions are added when required for
the test to pass.
