In the functional tests, we often compare dicts with assert_equal, but the
output makes it very hard to tell exactly which entry in the dicts don't
match when there are a lot of entries and only minor differences. Change
the output to make it clearer.
bf5662c678 test: enable v2 for python p2p depending on global --v2transport flag (Martin Zumsande)
6e9e39da43 test: Don't use v2transport when it's too slow. (Martin Zumsande)
87549c8f89 test: enable p2p_invalid_messages.py with v2transport (Martin Zumsande)
5fc9db504b test: enable p2p_sendtxrcncl.py with v2transport (Martin Zumsande)
Pull request description:
#24748 added v2 transport to the python `P2PConnection`, but so far each test that wants to make use of it needs to enable it on an individual basis.
This PR changes it so that if the test suite is run with `--v2transport` option, v2 is used in each test by default, not only for connections between two bitcoind instances as before, but also wherever `P2PConnection` is used. Individual tests can override this global option.
To do that, a few tests need to be adjusted.
In addition, I added a commit to always use v1 in a few select subtests that send a large number of large messages (e.g. large reorgs). These tests don't have a fundamental problem with v2 but become very slow due to the unoptimised python ChaCha20 implementation (~30 minutes on my computer, so probably not suitable to be run in the CI).
As a result, `python3 test_runner.py --v2transport` should succeed and use `v2` everywhere (unless v1 is chosen explicitly).
[Edit]: To make the "test each commit" CI pass, several test fixes were squashed into the last commit, which actually enables v2 p2p for `P2PConnection`. I have an unsquashed version at https://github.com/mzumsande/bitcoin/tree/202401_bip324_alltests_unsquashed, in case that helps with review.
ACKs for top commit:
fjahr:
tACK bf5662c678
vasild:
ACK bf5662c678
stratospher:
reACK bf5662c6.
theStack:
Tested ACK bf5662c678
Tree-SHA512: 4f5a08248ba8a755f7d0f48deb2b79bef03292345cacb7deef01be955481093800e4e56ff218ea56734eef5de1fb3ab0f04657447ea27d393bb536539d7b289d
This changes the default behavior, individual tests can overwrite this option.
As a result, it is possible to run the entire test suite with
--v2transport, and all connections to the python p2p will then use it.
Also adjust several tests that are already running with --v2transport in the
test runner (although they actually made v1 connection before this change).
This is done in the same commit so that there isn't an
intermediate commit in which the CI fails.
29029df5c7 [doc] v3 signaling in mempool-replacements.md (glozow)
e643ea795e [fuzz] v3 transactions and sigop-adjusted vsize (glozow)
1fd16b5c62 [functional test] v3 transaction submission (glozow)
27c8786ba9 test framework: Add and use option for tx-version in MiniWallet methods (MarcoFalke)
9a1fea55b2 [policy/validation] allow v3 transactions with certain restrictions (glozow)
eb8d5a2e7d [policy] add v3 policy rules (glozow)
9a29d470fb [rpc] return full string for package_msg and package-error (glozow)
158623b8e0 [refactor] change Workspace::m_conflicts and adjacent funcs/structs to use Txid (glozow)
Pull request description:
See #27463 for overall package relay tracking.
Delving Bitcoin discussion thread: https://delvingbitcoin.org/t/v3-transaction-policy-for-anti-pinning/340
Delving Bitcoin discussion for LN usage: https://delvingbitcoin.org/t/lightning-transactions-with-v3-and-ephemeral-anchors/418
Rationale:
- There are various pinning problems with RBF and our general ancestor/descendant limits. These policies help mitigate many pinning attacks and make package RBF feasible (see #28984 which implements package RBF on top of this). I would focus the most here on Rule 3 pinning. [1][2]
- Switching to a cluster-based mempool (see #27677 and #28676) requires the removal of CPFP carve out, which applications depend on. V3 + package RBF + ephemeral anchors + 1-parent-1-child package relay provides an intermediate solution.
V3 policy is for "Priority Transactions." [3][4] It allows users to opt in to more restrictive topological limits for shared transactions, in exchange for the more robust fee-bumping abilities that offers. Even though we don't have cluster limits, we are able to treat these transactions as having as having a maximum cluster size of 2.
Immediate benefits:
- You can presign a transaction with 0 fees (not just 1sat/vB!) and add a fee-bump later.
- Rule 3 pinning is reduced by a significant amount, since the attacker can only attach a maximum of 1000vB to your shared transaction.
This also enables some other cool things (again see #27463 for overall roadmap):
- Ephemeral Anchors
- Package RBF for these 1-parent-1-child packages. That means e.g. a commitment tx + child can replace another commitment tx using the child's fees.
- We can transition to a "single anchor" universe without worrying about package limit pinning. So current users of CPFP carve out would have something else to use.
- We can switch to a cluster-based mempool [5] (#27677#28676), which removes CPFP carve out [6].
[1]: Original mailing list post and discussion about RBF pinning problems https://gist.github.com/glozow/25d9662c52453bd08b4b4b1d3783b9ff, https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2022-January/019817.html
[2]: A FAQ is "we need this for cluster mempool, but is this still necessary afterwards?" There are some pinning issues that are fixed here and not fully fixed in cluster mempool, so we will still want this or something similar afterward.
[3]: Mailing list post for v3 https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2022-September/020937.html
[4]: Original PR #25038 also contains a lot of the discussion
[5]: https://delvingbitcoin.org/t/an-overview-of-the-cluster-mempool-proposal/393/7
[6]: https://delvingbitcoin.org/t/an-overview-of-the-cluster-mempool-proposal/393#the-cpfp-carveout-rule-can-no-longer-be-supported-12
ACKs for top commit:
sdaftuar:
ACK 29029df5c7
achow101:
ACK 29029df5c7
instagibbs:
ACK 29029df5c7 modulo that
Tree-SHA512: 9664b078890cfdca2a146439f8835c9d9ab483f43b30af8c7cd6962f09aa557fb1ce7689d5e130a2ec142235dbc8f21213881baa75241c5881660f9008d68450
facafa90f7 test: Fix CPartialMerkleTree.nTransactions signedness (MarcoFalke)
Pull request description:
It is unsigned in Bitcoin Core, so the tests should match it:
aa9231fafe/src/merkleblock.h (L59)
Large positive values, or "negative" values, are rejected anyway, but it still seems fine to fix this.
The bug was introduced when the code was written in d280617bf5.
(Lowercase `i` means signed, see https://docs.python.org/3/library/struct.html#format-characters)
ACKs for top commit:
theStack:
LGTM ACK facafa90f7
Empact:
ACK facafa90f7
Tree-SHA512: 35ac11bb5382dffe132bfae6097efc343ef6c06b1b4b1545130ca27b228ca6894679004862fee921b095172abaddbef5972c24d9bc195ce970f35643bd4a0f09
By adding to the test framework a wait until the v2 handshake
is completed, so that p2p_sendtxrcncl.py (which doesn't need
to be changed itself) doesnt't send out any other messages before that.
e7fd70f4b6 [test] make v2transport arg in addconnection mandatory and few cleanups (stratospher)
Pull request description:
- make `v2transport` argument in `addconnection` regression-testing only RPC mandatory. https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1470738750
- previously it was an optional arg with default `false` value.
- only place this RPC is used is in the [functional tests](11b436a66a/test/functional/test_framework/test_node.py (L742)) where we always pass the appropriate `v2transport` option to the RPC anyways. (and that too just for python dummy peer(`P2PInterface`) and bitcoind(`TestNode`) interactions)
- rename `v2_handshake()` to `_on_data_v2_handshake()` https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1466958424
- more compact return statement in `wait_for_reconnect()` https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1466979708
- assertion to check that empty version packets are received from `TestNode`.
ACKs for top commit:
glozow:
ACK e7fd70f4b6
theStack:
Code-review ACK e7fd70f4b6
mzumsande:
Code Review ACK e7fd70f4b6
Tree-SHA512: e66e29baccd91e1e4398b91f7d45c5fc7c2841d77d8a6178734586017bf2be63496721649da91848dec71da605ee31664352407d5bb896e624cc693767c61a1f
The test framework's p2p implementation currently sends out it's VERSION
message immediately after an inbound connection (i.e. TestNode outbound
connection) is made. This doesn't follow the usual protocol flow where
the initiator sends a version first, and the responders processes that
and only then responds with its own version message. Change that
accordingly by only sending immediate VERSION message for outbound
connections (or after v2 handshake for v2 connections, respectively),
and sending out VERSION messages as response for incoming VERSION
messages (i.e. in the function `on_version`) for inbound connections.
Note that some of the overruled `on_version` methods in functional tests
needed to be changed to send the version explicitly.
In the course of executing the asyncio data reception callback during a
v2 handshake, it's possible that the receive buffer already contains
data for after the handshake (usually a VERSION message for inbound
connections).
If we don't process that data immediately, we would do so after the next
message is received, but with the adapted protocol flow introduced in
the next commit, there is no next message, as the TestNode wouldn't
continue until we send back our own version in `on_version`. Fix this by
calling `self._on_data` immediately if there's data left in the receive
buffer after a completed v2 handshake.
This deduplicates code for sending out the VERSION message
(if available and not sent yet), currently used at three
different places:
1) in the `connection_made` asyncio callback
(for v1 connections that are not v2 reconnects)
2) at the end of `v2_handshake`, if the v2 handshake succeeded
3) in the `on_version` callback, if a reconnection with v1 happens
0bef1042ce net: enable v2transport by default (Pieter Wuille)
Pull request description:
This enables BIP324's v2 transport by default (see #27634):
* Inbound connections will auto-sense whether v1 or v2 is in use.
* Automatic outbound connections will use v2 if `NODE_P2P_V2` was set in addr gossip, but retry with v1 if met with immediate failure.
* Manual outbound connections will default to v2, but retry with v1 if met with immediate failure.
It remains possible to run with `-v2transport=0` to disable all of these, and make all outbound and inbound connections v1. It also remains possible to specify the `v2transport` argument to the `addnode` RPC as `false`, to disable attempting a v2 connection for that particular added node.
ACKs for top commit:
stratospher:
ACK 0bef104.
josibake:
reACK 0bef1042ce
achow101:
ACK 0bef1042ce
naumenkogs:
ACK 0bef1042ce
theStack:
ACK 0bef1042ce
willcl-ark:
crACK 0bef1042ce
BrandonOdiwuor:
utACK 0bef1042ce
pablomartin4btc:
re ACK 0bef1042ce
kristapsk:
utACK 0bef1042ce
Tree-SHA512: 3f17a91e318b9304c40c74a7a5b231149f664ae684d13e9739a05be6c05ba9720f3c3c62da6a73ace0ae8ce733f1c8410b211f9fa15694e6a8d28999ab9882d8
`TestNode::add_outbound_p2p_connection()` is the only place where
addconnection test-only RPC is used. here, we always pass the
appropriate v2transport option to addconnection RPC.
currently the v2transport option for addconnection RPC is optional.
so simply make the v2transport option mandatory instead.
55556a64a8 test: Remove struct import from messages.py (MarcoFalke)
fa3fa86dda scripted-diff: test: Use int from_bytes and to_bytes over struct packing (MarcoFalke)
fafc0d68ee test: Use int from_bytes and to_bytes over struct packing (MarcoFalke)
fa3886b7c6 test: Treat msg_version.relay as unsigned (MarcoFalke)
Pull request description:
`struct` has many issues in messages.py:
* For unpacking, it requires to specify the length a second time, even when it is already clear from the `f.read(num_bytes)` context.
* For unpacking, it is designed to support a long format string and returning a tuple of many values. However, except for 3 instances in `messages.py`, usually only a single value is unpacked and all those cases require an `[0]` access.
* For packing and unpacking of a single value, the format string consists of characters that may be confusing and may need to be looked up in the documentation, as opposed to using easy to understand self-documenting code.
I presume the above issues lead to accidentally treat `msg_version.relay` as a "signed bool", which is fine, but confusing.
Fix all issues by using the built-in `int` helpers `to_bytes` and `from_bytes` via a scripted diff.
Review notes:
* `struct.unpack` throws an error if the number of bytes passed is incorrect. `int.from_bytes` doesn't know about "missing" bytes and treats an empty byte array as `int(0)`. "Extraneous" bytes should never happen, because all `read` calls are limited in this file. If it is important to keep this error behavior, a helper `int_from_stream(stream, num_bytes, bytes, byteorder, *, **kwargs)` can be added, which checks the number of bytes read from the stream.
* For `struct.pack` and `int.to_bytes` the error behavior is the same, although the error messages are not identical.
ACKs for top commit:
stickies-v:
ACK 55556a64a8
theStack:
re-ACK 55556a64a8
Tree-SHA512: 1cef8cdfd763fb424ed4b8be850a834b83fd0ef47fbea626a29784eb4f4832d44e42c4fe05b298b6070a933ef278b0222289a9955a97c86707e091e20bbb247a
bc9283c441 [test] Add functional test to test early key response behaviour in BIP 324 (stratospher)
ffe6a56d75 [test] Check whether v2 TestNode performs downgrading (stratospher)
ba737358a3 [test] Add functional tests to test v2 P2P behaviour (stratospher)
4115cf9956 [test] Ignore BIP324 decoy messages (stratospher)
8c054aa04d [test] Allow inbound and outbound connections supporting v2 P2P protocol (stratospher)
382894c3ac [test] Reconnect using v1 P2P when v2 P2P terminates due to magic byte mismatch (stratospher)
a94e350ac0 [test] Build v2 P2P messages (stratospher)
bb7bffed79 [test] Use lock for sending P2P messages in test framework (stratospher)
5b91fb14ab [test] Read v2 P2P messages (stratospher)
05bddb20f5 [test] Perform initial v2 handshake (stratospher)
a049d1bd08 [test] Introduce EncryptedP2PState object in P2PConnection (stratospher)
b89fa59e71 [test] Construct class to handle v2 P2P protocol functions (stratospher)
8d6c848a48 [test] Move MAGIC_BYTES to messages.py (stratospher)
595ad4b168 [test/crypto] Add ECDH (stratospher)
4487b80517 [rpc/net] Allow v2 p2p support in addconnection (stratospher)
Pull request description:
This PR introduces support for v2 P2P encryption(BIP 324) in the existing functional test framework and adds functional tests for the same.
### commits overview
1. introduces a new class `EncryptedP2PState` to store the keys, functions for performing the initial v2 handshake and encryption/decryption.
3. this class is used by `P2PConnection` in inbound/outbound connections to perform the initial v2 handshake before the v1 version handshake. Only after the initial v2 handshake is performed do application layer P2P messages(version, verack etc..) get exchanged. (in a v2 connection)
- `v2_state` is the object of class `EncryptedP2PState` in `P2PConnection` used to store its keys, session-id etc.
- a node [advertising](https://github.com/stratospher/blogosphere/blob/main/integration_test_bip324.md#advertising-to-support-v2-p2p) support for v2 P2P is different from a node actually [supporting v2 P2P](https://github.com/stratospher/blogosphere/blob/main/integration_test_bip324.md#supporting-v2-p2p) (differ when false advertisement of services occur)
- introduce a boolean variable `supports_v2_p2p` in `P2PConnection` to denote if it supports v2 P2P.
- introduce a boolean variable `advertises_v2_p2p` to denote whether `P2PConnection` which mimics peer behaviour advertises V2 P2P support. Default option is `False`.
- In the test framework, you can create Inbound and Outbound connections to `TestNode`
1. During **Inbound Connections**, `P2PConnection` is the initiator [`TestNode` <--------- `P2PConnection`]
- Case 1:
- if the `TestNode` advertises/signals v2 P2P support (means `self.nodes[i]` set up with `"-v2transport=1"`), different behaviour will be exhibited based on whether:
1. `P2PConnection` supports v2 P2P
2. `P2PConnection` does not support v2 P2P
- In a real world scenario, the initiator node would intrinsically know if they support v2 P2P based on whatever code they choose to run. However, in the test scenario where we mimic peer behaviour, we have no way of knowing if `P2PConnection` should support v2 P2P or not. So `supports_v2_p2p` boolean variable is used as an option to enable support for v2 P2P in `P2PConnection`.
- Since the `TestNode` advertises v2 P2P support (using "-v2transport=1"), our initiator `P2PConnection` would send:
1. (if the `P2PConnection` supports v2 P2P) ellswift + garbage bytes to initiate the connection
2. (if the `P2PConnection` does not support v2 P2P) version message to initiate the connection
- Case 2:
- if the `TestNode` doesn't signal v2 P2P support; `P2PConnection` being the initiator would send version message to initiate a connection.
2. During **Outbound Connections** [TestNode --------> P2PConnection]
- initiator `TestNode` would send:
- (if the `P2PConnection` advertises v2 P2P) ellswift + garbage bytes to initiate the connection
- (if the `P2PConnection` advertises v2 P2P) version message to initiate the connection
- Suppose `P2PConnection` advertises v2 P2P support when it actually doesn't support v2 P2P (false advertisement scenario)
- `TestNode` sends ellswift + garbage bytes
- `P2PConnection` receives but can't process it and disconnects.
- `TestNode` then tries using v1 P2P and sends version message
- `P2PConnection` receives/processes this successfully and they communicate on v1 P2P
4. the encrypted P2P messages follow a different format - 3 byte length + 1-13 byte message_type + payload + 16 byte MAC
5. includes support for testing decoy messages and v2 connection downgrade(using false advertisement - when a v2 node makes an outbound connection to a node which doesn't support v2 but is advertised as v2 by some malicious
intermediary)
### run the tests
* functional test - `test/functional/p2p_v2_encrypted.py` `test/functional/p2p_v2_earlykeyresponse.py`
I'm also super grateful to @ dhruv for his really valuable feedback on this branch.
Also written a more elaborate explanation here - https://github.com/stratospher/blogosphere/blob/main/integration_test_bip324.md
ACKs for top commit:
naumenkogs:
ACK bc9283c441
mzumsande:
Code Review ACK bc9283c441
theStack:
Code-review ACK bc9283c441
glozow:
ACK bc9283c441
Tree-SHA512: 9b54ed27e925e1775e0e0d35e959cdbf2a9a1aab7bcf5d027e66f8b59780bdd0458a7a4311ddc7dd67657a4a2a2cd5034ead75524420d58a83f642a8304c9811
The C++ code treats bool as uint8_t, so the python tests should as well.
This also allows to simplify the code, because converting an empty byte
array to int gives int(0).
>>> int.from_bytes(b'')
0
- Add an optional `supports_v2_p2p` parameter to specify if the inbound
and outbound connections support v2 P2P protocol.
- In the `addconnection_callback` which gets called when creating
outbound connections, call the `addconnection` RPC with v2 P2P protocol
support enabled.
- When a v2 TestNode makes an outbound connection to a P2PInterface node
which doesn't support v2 but is advertised as v2 by some malicious
intermediary, the TestNode sends 64 bytes ellswift. The v1 node doesn't
understand this and disconnects. Then the v2 TestNode reconnects by
sending a v1/version message.
Messages are built, encrypted and sent over the socket in v2
connections. If a race condition happens between python's main
thread and p2p thread with both of them trying to send a message,
it's possible that the messages get encrypted with wrong keystream.
Messages are built and sent over the socket in v1 connections.
So there's no problem if messages are sent in the wrong order.
Co-authored-by: Martin Zumsande <mzumsande@gmail.com>
Co-authored-by: theStack <sebastian.falbesoner@gmail.com>
Instantiate this object when the connection supports v2 P2P transport
protocol.
- When a P2PConnection is opened, perform initiate_v2_handshake() if the
connection is an initiator. application layer messages are only sent after
the initial v2 handshake is over (for both initiator and responder).
The class `EncryptedP2PState` stores the 4 32-byte keys, session id,
garbage terminators, whether it's an initiator/responder, whether the
initial handshake has been completed etc.. It also contains functions
to perform the v2 handshake and to encrypt/decrypt p2p v2 messages.
- In an inbound connection to TestNode, P2PConnection is the initiator
and `initiate_v2_handshake()`, `complete_handshake()`, `authenticate_handshake()`
are called on it. [ TestNode <----------------- P2PConnection ]
- In an outbound connection from TestNode, P2PConnection is the responder
and `respond_v2_handshake()`, `complete_handshake()`, `authenticate_handshake()`
are called on it. [ TestNode -----------------> P2PConnection ]
This avoids circular dependency happening when importing MAGIC_BYTES.
Before,
p2p.py <--import for EncryptedP2PState-- v2_p2p.py
| ^
| |
└---------import for MAGIC_BYTES----------┘
Now, MAGIC_BYTES are kept separately in messages.py
Co-authored-by: Martin Zumsande <mzumsande@gmail.com>
878d914777 doc: test: mention OS detection preferences in style guideline (Sebastian Falbesoner)
4c65ac96f8 test: detect OS consistently using `platform.system()` (Sebastian Falbesoner)
37324ae3df test: use `skip_if_platform_not_linux` helper where possible (Sebastian Falbesoner)
Pull request description:
There are at least three ways to detect the operating system in Python3:
- `os.name` (https://docs.python.org/3.9/library/os.html#os.name)
- `sys.platform` (https://docs.python.org/3.9/library/sys.html#sys.platform)
- `platform.system()` (https://docs.python.org/3.9/library/platform.html#platform.system)
We are currently using all of them in functional tests (both in individual tests and shared test framework code), which seems a bit messy. This PR consolidates into using `platform.system()`, as it appears to be one most consistent and easy to read (see also [IRC discussion](https://bitcoin-irc.chaincode.com/bitcoin-core-dev/2023-12-08#989301;) and table below). `sys.platform` is inconsistent as it has the major version number encoded for BSD systems, which doesn't make much sense for e.g. OpenBSD, where there is no concept of major versions, but instead the version is simply increased by 0.1 on each release.
Note that `os.name` is still useful to detect whether we are running a POSIX system (see `BitcoinTestFramework.skip_if_platform_not_posix`), so for this use-case it is kept as only exception. The following table shows values for common operating systems, found via
```
$ python3 -c "import os; import sys; import platform; print(os.name, sys.platform, platform.system())"
```
| OS | os.name | sys.platform | platform.system() |
|--------------|---------|--------------|--------------------|
| Linux 6.2.0 | posix | linux | Linux |
| MacOS* | posix | darwin | Darwin |
| OpenBSD 7.4 | posix | openbsd7 | OpenBSD |
| Windows* | nt | win32 | Windows |
\* = I neither have a MacOS nor a Windows machine available, so I extracted the values from documentation and our current code. Also I'm relying on CI for testing the relevant code-paths. Having reviewers to this this locally would be very appreciated, if this gets Concept ACKed.
ACKs for top commit:
kevkevinpal:
ACK [878d914](878d914777)
achow101:
ACK 878d914777
hebasto:
ACK 878d914777, I have reviewed the code and it looks OK.
pablomartin4btc:
tACK 878d914777
Tree-SHA512: 24513d493e47f572028c843260b81c47c2c29bfb701991050255c9f9529cd19065ecbc7b3b6e15619da7f3f608b4825c345ce6fee30d8fd1eaadbd08cff400fc
Only relying on the number of peers for detecting a new connection
suffers from race conditions, as unrelated previous peers could
disconnect at anytime in-between. Use the more robust approach of
watching for an increased highest peer id instead (again using the
`getpeerinfo` RPC call), with a newly introduced context manager
method `TestNode.wait_for_new_peer()`.
Fixes#29009.
35fb9930ad test: enable v2 transport for p2p_timeouts.py (Martin Zumsande)
2c1669c37a test: enable v2 transport for rpc_net.py (Sebastian Falbesoner)
cc961c2695 test: enable v2 transport for p2p_node_network_limited.py (Sebastian Falbesoner)
3598a1b5c9 test: enable --v2transport in combination with --usecli (Martin Zumsande)
68a9001751 test: persist -v2transport over restarts and respect -v2transport=0 (Martin Zumsande)
Pull request description:
This makes the functional test suite compatible with BIP324, so that
`python3 test_runner.py --v2transport`
should succeed (currently, 12 tests fail for me on master).
Includes two commits by TheStack I found in an old discussion https://github.com/bitcoin/bitcoin/pull/28331#discussion_r1326714164
Note that even though all tests should pass, the python `p2p.py` module will do v2 connections only after the merge of #24748, so that for now only connections between two full nodes will actually run v2.
Some of the fixed tests were added with `--v2transport` to the test runner. Though after #24748 we might also want to consider running the entire suite with `--v2transport` in some CI.
ACKs for top commit:
sipa:
utACK 35fb9930ad. Thanks for taking care of this.
achow101:
ACK 35fb9930ad
theStack:
ACK 35fb9930ad
stratospher:
ACK 35fb993.
Tree-SHA512: 80dc0bf211fa525ff1d092043aea9f222f14c02e5832a548fb8b83b9ede1fcee03c5e8ade0d05c331bdaa492af9c1cf3d0f0b15b846673c6eacea82dd4cefbc3
a478c817b2 test: replace `Callable`/`Iterable` with their `collections.abc` alternative (PEP 585) (stickies-v)
4b9afb18e6 scripted-diff: use PEP 585 built-in collection types for verify-binary script (Sebastian Falbesoner)
d516cf83ed test: use built-in collection types for type hints (Python 3.9 / PEP 585) (Sebastian Falbesoner)
Pull request description:
With Python 3.9 / [PEP 585](https://peps.python.org/pep-0585/), [type hinting has become a little less awkward](https://docs.python.org/3.9/whatsnew/3.9.html#type-hinting-generics-in-standard-collections), as for collection types one doesn't need to import the corresponding capitalized types (`Dict`, `List`, `Set`, `Tuple`, ...) anymore, but can use the built-in types directly (see https://peps.python.org/pep-0585/#implementation for the full list).
This PR applies the replacement for all Python scripts (i.e. in the contrib and test folders) for the basic types, i.e.:
- typing.Dict -> dict
- typing.List -> list
- typing.Set -> set
- typing.Tuple -> tuple
For an additional check, I ran mypy 1.6.1 on both master and the PR branch via
```
$ mypy --ignore-missing-imports --explicit-package-bases $(git ls-files "*.py")
```
and verified that the output is identical -- (from the 22 identified problems, most look like false-positives, it's probably worth it to go deeper here and address them in a follow-up though).
ACKs for top commit:
stickies-v:
ACK a478c817b2
fanquake:
ACK a478c817b2
Tree-SHA512: 6948c905f6abd644d84f09fcb3661d7edb2742e8f2b28560008697d251d77a61a1146ab4b070e65b0d27acede7a5256703da7bf6eb1c7c3a897755478c76c6e8
43de4d3630 doc: fix typos (Sjors Provoost)
Pull request description:
This PR fixes typos found by lint-spelling.py using codespell 2.2.6.
Our CI linter job uses codespell 2.2.5 and found fewer typos that I did locally. In any case it's happy now.
ACKs for top commit:
pablomartin4btc:
re ACK 43de4d3630
Tree-SHA512: c032fe86cb49c924a468385653b31f309a9db68c478d70335bba3e65a1ff3826abe80284fe00a090ab5a509e1edbf17e476f6922fb15d055e50f1103dad2ccb0
By renaming the "command" send_cli arg. The old name was unsuitable
because the "addnode" RPC has its own "command" arg, leading to
ambiguity when included in kwargs.
Can be tested with
"python3 wallet_multiwallet.py --usecli --v2transport"
which fails on master because of this (python throws a TypeError).
Before, a global -v2transport provided to the test would be dropped
when restarting the node within a test and specifying any extra_args.
Fix this by adding "v2transport=1" to args (not extra_args) based
on the global parameter, and deciding for each (re)start of the node
based on this default and test-specific extra_args
(which take precedence over args) whether v2 should be used.
fa02598469 test: Add missing sync on send_version in peer_connect (MarcoFalke)
Pull request description:
Without the sync, the logic will be racy. For example, `p2p_sendtxrcncl.py` is failing locally (and on CI occasionally), because non-version messages will be sent before the version message:
```py
self.log.info('SENDTXRCNCL with version=0 triggers a disconnect')
sendtxrcncl_low_version = create_sendtxrcncl_msg()
sendtxrcncl_low_version.version = 0
peer = self.nodes[0].add_p2p_connection(PeerNoVerack(), send_version=True, wait_for_verack=False)
with self.nodes[0].assert_debug_log(["txreconciliation protocol violation"]):
peer.send_message(sendtxrcncl_low_version)
peer.wait_for_disconnect()
```
```
test 2023-11-02T08:15:19.620000Z TestFramework (INFO): SENDTXRCNCL with version=0 triggers a disconnect
test 2023-11-02T08:15:19.621000Z TestFramework.p2p (DEBUG): Connecting to Bitcoin Node: 127.0.0.1:11312
test 2023-11-02T08:15:19.624000Z TestFramework.p2p (DEBUG): Connected & Listening: 127.0.0.1:11312
test 2023-11-02T08:15:19.798000Z TestFramework.p2p (DEBUG): Send message to 127.0.0.1:11312: msg_sendtxrcncl(version=0, salt=2)
test 2023-11-02T08:15:19.799000Z TestFramework.p2p (DEBUG): Send message to 127.0.0.1:11312: msg_version(nVersion=70016 nServices=9 nTime=Thu Nov 2 08:15:19 2023 addrTo=CAddress(nServices=1 net=IPv4 addr=127.0.0.1 port=11312) addrFrom=CAddress(nServices=1 net=IPv4 addr=0.0.0.0 port=0) nNonce=0x369AC031CDA96022 strSubVer=/python-p2p-tester:0.0.3/ nStartingHeight=-1 relay=1)
node0 2023-11-02T08:15:19.804409Z [net] [net.cpp:3676] [CNode] [net] Added connection peer=0
node0 2023-11-02T08:15:19.805256Z [net] [net.cpp:1825] [CreateNodeFromAcceptedSocket] [net] connection from 127.0.0.1:55964 accepted
node0 2023-11-02T08:15:19.809861Z [msghand] [net_processing.cpp:3356] [ProcessMessage] [net] received: sendtxrcncl (12 bytes) peer=0
node0 2023-11-02T08:15:19.810297Z [msghand] [net_processing.cpp:3582] [ProcessMessage] [net] non-version message before version handshake. Message "sendtxrcncl" from peer=0
node0 2023-11-02T08:15:19.810928Z [msghand] [net_processing.cpp:3356] [ProcessMessage] [net] received: version (111 bytes) peer=0
...
test 2023-11-02T09:35:20.166000Z TestFramework.utils (ERROR): wait_until() failed. Predicate: ''''
def test_function():
if check_connected:
assert self.is_connected
return test_function_in()
'''
test 2023-11-02T09:35:20.187000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "/ci_container_base/ci/scratch/build/bitcoin-s390x-linux-gnu/test/functional/test_framework/test_framework.py", line 132, in main
self.run_test()
File "/ci_container_base/ci/scratch/build/bitcoin-s390x-linux-gnu/test/functional/p2p_sendtxrcncl.py", line 188, in run_test
peer.wait_for_disconnect()
File "/ci_container_base/ci/scratch/build/bitcoin-s390x-linux-gnu/test/functional/test_framework/p2p.py", line 478, in wait_for_disconnect
self.wait_until(test_function, timeout=timeout, check_connected=False)
File "/ci_container_base/ci/scratch/build/bitcoin-s390x-linux-gnu/test/functional/test_framework/p2p.py", line 470, in wait_until
wait_until_helper_internal(test_function, timeout=timeout, lock=p2p_lock, timeout_factor=self.timeout_factor)
File "/ci_container_base/ci/scratch/build/bitcoin-s390x-linux-gnu/test/functional/test_framework/util.py", line 275, in wait_until_helper_internal
raise AssertionError("Predicate {} not true after {} seconds".format(predicate_source, timeout))
AssertionError: Predicate ''''
def test_function():
if check_connected:
assert self.is_connected
return test_function_in()
''' not true after 4800.0 seconds
ACKs for top commit:
mzumsande:
ACK fa02598469
Tree-SHA512: 78871f603d387e2df8c0acbdfa95441fa186f80e94593021bb219bbf1bc9dc7efc4e266bd254b5cc41114c38227ff3b7f6172335d9bb828427f0a2acffde752d
