p2p: Use legacy relaying to download blocks in blocks-only mode#22340
p2p: Use legacy relaying to download blocks in blocks-only mode#22340dergoegge wants to merge 5 commits intobitcoin:masterfrom
Conversation
|
Over the last 24h i ran a blocks-only node that saw 106 blocks and downloaded 3.8MB in |
|
@jnewbery @amitiuttarwar @sipa @ajtowns Any thoughts? |
|
Seems workable? Compact blocks should be seeing:
Which should be ~38kB per block or about 5MB per day/144 blocks. Legacy block relay should just be:
So this makes sense to me. |
jnewbery
left a comment
There was a problem hiding this comment.
Concept ACK. Requesting HB mode or sending GETDATA(MSG_CMPCT_BLOCK) is wasteful (for both sender and reciever) if the node can't reconstruct compact blocks.
e599338 to
6dfee13
Compare
|
utACK 6dfee13 |
|
Is this true for transactions received via |
@naumenkogs yes this PR would disable compact blocks even if txs were received through [1] Technically it would be possible for a blocks-only node to ask for compact blocks if its mempool size (significantly) exceeds the overhead of compact blocks. Do you think adding [1] or [2] to this PR would be worth it? |
|
I think [2] is better than [1]. Whether we want to add a new config option for this use case or just implement what you suggested originally — my personal choice would be to ask for thoughts at the #bitcoin-core-dev meeting. |
I agree that this seems like an edge case that wouldn't actually be used. Compact blocks exist to improve propagation speed/efficiency when a node is participating in tx relay. I'd prefer not to add a configuration option that we don't expect to be used, or to increase the complexity of this PR. |
To be clear, my worry is that some miners use the following setting: I don't have any evidence of someone using this, just wanted to point out we are going to break this use case here, so I thought asking around makes sense. |
|
@naumenkogs That seems very unlikely to me. |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, 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. |
|
Concept ACK |
|
utACK. Perhaps a new test case in |
|
utACK 6dfee13 |
rajarshimaitra
left a comment
There was a problem hiding this comment.
Concept ACK.
As others have mentioned a functional test covering this behaviour would be nice.
I just have the following nits.
307e220 to
217a80d
Compare
jnewbery
left a comment
There was a problem hiding this comment.
A few small suggestions on the new test.
217a80d to
8c4159a
Compare
|
Thanks for the review @jnewbery! I took all your suggestions. I also added two new test cases: |
|
Started looking at this PR. Rebased to current master, debug build is clean, test output
|
|
@jonatack that's odd. I tried rebasing on master & the tests are passing 🤔 And based on the logs you posted, I don't see why rebasing would cause a difference. The test file introduced is new, and I don't believe anything has changed recently around compact block p2p messages. Do you have any more info you can offer? |
|
It's late but I'll try again tomorrow (and do a review, looks like an interesting PR). I rebased and did a debug build with clang 13 on debian before running the test. Maybe it was spurious. |
|
Ok, the test runs fine. Here is what happened: I forgot about the footgun on master that currently doesn't build when DEBUG_ADDRMAN is defined. I built with my bash alias that does a debug build with that, the build failed, I didn't notice, and so I was running the test on the wrong build. I just verified that the failure I reported occurs on master. Apologies for the noise! Will review tomorrow. |
There was a problem hiding this comment.
Code review re-ACK ae54485 per git diff e21f2e4 ae54485, only changes since last review are minor (mostly documentation) improvements in the functional test; rebased to master, debug build clean, ran test a few times, including with vagrind
test/functional/p2p_compactblocks_blocksonly.py --valgrind failed for me locally on the first run (AssertionError: [node 0] Unable to connect to bitcoind after 60s) during test setup, but succeeded on the second and third runs.
Thanks for updating!
Your mileage may vary. In review feedback a year ago it was suggested to me to not worry too much about invalidating ACKs and I ended up "invalidating" 7 ACKs on that pull, which was subsequently merged. Since then I no longer worry about it and AFAICT doing so doesn't seem to have slowed anything down. (Sharing a thought in general, not for this case!) |
|
ACK ae54485 |
There was a problem hiding this comment.
tACK ae54485
Stepped through the test, verified expected behaviors.
Below are few comments inline, nothing blocking.
Aside
Taking this opportunity to mention some generic approaches I took for functional test reviews. Could be helpful for new review club folks:
- Start python debugging and step through the *.py file.
- The output will show at the start of the test, the temp directory where regtest nodes are created. - You can talk to them while you pause the python file, with
bitcoin-cli -datadir=/tmp-filepath/node0/ <command>. - You can
tail -ftheirdebug.logand only activate the required logging(net, rpc, etc depending on the test you are trying) usingbitcoin-cli, to see communications happening in each nodes. Verify they are occurring as expected. - If you want to print something from src/*.cpp, use
LogPrintf()to print in thedebug.logfile, that you were already tailing. - Use python debug console to work with the python objects, call functions, see object attributes are updated as expected.
- Try to identify edge cases by changing test variables and observing failures.
ae54485 to
f30eb1f
Compare
|
Thanks @rajarshimaitra and @jonatack for the reviews and advice!
git range-diff nocmpct_blocksonly_2...nocmpct_blocksonly_3 |
|
ACK f30eb1f |
f30eb1f to
c24c52d
Compare
|
git range-diff nocmpct_blocksonly_3...nocmpct_blocksonly_4 |
|
Code review ACK c24c52d |
|
ACK c24c52d |
Co-authored-by: Amiti Uttarwar <[email protected]>
…w bandwidth connection. Co-authored-by: Amiti Uttarwar <[email protected]>
…h connections. Co-authored-by: Amiti Uttarwar <[email protected]>
c24c52d to
18c5b23
Compare
|
Rebased on latest master. @jnewbery @theStack @jonatack @naumenkogs @rajarshimaitra I would appreciate it if you folks could have another look and re-ACK since i think this is pretty close to being ready :) |
|
ACK 18c5b23 |
|
tACK 18c5b23 |
|
reACK 18c5b23 Did the rebase myself and verified the same result. |
theStack
left a comment
There was a problem hiding this comment.
re-ACK 18c5b23 🥛
After my previous ACK (on a single commit with the changes in net_processing), the PR has been rebased and tests have been added. Those LGTM, the only place I found confusing is the assertion checking that the number of received sendcmpct messages on a peer from node after handshake is 2 initially, and not 1. I had to dig in the code to find out why:
bitcoin/src/net_processing.cpp
Lines 2721 to 2730 in 571bb94
Not a big deal, but if a follow-up PR is planned, probably a small comment could be added to the test explaining why 2 messages are expected (sendcmpct(0, 2) and sendcmpct(0, 1)).
…cks in blocks-only mode 18c5b23 [test] Test that -blocksonly nodes still serve compact blocks. (Niklas Gögge) a79ad65 [test] Test that getdata(CMPCT) is still sent on regular low bandwidth connections. (Niklas Gögge) 5e231c1 [test] Test that -blocksonly nodes do not send getdata(CMPCT) on a low bandwidth connection. (Niklas Gögge) 5bf6587 [test] Test that -blocksonly nodes do not request high bandwidth mode. (Niklas Gögge) 0dc8bf5 [net processing] Dont request compact blocks in blocks-only mode (Niklas Gögge) Pull request description: A blocks-only node does not participate in transaction relay to reduce its own bandwidth usage and therefore does not have a mempool. The use of compact blocks is not beneficial to such a node since it will always have to download full blocks. In both high- and low-bandwidth relaying the `cmpctblock` message is sent. This represent a bandwidth overhead for blocks-only nodes because the `cmpctblock` message is several times larger in the average case than the equivalent `headers` or `inv` announcement.  >**Example:** >A block with 2000 txs results in a `cmpctblock` with 2000*6 bytes in short ids. This is several times larger than the equivalent 82 bytes for a `headers` message or 37 bytes for an `inv`. ## Approach This PR makes blocks-only nodes always use the legacy relaying to download new blocks. It does so by making blocks-only nodes never initiate a high-bandwidth block relay connection by disabling the sending of `sendcmpct(1)`. Additionally a blocks-only node will never request a compact block using `getdata(CMPCT)`. A blocks-only node will continue to serve compact blocks to its peers in both high- and low-bandwidth mode. ACKs for top commit: naumenkogs: ACK 18c5b23 rajarshimaitra: tACK bitcoin/bitcoin@18c5b23 jnewbery: reACK 18c5b23 theStack: re-ACK 18c5b23 🥛 Tree-SHA512: 0c78804aa397513d41f97fe314efb815efcd852d452dd903df9d4749280cd3faaa010fa9b51d7d5168b8a77e08c8ab0a491ecdbdb3202f2e9cd5137cddc74624
|
This has been merged. |
A blocks-only node does not participate in transaction relay to reduce its own bandwidth usage and therefore does not have a mempool. The use of compact blocks is not beneficial to such a node since it will always have to download full blocks.
In both high- and low-bandwidth relaying the
cmpctblockmessage is sent. This represent a bandwidth overhead for blocks-only nodes because thecmpctblockmessage is several times larger in the average case than the equivalentheadersorinvannouncement.Approach
This PR makes blocks-only nodes always use the legacy relaying to download new blocks.
It does so by making blocks-only nodes never initiate a high-bandwidth block relay connection by disabling the sending of
sendcmpct(1). Additionally a blocks-only node will never request a compact block usinggetdata(CMPCT).A blocks-only node will continue to serve compact blocks to its peers in both high- and low-bandwidth mode.