p2p: Make short id collision detectable when adding wtxids to tx reconciliation sets

src/node/txreconciliation.cpp

@@ -142,17 +142,20 @@ public:
         return ReconciliationRegisterResult::SUCCESS;
     }
 
-    bool AddToSet(NodeId peer_id, const Wtxid& wtxid) EXCLUSIVE_LOCKS_REQUIRED(!m_txreconciliation_mutex)
+    AddToSetResult AddToSet(NodeId peer_id, const Wtxid& wtxid) EXCLUSIVE_LOCKS_REQUIRED(!m_txreconciliation_mutex)
     {
         AssertLockNotHeld(m_txreconciliation_mutex);
         LOCK(m_txreconciliation_mutex);
         auto peer_state = GetRegisteredPeerState(peer_id);
-        if (!peer_state) return false;
+        if (!peer_state) return AddToSetResult::Failed();
+
+        // TODO: We should compute the short_id here here first and see if there's any collision
+        // if so, return AddToSetResult::Collision(wtxid)
 
         // Transactions which don't make it to the set due to the limit are announced via fan-out.
         if (peer_state->m_local_set.size() >= MAX_RECONSET_SIZE) {
             LogPrintLevel(BCLog::TXRECONCILIATION, BCLog::Level::Debug, "Reconciliation set maximum size reached for peer=%d.\n", peer_id);
-            return false;
+            return AddToSetResult::Failed();
         }
 
         // The caller currently keeps track of the per-peer transaction announcements, so it
@@ -163,7 +166,7 @@ public:
                                                                         "Now the set contains %i transactions.\n",
                           wtxid.ToString(), peer_id, peer_state->m_local_set.size());
         }
-        return true;
+        return AddToSetResult::Succeeded();
     }
 
     bool TryRemovingFromSet(NodeId peer_id, const Wtxid& wtxid) EXCLUSIVE_LOCKS_REQUIRED(!m_txreconciliation_mutex)
@@ -209,6 +212,27 @@ public:
     }
 };
 
+AddToSetResult::AddToSetResult(bool succeeded, std::optional<Wtxid> collision)
+{
+    m_succeeded = succeeded;
+    m_collision = collision;
+}
+
+AddToSetResult AddToSetResult::Succeeded()
+{
+    return AddToSetResult(true, std::nullopt);
+}
+
+AddToSetResult AddToSetResult::Failed()
+{
+    return AddToSetResult(false, std::nullopt);
+}
+
+AddToSetResult AddToSetResult::Collision(Wtxid wtxid)
+{
+    return AddToSetResult(false, std::make_optional(wtxid));
+}
+
 TxReconciliationTracker::TxReconciliationTracker(uint32_t recon_version) : m_impl{std::make_unique<TxReconciliationTracker::Impl>(recon_version)} {}
 
 TxReconciliationTracker::~TxReconciliationTracker() = default;
@@ -224,7 +248,7 @@ ReconciliationRegisterResult TxReconciliationTracker::RegisterPeer(NodeId peer_i
     return m_impl->RegisterPeer(peer_id, is_peer_inbound, peer_recon_version, remote_salt);
 }
 
-bool TxReconciliationTracker::AddToSet(NodeId peer_id, const Wtxid& wtxid)
+AddToSetResult TxReconciliationTracker::AddToSet(NodeId peer_id, const Wtxid& wtxid)
 {
     return m_impl->AddToSet(peer_id, wtxid);
 }

src/node/txreconciliation.h

@@ -10,6 +10,7 @@
 
 #include <memory>
 #include <tuple>
+#include <optional>
 
 /** Supported transaction reconciliation protocol version */
 static constexpr uint32_t TXRECONCILIATION_VERSION{1};
@@ -27,6 +28,23 @@ enum class ReconciliationRegisterResult {
     PROTOCOL_VIOLATION,
 };
 
+/**
+ * Record whether or not a wtxid was successfully added to a reconciliation set.
+ * In case of failure, check whether this was due to a shortid collision and record
+ * the colliding wtxid.
+*/
+class AddToSetResult
+{
+    public:
+        bool m_succeeded;
+        std::optional<Wtxid> m_collision;
+
+        explicit AddToSetResult(bool added, std::optional<Wtxid> conflict);
+        static AddToSetResult Succeeded();
+        static AddToSetResult Failed();
+        static AddToSetResult Collision(Wtxid);
+};
+
 /**
  * Transaction reconciliation is a way for nodes to efficiently announce transactions.
  * This object keeps track of all txreconciliation-related communications with the peers.
@@ -85,7 +103,7 @@ public:
      * of the peer, so that it will be reconciled later, unless the set limit is reached.
      * Returns whether the transaction appears in the set.
      */
