p2p, refactor: Switch to enum class for ReconciliationRegisterResult

While doing this, add a new value: ALREADY_REGISTERED.

Author: naumenkogs

src/net_processing.cpp

@@ -3510,24 +3510,21 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
             return;
         }
 
-        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(),
                                                                                      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,
-        // nothing to be done.
-        if (result == ReconciliationRegisterResult::PROTOCOL_VIOLATION) {
+        switch (result) {
+        case ReconciliationRegisterResult::NOT_FOUND:
+        case ReconciliationRegisterResult::SUCCESS:
+            break;
+        case ReconciliationRegisterResult::ALREADY_REGISTERED:
+            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;
+        case ReconciliationRegisterResult::PROTOCOL_VIOLATION:
             LogPrintLevel(BCLog::NET, BCLog::Level::Debug, "txreconciliation protocol violation from peer=%d; disconnecting\n", pfrom.GetId());
             pfrom.fDisconnect = true;
             return;

src/node/txreconciliation.cpp

@@ -101,11 +101,13 @@ class TxReconciliationTracker::Impl
         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<uint64_t>(&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<TxReconciliationState>(recon_state->second)) {
+            return ReconciliationRegisterResult::ALREADY_REGISTERED;
+        }
+
+        uint64_t local_salt = *std::get_if<uint64_t>(&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 @@ class TxReconciliationTracker::Impl
         // 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)

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,
 };
 
 /**

src/test/txreconciliation_tests.cpp

@@ -33,6 +33,9 @@ BOOST_AUTO_TEST_CASE(RegisterPeerTest)
     BOOST_REQUIRE(tracker.RegisterPeer(1, true, 2, salt) == ReconciliationRegisterResult::SUCCESS);
     BOOST_CHECK(tracker.IsPeerRegistered(1));
 
+    // Try registering for the second time.
+    BOOST_REQUIRE(tracker.RegisterPeer(1, false, 1, salt) == ReconciliationRegisterResult::ALREADY_REGISTERED);
+
     // Do not register if there were no pre-registration for the peer.
     BOOST_REQUIRE(tracker.RegisterPeer(100, true, 1, salt) == ReconciliationRegisterResult::NOT_FOUND);
     BOOST_CHECK(!tracker.IsPeerRegistered(100));