Non-trivial changes include:
sendtxrcncl message (summarized in the BIP PR);sendtxrcncl although we are in blocksonly and notified the peer with fRelay=0;sendtxrcncl to feeler connections.3272@@ -3273,8 +3273,9 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
3273 // - protocol version per the VERSION message supports WTXID_RELAY;
3274 // - we intended to exchange transactions over this connection while establishing it
3275 // and the peer indicated support for transaction relay in the VERSION message;
3276- // - we are not in -blocksonly mode.
3277- if (pfrom.m_relays_txs && !m_ignore_incoming_txs) {
3278+ // - we are not in -blocksonly mode;
3279+ // - this is not a feeler.
3280+ if (pfrom.m_relays_txs && !m_ignore_incoming_txs && !pfrom.IsFeelerConn()) {
cfd5c27dcdc39cf7a65c56c3dbf3bf60ea0c475f:
There are several unrelated things:
RejectIncomingTxs should be used here. The difference is that the Relay permission will be carved out. I don’t know the answer, I just wanted to raise the point.
RejectIncomingTxs is of course relevant, but not 100% match, if you just look at the name. I’d better keep the code as-is now, and refactor this separately (dunno how yet)
By the latter I mean that even RejectIncomingTxs-ing (whatever that could mean) doesn’t necessarily mean we should reject reconciling. Might be useful to reconcile even if we “should reject”. Not 100% sure (certainly not how Erlay intends to work in the default case).
Anyway, not changing the code until further consideration.
Not sure what’s the best way to attack this w.r.t. this PR
Happy to work on this, unless you want to
GETADDR, so the SENDTXRCNCL message seems to be consistent. Changing the tx-relay allocation could still be done, though. See https://github.com/bitcoin/bitcoin/pull/26396
IsAddrFetchConn() ? I think we should restrain ourselves to send ConnectionType::ADDR_FETCH.
m_relays_tx), and done in a separate PR :)
Let’s hope Marco updates that one for now.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
See the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
40 BOOST_REQUIRE(!tracker.IsPeerRegistered(0));
41- BOOST_REQUIRE(tracker.RegisterPeer(0, true, true, false, 1, salt) == ReconciliationRegisterResult::SUCCESS);
42+ BOOST_REQUIRE_EQUAL(tracker.RegisterPeer(0, true, 1, salt), ReconciliationRegisterResult::SUCCESS);
43 BOOST_CHECK(tracker.IsPeerRegistered(0));
44-
45- // Reconciliation version is higher than ours, should be able to register.
In commit 2269d91d2a p2p: Drop roles from sendtxrcncl there is this change:
0 // Reconciliation version is higher than ours, should be able to register.
1 BOOST_REQUIRE(!tracker.IsPeerRegistered(1));
2 tracker.PreRegisterPeer(1);
3- BOOST_REQUIRE(tracker.RegisterPeer(1, true, true, false, 2, salt) == ReconciliationRegisterResult::SUCCESS);
4+ BOOST_REQUIRE(tracker.RegisterPeer(1, false, 1, salt) == ReconciliationRegisterResult::SUCCESS);
5 BOOST_CHECK(tracker.IsPeerRegistered(1));
I think that should be
0+ BOOST_REQUIRE(tracker.RegisterPeer(1, true, 2, salt) == ReconciliationRegisterResult::SUCCESS);
(it is further modified by subsequent commits and I think that the cumulative diff is ok, but still each commit should make sense on its own)
3505- // support transaction relay.
3506+ if (!peer->GetTxRelay() || m_ignore_incoming_txs) {
3507 LogPrintLevel(BCLog::NET, BCLog::Level::Debug, "sendtxrcncl received from peer=%d to which we indicated no tx relay; disconnecting\n", pfrom.GetId());
3508 pfrom.fDisconnect = true;
3509 return;
3510 }
In commit 60a066da70 p2p: Disconnect peer after sendtxrcncl if we are in blocksonly
It could happen that m_ignore_incoming_txs is true (thus we disconnect the peer), but we did not indicate no tx relay to that peer as mentioned in the log message. That is - we sent relay=true in the VERSION message.
This could happen if the peer has NetPermissionFlags::Relay permission.
Or the other way around - we may send relay=false in the VERSION message, but not disconnect the peer here (block-only or feeler connections).
Would it be better to use PeerManagerImpl::RejectIncomingTxs() here instead of m_ignore_incoming_txs? The relay flag in the VERSION message is set based on it in PeerManagerImpl::PushNodeVersion().
It may also happen that we disconnect the peer because peer->GetTxRelay() is not set because the peer did not sent us relay=true in their VERSION message and we don’t provide bloom filter to them. I think this warrants a separate log message, something like:
“sendtxrcncl received from peer=%d that indicated no tx relay; disconnecting\n”
These flags are confusing :(
Indeed, having fRelay=false I think is not currently compatible with reconciliations.
So, instead of GetTxRelay(), we better look at pfrom.m_relays_txs, right?
These flags are confusing :(
Yes! :frowning_face:
Indeed, having
fRelay=falseI think is not currently compatible with reconciliations.
I guess you mean the relay flag in the either VERSION message - sent or received, right?
What about clarifying the BIP:
0-sendtxrcncl ... Must not be sent if peer specified no support for transaction relay (fRelay=0) in "version". Otherwise, the sender should be disconnected.
1+sendtxrcncl ... Must not be sent if either peer specified no support for transaction relay (fRelay=0) in "version". Otherwise, the sender should be disconnected.
So, instead of
GetTxRelay(), we better look atpfrom.m_relays_txs, right?
Yes, this is closer to checking whether the peer sent us relay=false in VERSION. However it is not identical - fRelay may be true, but pfrom.m_relays_txs be false if pfrom.IsBlockOnlyConn(). It is ok to accept this discrepancy because for block-only connections we sent relay=false in our VERSION message.
That said, maybe the condition should be:
0-if (!peer->GetTxRelay() || m_ignore_incoming_txs) {
1+if (!pfrom.m_relays_txs /* they sent relay=false */ ||
2+ RejectIncomingTxs(pfrom) /* we sent relay=false */) {
3543+ if (result == ReconciliationRegisterResult::NOT_FOUND) {
3544+ LogPrintLevel(BCLog::NET, BCLog::Level::Debug, "Ignore unexpected txreconciliation signal from peer=%d\n", pfrom.GetId());
3545+ return;
3546+ }
3547+
3548 return;
return inside the if is unnecessary
127- // The peer set both flags to false, we treat it as a protocol violation.
128- if (!(they_initiate || we_initiate)) return PROTOCOL_VIOLATION;
129-
130- LogPrintLevel(BCLog::TXRECONCILIATION, BCLog::Level::Debug, "Register peer=%d with the following params: " /* Continued */
131- "we_initiate=%i, they_initiate=%i.\n",
132- peer_id, we_initiate, they_initiate);
SENDTXRCNCL , this still mentioning the role assignment based on booleans. I would also suggest to document somewhere, in protocol.h or txreconciliation.cpp, near to m_we_initiate how the role assignment is made.
Approach ACK
I reviewed the code, and all points I found were already mentioned by others, so I’d be happy to ACK after the oustanding comments are addressed.
108- if (recon_state == m_states.end()) return NOT_FOUND;
109+ if (recon_state == m_states.end()) return ReconciliationRegisterResult::NOT_FOUND;
110 uint64_t* local_salt = std::get_if<uint64_t>(&recon_state->second);
111 // A peer is already registered. This should be checked by the caller.
112- Assume(local_salt);
113+ assert(local_salt);
There is some inconsistency between handling these edge cases. This assert() also looks a bit dangerous to me wrt to future changes - it seems that if the peer sends us two times SENDTXRCNCL that could trigger a remote crash by tripping this assert(). The only thing that is stopping this now is the IsPeerRegistered() in the caller, but it is not obvious there that if it is removed, grave things will ensue. I can imagine it getting wiped away by a future “refactor”.
If the caller has checked IsPeerRegistered() before this, then we will never return NOT_FOUND from here either. So that may as well be an assert too :fire:
What about not having such an assert at all and returning ReconciliationRegisterResult::NOT_FOUND if the peer is not pre-registered (like now) and ReconciliationRegisterResult::ALREADY_REGISTERED if the peer is already registered? Then the caller need not to check IsPeerRegistered() before calling this method.
3272@@ -3273,17 +3273,14 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
3273
3274 if (greatest_common_version >= WTXID_RELAY_VERSION && m_txreconciliation) {
Not sure if that is better, just an idea, feel free to ignore:
0diff --git i/src/net_processing.cpp w/src/net_processing.cpp
1index 376a263183..0ed7781b2c 100644
2--- i/src/net_processing.cpp
3+++ w/src/net_processing.cpp
4@@ -3265,26 +3265,27 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
5 (fRelay || (peer->m_our_services & NODE_BLOOM))) {
6 auto* const tx_relay = peer->SetTxRelay();
7 {
8 LOCK(tx_relay->m_bloom_filter_mutex);
9 tx_relay->m_relay_txs = fRelay; // set to true after we get the first filter* message
10 }
11- if (fRelay) pfrom.m_relays_txs = true;
12- }
13-
14- if (greatest_common_version >= WTXID_RELAY_VERSION && m_txreconciliation) {
15- // Per BIP-330, we announce txreconciliation support if:
16- // - protocol version per the peer's VERSION message supports WTXID_RELAY;
17- // - transaction relay is supported per the peer's VERSION message (see m_relays_txs);
18- // - this is not a block-relay-only connection and not a feeler (see m_relays_txs);
19- // - this is not an addr fetch connection;
20- // - we are not in -blocksonly mode.
21- if (pfrom.m_relays_txs && !pfrom.IsAddrFetchConn() && !m_ignore_incoming_txs) {
22- const uint64_t recon_salt = m_txreconciliation->PreRegisterPeer(pfrom.GetId());
23- m_connman.PushMessage(&pfrom, msg_maker.Make(NetMsgType::SENDTXRCNCL,
24- TXRECONCILIATION_VERSION, recon_salt));
25+ if (fRelay) {
26+ pfrom.m_relays_txs = true;
27+
28+ if (greatest_common_version >= WTXID_RELAY_VERSION &&
29+ m_txreconciliation &&
30+ !pfrom.IsAddrFetchConn() && !m_ignore_incoming_txs) {
31+ // Per BIP-330, we announce txreconciliation support if:
32+ // - protocol version per the peer's VERSION message supports WTXID_RELAY;
33+ // - transaction relay is supported per the peer's VERSION message (see m_relays_txs);
34+ // - this is not a block-relay-only connection and not a feeler (see m_relays_txs);
35+ // - this is not an addr fetch connection;
36+ // - we are not in -blocksonly mode.
37+ const uint64_t recon_salt = m_txreconciliation->PreRegisterPeer(pfrom.GetId());
38+ m_connman.PushMessage(&pfrom, msg_maker.Make(NetMsgType::SENDTXRCNCL, TXRECONCILIATION_VERSION, recon_salt));
39+ }
40 }
41 }
42
43 m_connman.PushMessage(&pfrom, msg_maker.Make(NetMsgType::VERACK));
44
45 // Potentially mark this peer as a preferred download peer.
assert().
3553+ break;
3554+ case ReconciliationRegisterResult::ALREADY_REGISTERED: {
3555+ LogPrintLevel(BCLog::NET, BCLog::Level::Debug, "txreconciliation protocol violation from peer=%d (sendtxrcncl received from already registered peer); disconnecting\n", pfrom.GetId());
3556+ pfrom.fDisconnect = true;
3557+ return;
3558+ };
{ } brackets shouldn’t be necessary.
3532
3533+ uint32_t peer_txreconcl_version;
3534+ uint64_t remote_salt;
3535+ vRecv >> peer_txreconcl_version >> remote_salt;
3536+
3537 const ReconciliationRegisterResult result = m_txreconciliation->RegisterPeer(pfrom.GetId(), pfrom.IsInboundConn(),
refactor: Switch to enum class for ReconciliationRegisterResult now contains also the behavioral change of RegisterPeer() wrt its return value and the removal of the assert.
3536 LogPrintLevel(BCLog::NET, BCLog::Level::Debug, "txreconciliation protocol violation from peer=%d (sendtxrcncl received from already registered peer); disconnecting\n", pfrom.GetId());
3537 pfrom.fDisconnect = true;
3538 return;
3539 case ReconciliationRegisterResult::PROTOCOL_VIOLATION:
3540- LogPrintLevel(BCLog::NET, BCLog::Level::Debug, "txreconciliation protocol violation from peer=%d; disconnecting\n", pfrom.GetId());
3541+ LogPrintLevel(BCLog::NET, BCLog::Level::Debug, "txreconciliation protocol violation from peer=%d; disconnecting\n", pfrom.GetId());
case lines aren’t indented wrt the switch line, see other examples or apply clang-format script.
3280+ // - transaction relay is supported per the peer's VERSION message (see m_relays_txs);
3281+ // - this is not a block-relay-only connection and not a feeler (see m_relays_txs);
3282+ // - this is not an addr fetch connection;
3283 // - we are not in -blocksonly mode.
3284- if (pfrom.m_relays_txs && !m_ignore_incoming_txs) {
3285+ if (pfrom.m_relays_txs && !pfrom.IsAddrFetchConn() && !m_ignore_incoming_txs) {
AddrFetch without any tangible benefits (or are there any?) - I’d like to think of them as normal outbound connections, where we just happen to disconnect after we get the GETADDR answer, and while I admit that concerns such as #26396 (comment) aren’t super strong, I think it’s also easier mental model and less complicated code not to special-case for each message whether AddrFetch connections need it.
38@@ -39,7 +39,8 @@ class TxReconciliationState
39 * the following commits.
40 *
41 * Reconciliation protocol assumes using one role consistently: either a reconciliation
42- * initiator (requesting sketches), or responder (sending sketches). This defines our role.
43+ * initiator (requesting sketches), or responder (sending sketches). This defines our role,
44+ * based on the direction of the p2p connection.
Note you have a bunch of references to the term initiator left in reconciatliation.{h,cpp}:
0src/node/txreconciliation.cpp: * initiator (requesting sketches), or responder (sending sketches). This defines our role,
1src/node/txreconciliation.h: * 2. At regular intervals, a txreconciliation initiator requests a sketch from a peer, where a
2src/node/txreconciliation.h: * 3. Once the initiator received a sketch from the peer, the initiator computes a local sketch,
3src/node/txreconciliation.h: * 4b. If the difference was larger than estimated, initial txreconciliation fails. The initiator
4src/node/txreconciliation.h: * SUCCESS. The initiator knows full symmetrical difference and can request what the initiator is
5src/node/txreconciliation.h: * FAILURE. The initiator notifies the peer about the failure and announces all transactions from
It’s minor, I think they can be addressed in its own commit here or in #26283 to avoid another review round.
sendtxrcncl message.
ACK 58786a1
Verified the test coverage updates in removal of initiator/responder and after defining better sendtxrcncl policy.
This feature was currently redundant (although could have provided
more flexibility in the future), and already been causing confusion.
ACK d44438c7
Note the new commit d44438c reodering functional tests for readability.
The cumulative diff as of d44438c7d7b98d790052f8848de4aaa6d0ea7704 looks ok, but the individual commits seem somewhat messed up:
d7239b5d84f7f658c504002a8a7df75d1f90792b test: Expand unit and functional tests for txreconciliation
contains non-test changes related to the return value of RegisterPeer().
9cb6b33c62a6d3afe478727f782ae676f78bed07 p2p, refactor: Switch to enum class for ReconciliationRegisterResult
has some indentation changes, and not the enum class changes
29df2d79d1b48621829b98d667c1c899d94d2ed5 p2p, refactor: Minor code improvements
has some adverse indentation changes and introduces the unnecessary { }
5a183ddd8d8f46e6b89cce3a24ab3d8c9b5ddac4 p2p, refactor: Extend logs for unexpected sendtxrcncl
fixes the adverse indentation from 29df2d79d1b48621829b98d667c1c899d94d2ed5 but not the unnecessary { }
While doing this, add a new value: ALREADY_REGISTERED.
ACK 46339d29b10c9fb597af928c21c34945d76bbd22
:sun_with_face:
3532- // A peer is already registered, meaning we already received SENDTXRCNCL from them.
3533+ const ReconciliationRegisterResult result = m_txreconciliation->RegisterPeer(pfrom.GetId(), pfrom.IsInboundConn(),
3534+ peer_txreconcl_version, remote_salt);
3535+ switch (result) {
3536+ case ReconciliationRegisterResult::NOT_FOUND:
3537+ LogPrintLevel(BCLog::NET, BCLog::Level::Debug, "Ignore unexpected txreconciliation signal from peer=%d\n", pfrom.GetId());
ACK 46339d2
Did another parse of the code changes to verify there wasn’t functional modification.