test: rpc: add last block announcement time to getpeerinfo result #27052
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/27052. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, 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. |
20368e3 to
becf380
Compare
|
Force pushed to fix CI failure |
|
Concept ACK. I spent quite a bit of time trying to test #26172 (my findings are documented here) and can confirm the difficulty of testing it with the current state of the test framework. I also support the suggestion to add |
src/net.h
Outdated
There was a problem hiding this comment.
If you change the type, why not use NodeSeconds (a time point)? While std::chrono::seconds (a duration) is used widely in the codebase, because all durations that denote a time point are since epoch, using a dedicated type such as NodeSeconds might be clearer.
src/net.h
Outdated
There was a problem hiding this comment.
I don't think this belongs in CNode. It is used in our outbound eviction logic which is part of net processing and it isn't used in net at all (imo, the eviction logic should be encapsulated in its own module entirely #25268 😉).
Also afaict, there is no need to move m_last_block_announcement, because you can just return it in PeerManager::GetNodeStateStats.
becf380 to
efc2e70
Compare
efc2e70 to
16d2903
Compare
16d2903 to
2e80e3b
Compare
2e80e3b to
6c564aa
Compare
|
Force pushed from becf38061b2625bcc629293fec0dce7c27292e14 to
I'll take this out of draft now since I think it's ready for review. |
559b091 to
5bcf849
Compare
Copy its value from CNodeState. Unused until the next commit.
5bcf849 to
f7788f6
Compare
Copy its value from CNodeState. Unused until the next commit. Github-Pull: bitcoin#27052 Rebased-From: ff699907ed9418fc522e56f54595e957193853dc
Github-Pull: bitcoin#27052 Rebased-From: e9a0e210418accf8336984d143c5f7c62efabfa3
Github-Pull: bitcoin#27052 Rebased-From: b15ecabff363de98899531911db6c4abe3f783e7
Github-Pull: bitcoin#27052 Rebased-From: 5bcf8491d15ac148507a4d172469e1fd9d12078e
test/functional/p2p_block_times.py
Outdated
There was a problem hiding this comment.
first to announced a new block → first to announce a new block
f7788f6 to
cbe4603
Compare
| // to their network, counting both outbound-full-relay and manual peers. | ||
| NodeId worst_peer = -1; | ||
| int64_t oldest_block_announcement = std::numeric_limits<int64_t>::max(); | ||
| std::optional<NodeSeconds> oldest_block_announcement; |
There was a problem hiding this comment.
I'm not sure I understand why this is optional. Originally, it was set to a sentinel value.
I don't see a situation where it wouldn't have a value.
My suggestion → NodeSeconds oldest_block_announcement = NodeSeconds::max();
That way .has_value() checks and *oldest_block_announcement can be avoided.
There was a problem hiding this comment.
optional + has_value() communicates intent much more clearly than the sentinel imo. Though admittedly it's awkward as long as worst_peer uses a sentinel as well.
I'd expect to see std::optional<std::pair<NodeId, NodeSeconds>> worst_peer, personally.
This PR adds
last_block_announcementto the per-peergetpeerinfoRPC result. This is the most recent time that this peer was the first to notify our node of a new block (one that we didn't already know about), or zero if this peer has never been the first to notify us of a new block. This timestamp already exists internally and is used for stale-tip eviction logic; this PR exposes it at the RPC layer.This PR started out as a suggestion for additional test coverage, see #26172 (comment). It turned out that the easiest way to test (already-merged) #26172 is to add this field to
getpeerinfoand have a functional test verify its value. But it may also be useful to have this result in its own right, similar to that RPC's existinglast_blockfield -- it indicates something about the quality of our peers. It allows one to predict which peer will be evicted when the stale tip logic activates. (I'm not sure if that would be useful, but it may be.)The functional test added here fails without #26172, which is the main goal.
This PR does not test the actual stale-tip eviction logic; that's difficult to do with a functional test. But it does test the correctness of the timestamp that the eviction logic depends on. #23352 is an attempt to test the eviction logic using a unit test, it's not ready to be merged yet. I think having both kinds of tests would be beneficial.