backport: merge bitcoin#21594, #21843, #22306, #22211, #22387, #21528, #22616, #22604, #22960, #23218 (networking backports: part 3)

[kwvg]

Additional Information

• Dependency for backport: merge bitcoin#20769, #19499, #23575, #23695, #21160, #24692, partial bitcoin#20196, #25176, merge bitcoin-core/gui#526 (networking backports: part 4) #5982
• Population of ADDRS in p2p_addr(v2)_relay in Dash is done in the test object (source) as opposed to upstream, where it is done in the global state (source). This is because Dash specifically relies on self.mocktime instead of Bitcoin, which will work with simply sampling current time (time.time()).
  • bitcoin#22211 adds changes (source) that add usage of ADDRS outside the test object. That, alongside with other considerations, resulted in dash#5967 and a discussion (source)
  • Eventually, following the footsteps of dash#5967, ADDRS was defined outside but setup within the test object. This worked just fine (build) but displeased the linter (build) 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), which pleased the linter (build) but broken the test (build) 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)
• Working on 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) and can always check if transaction relaying is permitted by checking if it's initialized (source).
  • Dash unconditionally initializes it (source). Earlier, Dash used to check if it's appropriate to relay transactions by checking if it can relay addresses (source), which at the time, simply meant, it wasn't a block-only connection (source).
  • This mutual exclusivity no longer held true in dash#5964 and therefore, some transaction relay decisions were bound to not being a block-only connection (commit) but some were left behind, adopting RelayAddrsWithPeer() (source), which, to be noted, is determined by the initialization status of Peer::m_addr_known (source), which, so far, was pegged to not block-relay connection status (source).
  • bitcoin#21528 got rid of RelayAddrsWithPeer() and replaced it with Peer::m_addr_relay_enabled (source), which is setup using Peer::SetupAddressRelay() (source). 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).

    • 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) but other changes where the condition itself has been changed (RelayAddrsWithPeer() > !CNode::IsBlockOnlyConn()) (source)

  • This does mean that in dash#5982, Peer::m_block_relay_only is introduced to be the counterpart to Peer::m_addr_relay_enabled (source) to account for some CConnman logic being moved into PeerManager (source), which, in a way, reverts dash#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 and dash#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:

• 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
• I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

[knst]

21528 - please include backport bitcoin#22618 which has release notes for this changes

[kwvg]

Resolved in latest push!

[PastaPastaPasta]

Generally looks good

[PastaPastaPasta]

this feels wrong; 21528 doesn't introduce any usage of IsBlockOnlyConn? why not use peer->m_addr_relay_enabled?

[kwvg]

This has already been explained in the PR description but the switchover to IsBlockOnlyConn has been split into a separate commit to avoid association with changes from the backport.

[knst]

LGTM

[UdjinM6]

utACK

[PastaPastaPasta]

utACK 1fedf47