p2p: Use legacy relaying to download blocks in blocks-only mode #22340

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.

Over the last 24h i ran a blocks-only node that saw 106 blocks and downloaded 3.8MB in cmpctblock messages. Thats an average of 12KB bytes per cmpctblock message received from three high-bandwidth peers. Only 26KB would have been downloaded if headers messages would have been used instead. Assuming an average block size of 1.38MB the cmpctblock messages represented 2.5% of downloaded block data.

@jnewbery @amitiuttarwar @sipa @ajtowns Any thoughts?

Seems workable? Compact blocks should be seeing:

• 12kB for short tx ids per cmpctblock message announcing a new block (for each high bw peer per block)
• 2kB per getblocktxn message asking for the tx data (per block)
• ~0 overhead for blocktxn message providing the block txns

Which should be ~38kB per block or about 5MB per day/144 blocks. Legacy block relay should just be:

• ~0 overhead for headers message
• ~0 overhead for GETDATA MSG_BLOCK request
• ~0 ovehead for BLOCK message

So this makes sense to me.

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.

utACK 6dfee13f650521f7542df0926aff01af9ac6a328

Is this true for transactions received via sendrawtransaction? What if some node, for whatever reason, receives all transactions, while having blocks-only mode? For them, compact blocks are going to be beneficial, but this PR will disable it?

For them, compact blocks are going to be beneficial, but this PR will disable it? @naumenkogs yes this PR would disable compact blocks even if txs were received through sendrawtransaction and i think overall that's ok since that seems like an edge case to me. I would presume most blocks-only nodes don't submit enough txs through sendrawtransaction for this to matter, especially not continuously so that most blocks contain many of those txs. Or if a node is broadcasting that many txs it might not care about the additional bandwidth?

