test: Fix p2p_addr_relay.py
#5967
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The value of nServices for `NODE_NETWORK | NODE_WITNESS`, the default for
p2p_addr{v2}_relay.py in Bitcoin Core, is 9. Dash doesn't implement SegWit
and so the corresponding nServices value is `NODE_NETWORK`, which is 1.
kwvg
suggested changes
Apr 2, 2024
| @@ -16,22 +16,11 @@ | |||
| from test_framework.util import ( | |||
| assert_equal, | |||
| ) | |||
| import time | |||
|
|
|||
| ADDRS = [] | |||
There was a problem hiding this comment.
Can ADDRS remain outside?
In the last backport that modified p2p_addr_relay.py (bitcoin#19272), it has remained outside. Scope pollution shouldn't be much of a concern since tests don't include each other.
I mention this because a similar change was done for p2p_addrv2_relay.py (see develop vs bitcoin#19954, the backport that introduced this test) and I had to revert it in a WIP PR to make way for bitcoin#22211 because ADDRS was being accessed by both AddrReceiver and AddrTest in the changes introduced.
Also, ADDRS in p2p_addr_relay.py is anyways done away with in bitcoin#21707 (diff) so might be worthwhile to keep it in line with upstream to make the changes easier to follow when it is eventually done away with.
PastaPastaPasta
approved these changes
Apr 2, 2024
PastaPastaPasta
closed this pull request by merging all changes
into
dashpay:develop
in
14a923a
5 tasks
PastaPastaPasta
added a commit
that referenced
this pull request
Apr 15, 2024
, bitcoin#22211, bitcoin#22387, bitcoin#21528, bitcoin#22616, bitcoin#22604, bitcoin#22960, bitcoin#23218 (networking backports: part 3) 1fedf47 test: add type annotation for `ADDRS` in `p2p_addrv2_relay` (Kittywhiskers Van Gogh) 022b76f merge bitcoin#23218: Use mocktime for ping timeout (Kittywhiskers Van Gogh) 45d9e58 merge bitcoin#22960: Set peertimeout in write_config (Kittywhiskers Van Gogh) 06e909b merge bitcoin#22604: address rate-limiting follow-ups (Kittywhiskers Van Gogh) 60b3e08 merge bitcoin#22616: address relay fixups (Kittywhiskers Van Gogh) 8b8fbc5 merge bitcoin#22618: Small follow-ups to 21528 (Kittywhiskers Van Gogh) 18fe765 merge bitcoin#21528: Reduce addr blackholes (Kittywhiskers Van Gogh) c1874c6 net_processing: gate `m_tx_relay` access behind `!IsBlockOnlyConn()` (Kittywhiskers Van Gogh) 602d13d merge bitcoin#22387: Rate limit the processing of rumoured addresses (Kittywhiskers Van Gogh) fe66202 merge bitcoin#22211: relay I2P addresses even if not reachable (by us) (Kittywhiskers Van Gogh) 7e08db5 merge bitcoin#22306: Improvements to p2p_addr_relay.py (Kittywhiskers Van Gogh) ff3497c merge bitcoin#21843: enable GetAddr, GetAddresses, and getnodeaddresses by network (Kittywhiskers Van Gogh) 51edeb0 merge bitcoin#21594: add network field to getnodeaddresses (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * Dependency for #5982 * Population of `ADDRS` in `p2p_addr`(`v2`)`_relay` in Dash is done in the test object ([source](https://github.com/dashpay/dash/blob/0a62b9f985866249964ed703108a8dc617c73ba3/test/functional/p2p_addrv2_relay.py#L42-L49)) as opposed to upstream, where it is done in the global state ([source](https://github.com/maflcko/bitcoin-core/blob/d930c7f5b091687eb4208a5ffe8a2abe311d8054/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](bitcoin#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](#5967) and a discussion ([source](https://github.com/dashpay/dash/pull/5967/files#r1548101561)) * Eventually, following the footsteps of [dash#5967](#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](262d006)), 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](cb6d36d)) * Working on [bitcoin#21528](bitcoin#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](https://github.com/amitiuttarwar/bitcoin/blob/3f7250b328b8b2f5d63f323702445ac5c989b73d/src/net.cpp#L2989-L2991)) and can always check if transaction relaying is permitted by checking if it's initialized ([source](https://github.com/amitiuttarwar/bitcoin/blob/3f7250b328b8b2f5d63f323702445ac5c989b73d/src/net_processing.cpp#L1820-L1826)). * Dash unconditionally initializes it ([source](https://github.com/dashpay/dash/blob/0a62b9f985866249964ed703108a8dc617c73ba3/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](https://github.com/dashpay/dash/blob/dc6f52ac99a3b6fb8b3dfe37db4d6db1f7b1e719/src/net_processing.cpp#L2134-L2140)), which at the time, simply meant, it wasn't a block-only connection ([source](https://github.com/dashpay/dash/blob/dc6f52ac99a3b6fb8b3dfe37db4d6db1f7b1e719/src/net.h#L568-L572)). * This mutual exclusivity no longer held true in [dash#5964](#5964) and therefore, some transaction relay decisions were bound to **not** being a block-only connection ([commit](26c39f5)) but some were left behind, adopting `RelayAddrsWithPeer()` ([source](https://github.com/dashpay/dash/blob/0a62b9f985866249964ed703108a8dc617c73ba3/src/net_processing.cpp#L2215-L2221)), which, to be noted, is determined by the initialization status of `Peer::m_addr_known` ([source](https://github.com/dashpay/dash/blob/0a62b9f985866249964ed703108a8dc617c73ba3/src/net_processing.cpp#L839-L842)), which, so far, was pegged to **not** block-relay connection status ([source](https://github.com/dashpay/dash/blob/0a62b9f985866249964ed703108a8dc617c73ba3/src/net_processing.cpp#L1319)). * [bitcoin#21528](bitcoin#21528) got rid of `RelayAddrsWithPeer()` and replaced it with `Peer::m_addr_relay_enabled` ([source](https://github.com/amitiuttarwar/bitcoin/blob/3f7250b328b8b2f5d63f323702445ac5c989b73d/src/net_processing.cpp#L237-L251)), which is setup using `Peer::SetupAddressRelay()` ([source](https://github.com/amitiuttarwar/bitcoin/blob/3f7250b328b8b2f5d63f323702445ac5c989b73d/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](https://github.com/amitiuttarwar/bitcoin/blob/3f7250b328b8b2f5d63f323702445ac5c989b73d/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](109c5a9#diff-6875de769e90cec84d2e8a9c1b962cdbcda44d870d42e4215827e599e11e90e3L2131-L2134)) but other changes where the condition itself has been changed (`RelayAddrsWithPeer()` > `!CNode::IsBlockOnlyConn()`) ([source](109c5a9#diff-6875de769e90cec84d2e8a9c1b962cdbcda44d870d42e4215827e599e11e90e3R2256-R2259)) * This does mean that in [dash#5982](#5982), `Peer::m_block_relay_only` is introduced to be the counterpart to `Peer::m_addr_relay_enabled` ([source](https://github.com/dashpay/dash/blame/45b48dae0a66fd918834d012cc8e1e17b5824abe/src/net_processing.cpp#L321-L322)) to account for some `CConnman` logic being moved into `PeerManager` ([source](https://github.com/dashpay/dash/blame/45b48dae0a66fd918834d012cc8e1e17b5824abe/src/net_processing.cpp#L2186-L2195)), which, in a way, reverts [dash#5339](#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](#5964) and [dash#5967](#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 1fedf47 Tree-SHA512: 533d33f79a0d9fd730073b3b9a58baf1dd3b0c95823e765c88a43cc974970ed3609bf1863c63ac7fc5586d1437e5250b0a2d3005468da09e407110a412bd0264
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue being fixed or feature implemented
p2p_addr_relay.pyis broken but we don't see it in CI because it doesn't test everything it should atm. This weird behaviour was discovered by @kwvg while preparing #5964.What was done?
Bring back the missing check and fix the test. Borrowed one fix from #5964 to make this PR complete.
How Has This Been Tested?
Run
p2p_addr_relay.pyBreaking Changes
n/a
Checklist: