p2p: Erlay support signaling by naumenkogs · Pull Request #23443 · bitcoin/bitcoin

naumenkogs · 2021-11-05T12:51:10Z

This is a part of the Erlay project:

This PR adds a new p2p message sendtxrcncl signaling for reconciliation support.
Before sending that message, a node is supposed to "pre-register" the peer by generating and storing an associated reconciliation salt component.
Once the salts are exchanged within this new message, nodes "register" each other for future reconciliations by computing and storing the aggregate salt, along with the reconciliation parameters based on the connection direction.

DrahtBot · 2021-11-05T20:10:58Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#26151 (refactor: Guard TxRequestTracker by its own lock instead of cs_main by dergoegge)
#25957 (wallet: fast rescan with BIP157 block filters for descriptor wallets by theStack)
#25572 (refactor: Introduce EvictionManager and use it for the inbound eviction logic by dergoegge)
#24545 (BIP324: Enable v2 P2P encrypted transport by dhruv)
#23233 (BIP324: Add encrypted p2p transport {de}serializer by dhruv)

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.

Coverage

Coverage	Change (pull 23443, 4dc7bac65e41a1f2bbbae084b8bbaeee70cec700)	Reference (master, `ed4eeaf`)
Lines	+0.0276 %	83.7655 %
Functions	+0.0294 %	80.7326 %
Branches	+0.0133 %	51.3151 %

^{Updated at: 2022-05-10T14:16:22.523123.}

ariard

Reviewed until 85992f3. So far only minor comments.

src/net_processing.cpp

src/protocol.h

src/txreconciliation.cpp

src/net_processing.cpp

src/txreconciliation.cpp

ariard

Finished a first parse, including verifying unit test coverage of the new TxReconciliationTracker class. LGTM

src/test/txreconciliation_tests.cpp

rebroad · 2021-11-25T20:12:28Z

txreconciliation.cpp:43:20: warning: private field 'm_k0' is not used [-Wunused-private-field]
    const uint64_t m_k0, m_k1;
                   ^
txreconciliation.cpp:43:26: warning: private field 'm_k1' is not used [-Wunused-private-field]
    const uint64_t m_k0, m_k1;

maflcko

left some nits

src/Makefile.am

src/protocol.h

src/test/txreconciliation_tests.cpp

maflcko · 2021-11-29T15:24:53Z

Run process_message with args ['/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz', '-runs=1', '/tmp/cirrus-ci-build/ci/scratch/qa-assets/fuzz_seed_corpus/process_message']fuzz: test/fuzz/process_message.cpp:57: auto initialize_process_message()::(anonymous class)::operator()() const: Assertion "GetNumMsgTypes() == getAllNetMessageTypes().size()" && check' failed.

src/net_processing.cpp

mzumsande

Did you think about a -disablerecon command or something similar, to not activate Erlay until all the parts have been merged?
Providing the opportunity to opt in/out would probably make sense anyway, but it would also make the review process of its parts independent from releases - e.g. I also don't think it makes sense to have nodes running master exchange SENDRECON before Erlay has been fully merged.

src/node/txreconciliation.cpp

src/node/txreconciliation.h

naumenkogs · 2021-12-07T10:39:02Z

Did you think about a -disablerecon command or something similar, to not activate Erlay until all the parts have been merged?

Yeah I was also thinking about how it's weird to send SENDRECON messages while they don't mean anything, but couldn't come up with any good solution.
For starters, I could comment out sending the message at least I guess.

I guess a flag is better if someone wants to test this stuff via functional tests (without modifying the source code), so yeah, that's probably an ultimate solution. I'll add a commit for that.

glozow

Concept ACK, thanks for splitting it up. I did a first pass, mostly comparing to the BIP330 specification, looking at test coverage, and a little bit of implementation.

src/node/txreconciliation.h

src/net_processing.cpp

src/node/txreconciliation.cpp

src/node/txreconciliation.h

src/node/txreconciliation.cpp

src/test/txreconciliation_tests.cpp

test/functional/test_framework/messages.py

src/net_processing.cpp

GeneFerneau · 2021-12-07T20:38:43Z

utACK ea3b87a

Code review, everything looks good to me outside nits raised by others.

Will run the fuzzer for some cycles, and report back results.

stickies-v

Approach ACK, code review until 103e52d - didn't look at tests yet.

This PR introduces the logic to register newly connected peers as peers that support transaction reconciliation, and initialize the necessary data structures required to do the reconciliation later on.

