p2p: Erlay support signaling follow-ups #26359

naumenkogs · 2022-10-21T08:08:43Z

Non-trivial changes include:

Getting rid of roles in sendtxrcncl message (summarized in the BIP PR);
Disconnect the peer if it send sendtxrcncl although we are in blocksonly and notified the peer with fRelay=0;
Don't send sendtxrcncl to feeler connections.

src/net_processing.cpp

DrahtBot · 2022-10-21T10:18:19Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	vasild, ariard, mzumsande

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#26283 (p2p: Fill reconciliation sets and request reconciliation (Erlay) by naumenkogs)
#25572 (refactor: Introduce EvictionManager and use it for the inbound eviction logic by dergoegge)
#25268 (refactor: Introduce EvictionManager by dergoegge)
#21515 (Erlay: bandwidth-efficient transaction relay protocol by naumenkogs)

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.

vasild

Approach ACK 2b9cd5f

src/test/txreconciliation_tests.cpp

src/net_processing.cpp

src/node/txreconciliation.cpp

src/net_processing.cpp

mzumsande

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.

vasild

ACK 2193344 modulo the failing test which seems to need some tweaking and the comment below wrt assert().

src/node/txreconciliation.cpp

vasild · 2022-11-07T17:54:36Z

src/net_processing.cpp

