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. ConflictsNo conflicts as of last run. |
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
| @@ -505,6 +505,10 @@ class CNode | |||
| * peer eviction criterium in CConnman::AttemptToEvictConnection. */ | |||
| std::atomic<std::chrono::seconds> m_last_block_time{0s}; | |||
|
|
|||
| /** UNIX epoch time of the last block announcement from this peer that we had | |||
| * not yet seen from any peer. Used for potential stale-tip eviction. */ | |||
| std::atomic<std::chrono::seconds> m_last_block_announcement{0s}; | |||
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
| @@ -505,6 +506,10 @@ class CNode | |||
| * peer eviction criterium in CConnman::AttemptToEvictConnection. */ | |||
| std::atomic<std::chrono::seconds> m_last_block_time{0s}; | |||
|
|
|||
| /** UNIX epoch time of the last block announcement from this peer that we had | |||
| * not yet seen from any peer. Used for potential stale-tip eviction. */ | |||
| std::atomic<std::chrono::seconds> m_last_block_announcement{0s}; | |||
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
|
Needs rebase, if still relevant |
21ad9f8 to
559b091
Compare
|
Rebased for merge conflicts and to adopt @naiyoma's review suggestion (thank you!). |
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: ff69990
Github-Pull: bitcoin#27052 Rebased-From: e9a0e21
Github-Pull: bitcoin#27052 Rebased-From: b15ecab
Github-Pull: bitcoin#27052 Rebased-From: 5bcf849
test/functional/p2p_block_times.py
Outdated
| time (currently 30 minutes, see TipMayBeStale()), the client, suspecting | ||
| that there may be new blocks that its peers are not announcing, will | ||
| add an extra outbound peer and disconnect (evict) the peer that has | ||
| least recently been the first to announced a new block to us. (If there |
There was a problem hiding this comment.
first to announced a new block → first to announce a new block
f7788f6 to
cbe4603
Compare
| @@ -5189,7 +5189,7 @@ void PeerManagerImpl::EvictExtraOutboundPeers(std::chrono::seconds now) | |||
| // Protect peers from eviction if we don't have another connection | |||
| // 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.