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 2b9cd5f7b6c0cba03da7b234eb638296eeb05cfa

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

            if (fRelay) pfrom.m_relays_txs = true;
        }

        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 1597210a6d244927651b6a4b573b884cc6f9236c

src/net_processing.cpp

mzumsande

Code Review ACK 58786a102ae716332c2d56a00a10726fb7e305f1

src/net_processing.cpp

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

src/net_processing.cpp

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

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 d44438c7

Note the new commit d44438c reodering functional tests for readability.

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

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.

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

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

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 Outdated 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 Outdated 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

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!

Reviewers

naumenkogs commented Oct 21, 2022 •

edited by maflcko

Loading

DrahtBot commented Oct 21, 2022 •

edited

Loading

ariard left a comment •

edited

Loading