[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. [2] A config option enabling compact blocks for those individual blocks-only nodes that heavily use sendrawtransaction could also help.

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.

yes this PR would disable compact blocks even if txs were received through sendrawtransaction and i think overall that's ok since that seems like an edge case to me.

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.

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: A node in blocks-only mode with low-latency requirements (hence mining) is constantly fed with transactions via sendrawtransaction, for whatever reason (e.g., they consider p2p tx relay insecure and disable it).

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.

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #20799 (net processing: Only support version 2 compact blocks by jnewbery)

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 p2p_compactblocks.py or p2p_compactblocks_hb.py might be a good addition here, or in a follow-up? This changes behaviour but doesn't fail any tests.

utACK 6dfee13f650521f7542df0926aff01af9ac6a328 but (as others have said) a test would be nice

Concept ACK.

As others have mentioned a functional test covering this behaviour would be nice.

I just have the following nits.

utACK 6dfee13f650521f7542df0926aff01af9ac6a328

A few small suggestions on the new test.

Thanks for the review @jnewbery! I took all your suggestions.

I also added two new test cases: One to test that on regular low bandwidth connections (no blocksonly nodes involved) getdata(CMPCT) is still sent. 8b1b308555382ea1b7390615371ecaa066c4af3b And one to test that blocksonly nodes still serve compact blocks in hb and lb mode. 8c4159aeccdad58a71ff61b3e21b26a4f24a6029

utACK 8c4159aeccdad58a71ff61b3e21b26a4f24a6029

Thanks for being so responsive to review. I have a few more comments for your consideration inline. Nothing blocking.

Started looking at this PR. Rebased to current master, debug build is clean, <strike>but the new test times out for me locally on every run (out of five).</strike>

<details><summary>test output</summary><p>

$ test/functional/p2p_compactblocks_blocksonly.py 
2021-08-10T15:36:10.388000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_ypwy_8d2
2021-08-10T15:36:11.582000Z TestFramework (INFO): Test that blocksonly nodes do not request high bandwidth mode.
2021-08-10T15:37:11.677000Z TestFramework.utils (ERROR): wait_until() failed. Predicate: ''''
        def test_function():
            if check_connected:
                assert self.is_connected
            return test_function_in()
'''
2021-08-10T15:37:11.677000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
  File "/home/jon/projects/bitcoin/bitcoin/test/functional/test_framework/test_framework.py", line 131, in main
    self.run_test()
  File "/home/jon/projects/bitcoin/bitcoin/test/functional/p2p_compactblocks_blocksonly.py", line 79, in run_test
    p2p_conn_blocksonly.wait_until(lambda: p2p_conn_blocksonly.message_count['sendcmpct'] == 2)
  File "/home/jon/projects/bitcoin/bitcoin/test/functional/test_framework/p2p.py", line 451, in wait_until
    wait_until_helper(test_function, timeout=timeout, lock=p2p_lock, timeout_factor=self.timeout_factor)
  File "/home/jon/projects/bitcoin/bitcoin/test/functional/test_framework/util.py", line 256, in wait_until_helper
    raise AssertionError("Predicate {} not true after {} seconds".format(predicate_source, timeout))
AssertionError: Predicate ''''
        def test_function():
            if check_connected:
                assert self.is_connected
            return test_function_in()
''' not true after 60.0 seconds
2021-08-10T15:37:11.729000Z TestFramework (INFO): Stopping nodes
2021-08-10T15:37:11.891000Z TestFramework (WARNING): Not cleaning up dir /tmp/bitcoin_func_test_ypwy_8d2
2021-08-10T15:37:11.892000Z TestFramework (ERROR): Test failed. Test logging available at /tmp/bitcoin_func_test_ypwy_8d2/test_framework.log
2021-08-10T15:37:11.894000Z TestFramework (ERROR): 
2021-08-10T15:37:11.896000Z TestFramework (ERROR): Hint: Call /home/jon/projects/bitcoin/bitcoin/test/functional/combine_logs.py '/tmp/bitcoin_func_test_ypwy_8d2' to consolidate all logs
2021-08-10T15:37:11.896000Z TestFramework (ERROR): 
2021-08-10T15:37:11.897000Z TestFramework (ERROR): If this failure happened unexpectedly or intermittently, please file a bug and provide a link or upload of the combined log.
2021-08-10T15:37:11.897000Z TestFramework (ERROR): https://github.com/bitcoin/bitcoin/issues
2021-08-10T15:37:11.897000Z TestFramework (ERROR): 

</p></details>

@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.

@jnewbery great suggestions! Incorporated them all again.

nocmpct_blocksonly_0 -> nocmpct_blocksonly_1

<details> <summary>git range-diff nocmpct_blocksonly_0...nocmpct_blocksonly_1</summary>

1:  5a29943a9 ! 1:  e58cfb118 [test] Test that blocksonly nodes do not request high bandwidth mode.
    @@ test/functional/p2p_compactblocks_blocksonly.py (new)
     +
     +    def run_test(self):
     +        # Nodes will only request hb compact blocks mode when they're out of IBD
    -+        for i in range(3):
    -+            assert not self.nodes[i].getblockchaininfo()['initialblockdownload']
    ++        for node in self.nodes:
    ++            assert not node.getblockchaininfo()['initialblockdownload']
     +
     +        p2p_conn_blocksonly = self.nodes[0].add_p2p_connection(P2PInterface())
     +        p2p_conn_high_bw = self.nodes[1].add_p2p_connection(P2PInterface())
    -+        assert_equal(p2p_conn_blocksonly.message_count['sendcmpct'], 2)
    -+        assert_equal(p2p_conn_high_bw.message_count['sendcmpct'], 2)
    -+        p2p_conn_blocksonly.send_and_ping(msg_sendcmpct(announce=False, version=2))
    -+        p2p_conn_high_bw.send_and_ping(msg_sendcmpct(announce=False, version=2))
    ++        for conn in [p2p_conn_blocksonly, p2p_conn_high_bw]:
    ++            assert_equal(conn.message_count['sendcmpct'], 2)
    ++            conn.send_and_ping(msg_sendcmpct(announce=False, version=2))
     +
     +        # Nodes:
     +        #   0 -> blocksonly
    @@ test/functional/p2p_compactblocks_blocksonly.py (new)
     +        #   p2p_conn_high_bw    ---> node1
     +        #   node2 (no connections)
     +        #
    -+        # node2 produces blocks which get passed to the reset of the nodes
    ++        # node2 produces blocks which get passed to the rest of the nodes
     +        # through the respective p2p connections.
     +
     +        self.log.info("Test that blocksonly nodes do not request high bandwidth mode.")
2:  beeea3fbc = 2:  c99978d94 [test] Test that blocksonly nodes do not send getdata(CMPCT) on a low bandwidth connection.
3:  8b1b30855 ! 3:  770c5f1a4 [test] Test that getdata(CMPCT) is still sent on regular low bandwdith connections.
    @@ test/functional/p2p_compactblocks_blocksonly.py: class P2PCompactBlocksBlocksOnl

              p2p_conn_blocksonly = self.nodes[0].add_p2p_connection(P2PInterface())
              p2p_conn_high_bw = self.nodes[1].add_p2p_connection(P2PInterface())
    +-        for conn in [p2p_conn_blocksonly, p2p_conn_high_bw]:
     +        p2p_conn_low_bw = self.nodes[3].add_p2p_connection(P2PInterface())
    -         assert_equal(p2p_conn_blocksonly.message_count['sendcmpct'], 2)
    -         assert_equal(p2p_conn_high_bw.message_count['sendcmpct'], 2)
    -+        assert_equal(p2p_conn_low_bw.message_count['sendcmpct'], 2)
    -         p2p_conn_blocksonly.send_and_ping(msg_sendcmpct(announce=False, version=2))
    -         p2p_conn_high_bw.send_and_ping(msg_sendcmpct(announce=False, version=2))
    -+        p2p_conn_low_bw.send_and_ping(msg_sendcmpct(announce=False, version=2))
    ++        for conn in [p2p_conn_blocksonly, p2p_conn_high_bw, p2p_conn_low_bw]:
    +             assert_equal(conn.message_count['sendcmpct'], 2)
    +             conn.send_and_ping(msg_sendcmpct(announce=False, version=2))

    -         # Nodes:
    +@@ test/functional/p2p_compactblocks_blocksonly.py: class P2PCompactBlocksBlocksOnly(BitcoinTestFramework):
              #   0 -> blocksonly
              #   1 -> high bandwidth
              #   2 -> miner
    @@ test/functional/p2p_compactblocks_blocksonly.py: class P2PCompactBlocksBlocksOnl
     +        #   p2p_conn_low_bw     ---> node3
              #   node2 (no connections)
              #
    -         # node2 produces blocks which get passed to the reset of the nodes
    +         # node2 produces blocks which get passed to the rest of the nodes
     @@ test/functional/p2p_compactblocks_blocksonly.py: class P2PCompactBlocksBlocksOnly(BitcoinTestFramework):
              p2p_conn_high_bw.wait_until(lambda: p2p_conn_high_bw.message_count['sendcmpct'] == 3)
              assert_equal(p2p_conn_high_bw.last_message['sendcmpct'].announce, True)
4:  8c4159aec ! 4:  e21f2e4f0 [test] Test that blocksonly nodes still serve compact blocks.
    @@ test/functional/p2p_compactblocks_blocksonly.py: class P2PCompactBlocksBlocksOnl
     +        self.log.info("Test that blocksonly nodes still serve compact blocks.")
     +
     +        def test_for_cmpctblock(block):
    -+            def test():
    -+                sha256 = p2p_conn_blocksonly.last_message['cmpctblock'].header_and_shortids.header.rehash()
    -+                return sha256 == block.sha256
    -+            return test
    ++            if 'cmpctblock' not in p2p_conn_blocksonly.last_message:
    ++                return False
    ++            return p2p_conn_blocksonly.last_message['cmpctblock'].header_and_shortids.header.rehash() == block.sha256
     +
     +        p2p_conn_blocksonly.send_message(msg_getdata([CInv(MSG_CMPCT_BLOCK, block0.sha256)]))
    -+        p2p_conn_blocksonly.sync_send_with_ping()
    -+        p2p_conn_blocksonly.wait_until(test_for_cmpctblock(block0))
    ++        p2p_conn_blocksonly.wait_until(lambda: test_for_cmpctblock(block0))
     +
     +        # Request high bandwidth mode from node0
     +        p2p_conn_blocksonly.send_and_ping(msg_sendcmpct(announce=True, version=2))
    @@ test/functional/p2p_compactblocks_blocksonly.py: class P2PCompactBlocksBlocksOnl
     +        block2 = self.build_block_on_tip()
     +        self.nodes[0].submitblock(block1.serialize().hex())
     +        self.nodes[0].submitblock(block2.serialize().hex())
    -+        p2p_conn_blocksonly.wait_until(test_for_cmpctblock(block2))
    ++        p2p_conn_blocksonly.wait_until(lambda: test_for_cmpctblock(block2))
     +
      if __name__ == '__main__':
          P2PCompactBlocksBlocksOnly().main()

</details>

utACK e21f2e4f04c5f15eccac529c6e8c59aba5283351

Code review ACK e21f2e4f04c5f15eccac529c6e8c59aba5283351 modulo some minor fixups

Thanks for adding the tests! Verified that they fail as expected on master :)