-    bool AddToSet(NodeId peer_id, const Wtxid& wtxid);
+    AddToSetResult AddToSet(NodeId peer_id, const Wtxid& wtxid);
 
     /**
      * Before Step 2, we might want to remove a wtxid from the reconciliation set, for example if

src/test/txreconciliation_tests.cpp

@@ -89,27 +89,43 @@ BOOST_AUTO_TEST_CASE(AddToSetTest)
 
     Wtxid wtxid{Wtxid::FromUint256(frc.rand256())};
 
+    // If the peer is not registered, adding to the set fails
     BOOST_REQUIRE(!tracker.IsPeerRegistered(peer_id0));
-    BOOST_REQUIRE(!tracker.AddToSet(peer_id0, wtxid));
+    auto r = tracker.AddToSet(peer_id0, wtxid);
+    BOOST_REQUIRE(!r.m_succeeded);
+    BOOST_REQUIRE(!r.m_collision.has_value());
 
+    // As long as the peer is registered, adding a new wtxid to the set should work
     tracker.PreRegisterPeer(peer_id0);
     BOOST_REQUIRE_EQUAL(tracker.RegisterPeer(peer_id0, true, 1, 1), ReconciliationRegisterResult::SUCCESS);
     BOOST_CHECK(tracker.IsPeerRegistered(peer_id0));
 
-    BOOST_REQUIRE(tracker.AddToSet(peer_id0, wtxid));
+    r = tracker.AddToSet(peer_id0, wtxid);
+    BOOST_REQUIRE(r.m_succeeded);
+    BOOST_REQUIRE(!r.m_collision.has_value());
 
+    // If the peer is dropped, adding wtxids to its set should fail
     tracker.ForgetPeer(peer_id0);
     Wtxid wtxid2{Wtxid::FromUint256(frc.rand256())};
-    BOOST_REQUIRE(!tracker.AddToSet(peer_id0, wtxid2));
+    r = tracker.AddToSet(peer_id0, wtxid2);
+    BOOST_REQUIRE(!r.m_succeeded);
+    BOOST_REQUIRE(!r.m_collision.has_value());
 
     NodeId peer_id1 = 1;
     tracker.PreRegisterPeer(peer_id1);
     BOOST_REQUIRE_EQUAL(tracker.RegisterPeer(peer_id1, true, 1, 1), ReconciliationRegisterResult::SUCCESS);
     BOOST_CHECK(tracker.IsPeerRegistered(peer_id1));
 
+    // As long as the peer is registered and the transaction is not in the set, adding it should succeed
     for (size_t i = 0; i < MAX_RECONSET_SIZE; ++i)
-        BOOST_REQUIRE(tracker.AddToSet(peer_id1, Wtxid::FromUint256(frc.rand256())));
+        r = tracker.AddToSet(peer_id1, Wtxid::FromUint256(frc.rand256()));
-    BOOST_REQUIRE(!tracker.AddToSet(peer_id1, Wtxid::FromUint256(frc.rand256())));
+        BOOST_REQUIRE(r.m_succeeded);
+        BOOST_REQUIRE(!r.m_collision.has_value());
+
+    // Trying to add the same item twice should fail
+    r = tracker.AddToSet(peer_id1, Wtxid::FromUint256(frc.rand256()));
+    BOOST_REQUIRE(!r.m_succeeded);
+    BOOST_REQUIRE(!r.m_collision.has_value());
 }
 
 BOOST_AUTO_TEST_CASE(TryRemovingFromSetTest)
@@ -128,11 +144,11 @@ BOOST_AUTO_TEST_CASE(TryRemovingFromSetTest)
     BOOST_CHECK(tracker.IsPeerRegistered(peer_id0));
 
     BOOST_REQUIRE(!tracker.TryRemovingFromSet(peer_id0, wtxid));
-    BOOST_REQUIRE(tracker.AddToSet(peer_id0, wtxid));
+    BOOST_REQUIRE(tracker.AddToSet(peer_id0, wtxid).m_succeeded);
     BOOST_REQUIRE(tracker.TryRemovingFromSet(peer_id0, wtxid));
     BOOST_REQUIRE(!tracker.TryRemovingFromSet(peer_id0, wtxid));
 
-    BOOST_REQUIRE(tracker.AddToSet(peer_id0, wtxid));
+    BOOST_REQUIRE(tracker.AddToSet(peer_id0, wtxid).m_succeeded);
     tracker.ForgetPeer(peer_id0);
     BOOST_REQUIRE(!tracker.TryRemovingFromSet(peer_id0, wtxid));
 }