p2p: Drop roles from sendtxrcncl

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

5 files changed, 24 insertions, 58 deletions

diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 6aaacd5068..71bf48798d 100644
--- a/src/net_processing.cpp
+++ b/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,11 +3510,6 @@ 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());
@@ -3526,10 +3517,12 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
             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,
diff --git a/src/node/txreconciliation.cpp b/src/node/txreconciliation.cpp
index 974358fcda..3552cfd8f2 100644
--- a/src/node/txreconciliation.cpp
+++ b/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;
@@ -93,9 +94,8 @@ public:
         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 @@ public:
         // 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)
diff --git a/src/node/txreconciliation.h b/src/node/txreconciliation.h
index a4f0870914..caaf1777e9 100644
--- a/src/node/txreconciliation.h
+++ b/src/node/txreconciliation.h
@@ -72,8 +72,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).
diff --git a/src/protocol.h b/src/protocol.h
index 17a363b1d3..51fabf8da0 100644
--- a/src/protocol.h
+++ b/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.
  */
diff --git a/src/test/txreconciliation_tests.cpp b/src/test/txreconciliation_tests.cpp
index bd74998002..6b72e5d0f0 100644
--- a/src/test/txreconciliation_tests.cpp
+++ b/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);