I think everything is well laid out and implemented, so most of my comments are quite nitty and focus on readability etc. They're subjective, feel free to ignore.

Some general thoughts/suggestions:

"SENDRECON" does not hint that it corresponds to transactions, would it make sense to rename this to "SENDTXRECON" so people can quickly understand what it's about without know the Erlay code?
I understand you're renaming the two roles to "initiator" & "responder", there are still quite a few instances of "requestor", "sender", "receiver" throughout the code and in BIP300. Would suggest to replace them all in both documentation and variable names, consistent naming makes it much easier to follow.

src/node/txreconciliation.h

src/node/txreconciliation.cpp

jnewbery

Did you think about a -disablerecon command or something similar, to not activate Erlay until all the parts have been merged?

I agree that this should be added, although to bikeshed I'd call it -erlay and default to false.

src/node/txreconciliation.h

src/node/txreconciliation.cpp

src/node/txreconciliation.h

src/net_processing.cpp

src/node/txreconciliation.cpp

src/net_processing.cpp

test/functional/test_framework/messages.py

naumenkogs · 2021-12-10T14:14:56Z

Still going through comments, beware it's not the final shape from my side.

src/test/txreconciliation_tests.cpp

vasild

ACK e91690e

Some non showstoppers below.

vasild · 2022-10-07T09:00:42Z

src/init.cpp

    argsman.AddArg("-seednode=<ip>", "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=<n>", 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);


nit: "per BIP 330" looks too opaque for this help text, it is too long and technical to read the BIP itself in order to understand what this option is about.

Suggested change

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("-txreconciliation", strprintf("Enable transaction reconciliations for more efficient transaction propagation (default: %d)", DEFAULT_TXRECONCILIATION_ENABLE), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::CONNECTION);

There are other BIP xyz mentions in the help texts, feel free to ignore.

vasild · 2022-10-07T09:43:56Z

src/node/txreconciliation.cpp

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


You can remove this assert() from above:

assert(m_states.find(peer_id) == m_states.end());

They both check for the same thing. That way we will do one lookup instead of two, maybe insignificant wrt performance but will be less code.

PS why use assert() in one place and Assert() in another? Did you intend to use Assume()?

vasild · 2022-10-07T10:32:04Z

src/node/txreconciliation.cpp

