diff --git a/src/node/txreconciliation.cpp b/src/node/txreconciliation.cpp index 30db634b77e..cf7d7668e15 100644 --- a/src/node/txreconciliation.cpp +++ b/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 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(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); } diff --git a/src/node/txreconciliation.h b/src/node/txreconciliation.h index ee3302d0cff..c0a88d4fef2 100644 --- a/src/node/txreconciliation.h +++ b/src/node/txreconciliation.h @@ -10,6 +10,7 @@ #include #include +#include /** 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 m_collision; + + explicit AddToSetResult(bool added, std::optional 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 diff --git a/src/test/txreconciliation_tests.cpp b/src/test/txreconciliation_tests.cpp index 1bb9297f378..0f46762f596 100644 --- a/src/test/txreconciliation_tests.cpp +++ b/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()))); - BOOST_REQUIRE(!tracker.AddToSet(peer_id1, Wtxid::FromUint256(frc.rand256()))); + r = 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)); }