p2p: Erlay support signaling #23443

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, `4dc7bac`)	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

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

@@ -484,6 +485,7 @@ void SetupServerArgs(ArgsManager& argsman)
    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 Show resolved Hide resolved

naumenkogs added 7 commits October 17, 2022 12:35

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

naumenkogs force-pushed the 2021-11-erlay1 branch from e91690e to e56d1d2 Compare October 17, 2022 09:38

DrahtBot removed the Needs rebase label Oct 17, 2022

vasild approved these changes Oct 17, 2022

View reviewed changes

glozow merged commit e7a0e96 into bitcoin:master Oct 17, 2022

maflcko reviewed Oct 18, 2022

View reviewed changes

dergoegge mentioned this pull request Oct 21, 2022

Intermitted failure in p2p_sendtxrcncl.py #26364

Closed

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Oct 23, 2022

Merge bitcoin#23443: p2p: Erlay support signaling

233608b

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

bitcoin locked and limited conversation to collaborators Oct 25, 2023

p2p: Erlay support signaling #23443

p2p: Erlay support signaling #23443

Uh oh!

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!

naumenkogs commented Nov 5, 2021 •

edited

Loading

DrahtBot commented Nov 5, 2021 •

edited by maflcko

Loading

stickies-v left a comment •

edited

Loading

sipa commented Oct 17, 2022 •

edited

Loading

maflcko commented Oct 17, 2022 •

edited

Loading