+class TxReconciliationTracker::Impl
+{
+private:
+    mutable Mutex m_txreconciliation_mutex;


nit: "txreconciliation" is already in the name of the class TxReconciliationTracker. This can be just m_mutex.

For example, another member is called m_states, not m_txreconciliation_states.

This was the initial version, but then @jonatack asked for the more verbose name, and I didn't care much... Maybe he has to say something.

The rationale is that m_txreconciliation_mutex returns more useful results than m_mutex when git grepping the codebase, and the more specific mutex naming seems to be a convention in the codebase as much as the generic naming.

vasild · 2022-10-07T11:01:45Z

src/net_processing.cpp

+            return;
+        }
+
+        if (!peer->GetTxRelay()) {


Yeah, the code and the comments disagree:

if (!peer->GetTxRelay()) { // Disconnect peers that send a SENDTXRCNCL message even though we indicated we don't // support transaction relay.

We set txrelay (thus GetTxRelay() will be true) on this condition:

!pfrom.IsBlockOnlyConn() && (fRelay || (peer->m_our_services & NODE_BLOOM)) // fRelay comes from the peer

but we indicate transaction relay support on this condition:

!m_ignore_incoming_txs && !pnode.IsBlockOnlyConn() && !pnode.IsFeelerConn();

They are not the same.

Also noted by @jonatack here.

vasild · 2022-10-07T11:08:51Z

src/node/txreconciliation.cpp

+        if (recon_state == m_states.end()) return NOT_FOUND;
+        uint64_t* local_salt = std::get_if<uint64_t>(&recon_state->second);
+        // A peer is already registered. This should be checked by the caller.
+        Assume(local_salt);


This should be assert() since we dereference local_salt below. It will crash anyway if it is nullptr. Better crash with a meaningful message produced by the assert().

vasild · 2022-10-07T11:20:43Z

src/test/txreconciliation_tests.cpp

+
+BOOST_AUTO_TEST_CASE(RegisterPeerTest)
+{
+    TxReconciliationTracker tracker(1);


Use TXRECONCILIATION_VERSION instead of 1?

vasild · 2022-10-07T11:31:48Z

src/test/txreconciliation_tests.cpp

+    tracker.PreRegisterPeer(0);
+
+    // Both roles are false, don't register.
+    BOOST_CHECK(tracker.RegisterPeer(/*peer_id=*/0, /*is_peer_inbound=*/true,


Here and elsewhere, BOOST_CHECK_EQUAL(x, y) is preferred over BOOST_CHECK(x == y) because the former gives a better message should the check fail.

vasild · 2022-10-10T08:45:42Z

In the OP:

the link to BIP 330 can be updated and this can be removed: "Note this is not what's in the bitcoin/bips repo, but an updated version".
s/sendrecon/sendtxrcncl

If we're connecting to the peer which might support transaction reconciliation, we announce we want to reconcile with them. We store the reconciliation salt so that when the peer responds with their salt, we are able to compute the full reconciliation salt. This behavior is enabled with a CLI flag.

Once we received a reconciliation announcement support message from a peer and it doesn't violate our protocol, we store the negotiated parameters which will be used for future reconciliations.

We optimistically pre-register a peer for txreconciliations upon sending txreconciliation support announcement. But if, at VERACK, we realize that the peer never sent WTXIDRELAY message, we should unregister the peer from txreconciliations, because txreconciliations rely on wtxids.

glozow · 2022-10-17T10:41:38Z

Looks like rebased for #25667.
Pinging @sipa @dergoegge @vasild @ariard @mzumsande for re-ACK

vasild

ACK e56d1d2

vasild · 2022-10-17T11:48:07Z

The newly added test p2p_sendtxrcncl.py failed just on "Win64 native" 😭

dergoegge · 2022-10-17T13:45:18Z

re-ACK e56d1d2

sipa · 2022-10-17T16:55:19Z

re-ACK e56d1d2. No differences with a rebase of previously reviewed e91690e.

glozow · 2022-10-17T17:01:34Z

The newly added test p2p_sendtxrcncl.py failed just on "Win64 native" sob

Job: https://cirrus-ci.com/task/6088863331385344
Reran and it turned green. Maybe coincidental timeout?

maflcko · 2022-10-17T17:04:49Z

The reason was likely

node0 2022-10-17T10:37:19.451252Z [C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\src\net_processing.cpp:3460] [ProcessMessage] [net] wtxidrelay received after verack from peer=8; disconnecting

https://cirrus-ci.com/task/6088863331385344?logs=functional_tests#L605

However, I can't see how this could possibly happen in the framework. (peer8 is a python node)

mzumsande · 2022-10-17T17:06:07Z

re-ACK e56d1d2

naumenkogs · 2022-10-17T19:00:33Z

Thank you all for making this happen! I will create a PR for fixups, and finish the second PR shortly.

ariard · 2022-10-17T23:16:24Z

Post-Merge ACK e91690e

maflcko · 2022-10-18T06:21:19Z

src/net_processing.cpp

+            // - we intended to exchange transactions over this connection while establishing it
+            //   and the peer indicated support for transaction relay in the VERSION message;
+            // - we are not in -blocksonly mode.
+            if (pfrom.m_relays_txs && !m_ignore_incoming_txs) {


Is this going to send SENDTXRCNCL to feeler connections?

maflcko · 2022-10-18T12:09:02Z

src/init.cpp

    argsman.AddArg("-seednode=<ip>", "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=<n>", 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);


Probably unrelated, but it might be good to split the connection section into net and net_processing options. (This one would go into net_processing)

DrahtBot added Build system P2P labels Nov 5, 2021

naumenkogs force-pushed the 2021-11-erlay1 branch from 2fbed48 to 20c4769 Compare November 5, 2021 17:22

This was referenced Nov 5, 2021

BIP324: Add encrypted p2p transport {de}serializer #23233

Closed

Erlay: bandwidth-efficient transaction relay protocol #21515

Closed

fanquake removed the Build system label Nov 8, 2021

naumenkogs force-pushed the 2021-11-erlay1 branch 2 times, most recently from 6c2b134 to 90cd7a9 Compare November 11, 2021 06:33

naumenkogs force-pushed the 2021-11-erlay1 branch from 90cd7a9 to c866831 Compare November 17, 2021 14:36

ariard reviewed Nov 23, 2021

View reviewed changes

naumenkogs force-pushed the 2021-11-erlay1 branch from c866831 to 5ba375e Compare November 23, 2021 11:31

ariard reviewed Nov 24, 2021

View reviewed changes

src/test/txreconciliation_tests.cpp Outdated Show resolved Hide resolved

naumenkogs force-pushed the 2021-11-erlay1 branch from 5ba375e to 6a79aae Compare November 29, 2021 07:51

maflcko reviewed Nov 29, 2021

View reviewed changes

src/Makefile.am Outdated Show resolved Hide resolved

src/protocol.h Outdated Show resolved Hide resolved

src/test/txreconciliation_tests.cpp Outdated Show resolved Hide resolved

mzumsande reviewed Nov 30, 2021

View reviewed changes

src/net_processing.cpp Outdated Show resolved Hide resolved

naumenkogs force-pushed the 2021-11-erlay1 branch 2 times, most recently from cdaabda to ea3b87a Compare December 2, 2021 11:38

mzumsande reviewed Dec 6, 2021

View reviewed changes

src/node/txreconciliation.cpp Outdated Show resolved Hide resolved

src/node/txreconciliation.h Outdated Show resolved Hide resolved

glozow reviewed Dec 7, 2021

View reviewed changes

stickies-v reviewed Dec 8, 2021

View reviewed changes

jnewbery reviewed Dec 8, 2021

View reviewed changes

naumenkogs force-pushed the 2021-11-erlay1 branch 2 times, most recently from 5136f50 to 07b9357 Compare December 10, 2021 14:14

maflcko reviewed Dec 10, 2021

View reviewed changes

src/test/txreconciliation_tests.cpp Outdated Show resolved Hide resolved

vasild approved these changes Oct 7, 2022

View reviewed changes

naumenkogs mentioned this pull request Oct 8, 2022

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

Closed

naumenkogs added 9 commits October 17, 2022 12:00

log: Add tx reconciliation log category

24e36fa

p2p: Forget peer's reconciliation state on disconnect

4470acf

Add helper to see if a peer is registered for reconciliations

36cf6bf

p2p: Finish negotiating reconciliation support

88d326c

Once we received a reconciliation announcement support message from a peer and it doesn't violate our protocol, we store the negotiated parameters which will be used for future reconciliations.

test: Add unit tests for reconciliation negotiation

b99ee9d

test: Add functional tests for sendtxrcncl from inbound

cfcef60

test: Add functional tests for sendtxrcncl message from outbound

e56d1d2

vasild approved these changes Oct 17, 2022

View reviewed changes

maflcko reviewed Oct 18, 2022

View reviewed changes

dergoegge mentioned this pull request Oct 21, 2022

Intermitted failure in p2p_sendtxrcncl.py #26364

Closed

DrahtBot mentioned this pull request Oct 24, 2022

test: Fix intermittent issue in p2p_sendtxrcncl.py #26381

Merged

This was referenced Nov 4, 2022

test: fix intermittent failure in p2p_sendtxrcncl.py #26448

Merged

scripted diff: fix E275 lint #26468

Closed

Sjors mentioned this pull request Nov 29, 2022

Erlay status in getpeerinfo #26602

Closed

naumenkogs mentioned this pull request Oct 13, 2023

Erlay Project Tracking #28646

Closed

17 tasks

	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("-txreconciliation", strprintf("Enable transaction reconciliations for more efficient transaction propagation (default: %d)", DEFAULT_TXRECONCILIATION_ENABLE), ArgsManager::ALLOW_ANY \| ArgsManager::DEBUG_ONLY, OptionsCategory::CONNECTION);

Conversation

naumenkogs commented Nov 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Nov 5, 2021 • edited by maflcko Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Coverage

Uh oh!

ariard 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!

Uh oh!

Uh oh!

ariard left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rebroad commented Nov 25, 2021

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maflcko commented Nov 29, 2021

Uh oh!

Uh oh!

mzumsande left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

naumenkogs commented Dec 7, 2021

Uh oh!

glozow 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!

Uh oh!

Uh oh!

GeneFerneau commented Dec 7, 2021

Uh oh!

stickies-v 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

naumenkogs commented Nov 5, 2021 •

edited

Loading

DrahtBot commented Nov 5, 2021 •

edited by maflcko

Loading

stickies-v left a comment •

edited

Loading

vasild Oct 7, 2022 •

edited

Loading

jonatack Oct 20, 2022 •

edited

Loading

sipa commented Oct 17, 2022 •

edited

Loading

maflcko commented Oct 17, 2022 •

edited

Loading