Allow getblockfrompeer to use any peer

kroese commented at 6:44 pm on May 13, 2023: none

Please describe the feature you’d like to see added.

When you use getblockfrompeer you have to specify an index of a specific peer. But it would be much easier if it allowed you to use ANY peer.

For example, if you run a pruned node, and your first peer happens to be also pruned, the call will fail. If the call tried all nodes, until it finds a peer that has the block, it wouldn’t require you that implement any logic for that.

No response

Describe the solution you’d like

The solution would be simple: if you don’t specify any index, then try all peers untill you find one that does have block, otherwise fail.

Describe any alternatives you’ve considered

No response

Please leave any additional context

No response

kroese added the label Feature on May 13, 2023

kroese commented at 7:03 pm on May 13, 2023: none

Maybe @Sjors can look into this?

furszy commented at 7:17 pm on May 13, 2023: member

For example, if you run a pruned node, and your first peer happens to be also pruned, the call will fail. If the call tried all nodes, until it finds a peer that has the block, it wouldn’t require you that implement any logic for that.

Based on your words, guess that you are calling getblockfrompeer with a fixed index of 1? You could instead use getpeerinfo and filter the result by service. Only requesting the block to another full node.

kroese commented at 7:35 pm on May 13, 2023: none

Based on your words, guess that you are calling getblockfrompeer with a fixed index of 1? You could instead use getpeerinfo and filter the result by service. Only requesting the block to another full node.

I’m aware you could workaround this issue by implementing a bunch of manual logic on the client-side. But even if you skip the pruned nodes and this full node couldn’t provide you the block (for whatever reason), it would be handy if the call would just try the next one.

furszy commented at 8:08 pm on May 13, 2023: member

I’m aware you could workaround this issue by implementing a bunch of manual logic on the client-side. But even if you skip the pruned nodes and this full node couldn’t provide you the block, it would be handy if the call would just try the next one.

Sure. Was providing you a simple workaround to the described issue.

I actually have done something similar for #21267 (in that case is a block filter tracker but here will be for blocks only). So, I’m happy to work on it. Will tackle it soon.

Small note: the “cannot provide block” is actually a getdata request timeout. The RPC command isn’t synchronously fetching the block from the other peer, so the command call will not be the one that tries it again.

Sjors commented at 8:28 am on May 16, 2023: member

I would like this as well but I was too lazy to implement it.

The downside is that it would make the call asynchronous and potentially very long (needing -rpcclienttimeout=0). But this would only be the case if the user doesn’t specify a peer.

It would also need a progress indicator, which our RPC doesn’t really offer. The debug log can of course be used for that. Or you could go all fancy and have the node track the status of the block fetch, including the ability to cancel it.

The upside is that it can fix the weird quirk that we disconnect from any peer that fails to deliver the block. This is because net_processing works under the assumption that we only ask blocks from peers that told us they have it, so if they don’t deliver we assume there’s a problem with the peer. But if we try the next peer before this timeout, BlockRequested clears the in flight map and no timeout will occur. Well, except for the last attempt.

You could also request the block from all peers at the same time, but not without a major refactor (more than #27626 probably).

For the pruned node case @furszy is working on things are potentially a bit simpler. All of our non-pruned peers should have the block. That means the user wouldn’t need to specifify a peer and instead we can just pick one.

You could implement that special case for the getblockfrompeer RPC:

if the user doesn’t specify a peer and the block is not an ancestor of the tip, fail with “must specify peer when fetching a stale block”; otherwise
pick a random non-pruned node and fetch it (fail if we only have pruned peers)

sipa commented at 11:13 am on May 16, 2023: member

I don’t think this is hard to implement, and doesn’t need #27626 (it wouldn’t use compact blocks).

We effectively already have a background block download fetching algorithm, that regularly decides which blocks should be fetched from which peers (see FindNextBlocksToDownload in net_processing.cpp). I think it should be possible to have a list/set somewhere in the net processing state of CBlockIndexes/uint256s/… of blocks to-redownload. If any peer served by FindNextBlocksToDownload happens to have one of those refetch ones, they can get added to the fetching logic.

furszy commented at 1:07 pm on May 16, 2023: member

Ok great sipa and Sjors. Notes taken.

I started implementing this in a separate class BlockRequestTracker with its own scheduled task to detect the timeout and re-try + follow the requests progress (which also involves listening to the node disconnection signal). But, agree that can also add it to the FindNextBlocksToDownload existent mechanism as well. Could merge both worlds and slowly start decoupling the block downloading logic into a separate module while continue using the SendMessages thread to detect timeouts and queue requests.

For the pruned node case @furszy is working on things are potentially a bit simpler. All of our non-pruned peers should have the block. That means the user wouldn’t need to specifify a peer and instead we can just pick one.

Well, peers could delay the response or directly not answer. Stalling the post-filter-matching blocks sync mechanism. So, we also need a blocks tracking module in that branch too.

kroese commented at 3:11 pm on May 16, 2023: none

Well, peers could delay the response or directly not answer. Stalling the post-filter-matching blocks sync mechanism.

Yes, I think that peers who have reached their maxuploadtarget will also refuse to provide the block unless you have the whitelist / download permission. So there can be some edge-cases where non-pruned peers still fail to deliver it.

Sjors commented at 4:51 pm on May 16, 2023: member

If it fails you have to try again. It would presumably auto-pick the same peer, so you’d have to revert to manually picking one. Agree that’s not ideal.

furszy commented at 5:16 pm on May 16, 2023: member

If it fails you have to try again. It would presumably auto-pick the same peer, so you’d have to revert to manually picking one. Agree that’s not ideal.

If the block request timeouts (fails), we disconnect the peer. So, if someone wants to use this as is now, it’s ok to do another RPC call after 10 minutes, when the remote peer is no longer in the getpeerinfo response.

Still, not ideal as it involves some extra RPC calls. Better to keep moving on providing a better tracking mechanism.

kroese commented at 6:49 pm on October 16, 2023: none

@furszy Is this issue still on your radar? You started implementing a BlockRequestTracker but I guess that work was abandoned?

furszy commented at 7:16 pm on October 16, 2023: member

@furszy Is this issue still on your radar? You started implementing a BlockRequestTracker but I guess that work was abandoned?

Not abandoned. Just waiting for review. #27837 (the block request tracker module) depends on #28120 which indirectly depends on #28170 (these two are bug fixes). Right now we are in a feature freeze period (#27758), but feel very welcome to try it and write your feedback there. Would be nice to start generating movement now, so when v26 is released we can focus on it.

Allow getblockfrompeer to use any peer #27652

Please describe the feature you’d like to see added.

Is your feature related to a problem, if so please describe it.

Describe the solution you’d like

Describe any alternatives you’ve considered

Please leave any additional context