Consider documenting this change in behavior with a release note?

Feel free to ignore the non-fixup suggestions below. Happy to re-ACK if you update.

Consider also updating doc/reduce-traffic.md regarding the effects of setting -blocksonly (maybe also in the Memory Pool section of doc/reduce-memory.md).

@jonatack Thanks for the review! I took most of your suggestions.

I did not modify 6dfee13f650521f7542df0926aff01af9ac6a328 because i don't want to invalidate the utACKs that this commit has already gotten. I will make a follow up PR for those suggestions as well as the other doc changes (Release note, doc/reduce-traffic.md).

nocmpct_blocksonly_1 -> nocmpct_blocksonly_2

<details> <summary>git range-diff nocmpct_blocksonly_1...nocmpct_blocksonly_2</summary>

1:  e58cfb118 ! 1:  1bd2f889c [test] Test that blocksonly nodes do not request high bandwidth mode.
    @@ Metadata
     Author: Niklas Gögge <n.goeggi@gmail.com>
     
      ## Commit message ##
    -    [test] Test that blocksonly nodes do not request high bandwidth mode.
    +    [test] Test that -blocksonly nodes do not request high bandwidth mode.
     
         Co-authored-by: Amiti Uttarwar <amiti@uttarwar.org>
     
      ## test/functional/p2p_compactblocks_blocksonly.py (new) ##
     @@
     +#!/usr/bin/env python3
    -+# Copyright (c) 2019-2020 The Bitcoin Core developers
    ++# Copyright (c) 2021-2021 The Bitcoin Core developers
     +# Distributed under the MIT software license, see the accompanying
     +# file COPYING or http://www.opensource.org/licenses/mit-license.php.
     +"""Test that a node in blocksonly mode does not request compact blocks."""
    @@ test/functional/p2p_compactblocks_blocksonly.py (new)
     +
     +class P2PCompactBlocksBlocksOnly(BitcoinTestFramework):
     +    def set_test_params(self):
    -+        self.num_nodes = 3
     +        self.extra_args = [["-blocksonly"], [], []]
    ++        self.num_nodes = len(self.extra_args)
     +
     +    def setup_network(self):
     +        self.setup_nodes()
    @@ test/functional/p2p_compactblocks_blocksonly.py (new)
     +        #   p2p_conn_high_bw    ---> node1
     +        #   node2 (no connections)
     +        #
    -+        # node2 produces blocks which get passed to the rest of the nodes
    ++        # node2 produces blocks that are passed to the rest of the nodes
     +        # through the respective p2p connections.
     +
    -+        self.log.info("Test that blocksonly nodes do not request high bandwidth mode.")
    ++        self.log.info("Test that -blocksonly nodes do not select peers for BIP 152 high bandwidth mode.")
     +
     +        block0 = self.build_block_on_tip()
     +
    -+        # A blocksonly node should not request high bandwidth mode upon
    ++        # A -blocksonly node should not request BIP152 high bandwidth mode upon
     +        # receiving a new valid block at the tip.
     +        p2p_conn_blocksonly.send_and_ping(msg_block(block0))
     +        assert_equal(int(self.nodes[0].getbestblockhash(), 16), block0.sha256)
     +        p2p_conn_blocksonly.wait_until(lambda: p2p_conn_blocksonly.message_count['sendcmpct'] == 2)
     +        assert_equal(p2p_conn_blocksonly.last_message['sendcmpct'].announce, False)
     +
    -+        # A normal node participating in transaction relay should request high
    -+        # bandwidth mode upon receiving a new valid block at the tip.
    ++        # A normal node participating in transaction relay should request BIP152
    ++        # high bandwidth mode upon receiving a new valid block at the tip.
     +        p2p_conn_high_bw.send_and_ping(msg_block(block0))
     +        assert_equal(int(self.nodes[1].getbestblockhash(), 16), block0.sha256)
     +        p2p_conn_high_bw.wait_until(lambda: p2p_conn_high_bw.message_count['sendcmpct'] == 3)
