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

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #23813 (Add test and docs for getblockfrompeer with pruning by fjahr)

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.

I started testing it by creating a functional test. Not sure if the approach is right, I started two nodes, the second one with -prune=550 and then, node0 mines 20 blocks, I get the bestblockhash from node0 and use it on getblockfrompeer (node1 --> node0), it should return an error (apparently it worked):

def run_test(self):
        self.log.info("Mine 20 blocks on Node 0")
        self.generate(self.nodes[0], 200, sync_fun=self.no_op)
        assert_equal(self.nodes[0].getblockcount(), 400)

        self.log.info("Connect nodes")
        self.connect_nodes(0, 1)

        peers = self.nodes[1].getpeerinfo()
        assert_equal(len(peers), 1)
        peer_1_peer_0_id = peers[0]["id"]
        best_block_hash_0 = self.nodes[0].getbestblockhash()
        assert_raises_rpc_error(-1, 'In prune mode, only blocks that the node has already synced previously can be fetched from a peer', self.nodes[1].getblockfrompeer, best_block_hash_0, peer_1_peer_0_id)

In master branch, this test would fail, because an exeception won't be raised.

I started testing it by creating a functional test. Not sure if the approach is right, I started two nodes, the second one with -prune=550 and then, node0 mines 20 blocks, I get the bestblockhash from node0 and use it on getblockfrompeer (node1 --> node0), it should return an error (apparently it worked):

Hey @brunoerg , thanks for giving it a try but unfortunately I don't think this approach works reliably. The problem is that the outcome of this test is a race because node 1 is downloading the blocks from node 0 in the background. It may have the current tip in flight by the time the assert is called, or not. We try to avoid tests where the outcome is not 100% reliable because we are seeing intermittent test failures quite often already.

What would be needed is a reliable way to let node 1 sync the headers but then prevent it from syncing the blocks. It seems we don't have something like this and I am now thinking about how this could be done and where else it could be useful.

The problem is that the outcome of this test is a race because node 1 is downloading the blocks from node 0 in the background.

Interesting, this is new for me.

def setup_network(self):
  self.setup_nodes()

even with this config (setup_network) node1 will download the blocks from node0 in the background after node0 mines more 200 blocks?

def run_test(self):
        self.log.info("Mine 200 blocks on Node 0")
        self.generate(self.nodes[0], 200, sync_fun=self.no_op)
        assert_equal(self.nodes[0].getblockcount(), 400)

        self.log.info("Connect nodes")
        self.connect_nodes(0, 1)

        peers = self.nodes[1].getpeerinfo()
        assert_equal(len(peers), 1)
        peer_1_peer_0_id = peers[0]["id"]
        best_block_hash_0 = self.nodes[0].getbestblockhash()
        assert_raises_rpc_error(-1, 'In prune mode, only blocks that the node has already synced previously can be fetched from a peer', self.nodes[1].getblockfrompeer, best_block_hash_0, peer_1_peer_0_id)

        self.sync_blocks()
        self.nodes[1].getblockfrompeer(best_block_hash_0, peer_1_peer_0_id)

I thought the first getblockfrompeer into the assert would fail because I didn't setup it to sync blocks and the second one would work because of self.sync_blocks().

even with this config (setup_network) node1 will download the blocks from node0 in the background after node0 mines more 20 blocks?

No, the nodes indeed can not sync if they are not connected. But you are connecting the nodes in your test here: self.connect_nodes(0, 1). In the moment the nodes are connected they also start syncing. You can see this for example by inserting print(self.nodes[1].getblockcount()) in the line before your first assert and then running the test a couple of times. You will see that the node is not at 200 blocks anymore and each time you run the test it will be a different number because of this race between the different processes.

I thought the first getblockfrompeer into the assert would fail because I didn't setup it to sync blocks and the second one would work because of self.sync_blocks().

What sync_blocks() does is it waits for all the nodes to be caught up with each other. This ensures the reverse of our problem doesn't happen: a test where all the nodes need to be caught up to continue does not fail intermittently and so with that the second assert is safe. But for the first assert we would basically need the inverse of the functionality, i.e. something that ensures that the nodes are definitely not caught up with each other until we have finished what we want to test.

I think we have a working test now. I remembered that we can submit the header via a P2PInterface to the pruning node and that seems to work. :)

You will see that the node is not at 200 blocks anymore and each time you run the test it will be a different number because of this race between the different processes.

Yes, my bad. That approach would work because node0 is mining 200 blocks more, so probably node1 didn't get the last one before the assertion but they're syncing and it could cause intermittent failures.

Concept ACK, but light selfish preference for getting #23706 in first, since there's a (small) conflict.

Weak concept NACK. I think it's better to allow it. Pruning is only best-effort anyway, not a guarantee.

Rebased

Weak concept NACK. I think it's better to allow it. Pruning is only best-effort anyway, not a guarantee.

Why do you think it's better to allow it? Do you have any specific use cases in mind? Of course there is no guarantee to stay below the exact number but for prune=550 this has the potential to double or tripple the number quickly. And the worst case scenario of crashing due to a full disc seems bad enough that it's worth to disable something where it's unclear if there is a use case for it at all (using the RPC in this way). If there is a use case I am genuinely interested in hearing about it and would then look for a better solution rather than just disallowing it.

In the context of ForkMonitor I use this feature to fetch blocks that are, by definition, at or below the tip height. I.e. stale blocks. This PR doesn't impact that use case, because these nodes are always up to date.

In fact, this PR adds some safety for when a fresh node is added to the site, or an existing node is reinstalled, and it's still catching up (though in practice we don't call getblockfrompeer on a node that is in IBD).

Another use case that seems obvious to me is to fetch an historical block, perhaps because you're rescanning a wallet. In that case I don't see any harm in waiting until the node is synced.

Rebased

Rebased

utACK 5826bf5

Very nice test.

ACK 5826bf5