test: p2p: check that peer’s announced starting height is remembered #33990

1. 
   theStack commented at 0:25 am on December 2, 2025: contributor

   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 getpeerinfo RPC and in some debug messages (see also e.g. https://github.com/bitcoin-dot-org/Bitcoin.org/issues/1387#issuecomment-252934859). 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]

2. test: check that peer's announced starting height is remembered

   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.

   52f96cc235
3. DrahtBot added the label Tests on Dec 2, 2025
4. 
   DrahtBot commented at 0:25 am on December 2, 2025: contributor

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   Code Coverage & Benchmarks

   For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33990.

   Reviews

   See the guideline for information on the review process. A summary of reviews will appear here.

5. theStack renamed this:
   test: check that peer's announced starting height is remembered
   test: p2p: check that peer's announced starting height is remembered
   on Dec 2, 2025
6. 
   rkrux commented at 11:17 am on December 2, 2025: contributor

   Concept ~0 52f96cc235d309d9156eb742036c859984b9a467

   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.

7. 
   brunoerg commented at 11:20 am on December 2, 2025: contributor

   Can be tested with the following diff:

    0diff --git a/src/net_processing.cpp b/src/net_processing.cpp
    1index 5dac413319..ec417b97ac 100644
    2--- a/src/net_processing.cpp
    3+++ b/src/net_processing.cpp
    4@@ -3535,7 +3535,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
    5             LOCK(pfrom.m_subver_mutex);
    6             pfrom.cleanSubVer = cleanSubVer;
    7         }
    8-        peer->m_starting_height = starting_height;
    9+        peer->m_starting_height = starting_height + 1;
   10
   11         // Only initialize the Peer::TxRelay m_relay_txs data structure if:
   12         // - this isn't an outbound block-relay-only connection, and
   

8. in test/functional/p2p_handshake.py:79 in 52f96cc235

   74@@ -65,6 +75,20 @@ def test_desirable_service_flags(self, node, service_flag_tests, desirable_servi
   75                 assert (services & desirable_service_flags) == desirable_service_flags
   76                 self.add_outbound_connection(node, conn_type, services, wait_for_disconnect=False)
   77 
   78+    def test_startingheight(self, node):
   79+        for fake_startheight in [-2**31, -1, 0, 1000000, 2**31-1] + [random.randint(-2**31, 2**31) for _ in range(5)]:
   

   ---

   ---

   

   brunoerg commented at 11:20 am on December 2, 2025:

   nit: Perhaps a comment explaining these values (edge cases for 32-bit integers) would be good.
9. 
   fanquake commented at 1:56 pm on December 6, 2025: member

   @ajtowns @mzumsande conceptual thoughts?
10. 
    ajtowns commented at 11:30 pm on December 6, 2025: contributor

    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 peer->m_starting_height entirely (and only reporting it in the receive version message debug log entry) might be better?

11. 
    maflcko commented at 11:41 am on December 7, 2025: member

    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 conntime field. If it exists to show which peer is ahead of whom, then the existing headers-sync and block-sync p2p flow seems a better way to figure it out, which is reported via the synced_headers, presynced_headers, or synced_blocks fields. If it exists just for debugging the sync logic, it seems best to just log it in the debug log.

    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.

12. 
    ajtowns commented at 9:18 pm on December 7, 2025: contributor

    Yeah, I also wonder what the use-case here is or was.

    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.

13. 
    mzumsande commented at 7:40 pm on December 8, 2025: contributor

    I agree that it isn’t useful, and deprecation would makes sense in my opinion. If we decide not to do that for some reason, a test makes sense though.
14. fanquake marked this as a draft on Dec 17, 2025

Contributors
theStack DrahtBot rkrux brunoerg fanquake ajtowns maflcko mzumsande

Labels
Tests