2:  c99978d94 ! 2:  abe3c5cf7 [test] Test that blocksonly nodes do not send getdata(CMPCT) on a low bandwidth connection.
    @@ Metadata
     Author: Niklas Gögge <n.goeggi@gmail.com>
     
      ## Commit message ##
    -    [test] Test that blocksonly nodes do not send getdata(CMPCT) on a low bandwidth connection.
    +    [test] Test that -blocksonly nodes do not send getdata(CMPCT) on a low bandwidth connection.
     
         Co-authored-by: Amiti Uttarwar <amiti@uttarwar.org>
     
    @@ test/functional/p2p_compactblocks_blocksonly.py: class P2PCompactBlocksBlocksOnl
              p2p_conn_high_bw.wait_until(lambda: p2p_conn_high_bw.message_count['sendcmpct'] == 3)
              assert_equal(p2p_conn_high_bw.last_message['sendcmpct'].announce, True)
      
    -+        self.log.info("Test that blocksonly nodes send getdata(BLOCK) instead"
    -+                      "of getdata(CMPCT) in low bandwidth mode.")
    ++        self.log.info("Test that -blocksonly nodes send getdata(BLOCK) instead"
    ++                      " of getdata(CMPCT) in BIP152 low bandwidth mode.")
     +
     +        block1 = self.build_block_on_tip()
     +
