From 6772cbf69cf075ac8dff3507bf9151400ed255b7 Mon Sep 17 00:00:00 2001 From: Gleb Naumenko Date: Tue, 8 Nov 2022 12:18:40 +0200 Subject: [PATCH 1/8] tests: stabilize sendtxrcncl test --- test/functional/p2p_sendtxrcncl.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/functional/p2p_sendtxrcncl.py b/test/functional/p2p_sendtxrcncl.py index fed9832a7d9..cef3f092165 100755 --- a/test/functional/p2p_sendtxrcncl.py +++ b/test/functional/p2p_sendtxrcncl.py @@ -110,8 +110,8 @@ class SendTxRcnclTest(BitcoinTestFramework): self.log.info('valid SENDTXRCNCL received') peer = self.nodes[0].add_p2p_connection(PeerNoVerack(), send_version=True, wait_for_verack=False) - peer.send_message(create_sendtxrcncl_msg()) - self.wait_until(lambda : "sendtxrcncl" in self.nodes[0].getpeerinfo()[-1]["bytesrecv_per_msg"]) + with self.nodes[0].assert_debug_log(["received: sendtxrcncl"]): + peer.send_message(create_sendtxrcncl_msg()) self.log.info('second SENDTXRCNCL triggers a disconnect') with self.nodes[0].assert_debug_log(["(sendtxrcncl received from already registered peer); disconnecting"]): peer.send_message(create_sendtxrcncl_msg()) From a60f729e293dcd11ca077b7c1c72b06119437faa Mon Sep 17 00:00:00 2001 From: Gleb Naumenko Date: Thu, 20 Oct 2022 16:38:50 +0300 Subject: [PATCH 2/8] 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/net_processing.cpp | 17 ++++------- src/node/txreconciliation.cpp | 31 +++++--------------- src/node/txreconciliation.h | 4 +-- src/protocol.h | 4 +-- src/test/txreconciliation_tests.cpp | 26 +++++------------ test/functional/p2p_sendtxrcncl.py | 34 ++-------------------- test/functional/test_framework/messages.py | 12 ++------ 7 files changed, 29 insertions(+), 99 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 6aaacd50688..71bf48798d9 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 974358fcda0..3552cfd8f29 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 a4f08709148..caaf1777e9b 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 17a363b1d32..51fabf8da0c 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 bd74998002c..6b72e5d0f04 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); diff --git a/test/functional/p2p_sendtxrcncl.py b/test/functional/p2p_sendtxrcncl.py index cef3f092165..dbe6f681d11 100755 --- a/test/functional/p2p_sendtxrcncl.py +++ b/test/functional/p2p_sendtxrcncl.py @@ -54,10 +54,8 @@ class PeerTrackMsgOrder(P2PInterface): super().on_message(message) self.messages.append(message) -def create_sendtxrcncl_msg(initiator=True): +def create_sendtxrcncl_msg(): sendtxrcncl_msg = msg_sendtxrcncl() - sendtxrcncl_msg.initiator = initiator - sendtxrcncl_msg.responder = not initiator sendtxrcncl_msg.version = 1 sendtxrcncl_msg.salt = 2 return sendtxrcncl_msg @@ -71,8 +69,6 @@ class SendTxRcnclTest(BitcoinTestFramework): self.log.info('SENDTXRCNCL sent to an inbound') peer = self.nodes[0].add_p2p_connection(SendTxrcnclReceiver(), send_version=True, wait_for_verack=True) assert peer.sendtxrcncl_msg_received - assert not peer.sendtxrcncl_msg_received.initiator - assert peer.sendtxrcncl_msg_received.responder assert_equal(peer.sendtxrcncl_msg_received.version, 1) self.nodes[0].disconnect_p2ps() @@ -117,22 +113,6 @@ class SendTxRcnclTest(BitcoinTestFramework): peer.send_message(create_sendtxrcncl_msg()) peer.wait_for_disconnect() - self.log.info('SENDTXRCNCL with initiator=responder=0 triggers a disconnect') - sendtxrcncl_no_role = create_sendtxrcncl_msg() - sendtxrcncl_no_role.initiator = False - sendtxrcncl_no_role.responder = False - peer = self.nodes[0].add_p2p_connection(PeerNoVerack(), send_version=True, wait_for_verack=False) - with self.nodes[0].assert_debug_log(["txreconciliation protocol violation"]): - peer.send_message(sendtxrcncl_no_role) - peer.wait_for_disconnect() - - self.log.info('SENDTXRCNCL with initiator=0 and responder=1 from inbound triggers a disconnect') - sendtxrcncl_wrong_role = create_sendtxrcncl_msg(initiator=False) - peer = self.nodes[0].add_p2p_connection(PeerNoVerack(), send_version=True, wait_for_verack=False) - with self.nodes[0].assert_debug_log(["txreconciliation protocol violation"]): - peer.send_message(sendtxrcncl_wrong_role) - peer.wait_for_disconnect() - self.log.info('SENDTXRCNCL with version=0 triggers a disconnect') sendtxrcncl_low_version = create_sendtxrcncl_msg() sendtxrcncl_low_version.version = 0 @@ -158,8 +138,6 @@ class SendTxRcnclTest(BitcoinTestFramework): peer = self.nodes[0].add_outbound_p2p_connection( SendTxrcnclReceiver(), wait_for_verack=True, p2p_idx=0, connection_type="outbound-full-relay") assert peer.sendtxrcncl_msg_received - assert peer.sendtxrcncl_msg_received.initiator - assert not peer.sendtxrcncl_msg_received.responder assert_equal(peer.sendtxrcncl_msg_received.version, 1) self.nodes[0].disconnect_p2ps() @@ -178,15 +156,7 @@ class SendTxRcnclTest(BitcoinTestFramework): peer = self.nodes[0].add_outbound_p2p_connection( PeerNoVerack(), wait_for_verack=False, p2p_idx=0, connection_type="block-relay-only") with self.nodes[0].assert_debug_log(["we indicated no tx relay; disconnecting"]): - peer.send_message(create_sendtxrcncl_msg(initiator=False)) - peer.wait_for_disconnect() - - self.log.info('SENDTXRCNCL with initiator=1 and responder=0 from outbound triggers a disconnect') - sendtxrcncl_wrong_role = create_sendtxrcncl_msg(initiator=True) - peer = self.nodes[0].add_outbound_p2p_connection( - PeerNoVerack(), wait_for_verack=False, p2p_idx=0, connection_type="outbound-full-relay") - with self.nodes[0].assert_debug_log(["txreconciliation protocol violation"]): - peer.send_message(sendtxrcncl_wrong_role) + peer.send_message(create_sendtxrcncl_msg()) peer.wait_for_disconnect() self.log.info('SENDTXRCNCL not sent if -txreconciliation flag is not set') diff --git a/test/functional/test_framework/messages.py b/test/functional/test_framework/messages.py index 252b49cc6d6..7389b1624e2 100755 --- a/test/functional/test_framework/messages.py +++ b/test/functional/test_framework/messages.py @@ -1840,29 +1840,23 @@ class msg_cfcheckpt: self.filter_type, self.stop_hash) class msg_sendtxrcncl: - __slots__ = ("initiator", "responder", "version", "salt") + __slots__ = ("version", "salt") msgtype = b"sendtxrcncl" def __init__(self): - self.initiator = False - self.responder = False self.version = 0 self.salt = 0 def deserialize(self, f): - self.initiator = struct.unpack(" Date: Mon, 14 Nov 2022 11:37:28 +0200 Subject: [PATCH 3/8] p2p, refactor: Switch to enum class for ReconciliationRegisterResult While doing this, add a new value: ALREADY_REGISTERED. --- src/net_processing.cpp | 21 +++++++++------------ src/node/txreconciliation.cpp | 18 ++++++++++-------- src/node/txreconciliation.h | 9 +++++---- src/test/txreconciliation_tests.cpp | 3 +++ 4 files changed, 27 insertions(+), 24 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 71bf48798d9..e90951ae1fc 100644 --- a/src/net_processing.cpp +++ b/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; diff --git a/src/node/txreconciliation.cpp b/src/node/txreconciliation.cpp index 3552cfd8f29..7b50b41dc74 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 caaf1777e9b..4591dd5df7c 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, }; /** diff --git a/src/test/txreconciliation_tests.cpp b/src/test/txreconciliation_tests.cpp index 6b72e5d0f04..1d6d4840c1d 100644 --- a/src/test/txreconciliation_tests.cpp +++ b/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)); From ac6ee5ba211d05869800497d6b518ea1ddd2c718 Mon Sep 17 00:00:00 2001 From: Gleb Naumenko Date: Mon, 14 Nov 2022 11:11:52 +0200 Subject: [PATCH 4/8] test: Expand unit and functional tests for txreconciliation --- src/test/txreconciliation_tests.cpp | 12 ++++++++---- test/functional/p2p_sendtxrcncl.py | 10 +++++++++- 2 files changed, 17 insertions(+), 5 deletions(-) diff --git a/src/test/txreconciliation_tests.cpp b/src/test/txreconciliation_tests.cpp index 1d6d4840c1d..49317d12dc5 100644 --- a/src/test/txreconciliation_tests.cpp +++ b/src/test/txreconciliation_tests.cpp @@ -22,17 +22,21 @@ BOOST_AUTO_TEST_CASE(RegisterPeerTest) BOOST_CHECK(tracker.RegisterPeer(/*peer_id=*/0, /*is_peer_inbound=*/true, /*peer_recon_version=*/0, salt) == ReconciliationRegisterResult::PROTOCOL_VIOLATION); - // Valid registration. + // Valid registration (inbound and outbound peers). BOOST_REQUIRE(!tracker.IsPeerRegistered(0)); 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, 2, salt) == ReconciliationRegisterResult::SUCCESS); + BOOST_REQUIRE(tracker.RegisterPeer(1, false, 1, salt) == ReconciliationRegisterResult::SUCCESS); BOOST_CHECK(tracker.IsPeerRegistered(1)); + // Reconciliation version is higher than ours, should be able to register. + BOOST_REQUIRE(!tracker.IsPeerRegistered(2)); + tracker.PreRegisterPeer(2); + BOOST_REQUIRE(tracker.RegisterPeer(2, true, 2, salt) == ReconciliationRegisterResult::SUCCESS); + BOOST_CHECK(tracker.IsPeerRegistered(2)); + // Try registering for the second time. BOOST_REQUIRE(tracker.RegisterPeer(1, false, 1, salt) == ReconciliationRegisterResult::ALREADY_REGISTERED); diff --git a/test/functional/p2p_sendtxrcncl.py b/test/functional/p2p_sendtxrcncl.py index dbe6f681d11..8f9e029da40 100755 --- a/test/functional/p2p_sendtxrcncl.py +++ b/test/functional/p2p_sendtxrcncl.py @@ -77,7 +77,7 @@ class SendTxRcnclTest(BitcoinTestFramework): peer.wait_for_verack() verack_index = [i for i, msg in enumerate(peer.messages) if msg.msgtype == b'verack'][0] sendtxrcncl_index = [i for i, msg in enumerate(peer.messages) if msg.msgtype == b'sendtxrcncl'][0] - assert(sendtxrcncl_index < verack_index) + assert sendtxrcncl_index < verack_index self.nodes[0].disconnect_p2ps() self.log.info('SENDTXRCNCL on pre-WTXID version should not be sent') @@ -121,6 +121,14 @@ class SendTxRcnclTest(BitcoinTestFramework): peer.send_message(sendtxrcncl_low_version) peer.wait_for_disconnect() + self.log.info('SENDTXRCNCL with version=2 is valid') + sendtxrcncl_higher_version = create_sendtxrcncl_msg() + sendtxrcncl_higher_version.version = 2 + peer = self.nodes[0].add_p2p_connection(PeerNoVerack(), send_version=True, wait_for_verack=False) + with self.nodes[0].assert_debug_log(['Register peer=6']): + peer.send_message(sendtxrcncl_higher_version) + self.nodes[0].disconnect_p2ps() + self.log.info('sending SENDTXRCNCL after sending VERACK triggers a disconnect') peer = self.nodes[0].add_p2p_connection(P2PInterface()) with self.nodes[0].assert_debug_log(["sendtxrcncl received after verack"]): From 00c5dec818f60e8297d42b49a919aa82c42821b5 Mon Sep 17 00:00:00 2001 From: Gleb Naumenko Date: Mon, 14 Nov 2022 11:18:31 +0200 Subject: [PATCH 5/8] p2p: Clarify sendtxrcncl policies --- src/net_processing.cpp | 24 ++++++++++++++------- test/functional/p2p_sendtxrcncl.py | 34 +++++++++++++++++++++++++++++- 2 files changed, 49 insertions(+), 9 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index e90951ae1fc..33c0fe433ae 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -3273,11 +3273,12 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, if (greatest_common_version >= WTXID_RELAY_VERSION && m_txreconciliation) { // Per BIP-330, we announce txreconciliation support if: - // - protocol version per the VERSION message supports WTXID_RELAY; - // - we intended to exchange transactions over this connection while establishing it - // and the peer indicated support for transaction relay in the VERSION message; + // - protocol version per the peer's VERSION message supports WTXID_RELAY; + // - transaction relay is supported per the peer's VERSION message (see m_relays_txs); + // - this is not a block-relay-only connection and not a feeler (see m_relays_txs); + // - this is not an addr fetch connection; // - we are not in -blocksonly mode. - if (pfrom.m_relays_txs && !m_ignore_incoming_txs) { + if (pfrom.m_relays_txs && !pfrom.IsAddrFetchConn() && !m_ignore_incoming_txs) { const uint64_t recon_salt = m_txreconciliation->PreRegisterPeer(pfrom.GetId()); m_connman.PushMessage(&pfrom, msg_maker.Make(NetMsgType::SENDTXRCNCL, TXRECONCILIATION_VERSION, recon_salt)); @@ -3496,20 +3497,27 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, } if (pfrom.fSuccessfullyConnected) { - // Disconnect peers that send a SENDTXRCNCL message after VERACK. LogPrintLevel(BCLog::NET, BCLog::Level::Debug, "sendtxrcncl received after verack from peer=%d; disconnecting\n", pfrom.GetId()); pfrom.fDisconnect = true; return; } - if (!peer->GetTxRelay()) { - // Disconnect peers that send a SENDTXRCNCL message even though we indicated we don't - // support transaction relay. + // Peer must not offer us reconciliations if we specified no tx relay support in VERSION. + if (RejectIncomingTxs(pfrom)) { LogPrintLevel(BCLog::NET, BCLog::Level::Debug, "sendtxrcncl received from peer=%d to which we indicated no tx relay; disconnecting\n", pfrom.GetId()); pfrom.fDisconnect = true; return; } + // Peer must not offer us reconciliations if they specified no tx relay support in VERSION. + // This flag might also be false in other cases, but the RejectIncomingTxs check above + // eliminates them, so that this flag fully represents what we are looking for. + if (!pfrom.m_relays_txs) { + LogPrintLevel(BCLog::NET, BCLog::Level::Debug, "sendtxrcncl received from peer=%d which indicated no tx relay to us; disconnecting\n", pfrom.GetId()); + pfrom.fDisconnect = true; + return; + } + uint32_t peer_txreconcl_version; uint64_t remote_salt; vRecv >> peer_txreconcl_version >> remote_salt; diff --git a/test/functional/p2p_sendtxrcncl.py b/test/functional/p2p_sendtxrcncl.py index 8f9e029da40..fe3126ca1e1 100755 --- a/test/functional/p2p_sendtxrcncl.py +++ b/test/functional/p2p_sendtxrcncl.py @@ -10,6 +10,7 @@ from test_framework.messages import ( msg_verack, msg_version, msg_wtxidrelay, + NODE_BLOOM, ) from test_framework.p2p import ( P2PInterface, @@ -104,6 +105,22 @@ class SendTxRcnclTest(BitcoinTestFramework): assert not peer.sendtxrcncl_msg_received self.nodes[0].disconnect_p2ps() + self.log.info('SENDTXRCNCL for fRelay=false should not be sent (with NODE_BLOOM offered)') + self.restart_node(0, ["-peerbloomfilters", "-txreconciliation"]) + peer = self.nodes[0].add_p2p_connection(SendTxrcnclReceiver(), send_version=False, wait_for_verack=False) + no_txrelay_version_msg = msg_version() + no_txrelay_version_msg.nVersion = P2P_VERSION + no_txrelay_version_msg.strSubVer = P2P_SUBVERSION + no_txrelay_version_msg.nServices = P2P_SERVICES + no_txrelay_version_msg.relay = 0 + peer.send_message(no_txrelay_version_msg) + peer.wait_for_verack() + assert peer.nServices & NODE_BLOOM != 0 + assert not peer.sendtxrcncl_msg_received + self.nodes[0].disconnect_p2ps() + + self.restart_node(0, ["-txreconciliation"]) + self.log.info('valid SENDTXRCNCL received') peer = self.nodes[0].add_p2p_connection(PeerNoVerack(), send_version=True, wait_for_verack=False) with self.nodes[0].assert_debug_log(["received: sendtxrcncl"]): @@ -113,6 +130,15 @@ class SendTxRcnclTest(BitcoinTestFramework): peer.send_message(create_sendtxrcncl_msg()) peer.wait_for_disconnect() + self.restart_node(0, []) + self.log.info('SENDTXRCNCL if no txreconciliation supported is ignored') + peer = self.nodes[0].add_p2p_connection(PeerNoVerack(), send_version=True, wait_for_verack=False) + with self.nodes[0].assert_debug_log(['ignored, as our node does not have txreconciliation enabled']): + peer.send_message(create_sendtxrcncl_msg()) + self.nodes[0].disconnect_p2ps() + + self.restart_node(0, ["-txreconciliation"]) + self.log.info('SENDTXRCNCL with version=0 triggers a disconnect') sendtxrcncl_low_version = create_sendtxrcncl_msg() sendtxrcncl_low_version.version = 0 @@ -125,7 +151,7 @@ class SendTxRcnclTest(BitcoinTestFramework): sendtxrcncl_higher_version = create_sendtxrcncl_msg() sendtxrcncl_higher_version.version = 2 peer = self.nodes[0].add_p2p_connection(PeerNoVerack(), send_version=True, wait_for_verack=False) - with self.nodes[0].assert_debug_log(['Register peer=6']): + with self.nodes[0].assert_debug_log(['Register peer=1']): peer.send_message(sendtxrcncl_higher_version) self.nodes[0].disconnect_p2ps() @@ -160,6 +186,12 @@ class SendTxRcnclTest(BitcoinTestFramework): assert not peer.sendtxrcncl_msg_received self.nodes[0].disconnect_p2ps() + self.log.info("SENDTXRCNCL should not be sent if addrfetch") + peer = self.nodes[0].add_outbound_p2p_connection( + SendTxrcnclReceiver(), wait_for_verack=True, p2p_idx=0, connection_type="addr-fetch") + assert not peer.sendtxrcncl_msg_received + self.nodes[0].disconnect_p2ps() + self.log.info('SENDTXRCNCL if block-relay-only triggers a disconnect') peer = self.nodes[0].add_outbound_p2p_connection( PeerNoVerack(), wait_for_verack=False, p2p_idx=0, connection_type="block-relay-only") From 87493e112ee91923adf38b75491bedeb45f87c80 Mon Sep 17 00:00:00 2001 From: Gleb Naumenko Date: Mon, 14 Nov 2022 11:45:52 +0200 Subject: [PATCH 6/8] p2p, test, refactor: Minor code improvements --- src/init.cpp | 2 +- src/node/txreconciliation.cpp | 4 +--- src/test/txreconciliation_tests.cpp | 21 +++++++++++---------- 3 files changed, 13 insertions(+), 14 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index 24659de3df0..bd424d15f05 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -478,6 +478,7 @@ void SetupServerArgs(ArgsManager& argsman) argsman.AddArg("-onlynet=", "Make automatic outbound connections only to network (" + Join(GetNetworkNames(), ", ") + "). Inbound and manual connections are not affected by this option. It can be specified multiple times to allow multiple networks.", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); argsman.AddArg("-peerbloomfilters", strprintf("Support filtering of blocks and transaction with bloom filters (default: %u)", DEFAULT_PEERBLOOMFILTERS), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); argsman.AddArg("-peerblockfilters", strprintf("Serve compact block filters to peers per BIP 157 (default: %u)", DEFAULT_PEERBLOCKFILTERS), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); + argsman.AddArg("-txreconciliation", strprintf("Enable transaction reconciliations per BIP 330 (default: %d)", DEFAULT_TXRECONCILIATION_ENABLE), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::CONNECTION); // TODO: remove the sentence "Nodes not using ... incoming connections." once the changes from // https://github.com/bitcoin/bitcoin/pull/23542 have become widespread. argsman.AddArg("-port=", strprintf("Listen for connections on . Nodes not using the default ports (default: %u, testnet: %u, signet: %u, regtest: %u) are unlikely to get incoming connections. Not relevant for I2P (see doc/i2p.md).", defaultChainParams->GetDefaultPort(), testnetChainParams->GetDefaultPort(), signetChainParams->GetDefaultPort(), regtestChainParams->GetDefaultPort()), ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY, OptionsCategory::CONNECTION); @@ -486,7 +487,6 @@ void SetupServerArgs(ArgsManager& argsman) argsman.AddArg("-seednode=", "Connect to a node to retrieve peer addresses, and disconnect. This option can be specified multiple times to connect to multiple nodes.", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); argsman.AddArg("-networkactive", "Enable all P2P network activity (default: 1). Can be changed by the setnetworkactive RPC command", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); argsman.AddArg("-timeout=", strprintf("Specify socket connection timeout in milliseconds. If an initial attempt to connect is unsuccessful after this amount of time, drop it (minimum: 1, default: %d)", DEFAULT_CONNECT_TIMEOUT), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); - argsman.AddArg("-txreconciliation", strprintf("Enable transaction reconciliations per BIP 330 (default: %d)", DEFAULT_TXRECONCILIATION_ENABLE), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::CONNECTION); argsman.AddArg("-peertimeout=", strprintf("Specify a p2p connection timeout delay in seconds. After connecting to a peer, wait this amount of time before considering disconnection based on inactivity (minimum: 1, default: %d)", DEFAULT_PEER_CONNECT_TIMEOUT), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::CONNECTION); argsman.AddArg("-torcontrol=:", strprintf("Tor control port to use if onion listening enabled (default: %s)", DEFAULT_TOR_CONTROL), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); argsman.AddArg("-torpassword=", "Tor control port password (default: empty)", ArgsManager::ALLOW_ANY | ArgsManager::SENSITIVE, OptionsCategory::CONNECTION); diff --git a/src/node/txreconciliation.cpp b/src/node/txreconciliation.cpp index 7b50b41dc74..ed04a78cecf 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; } diff --git a/src/test/txreconciliation_tests.cpp b/src/test/txreconciliation_tests.cpp index 49317d12dc5..b018629e76a 100644 --- a/src/test/txreconciliation_tests.cpp +++ b/src/test/txreconciliation_tests.cpp @@ -12,19 +12,20 @@ BOOST_FIXTURE_TEST_SUITE(txreconciliation_tests, BasicTestingSetup) BOOST_AUTO_TEST_CASE(RegisterPeerTest) { - TxReconciliationTracker tracker(1); + TxReconciliationTracker tracker(TXRECONCILIATION_VERSION); const uint64_t salt = 0; // Prepare a peer for reconciliation. tracker.PreRegisterPeer(0); // Invalid version. - BOOST_CHECK(tracker.RegisterPeer(/*peer_id=*/0, /*is_peer_inbound=*/true, - /*peer_recon_version=*/0, salt) == ReconciliationRegisterResult::PROTOCOL_VIOLATION); + BOOST_CHECK_EQUAL(tracker.RegisterPeer(/*peer_id=*/0, /*is_peer_inbound=*/true, + /*peer_recon_version=*/0, salt), + ReconciliationRegisterResult::PROTOCOL_VIOLATION); // Valid registration (inbound and outbound peers). BOOST_REQUIRE(!tracker.IsPeerRegistered(0)); - BOOST_REQUIRE(tracker.RegisterPeer(0, true, 1, salt) == ReconciliationRegisterResult::SUCCESS); + BOOST_REQUIRE_EQUAL(tracker.RegisterPeer(0, true, 1, salt), ReconciliationRegisterResult::SUCCESS); BOOST_CHECK(tracker.IsPeerRegistered(0)); BOOST_REQUIRE(!tracker.IsPeerRegistered(1)); tracker.PreRegisterPeer(1); @@ -41,24 +42,24 @@ BOOST_AUTO_TEST_CASE(RegisterPeerTest) 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_REQUIRE_EQUAL(tracker.RegisterPeer(100, true, 1, salt), ReconciliationRegisterResult::NOT_FOUND); BOOST_CHECK(!tracker.IsPeerRegistered(100)); } BOOST_AUTO_TEST_CASE(ForgetPeerTest) { - TxReconciliationTracker tracker(1); + TxReconciliationTracker tracker(TXRECONCILIATION_VERSION); NodeId peer_id0 = 0; // 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, 1, 1) == ReconciliationRegisterResult::NOT_FOUND); + BOOST_CHECK_EQUAL(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, 1, 1) == ReconciliationRegisterResult::SUCCESS); + BOOST_REQUIRE_EQUAL(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)); @@ -66,14 +67,14 @@ BOOST_AUTO_TEST_CASE(ForgetPeerTest) BOOST_AUTO_TEST_CASE(IsPeerRegisteredTest) { - TxReconciliationTracker tracker(1); + TxReconciliationTracker tracker(TXRECONCILIATION_VERSION); NodeId peer_id0 = 0; BOOST_REQUIRE(!tracker.IsPeerRegistered(peer_id0)); tracker.PreRegisterPeer(peer_id0); BOOST_REQUIRE(!tracker.IsPeerRegistered(peer_id0)); - BOOST_REQUIRE(tracker.RegisterPeer(peer_id0, true, 1, 1) == ReconciliationRegisterResult::SUCCESS); + BOOST_REQUIRE_EQUAL(tracker.RegisterPeer(peer_id0, true, 1, 1), ReconciliationRegisterResult::SUCCESS); BOOST_CHECK(tracker.IsPeerRegistered(peer_id0)); tracker.ForgetPeer(peer_id0); From 14263c13f153b84e50191366a6f64f884ed4ddd9 Mon Sep 17 00:00:00 2001 From: Gleb Naumenko Date: Tue, 25 Oct 2022 11:44:11 +0300 Subject: [PATCH 7/8] p2p, refactor: Extend logs for unexpected sendtxrcncl --- src/net_processing.cpp | 2 ++ test/functional/p2p_sendtxrcncl.py | 12 ++++++++++++ 2 files changed, 14 insertions(+) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 33c0fe433ae..cfa3d2f2210 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -3526,6 +3526,8 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, peer_txreconcl_version, remote_salt); switch (result) { case ReconciliationRegisterResult::NOT_FOUND: + LogPrintLevel(BCLog::NET, BCLog::Level::Debug, "Ignore unexpected txreconciliation signal from peer=%d\n", pfrom.GetId()); + break; case ReconciliationRegisterResult::SUCCESS: break; case ReconciliationRegisterResult::ALREADY_REGISTERED: diff --git a/test/functional/p2p_sendtxrcncl.py b/test/functional/p2p_sendtxrcncl.py index fe3126ca1e1..b23308eee96 100755 --- a/test/functional/p2p_sendtxrcncl.py +++ b/test/functional/p2p_sendtxrcncl.py @@ -155,6 +155,18 @@ class SendTxRcnclTest(BitcoinTestFramework): peer.send_message(sendtxrcncl_higher_version) self.nodes[0].disconnect_p2ps() + self.log.info('unexpected SENDTXRCNCL is ignored') + peer = self.nodes[0].add_p2p_connection(PeerNoVerack(), send_version=False, wait_for_verack=False) + old_version_msg = msg_version() + old_version_msg.nVersion = 70015 + old_version_msg.strSubVer = P2P_SUBVERSION + old_version_msg.nServices = P2P_SERVICES + old_version_msg.relay = 1 + peer.send_message(old_version_msg) + with self.nodes[0].assert_debug_log(['Ignore unexpected txreconciliation signal from peer=2']): + peer.send_message(create_sendtxrcncl_msg()) + self.nodes[0].disconnect_p2ps() + self.log.info('sending SENDTXRCNCL after sending VERACK triggers a disconnect') peer = self.nodes[0].add_p2p_connection(P2PInterface()) with self.nodes[0].assert_debug_log(["sendtxrcncl received after verack"]): From 46339d29b10c9fb597af928c21c34945d76bbd22 Mon Sep 17 00:00:00 2001 From: Gleb Naumenko Date: Wed, 2 Nov 2022 13:16:19 +0200 Subject: [PATCH 8/8] test, refactor: Reorder sendtxrcncl tests for better readability --- test/functional/p2p_sendtxrcncl.py | 81 ++++++++++++++++-------------- 1 file changed, 43 insertions(+), 38 deletions(-) diff --git a/test/functional/p2p_sendtxrcncl.py b/test/functional/p2p_sendtxrcncl.py index b23308eee96..0e349ef70cd 100755 --- a/test/functional/p2p_sendtxrcncl.py +++ b/test/functional/p2p_sendtxrcncl.py @@ -67,6 +67,8 @@ class SendTxRcnclTest(BitcoinTestFramework): self.extra_args = [['-txreconciliation']] def run_test(self): + # Check everything concerning *sending* SENDTXRCNCL + # First, *sending* to *inbound*. self.log.info('SENDTXRCNCL sent to an inbound') peer = self.nodes[0].add_p2p_connection(SendTxrcnclReceiver(), send_version=True, wait_for_verack=True) assert peer.sendtxrcncl_msg_received @@ -119,8 +121,46 @@ class SendTxRcnclTest(BitcoinTestFramework): assert not peer.sendtxrcncl_msg_received self.nodes[0].disconnect_p2ps() - self.restart_node(0, ["-txreconciliation"]) + # Now, *sending* to *outbound*. + self.log.info('SENDTXRCNCL sent to an outbound') + peer = self.nodes[0].add_outbound_p2p_connection( + SendTxrcnclReceiver(), wait_for_verack=True, p2p_idx=0, connection_type="outbound-full-relay") + assert peer.sendtxrcncl_msg_received + assert_equal(peer.sendtxrcncl_msg_received.version, 1) + self.nodes[0].disconnect_p2ps() + self.log.info('SENDTXRCNCL should not be sent if block-relay-only') + peer = self.nodes[0].add_outbound_p2p_connection( + SendTxrcnclReceiver(), wait_for_verack=True, p2p_idx=0, connection_type="block-relay-only") + assert not peer.sendtxrcncl_msg_received + self.nodes[0].disconnect_p2ps() + + self.log.info("SENDTXRCNCL should not be sent if feeler") + peer = self.nodes[0].add_outbound_p2p_connection(P2PFeelerReceiver(), p2p_idx=0, connection_type="feeler") + assert not peer.sendtxrcncl_msg_received + self.nodes[0].disconnect_p2ps() + + self.log.info("SENDTXRCNCL should not be sent if addrfetch") + peer = self.nodes[0].add_outbound_p2p_connection( + SendTxrcnclReceiver(), wait_for_verack=True, p2p_idx=0, connection_type="addr-fetch") + assert not peer.sendtxrcncl_msg_received + self.nodes[0].disconnect_p2ps() + + self.log.info('SENDTXRCNCL not sent if -txreconciliation flag is not set') + self.restart_node(0, []) + peer = self.nodes[0].add_p2p_connection(SendTxrcnclReceiver(), send_version=True, wait_for_verack=True) + assert not peer.sendtxrcncl_msg_received + self.nodes[0].disconnect_p2ps() + + self.log.info('SENDTXRCNCL not sent if blocksonly is set') + self.restart_node(0, ["-txreconciliation", "-blocksonly"]) + peer = self.nodes[0].add_p2p_connection(SendTxrcnclReceiver(), send_version=True, wait_for_verack=True) + assert not peer.sendtxrcncl_msg_received + self.nodes[0].disconnect_p2ps() + + # Check everything concerning *receiving* SENDTXRCNCL + # First, receiving from *inbound*. + self.restart_node(0, ["-txreconciliation"]) self.log.info('valid SENDTXRCNCL received') peer = self.nodes[0].add_p2p_connection(PeerNoVerack(), send_version=True, wait_for_verack=False) with self.nodes[0].assert_debug_log(["received: sendtxrcncl"]): @@ -163,7 +203,7 @@ class SendTxRcnclTest(BitcoinTestFramework): old_version_msg.nServices = P2P_SERVICES old_version_msg.relay = 1 peer.send_message(old_version_msg) - with self.nodes[0].assert_debug_log(['Ignore unexpected txreconciliation signal from peer=2']): + with self.nodes[0].assert_debug_log(['Ignore unexpected txreconciliation signal']): peer.send_message(create_sendtxrcncl_msg()) self.nodes[0].disconnect_p2ps() @@ -180,30 +220,7 @@ class SendTxRcnclTest(BitcoinTestFramework): peer.send_message(msg_verack()) self.nodes[0].disconnect_p2ps() - self.log.info('SENDTXRCNCL sent to an outbound') - peer = self.nodes[0].add_outbound_p2p_connection( - SendTxrcnclReceiver(), wait_for_verack=True, p2p_idx=0, connection_type="outbound-full-relay") - assert peer.sendtxrcncl_msg_received - assert_equal(peer.sendtxrcncl_msg_received.version, 1) - self.nodes[0].disconnect_p2ps() - - self.log.info('SENDTXRCNCL should not be sent if block-relay-only') - peer = self.nodes[0].add_outbound_p2p_connection( - SendTxrcnclReceiver(), wait_for_verack=True, p2p_idx=0, connection_type="block-relay-only") - assert not peer.sendtxrcncl_msg_received - self.nodes[0].disconnect_p2ps() - - self.log.info("SENDTXRCNCL should not be sent if feeler") - peer = self.nodes[0].add_outbound_p2p_connection(P2PFeelerReceiver(), p2p_idx=0, connection_type="feeler") - assert not peer.sendtxrcncl_msg_received - self.nodes[0].disconnect_p2ps() - - self.log.info("SENDTXRCNCL should not be sent if addrfetch") - peer = self.nodes[0].add_outbound_p2p_connection( - SendTxrcnclReceiver(), wait_for_verack=True, p2p_idx=0, connection_type="addr-fetch") - assert not peer.sendtxrcncl_msg_received - self.nodes[0].disconnect_p2ps() - + # Now, *receiving* from *outbound*. self.log.info('SENDTXRCNCL if block-relay-only triggers a disconnect') peer = self.nodes[0].add_outbound_p2p_connection( PeerNoVerack(), wait_for_verack=False, p2p_idx=0, connection_type="block-relay-only") @@ -211,18 +228,6 @@ class SendTxRcnclTest(BitcoinTestFramework): peer.send_message(create_sendtxrcncl_msg()) peer.wait_for_disconnect() - self.log.info('SENDTXRCNCL not sent if -txreconciliation flag is not set') - self.restart_node(0, []) - peer = self.nodes[0].add_p2p_connection(SendTxrcnclReceiver(), send_version=True, wait_for_verack=True) - assert not peer.sendtxrcncl_msg_received - self.nodes[0].disconnect_p2ps() - - self.log.info('SENDTXRCNCL not sent if blocksonly is set') - self.restart_node(0, ["-txreconciliation", "-blocksonly"]) - peer = self.nodes[0].add_p2p_connection(SendTxrcnclReceiver(), send_version=True, wait_for_verack=True) - assert not peer.sendtxrcncl_msg_received - self.nodes[0].disconnect_p2ps() - if __name__ == '__main__': SendTxRcnclTest().main()