p2p: Drop roles from sendtxrcncl

This feature was currently redundant (although could have provided
more flexibility in the future), and already been causing confusion.

Author: naumenkogs

src/net_processing.cpp

@@ -3279,11 +3279,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
             // - we are not in -blocksonly mode.
             if (pfrom.m_relays_txs && !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));
             }
         }
@@ -3514,22 +3510,19 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
             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());
             pfrom.fDisconnect = true;
             return;
         }
 
+        uint32_t peer_txreconcl_version;
+        uint64_t remote_salt;
+        vRecv >> peer_txreconcl_version >> remote_salt;
+
         const ReconciliationRegisterResult result = m_txreconciliation->RegisterPeer(pfrom.GetId(), pfrom.IsInboundConn(),
-                                                                                is_peer_initiator, is_peer_responder,
-                                                                                peer_txreconcl_version,
-                                                                                remote_salt);
+                                                                                     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,

src/node/txreconciliation.cpp

@@ -39,7 +39,8 @@ class TxReconciliationState
      * 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;
@@ -93,9 +94,8 @@ class TxReconciliationTracker::Impl
         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);
@@ -116,24 +116,11 @@ class TxReconciliationTracker::Impl
         // v1 is the lowest version, so suggesting something below must be a protocol violation.
         if (recon_version < 1) return 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;
-
-        // 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);
+        LogPrintLevel(BCLog::TXRECONCILIATION, BCLog::Level::Debug, "Register peer=%d (inbound=%i)\n",
+                      peer_id, is_peer_inbound);
 
         const uint256 full_salt{ComputeSalt(*local_salt, remote_salt)};
-        recon_state->second = TxReconciliationState(we_initiate, full_salt.GetUint64(0), full_salt.GetUint64(1));
+        recon_state->second = TxReconciliationState(!is_peer_inbound, full_salt.GetUint64(0), full_salt.GetUint64(1));
         return SUCCESS;
     }
 
@@ -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

@@ -72,8 +72,8 @@ class TxReconciliationTracker
      * 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

@@ -18,33 +18,23 @@ BOOST_AUTO_TEST_CASE(RegisterPeerTest)
     // 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(tracker.RegisterPeer(/*peer_id=*/0, /*is_peer_inbound=*/true,
+                                     /*peer_recon_version=*/0, salt) == ReconciliationRegisterResult::PROTOCOL_VIOLATION);
 
     // Valid registration.
     BOOST_REQUIRE(!tracker.IsPeerRegistered(0));
-    BOOST_REQUIRE(tracker.RegisterPeer(0, true, true, false, 1, salt) == ReconciliationRegisterResult::SUCCESS);
+    BOOST_REQUIRE(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, true, 2, salt) == ReconciliationRegisterResult::SUCCESS);
     BOOST_CHECK(tracker.IsPeerRegistered(1));
 
     // 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(tracker.RegisterPeer(100, true, 1, salt) == ReconciliationRegisterResult::NOT_FOUND);
     BOOST_CHECK(!tracker.IsPeerRegistered(100));
 }
 
@@ -56,12 +46,12 @@ BOOST_AUTO_TEST_CASE(ForgetPeerTest)
     // 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(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(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));
@@ -76,7 +66,7 @@ BOOST_AUTO_TEST_CASE(IsPeerRegisteredTest)
     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(tracker.RegisterPeer(peer_id0, true, 1, 1) == ReconciliationRegisterResult::SUCCESS);
     BOOST_CHECK(tracker.IsPeerRegistered(peer_id0));
 
     tracker.ForgetPeer(peer_id0);

test/functional/p2p_sendtxrcncl.py

@@ -54,10 +54,8 @@ def on_message(self, message):
         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
@@ -71,8 +69,6 @@ def run_test(self):
         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()
 
@@ -117,22 +113,6 @@ def run_test(self):
             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
@@ -158,8 +138,6 @@ def run_test(self):
         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()
 
@@ -178,15 +156,7 @@ def run_test(self):
         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.send_message(create_sendtxrcncl_msg())
             peer.wait_for_disconnect()
 
         self.log.info('SENDTXRCNCL not sent if -txreconciliation flag is not set')

test/functional/test_framework/messages.py

@@ -1840,29 +1840,23 @@ def __repr__(self):
             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)