3:  770c5f1a4 ! 3:  c652e187e [test] Test that getdata(CMPCT) is still sent on regular low bandwdith connections.
    @@ Metadata
     Author: Niklas Gögge <n.goeggi@gmail.com>
     
      ## Commit message ##
    -    [test] Test that getdata(CMPCT) is still sent on regular low bandwdith connections.
    +    [test] Test that getdata(CMPCT) is still sent on regular low bandwidth connections.
     
         Co-authored-by: Amiti Uttarwar <amiti@uttarwar.org>
     
    @@ test/functional/p2p_compactblocks_blocksonly.py: from test_framework.util import
      
      class P2PCompactBlocksBlocksOnly(BitcoinTestFramework):
          def set_test_params(self):
    --        self.num_nodes = 3
     -        self.extra_args = [["-blocksonly"], [], []]
    -+        self.num_nodes = 4
     +        self.extra_args = [["-blocksonly"], [], [], []]
    +         self.num_nodes = len(self.extra_args)
      
          def setup_network(self):
    -         self.setup_nodes()
     @@ test/functional/p2p_compactblocks_blocksonly.py: class P2PCompactBlocksBlocksOnly(BitcoinTestFramework):
      
              p2p_conn_blocksonly = self.nodes[0].add_p2p_connection(P2PInterface())
    @@ test/functional/p2p_compactblocks_blocksonly.py: class P2PCompactBlocksBlocksOnl
     +        #   p2p_conn_low_bw     ---> node3
              #   node2 (no connections)
              #
    -         # node2 produces blocks which get passed to the rest of the nodes
    +         # node2 produces blocks that are passed to the rest of the nodes
     @@ test/functional/p2p_compactblocks_blocksonly.py: class P2PCompactBlocksBlocksOnly(BitcoinTestFramework):
              p2p_conn_high_bw.wait_until(lambda: p2p_conn_high_bw.message_count['sendcmpct'] == 3)
              assert_equal(p2p_conn_high_bw.last_message['sendcmpct'].announce, True)
      
    -+        # Don't send a block from the p2p_conn_low_bw so the bitcoind node
    -+        # doesn't select it for high bw relay.
    ++        # Don't send a block from the p2p_conn_low_bw so the low bandwidth node
    ++        # doesn't select it for BIP152 high bandwidth relay.
     +        self.nodes[3].submitblock(block0.serialize().hex())
     +
    -         self.log.info("Test that blocksonly nodes send getdata(BLOCK) instead"
    -                       "of getdata(CMPCT) in low bandwidth mode.")
    +         self.log.info("Test that -blocksonly nodes send getdata(BLOCK) instead"
    +                       " of getdata(CMPCT) in BIP152 low bandwidth mode.")
      
     @@ test/functional/p2p_compactblocks_blocksonly.py: class P2PCompactBlocksBlocksOnly(BitcoinTestFramework):
              p2p_conn_high_bw.sync_send_with_ping()
              assert_equal(p2p_conn_high_bw.last_message['getdata'].inv, [CInv(MSG_CMPCT_BLOCK, block1.sha256)])
      
    -+        self.log.info("Test that getdata(CMPCT) is still sent on regular low bandwdith connections.")
    ++        self.log.info("Test that getdata(CMPCT) is still sent on BIP152 low bandwidth connections"
    ++                      " when no -blocksonly nodes are involved.")
     +
     +        p2p_conn_low_bw.send_and_ping(msg_headers(headers=[CBlockHeader(block1)]))
     +        p2p_conn_low_bw.sync_with_ping()
