Due to a missing height update; the node sends its pre-snapshot height to any peer connecting after the snapshot load but before the node receive a new tip block.
This does not make any difference for us because we are not using the
VERSION msg height field but it does not seem consistent plus other
software might be using the field.
Due to a missing height update; the node sends its
pre-snapshot height to any peer connecting after
the snapshot load but before the node receives a
new tip block.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For detailed information about the code coverage, see the test coverage report.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process. A summary of reviews will appear here.
<!--174a7506f384e20aa4161008e828411d-->
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.
2835 | @@ -2836,6 +2836,10 @@ static RPCHelpMan loadtxoutset() 2836 | throw JSONRPCError(RPC_INTERNAL_ERROR, strprintf("Unable to load UTXO snapshot: %s. (%s)", util::ErrorString(activation_result).original, path.utf8string())); 2837 | } 2838 | 2839 | + // Update peer manager best block 2840 | + const auto& tip = WITH_LOCK(chainman.GetMutex(), return chainman.ActiveTip()); 2841 | + node.peerman->SetBestBlock(tip->nHeight, std::chrono::seconds{tip->GetBlockTime()});
Are you sure this is the best place for this in the RPC? Looking at the other calls to SetBestBlock() they all seem to be happening in validation.cpp.
nit: You can also get the height out of metadata without the mutex (see below).
I am still undecided if we want this and need to think about it a little more.
We send this usually when we have a new block but the loading of the snapshot does not necessarily mean that we have the block, right? We only need the header for that. So we should probably set it when we get block itself I think.
Thanks for the review fjahr! Sorry for the delay. And thanks for the ping maflcko.
We send this usually when we have a new block but the loading of the snapshot does not necessarily mean that we have the block, right? We only need the header for that. So we should probably set it when we get block itself I think.
Yeah, we set this field when a new tip block is connected and only send it during the peer connection's initial handshake. I initially found it odd because brand new nodes running AssumeUTXO would connect to a peer, signal their best block height as 0, and then start requesting blocks above the snapshot height due to the tip sync priority. But yeah, signaling the snapshot height as the node's best block doesn't help as the node does not have the previous blocks anyway. All good, closing. Thanks!