Merge bitcoin/bitcoin#26359: p2p: Erlay support signaling follow-ups

46339d29b1 test, refactor: Reorder sendtxrcncl tests for better readability (Gleb Naumenko)
14263c13f1 p2p, refactor: Extend logs for unexpected sendtxrcncl (Gleb Naumenko)
87493e112e p2p, test, refactor: Minor code improvements (Gleb Naumenko)
00c5dec818 p2p: Clarify sendtxrcncl policies (Gleb Naumenko)
ac6ee5ba21 test: Expand unit and functional tests for txreconciliation (Gleb Naumenko)
bc84e24a4f p2p, refactor: Switch to enum class for ReconciliationRegisterResult (Gleb Naumenko)
a60f729e29 p2p: Drop roles from sendtxrcncl (Gleb Naumenko)
6772cbf69c tests: stabilize sendtxrcncl test (Gleb Naumenko)

Pull request description:

  Non-trivial changes include:
  - Getting rid of roles in `sendtxrcncl` message (summarized in the [BIP PR](https://github.com/bitcoin/bips/pull/1376));
  - Disconnect the peer if it send `sendtxrcncl` although we are in `blocksonly` and notified the peer with `fRelay=0`;
  - Don't send `sendtxrcncl` to feeler connections.

ACKs for top commit:
  vasild:
    ACK 46339d29b1
  ariard:
    ACK 46339d2
  mzumsande:
    Code Review ACK 46339d29b1

Tree-SHA512: b5cc6934b4670c12b7dbb3189e739ef747ee542ec56678bf4e4355bfb481b746d32363c173635685b71969b3fe4bd52b1c8ebd3ea3b35c82044bba69220f6417

src/init.cpp

@@ -477,6 +477,7 @@ void SetupServerArgs(ArgsManager& argsman)
     argsman.AddArg("-onlynet=<net>", "Make automatic outbound connections only to network <net> (" + Join(GetNetworkNames(), ", ") + "). Inbound and manual connections are not affected by this option. It can be specified multiple times to allow multiple networks.", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
     argsman.AddArg("-peerbloomfilters", strprintf("Support filtering of blocks and transaction with bloom filters (default: %u)", DEFAULT_PEERBLOOMFILTERS), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
     argsman.AddArg("-peerblockfilters", strprintf("Serve compact block filters to peers per BIP 157 (default: %u)", DEFAULT_PEERBLOCKFILTERS), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
+    argsman.AddArg("-txreconciliation", strprintf("Enable transaction reconciliations per BIP 330 (default: %d)", DEFAULT_TXRECONCILIATION_ENABLE), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::CONNECTION);
     // TODO: remove the sentence "Nodes not using ... incoming connections." once the changes from
     // https://github.com/bitcoin/bitcoin/pull/23542 have become widespread.
     argsman.AddArg("-port=<port>", strprintf("Listen for connections on <port>. Nodes not using the default ports (default: %u, testnet: %u, signet: %u, regtest: %u) are unlikely to get incoming connections. Not relevant for I2P (see doc/i2p.md).", defaultChainParams->GetDefaultPort(), testnetChainParams->GetDefaultPort(), signetChainParams->GetDefaultPort(), regtestChainParams->GetDefaultPort()), ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY, OptionsCategory::CONNECTION);
@@ -485,7 +486,6 @@ void SetupServerArgs(ArgsManager& argsman)
     argsman.AddArg("-seednode=<ip>", "Connect to a node to retrieve peer addresses, and disconnect. This option can be specified multiple times to connect to multiple nodes.", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
     argsman.AddArg("-networkactive", "Enable all P2P network activity (default: 1). Can be changed by the setnetworkactive RPC command", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
     argsman.AddArg("-timeout=<n>", strprintf("Specify socket connection timeout in milliseconds. If an initial attempt to connect is unsuccessful after this amount of time, drop it (minimum: 1, default: %d)", DEFAULT_CONNECT_TIMEOUT), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
-    argsman.AddArg("-txreconciliation", strprintf("Enable transaction reconciliations per BIP 330 (default: %d)", DEFAULT_TXRECONCILIATION_ENABLE), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::CONNECTION);
     argsman.AddArg("-peertimeout=<n>", strprintf("Specify a p2p connection timeout delay in seconds. After connecting to a peer, wait this amount of time before considering disconnection based on inactivity (minimum: 1, default: %d)", DEFAULT_PEER_CONNECT_TIMEOUT), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::CONNECTION);
     argsman.AddArg("-torcontrol=<ip>:<port>", strprintf("Tor control port to use if onion listening enabled (default: %s)", DEFAULT_TOR_CONTROL), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
     argsman.AddArg("-torpassword=<pass>", "Tor control port password (default: empty)", ArgsManager::ALLOW_ANY | ArgsManager::SENSITIVE, OptionsCategory::CONNECTION);

src/net_processing.cpp

@@ -3273,17 +3273,14 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
 
         if (greatest_common_version >= WTXID_RELAY_VERSION && m_txreconciliation) {
             // Per BIP-330, we announce txreconciliation support if:
-            // - protocol version per the VERSION message supports WTXID_RELAY;
-            // - we intended to exchange transactions over this connection while establishing it
-            //   and the peer indicated support for transaction relay in the VERSION message;
+            // - protocol version per the peer's VERSION message supports WTXID_RELAY;
+            // - transaction relay is supported per the peer's VERSION message (see m_relays_txs);
+            // - this is not a block-relay-only connection and not a feeler (see m_relays_txs);
+            // - this is not an addr fetch connection;
             // - we are not in -blocksonly mode.
-            if (pfrom.m_relays_txs && !m_ignore_incoming_txs) {
+            if (pfrom.m_relays_txs && !pfrom.IsAddrFetchConn() && !m_ignore_incoming_txs) {
                 const uint64_t recon_salt = m_txreconciliation->PreRegisterPeer(pfrom.GetId());
-                // We suggest our txreconciliation role (initiator/responder) based on
-                // the connection direction.
                 m_connman.PushMessage(&pfrom, msg_maker.Make(NetMsgType::SENDTXRCNCL,
-                                                             !pfrom.IsInboundConn(),
-                                                             pfrom.IsInboundConn(),
                                                              TXRECONCILIATION_VERSION, recon_salt));
             }
         }
@@ -3500,41 +3497,44 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
         }
 
         if (pfrom.fSuccessfullyConnected) {
-            // Disconnect peers that send a SENDTXRCNCL message after VERACK.
             LogPrintLevel(BCLog::NET, BCLog::Level::Debug, "sendtxrcncl received after verack from peer=%d; disconnecting\n", pfrom.GetId());
             pfrom.fDisconnect = true;
             return;
         }
 
-        if (!peer->GetTxRelay()) {
-            // Disconnect peers that send a SENDTXRCNCL message even though we indicated we don't
-            // support transaction relay.
+        // Peer must not offer us reconciliations if we specified no tx relay support in VERSION.
+        if (RejectIncomingTxs(pfrom)) {
             LogPrintLevel(BCLog::NET, BCLog::Level::Debug, "sendtxrcncl received from peer=%d to which we indicated no tx relay; disconnecting\n", pfrom.GetId());
             pfrom.fDisconnect = true;
             return;
         }
 
-        bool is_peer_initiator, is_peer_responder;
-        uint32_t peer_txreconcl_version;
-        uint64_t remote_salt;
-        vRecv >> is_peer_initiator >> is_peer_responder >> peer_txreconcl_version >> remote_salt;
-
-        if (m_txreconciliation->IsPeerRegistered(pfrom.GetId())) {
-            // A peer is already registered, meaning we already received SENDTXRCNCL from them.
-            LogPrintLevel(BCLog::NET, BCLog::Level::Debug, "txreconciliation protocol violation from peer=%d (sendtxrcncl received from already registered peer); disconnecting\n", pfrom.GetId());
+        // Peer must not offer us reconciliations if they specified no tx relay support in VERSION.
+        // This flag might also be false in other cases, but the RejectIncomingTxs check above
+        // eliminates them, so that this flag fully represents what we are looking for.
+        if (!pfrom.m_relays_txs) {
+            LogPrintLevel(BCLog::NET, BCLog::Level::Debug, "sendtxrcncl received from peer=%d which indicated no tx relay to us; disconnecting\n", pfrom.GetId());
             pfrom.fDisconnect = true;
             return;
         }
 
-        const ReconciliationRegisterResult result = m_txreconciliation->RegisterPeer(pfrom.GetId(), pfrom.IsInboundConn(),
-                                                                                is_peer_initiator, is_peer_responder,
-                                                                                peer_txreconcl_version,
-                                                                                remote_salt);
+        uint32_t peer_txreconcl_version;
+        uint64_t remote_salt;
+        vRecv >> peer_txreconcl_version >> remote_salt;
 
-        // If it's a protocol violation, disconnect.
-        // If the peer was not found (but something unexpected happened) or it was registered,
-        // nothing to be done.
-        if (result == ReconciliationRegisterResult::PROTOCOL_VIOLATION) {
+        const ReconciliationRegisterResult result = m_txreconciliation->RegisterPeer(pfrom.GetId(), pfrom.IsInboundConn(),
+                                                                                     peer_txreconcl_version, remote_salt);
+        switch (result) {
+        case ReconciliationRegisterResult::NOT_FOUND:
+            LogPrintLevel(BCLog::NET, BCLog::Level::Debug, "Ignore unexpected txreconciliation signal from peer=%d\n", pfrom.GetId());
+            break;
+        case ReconciliationRegisterResult::SUCCESS:
+            break;
+        case ReconciliationRegisterResult::ALREADY_REGISTERED:
+            LogPrintLevel(BCLog::NET, BCLog::Level::Debug, "txreconciliation protocol violation from peer=%d (sendtxrcncl received from already registered peer); disconnecting\n", pfrom.GetId());
+            pfrom.fDisconnect = true;
+            return;
+        case ReconciliationRegisterResult::PROTOCOL_VIOLATION:
             LogPrintLevel(BCLog::NET, BCLog::Level::Debug, "txreconciliation protocol violation from peer=%d; disconnecting\n", pfrom.GetId());
             pfrom.fDisconnect = true;
             return;

src/node/txreconciliation.cpp

@@ -39,7 +39,8 @@ public:
      * the following commits.
      *
      * Reconciliation protocol assumes using one role consistently: either a reconciliation
-     * initiator (requesting sketches), or responder (sending sketches). This defines our role.
+     * initiator (requesting sketches), or responder (sending sketches). This defines our role,
+     * based on the direction of the p2p connection.
      *
      */
     bool m_we_initiate;
@@ -81,31 +82,30 @@ public:
     {
         AssertLockNotHeld(m_txreconciliation_mutex);
         LOCK(m_txreconciliation_mutex);
-        // We do not support txreconciliation salt/version updates.
-        assert(m_states.find(peer_id) == m_states.end());
 
         LogPrintLevel(BCLog::TXRECONCILIATION, BCLog::Level::Debug, "Pre-register peer=%d\n", peer_id);
         const uint64_t local_salt{GetRand(UINT64_MAX)};
 
         // We do this exactly once per peer (which are unique by NodeId, see GetNewNodeId) so it's
         // safe to assume we don't have this record yet.
-        Assert(m_states.emplace(peer_id, local_salt).second);
+        Assume(m_states.emplace(peer_id, local_salt).second);
         return local_salt;
     }
 
-    ReconciliationRegisterResult RegisterPeer(NodeId peer_id, bool is_peer_inbound, bool is_peer_recon_initiator,
-                                     bool is_peer_recon_responder, uint32_t peer_recon_version,
-                                     uint64_t remote_salt) EXCLUSIVE_LOCKS_REQUIRED(!m_txreconciliation_mutex)
+    ReconciliationRegisterResult RegisterPeer(NodeId peer_id, bool is_peer_inbound, uint32_t peer_recon_version,
+                                              uint64_t remote_salt) EXCLUSIVE_LOCKS_REQUIRED(!m_txreconciliation_mutex)
     {
         AssertLockNotHeld(m_txreconciliation_mutex);
         LOCK(m_txreconciliation_mutex);
         auto recon_state = m_states.find(peer_id);
 
-        // A peer should be in the pre-registered state to proceed here.
-        if (recon_state == m_states.end()) return NOT_FOUND;
-        uint64_t* local_salt = std::get_if<uint64_t>(&recon_state->second);
-        // A peer is already registered. This should be checked by the caller.
-        Assume(local_salt);
+        if (recon_state == m_states.end()) return ReconciliationRegisterResult::NOT_FOUND;
+
+        if (std::holds_alternative<TxReconciliationState>(recon_state->second)) {
+            return ReconciliationRegisterResult::ALREADY_REGISTERED;
+        }
+
+        uint64_t local_salt = *std::get_if<uint64_t>(&recon_state->second);
 
         // If the peer supports the version which is lower than ours, we downgrade to the version
         // it supports. For now, this only guarantees that nodes with future reconciliation
@@ -114,27 +114,14 @@ public:
         // satisfactory (e.g. too low).
         const uint32_t recon_version{std::min(peer_recon_version, m_recon_version)};
         // v1 is the lowest version, so suggesting something below must be a protocol violation.
-        if (recon_version < 1) return PROTOCOL_VIOLATION;
+        if (recon_version < 1) return ReconciliationRegisterResult::PROTOCOL_VIOLATION;
 
-        // Must match SENDTXRCNCL logic.
-        const bool they_initiate = is_peer_recon_initiator && is_peer_inbound;
-        const bool we_initiate = !is_peer_inbound && is_peer_recon_responder;
+        LogPrintLevel(BCLog::TXRECONCILIATION, BCLog::Level::Debug, "Register peer=%d (inbound=%i)\n",
+                      peer_id, is_peer_inbound);
 
-        // If we ever announce support for both requesting and responding, this will need
-        // tie-breaking. For now, this is mutually exclusive because both are based on the
-        // inbound flag.
-        assert(!(they_initiate && we_initiate));
-
-        // The peer set both flags to false, we treat it as a protocol violation.
-        if (!(they_initiate || we_initiate)) return PROTOCOL_VIOLATION;
-
-        LogPrintLevel(BCLog::TXRECONCILIATION, BCLog::Level::Debug, "Register peer=%d with the following params: " /* Continued */
-                                                                    "we_initiate=%i, they_initiate=%i.\n",
-                      peer_id, we_initiate, they_initiate);
-
-        const uint256 full_salt{ComputeSalt(*local_salt, remote_salt)};
-        recon_state->second = TxReconciliationState(we_initiate, full_salt.GetUint64(0), full_salt.GetUint64(1));
-        return SUCCESS;
+        const uint256 full_salt{ComputeSalt(local_salt, remote_salt)};
+        recon_state->second = TxReconciliationState(!is_peer_inbound, full_salt.GetUint64(0), full_salt.GetUint64(1));
+        return ReconciliationRegisterResult::SUCCESS;
     }
 
     void ForgetPeer(NodeId peer_id) EXCLUSIVE_LOCKS_REQUIRED(!m_txreconciliation_mutex)
@@ -166,11 +153,9 @@ uint64_t TxReconciliationTracker::PreRegisterPeer(NodeId peer_id)
 }
 
 ReconciliationRegisterResult TxReconciliationTracker::RegisterPeer(NodeId peer_id, bool is_peer_inbound,
-                                                          bool is_peer_recon_initiator, bool is_peer_recon_responder,
                                                           uint32_t peer_recon_version, uint64_t remote_salt)
 {
-    return m_impl->RegisterPeer(peer_id, is_peer_inbound, is_peer_recon_initiator, is_peer_recon_responder,
-                                peer_recon_version, remote_salt);
+    return m_impl->RegisterPeer(peer_id, is_peer_inbound, peer_recon_version, remote_salt);
 }
 
 void TxReconciliationTracker::ForgetPeer(NodeId peer_id)

src/node/txreconciliation.h

@@ -16,10 +16,11 @@ static constexpr bool DEFAULT_TXRECONCILIATION_ENABLE{false};
 /** Supported transaction reconciliation protocol version */
 static constexpr uint32_t TXRECONCILIATION_VERSION{1};
 
-enum ReconciliationRegisterResult {
-    NOT_FOUND = 0,
-    SUCCESS = 1,
-    PROTOCOL_VIOLATION = 2,
+enum class ReconciliationRegisterResult {
+    NOT_FOUND,
+    SUCCESS,
+    ALREADY_REGISTERED,
+    PROTOCOL_VIOLATION,
 };
 
 /**
@@ -72,8 +73,8 @@ public:
      * Step 0. Once the peer agreed to reconcile txs with us, generate the state required to track
      * ongoing reconciliations. Must be called only after pre-registering the peer and only once.
      */
-    ReconciliationRegisterResult RegisterPeer(NodeId peer_id, bool is_peer_inbound, bool is_peer_recon_initiator,
-                                     bool is_peer_recon_responder, uint32_t peer_recon_version, uint64_t remote_salt);
+    ReconciliationRegisterResult RegisterPeer(NodeId peer_id, bool is_peer_inbound,
+                                              uint32_t peer_recon_version, uint64_t remote_salt);
 
     /**
      * Attempts to forget txreconciliation-related state of the peer (if we previously stored any).

src/protocol.h

@@ -259,9 +259,7 @@ extern const char* CFCHECKPT;
  */
 extern const char* WTXIDRELAY;
 /**
- * Contains 2 1-byte bools, a 4-byte version number and an 8-byte salt.
- * The 2 booleans indicate that a node is willing to participate in transaction
- * reconciliation, respectively as an initiator or as a receiver.
+ * Contains a 4-byte version number and an 8-byte salt.
  * The salt is used to compute short txids needed for efficient
  * txreconciliation, as described by BIP 330.
  */

src/test/txreconciliation_tests.cpp

@@ -12,56 +12,54 @@ BOOST_FIXTURE_TEST_SUITE(txreconciliation_tests, BasicTestingSetup)
 
 BOOST_AUTO_TEST_CASE(RegisterPeerTest)
 {
-    TxReconciliationTracker tracker(1);
+    TxReconciliationTracker tracker(TXRECONCILIATION_VERSION);
     const uint64_t salt = 0;
 
     // Prepare a peer for reconciliation.
     tracker.PreRegisterPeer(0);
 
-    // Both roles are false, don't register.
-    BOOST_CHECK(tracker.RegisterPeer(/*peer_id=*/0, /*is_peer_inbound=*/true,
-                                      /*is_peer_recon_initiator=*/false,
-                                      /*is_peer_recon_responder=*/false,
-                                      /*peer_recon_version=*/1, salt) ==
-                    ReconciliationRegisterResult::PROTOCOL_VIOLATION);
-
-    // Invalid roles for the given connection direction.
-    BOOST_CHECK(tracker.RegisterPeer(0, true, false, true, 1, salt) == ReconciliationRegisterResult::PROTOCOL_VIOLATION);
-    BOOST_CHECK(tracker.RegisterPeer(0, false, true, false, 1, salt) == ReconciliationRegisterResult::PROTOCOL_VIOLATION);
-
     // Invalid version.
-    BOOST_CHECK(tracker.RegisterPeer(0, true, true, false, 0, salt) == ReconciliationRegisterResult::PROTOCOL_VIOLATION);
+    BOOST_CHECK_EQUAL(tracker.RegisterPeer(/*peer_id=*/0, /*is_peer_inbound=*/true,
+                                           /*peer_recon_version=*/0, salt),
+                      ReconciliationRegisterResult::PROTOCOL_VIOLATION);
 
-    // Valid registration.
+    // Valid registration (inbound and outbound peers).
     BOOST_REQUIRE(!tracker.IsPeerRegistered(0));
-    BOOST_REQUIRE(tracker.RegisterPeer(0, true, true, false, 1, salt) == ReconciliationRegisterResult::SUCCESS);
+    BOOST_REQUIRE_EQUAL(tracker.RegisterPeer(0, true, 1, salt), ReconciliationRegisterResult::SUCCESS);
     BOOST_CHECK(tracker.IsPeerRegistered(0));
-
-    // Reconciliation version is higher than ours, should be able to register.
     BOOST_REQUIRE(!tracker.IsPeerRegistered(1));
     tracker.PreRegisterPeer(1);
-    BOOST_REQUIRE(tracker.RegisterPeer(1, true, true, false, 2, salt) == ReconciliationRegisterResult::SUCCESS);
+    BOOST_REQUIRE(tracker.RegisterPeer(1, false, 1, salt) == ReconciliationRegisterResult::SUCCESS);
     BOOST_CHECK(tracker.IsPeerRegistered(1));
 
+    // Reconciliation version is higher than ours, should be able to register.
+    BOOST_REQUIRE(!tracker.IsPeerRegistered(2));
+    tracker.PreRegisterPeer(2);
+    BOOST_REQUIRE(tracker.RegisterPeer(2, true, 2, salt) == ReconciliationRegisterResult::SUCCESS);
+    BOOST_CHECK(tracker.IsPeerRegistered(2));
+
+    // Try registering for the second time.
+    BOOST_REQUIRE(tracker.RegisterPeer(1, false, 1, salt) == ReconciliationRegisterResult::ALREADY_REGISTERED);
+
     // Do not register if there were no pre-registration for the peer.
-    BOOST_REQUIRE(tracker.RegisterPeer(100, true, true, false, 1, salt) == ReconciliationRegisterResult::NOT_FOUND);
+    BOOST_REQUIRE_EQUAL(tracker.RegisterPeer(100, true, 1, salt), ReconciliationRegisterResult::NOT_FOUND);
     BOOST_CHECK(!tracker.IsPeerRegistered(100));
 }
 
 BOOST_AUTO_TEST_CASE(ForgetPeerTest)
 {
-    TxReconciliationTracker tracker(1);
+    TxReconciliationTracker tracker(TXRECONCILIATION_VERSION);
     NodeId peer_id0 = 0;
 
     // Removing peer after pre-registring works and does not let to register the peer.
     tracker.PreRegisterPeer(peer_id0);
     tracker.ForgetPeer(peer_id0);
-    BOOST_CHECK(tracker.RegisterPeer(peer_id0, true, true, false, 1, 1) == ReconciliationRegisterResult::NOT_FOUND);
+    BOOST_CHECK_EQUAL(tracker.RegisterPeer(peer_id0, true, 1, 1), ReconciliationRegisterResult::NOT_FOUND);
 
     // Removing peer after it is registered works.
     tracker.PreRegisterPeer(peer_id0);
     BOOST_REQUIRE(!tracker.IsPeerRegistered(peer_id0));
-    BOOST_REQUIRE(tracker.RegisterPeer(peer_id0, true, true, false, 1, 1) == ReconciliationRegisterResult::SUCCESS);
+    BOOST_REQUIRE_EQUAL(tracker.RegisterPeer(peer_id0, true, 1, 1), ReconciliationRegisterResult::SUCCESS);
     BOOST_CHECK(tracker.IsPeerRegistered(peer_id0));
     tracker.ForgetPeer(peer_id0);
     BOOST_CHECK(!tracker.IsPeerRegistered(peer_id0));
@@ -69,14 +67,14 @@ BOOST_AUTO_TEST_CASE(ForgetPeerTest)
 
 BOOST_AUTO_TEST_CASE(IsPeerRegisteredTest)
 {
-    TxReconciliationTracker tracker(1);
+    TxReconciliationTracker tracker(TXRECONCILIATION_VERSION);
     NodeId peer_id0 = 0;
 
     BOOST_REQUIRE(!tracker.IsPeerRegistered(peer_id0));
     tracker.PreRegisterPeer(peer_id0);
     BOOST_REQUIRE(!tracker.IsPeerRegistered(peer_id0));
 
-    BOOST_REQUIRE(tracker.RegisterPeer(peer_id0, true, true, false, 1, 1) == ReconciliationRegisterResult::SUCCESS);
+    BOOST_REQUIRE_EQUAL(tracker.RegisterPeer(peer_id0, true, 1, 1), ReconciliationRegisterResult::SUCCESS);
     BOOST_CHECK(tracker.IsPeerRegistered(peer_id0));
 
     tracker.ForgetPeer(peer_id0);

test/functional/p2p_sendtxrcncl.py

@@ -10,6 +10,7 @@ from test_framework.messages import (
     msg_verack,
     msg_version,
     msg_wtxidrelay,
+    NODE_BLOOM,
 )
 from test_framework.p2p import (
     P2PInterface,
@@ -54,10 +55,8 @@ class PeerTrackMsgOrder(P2PInterface):
         super().on_message(message)
         self.messages.append(message)
 
-def create_sendtxrcncl_msg(initiator=True):
+def create_sendtxrcncl_msg():
     sendtxrcncl_msg = msg_sendtxrcncl()
-    sendtxrcncl_msg.initiator = initiator
-    sendtxrcncl_msg.responder = not initiator
     sendtxrcncl_msg.version = 1
     sendtxrcncl_msg.salt = 2
     return sendtxrcncl_msg
@@ -68,11 +67,11 @@ class SendTxRcnclTest(BitcoinTestFramework):
         self.extra_args = [['-txreconciliation']]
 
     def run_test(self):
+        # Check everything concerning *sending* SENDTXRCNCL
+        # First, *sending* to *inbound*.
         self.log.info('SENDTXRCNCL sent to an inbound')
         peer = self.nodes[0].add_p2p_connection(SendTxrcnclReceiver(), send_version=True, wait_for_verack=True)
         assert peer.sendtxrcncl_msg_received
-        assert not peer.sendtxrcncl_msg_received.initiator
-        assert peer.sendtxrcncl_msg_received.responder
         assert_equal(peer.sendtxrcncl_msg_received.version, 1)
         self.nodes[0].disconnect_p2ps()
 
@@ -81,7 +80,7 @@ class SendTxRcnclTest(BitcoinTestFramework):
         peer.wait_for_verack()
         verack_index = [i for i, msg in enumerate(peer.messages) if msg.msgtype == b'verack'][0]
         sendtxrcncl_index = [i for i, msg in enumerate(peer.messages) if msg.msgtype == b'sendtxrcncl'][0]
-        assert(sendtxrcncl_index < verack_index)
+        assert sendtxrcncl_index < verack_index
         self.nodes[0].disconnect_p2ps()
 
         self.log.info('SENDTXRCNCL on pre-WTXID version should not be sent')
@@ -108,58 +107,25 @@ class SendTxRcnclTest(BitcoinTestFramework):
         assert not peer.sendtxrcncl_msg_received
         self.nodes[0].disconnect_p2ps()
 
-        self.log.info('valid SENDTXRCNCL received')
-        peer = self.nodes[0].add_p2p_connection(PeerNoVerack(), send_version=True, wait_for_verack=False)
-        peer.send_message(create_sendtxrcncl_msg())
-        self.wait_until(lambda : "sendtxrcncl" in self.nodes[0].getpeerinfo()[-1]["bytesrecv_per_msg"])
-        self.log.info('second SENDTXRCNCL triggers a disconnect')
-        with self.nodes[0].assert_debug_log(["(sendtxrcncl received from already registered peer); disconnecting"]):
-            peer.send_message(create_sendtxrcncl_msg())
-            peer.wait_for_disconnect()
-
-        self.log.info('SENDTXRCNCL with initiator=responder=0 triggers a disconnect')
-        sendtxrcncl_no_role = create_sendtxrcncl_msg()
-        sendtxrcncl_no_role.initiator = False
-        sendtxrcncl_no_role.responder = False
-        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_no_role)
-            peer.wait_for_disconnect()
-
-        self.log.info('SENDTXRCNCL with initiator=0 and responder=1 from inbound triggers a disconnect')
-        sendtxrcncl_wrong_role = create_sendtxrcncl_msg(initiator=False)
-        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_wrong_role)
-            peer.wait_for_disconnect()
-
-        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()
-
-        self.log.info('sending SENDTXRCNCL after sending VERACK triggers a disconnect')
-        peer = self.nodes[0].add_p2p_connection(P2PInterface())
-        with self.nodes[0].assert_debug_log(["sendtxrcncl received after verack"]):
-            peer.send_message(create_sendtxrcncl_msg())
-            peer.wait_for_disconnect()
-
-        self.log.info('SENDTXRCNCL without WTXIDRELAY is ignored (recon state is erased after VERACK)')
-        peer = self.nodes[0].add_p2p_connection(PeerNoVerack(wtxidrelay=False), send_version=True, wait_for_verack=False)
-        with self.nodes[0].assert_debug_log(['Forget txreconciliation state of peer']):
-            peer.send_message(create_sendtxrcncl_msg())
-            peer.send_message(msg_verack())
+        self.log.info('SENDTXRCNCL for fRelay=false should not be sent (with NODE_BLOOM offered)')
+        self.restart_node(0, ["-peerbloomfilters", "-txreconciliation"])
+        peer = self.nodes[0].add_p2p_connection(SendTxrcnclReceiver(), send_version=False, wait_for_verack=False)
+        no_txrelay_version_msg = msg_version()
+        no_txrelay_version_msg.nVersion = P2P_VERSION
+        no_txrelay_version_msg.strSubVer = P2P_SUBVERSION
+        no_txrelay_version_msg.nServices = P2P_SERVICES
+        no_txrelay_version_msg.relay = 0
+        peer.send_message(no_txrelay_version_msg)
+        peer.wait_for_verack()
+        assert peer.nServices & NODE_BLOOM != 0
+        assert not peer.sendtxrcncl_msg_received
         self.nodes[0].disconnect_p2ps()
 
+        # Now, *sending* to *outbound*.
         self.log.info('SENDTXRCNCL sent to an outbound')
         peer = self.nodes[0].add_outbound_p2p_connection(
             SendTxrcnclReceiver(), wait_for_verack=True, p2p_idx=0, connection_type="outbound-full-relay")
         assert peer.sendtxrcncl_msg_received
-        assert peer.sendtxrcncl_msg_received.initiator
-        assert not peer.sendtxrcncl_msg_received.responder
         assert_equal(peer.sendtxrcncl_msg_received.version, 1)
         self.nodes[0].disconnect_p2ps()
 
@@ -174,20 +140,11 @@ class SendTxRcnclTest(BitcoinTestFramework):
         assert not peer.sendtxrcncl_msg_received
         self.nodes[0].disconnect_p2ps()
 
-        self.log.info('SENDTXRCNCL if block-relay-only triggers a disconnect')
+        self.log.info("SENDTXRCNCL should not be sent if addrfetch")
         peer = self.nodes[0].add_outbound_p2p_connection(
-            PeerNoVerack(), wait_for_verack=False, p2p_idx=0, connection_type="block-relay-only")
-        with self.nodes[0].assert_debug_log(["we indicated no tx relay; disconnecting"]):
-            peer.send_message(create_sendtxrcncl_msg(initiator=False))
-            peer.wait_for_disconnect()
-
-        self.log.info('SENDTXRCNCL with initiator=1 and responder=0 from outbound triggers a disconnect')
-        sendtxrcncl_wrong_role = create_sendtxrcncl_msg(initiator=True)
-        peer = self.nodes[0].add_outbound_p2p_connection(
-            PeerNoVerack(), wait_for_verack=False, p2p_idx=0, connection_type="outbound-full-relay")
-        with self.nodes[0].assert_debug_log(["txreconciliation protocol violation"]):
-            peer.send_message(sendtxrcncl_wrong_role)
-            peer.wait_for_disconnect()
+            SendTxrcnclReceiver(), wait_for_verack=True, p2p_idx=0, connection_type="addr-fetch")
+        assert not peer.sendtxrcncl_msg_received
+        self.nodes[0].disconnect_p2ps()
 
         self.log.info('SENDTXRCNCL not sent if -txreconciliation flag is not set')
         self.restart_node(0, [])
@@ -201,6 +158,76 @@ class SendTxRcnclTest(BitcoinTestFramework):
         assert not peer.sendtxrcncl_msg_received
         self.nodes[0].disconnect_p2ps()
 
+        # Check everything concerning *receiving* SENDTXRCNCL
+        # First, receiving from *inbound*.
+        self.restart_node(0, ["-txreconciliation"])
+        self.log.info('valid SENDTXRCNCL received')
+        peer = self.nodes[0].add_p2p_connection(PeerNoVerack(), send_version=True, wait_for_verack=False)
+        with self.nodes[0].assert_debug_log(["received: sendtxrcncl"]):
+            peer.send_message(create_sendtxrcncl_msg())
+        self.log.info('second SENDTXRCNCL triggers a disconnect')
+        with self.nodes[0].assert_debug_log(["(sendtxrcncl received from already registered peer); disconnecting"]):
+            peer.send_message(create_sendtxrcncl_msg())
+            peer.wait_for_disconnect()
+
+        self.restart_node(0, [])
+        self.log.info('SENDTXRCNCL if no txreconciliation supported is ignored')
+        peer = self.nodes[0].add_p2p_connection(PeerNoVerack(), send_version=True, wait_for_verack=False)
+        with self.nodes[0].assert_debug_log(['ignored, as our node does not have txreconciliation enabled']):
+            peer.send_message(create_sendtxrcncl_msg())
+        self.nodes[0].disconnect_p2ps()
+
+        self.restart_node(0, ["-txreconciliation"])
+
+        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()
+
+        self.log.info('SENDTXRCNCL with version=2 is valid')
+        sendtxrcncl_higher_version = create_sendtxrcncl_msg()
+        sendtxrcncl_higher_version.version = 2
+        peer = self.nodes[0].add_p2p_connection(PeerNoVerack(), send_version=True, wait_for_verack=False)
+        with self.nodes[0].assert_debug_log(['Register peer=1']):
+            peer.send_message(sendtxrcncl_higher_version)
+        self.nodes[0].disconnect_p2ps()
+
+        self.log.info('unexpected SENDTXRCNCL is ignored')
+        peer = self.nodes[0].add_p2p_connection(PeerNoVerack(), send_version=False, wait_for_verack=False)
+        old_version_msg = msg_version()
+        old_version_msg.nVersion = 70015
+        old_version_msg.strSubVer = P2P_SUBVERSION
+        old_version_msg.nServices = P2P_SERVICES
+        old_version_msg.relay = 1
+        peer.send_message(old_version_msg)
+        with self.nodes[0].assert_debug_log(['Ignore unexpected txreconciliation signal']):
+            peer.send_message(create_sendtxrcncl_msg())
+        self.nodes[0].disconnect_p2ps()
+
+        self.log.info('sending SENDTXRCNCL after sending VERACK triggers a disconnect')
+        peer = self.nodes[0].add_p2p_connection(P2PInterface())
+        with self.nodes[0].assert_debug_log(["sendtxrcncl received after verack"]):
+            peer.send_message(create_sendtxrcncl_msg())
+            peer.wait_for_disconnect()
+
+        self.log.info('SENDTXRCNCL without WTXIDRELAY is ignored (recon state is erased after VERACK)')
+        peer = self.nodes[0].add_p2p_connection(PeerNoVerack(wtxidrelay=False), send_version=True, wait_for_verack=False)
+        with self.nodes[0].assert_debug_log(['Forget txreconciliation state of peer']):
+            peer.send_message(create_sendtxrcncl_msg())
+            peer.send_message(msg_verack())
+        self.nodes[0].disconnect_p2ps()
+
+        # Now, *receiving* from *outbound*.
+        self.log.info('SENDTXRCNCL if block-relay-only triggers a disconnect')
+        peer = self.nodes[0].add_outbound_p2p_connection(
+            PeerNoVerack(), wait_for_verack=False, p2p_idx=0, connection_type="block-relay-only")
+        with self.nodes[0].assert_debug_log(["we indicated no tx relay; disconnecting"]):
+            peer.send_message(create_sendtxrcncl_msg())
+            peer.wait_for_disconnect()
+
 
 if __name__ == '__main__':
     SendTxRcnclTest().main()

test/functional/test_framework/messages.py

@@ -1840,29 +1840,23 @@ class msg_cfcheckpt:
             self.filter_type, self.stop_hash)
 
 class msg_sendtxrcncl:
-    __slots__ = ("initiator", "responder", "version", "salt")
+    __slots__ = ("version", "salt")
     msgtype = b"sendtxrcncl"
 
     def __init__(self):
-        self.initiator = False
-        self.responder = False
         self.version = 0
         self.salt = 0
 
     def deserialize(self, f):
-        self.initiator = struct.unpack("<?", f.read(1))[0]
-        self.responder = struct.unpack("<?", f.read(1))[0]
         self.version = struct.unpack("<I", f.read(4))[0]
         self.salt = struct.unpack("<Q", f.read(8))[0]
 
     def serialize(self):
         r = b""
-        r += struct.pack("<?", self.initiator)
-        r += struct.pack("<?", self.responder)
         r += struct.pack("<I", self.version)
         r += struct.pack("<Q", self.salt)
         return r
 
     def __repr__(self):
-        return "msg_sendtxrcncl(initiator=%i, responder=%i, version=%lu, salt=%lu)" %\
-            (self.initiator, self.responder, self.version, self.salt)
+        return "msg_sendtxrcncl(version=%lu, salt=%lu)" %\
+            (self.version, self.salt)