4:  e21f2e4f0 ! 4:  ae54485c4 [test] Test that blocksonly nodes still serve compact blocks.
    @@ Metadata
     Author: Niklas Gögge <n.goeggi@gmail.com>
     
      ## Commit message ##
    -    [test] Test that blocksonly nodes still serve compact blocks.
    +    [test] Test that -blocksonly nodes still serve compact blocks.
     
      ## test/functional/p2p_compactblocks_blocksonly.py ##
     @@ test/functional/p2p_compactblocks_blocksonly.py: from test_framework.messages import (
    @@ test/functional/p2p_compactblocks_blocksonly.py: class P2PCompactBlocksBlocksOnl
              p2p_conn_low_bw.sync_with_ping()
              assert_equal(p2p_conn_low_bw.last_message['getdata'].inv, [CInv(MSG_CMPCT_BLOCK, block1.sha256)])
      
    -+        self.log.info("Test that blocksonly nodes still serve compact blocks.")
    ++        self.log.info("Test that -blocksonly nodes still serve compact blocks.")
     +
     +        def test_for_cmpctblock(block):
     +            if 'cmpctblock' not in p2p_conn_blocksonly.last_message:
    @@ test/functional/p2p_compactblocks_blocksonly.py: class P2PCompactBlocksBlocksOnl
     +        p2p_conn_blocksonly.send_message(msg_getdata([CInv(MSG_CMPCT_BLOCK, block0.sha256)]))
     +        p2p_conn_blocksonly.wait_until(lambda: test_for_cmpctblock(block0))
     +
    -+        # Request high bandwidth mode from node0
    ++        # Request BIP152 high bandwidth mode from the -blocksonly node.
     +        p2p_conn_blocksonly.send_and_ping(msg_sendcmpct(announce=True, version=2))
     +
     +        block2 = self.build_block_on_tip()

</details>

Code review re-ACK ae54485c406bcf88c94450ffceabf709d5a18379 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!

I did not modify 6dfee13 because i don't want to invalidate the utACKs that this commit has already gotten

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 ae54485c406bcf88c94450ffceabf709d5a18379

tACK https://github.com/bitcoin/bitcoin/pull/22340/commits/ae54485c406bcf88c94450ffceabf709d5a18379

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 -f their debug.log and only activate the required logging(net, rpc, etc depending on the test you are trying) using bitcoin-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 the debug.log file, 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.

Thanks @rajarshimaitra and @jonatack for the reviews and advice!

nocmpct_blocksonly_2 -> nocmpct_blocksonly_3

<details> <summary>git range-diff nocmpct_blocksonly_2...nocmpct_blocksonly_3</summary>

1:  1bd2f889c ! 1:  ac90610e1 [test] Test that -blocksonly nodes do not request high bandwidth mode.
    @@ test/functional/p2p_compactblocks_blocksonly.py (new)
     +
     +    def setup_network(self):
     +        self.setup_nodes()
    ++        # Start network with everyone disconnected
     +        self.sync_all()
     +
     +    def build_block_on_tip(self):
    @@ test/functional/p2p_compactblocks_blocksonly.py (new)
     +        # node2 produces blocks that are passed to the rest of the nodes
     +        # through the respective p2p connections.
     +
    -+        self.log.info("Test that -blocksonly nodes do not select peers for BIP 152 high bandwidth mode.")
    ++        self.log.info("Test that -blocksonly nodes do not select peers for BIP152 high bandwidth mode")
     +
     +        block0 = self.build_block_on_tip()
     +
     +        # A -blocksonly node should not request BIP152 high bandwidth mode upon
     +        # receiving a new valid block at the tip.
    -+        p2p_conn_blocksonly.send_and_ping(msg_block(block0))
    ++        p2p_conn_blocksonly.send_message(msg_block(block0))
    ++        p2p_conn_blocksonly.wait_until(lambda: p2p_conn_blocksonly.message_count['block'] == 1)
     +        assert_equal(int(self.nodes[0].getbestblockhash(), 16), block0.sha256)
    -+        p2p_conn_blocksonly.wait_until(lambda: p2p_conn_blocksonly.message_count['sendcmpct'] == 2)
    ++        assert_equal(p2p_conn_blocksonly.message_count['sendcmpct'], 2)
     +        assert_equal(p2p_conn_blocksonly.last_message['sendcmpct'].announce, False)
     +
     +        # A normal node participating in transaction relay should request BIP152
2:  abe3c5cf7 ! 2:  90776a869 [test] Test that -blocksonly nodes do not send getdata(CMPCT) on a low bandwidth connection.
    @@ test/functional/p2p_compactblocks_blocksonly.py: class P2PCompactBlocksBlocksOnl
              assert_equal(p2p_conn_high_bw.last_message['sendcmpct'].announce, True)
      
     +        self.log.info("Test that -blocksonly nodes send getdata(BLOCK) instead"
    -+                      " of getdata(CMPCT) in BIP152 low bandwidth mode.")
    ++                      " of getdata(CMPCT) in BIP152 low bandwidth mode")
     +
     +        block1 = self.build_block_on_tip()
     +
