test: p2p: check that peer's announced starting height is remembered#33990
test: p2p: check that peer's announced starting height is remembered#33990theStack wants to merge 1 commit intobitcoin:masterfrom
Conversation
Note that the announced starting height is neither verified nor used in any other logic, so reporting a bogus value doesn't have any consequences -- it's only used for inspection via the `getpeerinfo` RPC and in some debug messages.
|
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/33990. ReviewsSee the guideline for information on the review process. |
rkrux
left a comment
There was a problem hiding this comment.
Concept ~0 52f96cc
I'm on the fence regarding whether this case testing just one field is warranted. I don't see much coverage gained in the corecheck.
Though there is a similar test_desirable_service_flags case as well but in it the connection is disconnected conditionally, so one can argue that there is more to test over there.
|
Can be tested with the following diff: diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 5dac413319..ec417b97ac 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -3535,7 +3535,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
LOCK(pfrom.m_subver_mutex);
pfrom.cleanSubVer = cleanSubVer;
}
- peer->m_starting_height = starting_height;
+ peer->m_starting_height = starting_height + 1;
// Only initialize the Peer::TxRelay m_relay_txs data structure if:
// - this isn't an outbound block-relay-only connection, and
|
| self.add_outbound_connection(node, conn_type, services, wait_for_disconnect=False) | ||
|
|
||
| def test_startingheight(self, node): | ||
| for fake_startheight in [-2**31, -1, 0, 1000000, 2**31-1] + [random.randint(-2**31, 2**31) for _ in range(5)]: |
There was a problem hiding this comment.
nit: Perhaps a comment explaining these values (edge cases for 32-bit integers) would be good.
|
@ajtowns @mzumsande conceptual thoughts? |
|
Seems fine to test it if we have code to export it over RPC. I don't think I've ever seen it be useful for debugging anything, and just knowing the height without knowing if it's the same header you have for that height or what the chainwork is doesn't seem very useful. So dropping |
|
Yeah, I also wonder what the use-case here is or was. If it is an estimate of how long the connection is established, there is the more accurate In any case, if it is kept, the rpc docs should be updated to say it is the height reported by the peer, not a confirmed or calculated value. |
Looks like it was introduced in v0.2.9 where it was used as a heuristic for determining if we're in IBD (due to being more than 2k blocks behind where our peers were), The check was removed in #20624, which also has some more context. |
|
Seems reasonable to deprecate the field if there is really no use-case, opened #34197 for that. Closing this one, it can always be re-opened if there is (for any reason) a preference to keep the field. |
…nfo` RPC 4ce3f4a rpc, net: deprecate `startingheight` field of `getpeerinfo` RPC (Sebastian Falbesoner) Pull request description: This PR deprecates the "startingheight" result field of the `getpeerinfo` RPC, following the discussion in #33990. Rationale: the reported starting height of a peer in the VERSION message is untrusted, and it doesn't seem to be useful anymore (after #20624), so deprecating the corresponding field seems reasonable. After that, it can be removed, along with the `m_starting_height` field of the Peer / CNodeStats structs, as it is sufficient to show the reported height only once at connection in the debug log. ACKs for top commit: optout21: crACK 4ce3f4a achow101: ACK 4ce3f4a fjahr: utACK 4ce3f4a rkrux: crACK 4ce3f4a janb84: cr ACK 4ce3f4a Tree-SHA512: b296a28d30084fd35c67a2162e85576e3365e5d6fffe5b1add500034c1850604ee8c37b61afe812bfab8a7cc20f6a9e22db445e3c371311a5f82a777e5700ebf
Note that the announced starting height is neither verified nor used in any other logic, so reporting a bogus value doesn't have any consequences -- it's only used for inspection via the `getpeerinfo` RPC and in some debug messages. Github-Pull: bitcoin#33990 Rebased-From: 52f96cc
Note that the announced starting height of a peer is neither verified nor used in any other logic, so reporting a bogus value [1] as done in the test doesn't have any consequences on the connection -- it's only used for inspection via the
getpeerinfoRPC and in some debug messages (see also e.g. bitcoin-dot-org/Bitcoin.org#1387 (comment)). Admittedly this is not terribly important, but I'd still think having test coverage for this simple reporting functionality is better than not having it.[1] any signed 32-bit integer is fine, i.e. in the range [-2^31,2^31-1]