Coming from #27652. Implementing the first part of it.
The idea is to let users call getblockfrompeer without providing any peer id.
The node will internally select one peer at random and make the getdata request.
This also fixes a bug where the user is allowed to call getblockfrompeer providing
an id of a peer that signals a “limited” service. As limited peers cannot provide historical
blocks, it is not correct to allow the user to do that.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| Approach ACK | ismaelsadeeq |
If your review is incorrectly listed, please react with š to this comment and the bot will ignore it on the next update.
Reviewers, 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.
Limited peers only store the last 288 blocks, not all historical blocks.
1779@@ -1780,13 +1780,24 @@ std::optional<std::string> PeerManagerImpl::FetchBlock(NodeId peer_id, const CBl
1780 {
1781 if (m_chainman.m_blockman.LoadingBlocks()) return "Loading blocks ...";
1782
1783+ // Only allow fetching from limited peers if the block height is within the allowed window
1784+ bool allow_limited_peers = false;
nit: feel free to ignore
0 bool allow_limited_peers{false};
161+ pruned_node.add_p2p_connection(P2PInterface(), services=NODE_NETWORK_LIMITED | NODE_WITNESS)
162+ limited_peer_id = next(peer for peer in pruned_node.getpeerinfo() if peer["servicesnames"] == ['WITNESS', 'NETWORK_LIMITED'])["id"]
163+
164+ pruned_block_10 = self.nodes[0].getblockhash(10)
165+ assert_raises_rpc_error(-1, "Cannot fetch old block from a limited peer", pruned_node.getblockfrompeer, pruned_block_10, limited_peer_id)
166+
It would be nice to include a test specifically targeting the scenario of retrieving a block within the last 288 blocks from a Network Limited peer.
The ‘prune’ option cannot be set below 550 MiB, it is unclear whether it allows pruning to the last 288 blocks. ( That is the possibility of a node not having any of the last 288 blocks ). We could just exclude limited peers from fetching the blocks?
The ‘prune’ option cannot be set below 550 MiB, it is unclear whether it allows pruning to the last 288 blocks. ( That is the possibility of a node not having any of the last 288 blocks ).
Have you seen FindFilesToPrune?, MIN_BLOCKS_TO_KEEP prevents pruning from deleting files that contain a block within the last 288 range.
Limited peers should just be excluded from fetching blocks, unless there are any cases where the last 288 blocks might not be available?
Not yet. Basically, one of the features that have locally for the on-going #21267 working path, is to be able to sync-up the node on-demand by doing getblockfrompeer requests instead of using the node’s automatic sync process. Which is later used for the filters matching block requests too.
Said that, no problem on excluding limited peers for now. We could enable them later, when the new feature is introduced.
434@@ -435,7 +435,8 @@ static RPCHelpMan getblockfrompeer()
435 "Returns an empty JSON object if the request was successfully scheduled.",
0 "Returns the peer id we are requesting the block from if the request was successfully scheduled.",
Approach ACK
Nice to have the option to call getpeerinfo without explicitly passing a peer id. Often, I get block from a random peer, and the current implementation’s random selection of a peer when no id is provided aligns with that.
Tests fail on the master pass on 31ae2d20da0800201376be71d41f19064edd3f92. I’ve left a few suggestions and a question.
groundwork for introducing the fetch block from "any" peer functionality.
If no 'peer_id' is provided, 'getblockfrompeer' will pick a random
full node to fetch the block from.
I would much prefer an approach where this is handled by the existing background block fetches, because that one is aware of which peers have which blocks, and can load balance across this, and re-issue on timeout etc.
It would make the functionality also usable for e.g. redownloading for wallet rescan after pruning or so.
I would much prefer an approach where this is handled by the existing background block fetches, because that one is aware of which peers have which blocks, and can load balance across this, and re-issue on timeout etc.
Guess that you are talking about the random peer selection part? Like, you are expecting to have a set of “retry” blocks in net_processing, so FindNextBlocksToDownload can take one of those, mark it as in-flight, request it to a peer, and track the timeout etc (like what you suggested in #27652)
Because, that is implemented in the follow-up #27837, in a slightly different way. With a BlockRequestTracker class, which does a bit more; with a pending requests queue instead of the retry set (I coded your suggestion first and IIRC, what didn’t convince me was the need to be continually traversing the retry set to see if any of those timed out).
The purpose of this PR was to limit the scope of the initial changes to getblockfrompeer so that the user is aware of the source from which the block is initially being fetched, without introducing additional functionality beyond that. Then, in #27837, the tracking aspect is implemented, along with a fallback mechanism to handle cases where the initial fetch fails, and other related functionalities.
But.. thinking further, what if instead of returning the initial peer id, we return the tracking request id from getblockfrompeer, then extend the command to return information about tracked block requests. With this, we wouldn’t need the random peer selection changes (nor most of the changes here), everything will be contained inside the background fetching logic. Which means that focus would be directly on #27837.
The only drawback of this approach is that the “fetch from any” functionality will not be available until the entire BlockRequestTracker class is introduced. Which I think that is fine.
What do you think?
Side note about the tracker class:
My idea is to start with this getblockfrompeer scenario, so we can add proper test coverage for the class, and then expand it to track what is currently being tracked by mapBlocksInFlight (which involves few more changes like the peer stalling detection).
It would make the functionality also usable for e.g. redownloading for wallet rescan after pruning or so.
Yeah. That one, and the rescan by using compact block filters, are my long-term goals.
@furszy Thanks for giving some background, I do see better where this is coming from now.
But yes, my thinking is that it’s the wrong approach to assign block requests to random peers, for multiple reasons:
And it seemed to me that tacking something on, without reusing the existing logic for downloading and tracking blocks, seemed both unnecessary and inferior to me.
However, thinking about it more, I realize there is perhaps something I missed, if that’s an intended use case: downloading blocks we don’t have a block header for. All the existing logic in FindNextBlockToDownload, the reasoning about which peers have what, and the timeout logic/tracking around block downloading in general is based on identifying blocks using their entry in the block index. For downloading blocks just by hash, very little of that is reusable. So perhaps the right (eventual) approach is actually treating these two separately: redownloading of known blocks using the existing logic, but downloading of unknown blocks using a new mechanism.
Regarding identification of tracking requests: is there a need for that? For normal IBD/sync, there is no such functionality either. Perhaps I’m missing the use case for this. If you do need an identifier to track (re)download requests, what about using the block hash itself? Multiple redownload requests for the same block don’t make sense anyway.
Thanks sipa for all the feedback!
@furszy Thanks for giving some background, I do see better where this is coming from now.
But yes, my thinking is that it’s the wrong approach to assign block requests to random peers, for multiple reasons:
- It doesn’t take into account which peer has what.
- If it gets assigned incorrectly, there is no way to recover by retrying from another peer (on timeout or disconnect).
- It ignores the existing information we have about currently outstanding block requests to peers (e.g. we may wish to avoid requesting from peers which already have many unanswered block requests).
And it seemed to me that tacking something on, without reusing the existing logic for downloading and tracking blocks, seemed both unnecessary and inferior to me.
Ok yeah, now I see the full picture, and agree. For manually requested blocks, #27837 introduces most of those points while re-using the existing downloading logic. It just need few adjustments.
However, thinking about it more, I realize there is perhaps something I missed, if that’s an intended use case: downloading blocks we don’t have a block header for. All the existing logic in FindNextBlockToDownload, the reasoning about which peers have what, and the timeout logic/tracking around block downloading in general is based on identifying blocks using their entry in the block index. For downloading blocks just by hash, very little of that is reusable. So perhaps the right (eventual) approach is actually treating these two separately: redownloading of known blocks using the existing logic, but downloading of unknown blocks using a new mechanism.
Hmm, yeah.
Right now, it is not possible to call getblockfrompeer with a hash of an unknown block. But, might be useful to enable it in the future. The tracking mechanism being introduced in #27837 can help with the timeouts, disconnections and even to set a max number of download attempts or a request TTL.
Regarding identification of tracking requests: is there a need for that? For normal IBD/sync, there is no such functionality either. Perhaps I’m missing the use case for this. If you do need an identifier to track (re)download requests, what about using the block hash itself? Multiple redownload requests for the same block don’t make sense anyway.
Yeah, no.. I had one of those fake epiphanies, probably originated by a lack of caffeine in my system.. block requests are tracked by hash in #27837. So we can easily retrieve their tracking information with it.
So, to summarize:
Iām going to decouple the commit that prevents users from calling getblockfrompeer with a network limited peer id into another PR (bugfix). Close this one, and adjust #27837 so it also handles the initial peer selection (removing the random selection). This is because the existing download logic is used in 27837 when the initial download attempt fails.
Labels
RPC/REST/ZMQ