3:  c652e187e ! 3:  e38eb9288 [test] Test that getdata(CMPCT) is still sent on regular low bandwidth connections.
    @@ test/functional/p2p_compactblocks_blocksonly.py: class P2PCompactBlocksBlocksOnl
     +        self.nodes[3].submitblock(block0.serialize().hex())
     +
              self.log.info("Test that -blocksonly nodes send getdata(BLOCK) instead"
    -                       " of getdata(CMPCT) in BIP152 low bandwidth mode.")
    +                       " of getdata(CMPCT) in BIP152 low bandwidth mode")
      
     @@ test/functional/p2p_compactblocks_blocksonly.py: class P2PCompactBlocksBlocksOnly(BitcoinTestFramework):
              p2p_conn_high_bw.sync_send_with_ping()
              assert_equal(p2p_conn_high_bw.last_message['getdata'].inv, [CInv(MSG_CMPCT_BLOCK, block1.sha256)])
      
     +        self.log.info("Test that getdata(CMPCT) is still sent on BIP152 low bandwidth connections"
    -+                      " when no -blocksonly nodes are involved.")
    ++                      " when no -blocksonly nodes are involved")
     +
     +        p2p_conn_low_bw.send_and_ping(msg_headers(headers=[CBlockHeader(block1)]))
     +        p2p_conn_low_bw.sync_with_ping()
