From a60f729e293dcd11ca077b7c1c72b06119437faa Mon Sep 17 00:00:00 2001 From: Gleb Naumenko Date: Thu, 20 Oct 2022 16:38:50 +0300 Subject: p2p: Drop roles from sendtxrcncl This feature was currently redundant (although could have provided more flexibility in the future), and already been causing confusion. --- src/node/txreconciliation.cpp | 31 ++++++++----------------------- src/node/txreconciliation.h | 4 ++-- 2 files changed, 10 insertions(+), 25 deletions(-) (limited to 'src/node') 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). -- cgit v1.2.3 From bc84e24a4f0736919ea4a76f7d45085587625aba Mon Sep 17 00:00:00 2001 From: Gleb Naumenko Date: Mon, 14 Nov 2022 11:37:28 +0200 Subject: p2p, refactor: Switch to enum class for ReconciliationRegisterResult While doing this, add a new value: ALREADY_REGISTERED. --- src/node/txreconciliation.cpp | 18 ++++++++++-------- src/node/txreconciliation.h | 9 +++++---- 2 files changed, 15 insertions(+), 12 deletions(-) (limited to 'src/node') diff --git a/src/node/txreconciliation.cpp b/src/node/txreconciliation.cpp index 3552cfd8f2..7b50b41dc7 100644 --- a/src/node/txreconciliation.cpp +++ b/src/node/txreconciliation.cpp @@ -101,11 +101,13 @@ public: 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(&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(recon_state->second)) { + return ReconciliationRegisterResult::ALREADY_REGISTERED; + } + + uint64_t local_salt = *std::get_if(&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,14 +116,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; 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)}; + 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 SUCCESS; + return ReconciliationRegisterResult::SUCCESS; } void ForgetPeer(NodeId peer_id) EXCLUSIVE_LOCKS_REQUIRED(!m_txreconciliation_mutex) diff --git a/src/node/txreconciliation.h b/src/node/txreconciliation.h index caaf1777e9..4591dd5df7 100644 --- a/src/node/txreconciliation.h +++ b/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, }; /** -- cgit v1.2.3 From 87493e112ee91923adf38b75491bedeb45f87c80 Mon Sep 17 00:00:00 2001 From: Gleb Naumenko Date: Mon, 14 Nov 2022 11:45:52 +0200 Subject: p2p, test, refactor: Minor code improvements --- src/node/txreconciliation.cpp | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) (limited to 'src/node') diff --git a/src/node/txreconciliation.cpp b/src/node/txreconciliation.cpp index 7b50b41dc7..ed04a78cec 100644 --- a/src/node/txreconciliation.cpp +++ b/src/node/txreconciliation.cpp @@ -82,15 +82,13 @@ 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; } -- cgit v1.2.3