@@ -3273,17 +3273,14 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,

        if (greatest_common_version >= WTXID_RELAY_VERSION && m_txreconciliation) {


Not sure if that is better, just an idea, feel free to ignore:

diff --git i/src/net_processing.cpp w/src/net_processing.cpp index 376a263183..0ed7781b2c 100644 --- i/src/net_processing.cpp +++ w/src/net_processing.cpp @@ -3265,26 +3265,27 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, (fRelay || (peer->m_our_services & NODE_BLOOM))) { auto* const tx_relay = peer->SetTxRelay(); { LOCK(tx_relay->m_bloom_filter_mutex); tx_relay->m_relay_txs = fRelay; // set to true after we get the first filter* message } - if (fRelay) pfrom.m_relays_txs = true; - } - - if (greatest_common_version >= WTXID_RELAY_VERSION && m_txreconciliation) { - // Per BIP-330, we announce txreconciliation support if: - // - 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 && !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)); + if (fRelay) { + pfrom.m_relays_txs = true; + + if (greatest_common_version >= WTXID_RELAY_VERSION && + m_txreconciliation && + !pfrom.IsAddrFetchConn() && !m_ignore_incoming_txs) { + // Per BIP-330, we announce txreconciliation support if: + // - 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. + const uint64_t recon_salt = m_txreconciliation->PreRegisterPeer(pfrom.GetId()); + m_connman.PushMessage(&pfrom, msg_maker.Make(NetMsgType::SENDTXRCNCL, TXRECONCILIATION_VERSION, recon_salt)); + } } } m_connman.PushMessage(&pfrom, msg_maker.Make(NetMsgType::VERACK)); // Potentially mark this peer as a preferred download peer.

Not sure about this — it's kinda difficult either way :) Will see what others say.

vasild

ACK 1597210

src/net_processing.cpp

mzumsande

Code Review ACK 58786a1

src/net_processing.cpp

mzumsande · 2022-11-09T20:37:24Z

src/net_processing.cpp

            // - 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) {


Not a big fan of special casing behavior for 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.

ariard

ACK 58786a1

Verified the test coverage updates in removal of initiator/responder and after defining better sendtxrcncl policy.

ariard · 2022-11-10T00:15:58Z

src/node/txreconciliation.cpp

@@ -39,7 +39,8 @@ class TxReconciliationState
     * 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.


Note you have a bunch of references to the term initiator left in reconciatliation.{h,cpp}:

src/node/txreconciliation.cpp: * initiator (requesting sketches), or responder (sending sketches). This defines our role, src/node/txreconciliation.h: * 2. At regular intervals, a txreconciliation initiator requests a sketch from a peer, where a src/node/txreconciliation.h: * 3. Once the initiator received a sketch from the peer, the initiator computes a local sketch, src/node/txreconciliation.h: * 4b. If the difference was larger than estimated, initial txreconciliation fails. The initiator src/node/txreconciliation.h: * SUCCESS. The initiator knows full symmetrical difference and can request what the initiator is src/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.

I think it makes sense to keep using the more abstract terms initiator / receiver within the txreconciliation module, even after removing it from the sendtxrcncl message.

This feature was currently redundant (although could have provided more flexibility in the future), and already been causing confusion.

naumenkogs · 2022-11-10T07:26:46Z

Applied clang-format everywhere.

ariard

ACK d44438c

Note the new commit d44438c reodering functional tests for readability.

vasild · 2022-11-12T15:01:35Z

The cumulative diff as of d44438c looks ok, but the individual commits seem somewhat messed up:

d7239b5 test: Expand unit and functional tests for txreconciliation
contains non-test changes related to the return value of RegisterPeer().
9cb6b33 p2p, refactor: Switch to enum class for ReconciliationRegisterResult
has some indentation changes, and not the enum class changes
29df2d7 p2p, refactor: Minor code improvements
has some adverse indentation changes and introduces the unnecessary { }
5a183dd p2p, refactor: Extend logs for unexpected sendtxrcncl
fixes the adverse indentation from 29df2d7 but not the unnecessary { }

While doing this, add a new value: ALREADY_REGISTERED.

naumenkogs · 2022-11-14T10:04:57Z

@vasild sorry for that mess, addressed everything without introducing behavior changes.

vasild

ACK 46339d2

🌞

ariard

ACK 46339d2

Did another parse of the code changes to verify there wasn't functional modification.

ariard · 2022-11-16T00:28:23Z

src/net_processing.cpp

+                                                                                     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());


nit: The message could be clearer saying "Ignore sendtxrcncl received from peer". "txreconciliation signal" is a bit hand-wawy.

mzumsande

Code Review ACK 46339d2

46339d2 test, refactor: Reorder sendtxrcncl tests for better readability (Gleb Naumenko) 14263c1 p2p, refactor: Extend logs for unexpected sendtxrcncl (Gleb Naumenko) 87493e1 p2p, test, refactor: Minor code improvements (Gleb Naumenko) 00c5dec p2p: Clarify sendtxrcncl policies (Gleb Naumenko) ac6ee5b test: Expand unit and functional tests for txreconciliation (Gleb Naumenko) bc84e24 p2p, refactor: Switch to enum class for ReconciliationRegisterResult (Gleb Naumenko) a60f729 p2p: Drop roles from sendtxrcncl (Gleb Naumenko) 6772cbf tests: stabilize sendtxrcncl test (Gleb Naumenko) Pull request description: Non-trivial changes include: - Getting rid of roles in `sendtxrcncl` message (summarized in the [BIP PR](bitcoin/bips#1376)); - Disconnect the peer if it send `sendtxrcncl` although we are in `blocksonly` and notified the peer with `fRelay=0`; - Don't send `sendtxrcncl` to feeler connections. ACKs for top commit: vasild: ACK 46339d2 ariard: ACK 46339d2 mzumsande: Code Review ACK 46339d2 Tree-SHA512: b5cc6934b4670c12b7dbb3189e739ef747ee542ec56678bf4e4355bfb481b746d32363c173635685b71969b3fe4bd52b1c8ebd3ea3b35c82044bba69220f6417

fanquake added the P2P label Oct 21, 2022

naumenkogs mentioned this pull request Oct 21, 2022

Erlay: bandwidth-efficient transaction relay protocol #21515

Closed

maflcko reviewed Oct 21, 2022

View reviewed changes

src/net_processing.cpp Outdated Show resolved Hide resolved

This was referenced Oct 23, 2022

refactor: Introduce EvictionManager and use it for the inbound eviction logic #25572

Closed

test: Fix intermittent issue in p2p_sendtxrcncl.py #26381

Merged

naumenkogs force-pushed the 2021-11-erlay1_followup branch from 15962a8 to 0ba8bd1 Compare October 25, 2022 08:44

naumenkogs marked this pull request as draft October 26, 2022 08:46

naumenkogs force-pushed the 2021-11-erlay1_followup branch 2 times, most recently from 885e8f9 to 2b9cd5f Compare October 26, 2022 11:02

naumenkogs marked this pull request as ready for review October 26, 2022 11:02

naumenkogs mentioned this pull request Oct 28, 2022

p2p: Fill reconciliation sets and request reconciliation (Erlay) #26283

Closed

vasild reviewed Oct 28, 2022

View reviewed changes

src/test/txreconciliation_tests.cpp Show resolved Hide resolved

src/net_processing.cpp Outdated Show resolved Hide resolved

src/net_processing.cpp Show resolved Hide resolved

src/net_processing.cpp Show resolved Hide resolved

src/net_processing.cpp Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Oct 29, 2022

refactor: Introduce EvictionManager #25268

Closed

ariard reviewed Oct 30, 2022

View reviewed changes

src/node/txreconciliation.cpp Show resolved Hide resolved

src/net_processing.cpp Outdated Show resolved Hide resolved

mzumsande reviewed Nov 1, 2022

View reviewed changes

mzumsande mentioned this pull request Nov 1, 2022

net: Avoid SetTxRelay for feeler connections #26396

Merged

naumenkogs force-pushed the 2021-11-erlay1_followup branch 2 times, most recently from 8a0175a to 23e347d Compare November 2, 2022 11:38

DrahtBot mentioned this pull request Nov 4, 2022

test: fix intermittent failure in p2p_sendtxrcncl.py #26448

Merged

DrahtBot added the Needs rebase label Nov 4, 2022

naumenkogs force-pushed the 2021-11-erlay1_followup branch from 23e347d to 2193344 Compare November 4, 2022 18:30

DrahtBot removed the Needs rebase label Nov 4, 2022

vasild reviewed Nov 7, 2022

View reviewed changes

DrahtBot mentioned this pull request Nov 8, 2022

scripted diff: fix E275 lint #26468

Closed

tests: stabilize sendtxrcncl test

6772cbf

naumenkogs force-pushed the 2021-11-erlay1_followup branch from 2193344 to 1597210 Compare November 8, 2022 10:19

maflcko changed the title ~~p2p, refactor: #23443 (Erlay support signaling) follow-ups~~ p2p: Erlay support signaling follow-ups Nov 8, 2022

vasild approved these changes Nov 8, 2022

View reviewed changes

src/net_processing.cpp Outdated Show resolved Hide resolved

src/net_processing.cpp Show resolved Hide resolved

naumenkogs force-pushed the 2021-11-erlay1_followup branch from 1597210 to 58786a1 Compare November 9, 2022 08:08

mzumsande reviewed Nov 9, 2022

View reviewed changes

ariard approved these changes Nov 10, 2022

View reviewed changes

p2p: Drop roles from sendtxrcncl

a60f729

This feature was currently redundant (although could have provided more flexibility in the future), and already been causing confusion.

naumenkogs force-pushed the 2021-11-erlay1_followup branch from 58786a1 to d44438c Compare November 10, 2022 07:26

ariard approved these changes Nov 11, 2022

View reviewed changes

naumenkogs added 6 commits November 14, 2022 11:37

p2p, refactor: Switch to enum class for ReconciliationRegisterResult

bc84e24

While doing this, add a new value: ALREADY_REGISTERED.

test: Expand unit and functional tests for txreconciliation

ac6ee5b

p2p: Clarify sendtxrcncl policies

00c5dec

p2p, test, refactor: Minor code improvements

87493e1

p2p, refactor: Extend logs for unexpected sendtxrcncl

14263c1

test, refactor: Reorder sendtxrcncl tests for better readability

46339d2

naumenkogs force-pushed the 2021-11-erlay1_followup branch from d44438c to 46339d2 Compare November 14, 2022 10:04

vasild approved these changes Nov 14, 2022

View reviewed changes

ariard reviewed Nov 16, 2022

View reviewed changes

fanquake requested a review from mzumsande November 16, 2022 10:07

mzumsande reviewed Nov 29, 2022

View reviewed changes

fanquake merged commit bcee94d into bitcoin:master Nov 30, 2022

naumenkogs mentioned this pull request Oct 13, 2023

Erlay Project Tracking #28646

Closed

17 tasks

bitcoin locked and limited conversation to collaborators Nov 30, 2023

		@@ -3273,17 +3273,14 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,

		if (greatest_common_version >= WTXID_RELAY_VERSION && m_txreconciliation) {

p2p: Erlay support signaling follow-ups #26359

p2p: Erlay support signaling follow-ups #26359

Uh oh!

Conversation

naumenkogs commented Oct 21, 2022 • edited by maflcko Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

DrahtBot commented Oct 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mzumsande left a comment

Choose a reason for hiding this comment

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vasild Nov 7, 2022

Choose a reason for hiding this comment

Uh oh!

naumenkogs Nov 8, 2022

Choose a reason for hiding this comment

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mzumsande left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mzumsande Nov 9, 2022

Choose a reason for hiding this comment

Uh oh!

ariard left a comment

Choose a reason for hiding this comment

Uh oh!

ariard Nov 10, 2022

Choose a reason for hiding this comment

Uh oh!

mzumsande Nov 10, 2022

Choose a reason for hiding this comment

Uh oh!

naumenkogs commented Nov 10, 2022

Uh oh!

ariard left a comment

Choose a reason for hiding this comment

Uh oh!

vasild commented Nov 12, 2022

Uh oh!

naumenkogs commented Nov 14, 2022

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

ariard left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ariard Nov 16, 2022

Choose a reason for hiding this comment

Uh oh!

mzumsande left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

naumenkogs commented Oct 21, 2022 •

edited by maflcko

Loading

DrahtBot commented Oct 21, 2022 •

edited

Loading

ariard left a comment •

edited

Loading