4:  ae54485c4 ! 4:  f30eb1fe4 [test] Test that -blocksonly nodes still serve compact blocks.
    @@ test/functional/p2p_compactblocks_blocksonly.py: class P2PCompactBlocksBlocksOnl
              p2p_conn_low_bw.sync_with_ping()
              assert_equal(p2p_conn_low_bw.last_message['getdata'].inv, [CInv(MSG_CMPCT_BLOCK, block1.sha256)])
      
    -+        self.log.info("Test that -blocksonly nodes still serve compact blocks.")
    ++        self.log.info("Test that -blocksonly nodes still serve compact blocks")
     +
     +        def test_for_cmpctblock(block):
     +            if 'cmpctblock' not in p2p_conn_blocksonly.last_message:

</details>

ACK f30eb1fe4a62fd9c927ba57b2a549678bde3f3c7

nocmpct_blocksonly_3 -> nocmpct_blocksonly_4

<details> <summary>git range-diff nocmpct_blocksonly_3...nocmpct_blocksonly_4</summary>

1:  ac90610e1 ! 1:  df6794e80 [test] Test that -blocksonly nodes do not request high bandwidth mode.
    @@ test/functional/p2p_compactblocks_blocksonly.py (new)
     +class P2PCompactBlocksBlocksOnly(BitcoinTestFramework):
     +    def set_test_params(self):
     +        self.extra_args = [["-blocksonly"], [], []]
    -+        self.num_nodes = len(self.extra_args)
    ++        self.num_nodes = 3
     +
     +    def setup_network(self):
     +        self.setup_nodes()
    @@ test/functional/p2p_compactblocks_blocksonly.py (new)
     +
     +        # A -blocksonly node should not request BIP152 high bandwidth mode upon
     +        # receiving a new valid block at the tip.
    -+        p2p_conn_blocksonly.send_message(msg_block(block0))
    -+        p2p_conn_blocksonly.wait_until(lambda: p2p_conn_blocksonly.message_count['block'] == 1)
    ++        p2p_conn_blocksonly.send_and_ping(msg_block(block0))
     +        assert_equal(int(self.nodes[0].getbestblockhash(), 16), block0.sha256)
     +        assert_equal(p2p_conn_blocksonly.message_count['sendcmpct'], 2)
     +        assert_equal(p2p_conn_blocksonly.last_message['sendcmpct'].announce, False)
2:  90776a869 = 2:  548115650 [test] Test that -blocksonly nodes do not send getdata(CMPCT) on a low bandwidth connection.
3:  e38eb9288 ! 3:  01db031b5 [test] Test that getdata(CMPCT) is still sent on regular low bandwidth connections.
    @@ test/functional/p2p_compactblocks_blocksonly.py: from test_framework.util import
      class P2PCompactBlocksBlocksOnly(BitcoinTestFramework):
          def set_test_params(self):
     -        self.extra_args = [["-blocksonly"], [], []]
    +-        self.num_nodes = 3
     +        self.extra_args = [["-blocksonly"], [], [], []]
    -         self.num_nodes = len(self.extra_args)
    ++        self.num_nodes = 4

          def setup_network(self):
    +         self.setup_nodes()
     @@ test/functional/p2p_compactblocks_blocksonly.py: class P2PCompactBlocksBlocksOnly(BitcoinTestFramework):

              p2p_conn_blocksonly = self.nodes[0].add_p2p_connection(P2PInterface())
4:  f30eb1fe4 = 4:  c24c52d51 [test] Test that -blocksonly nodes still serve compact blocks.

</details>

Code review ACK c24c52d51cc8faaa79dc554c64ca6d1ee231f112

ACK c24c52d

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 18c5b23a0f7adf9a08bcaa967ed800badf62d90a

tACK https://github.com/bitcoin/bitcoin/pull/22340/commits/18c5b23a0f7adf9a08bcaa967ed800badf62d90a

reACK 18c5b23a0f

Did the rebase myself and verified the same result.

re-ACK 18c5b23a0f7adf9a08bcaa967ed800badf62d90a 🥛

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:

https://github.com/bitcoin/bitcoin/blob/571bb94dfb5047c9be8fcbae5dae71de7256b86c/src/net_processing.cpp#L2721-L2730

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)).

This has been merged.