net: announce our height in the handshake, and say when we are behind - #56
Merged
Conversation
A node running this code cannot tell "in sync with everyone" from "not hearing anyone". The version message carries no height -- 0.1.0 never had the field and this fork inherited the gap -- so the only evidence of a problem is a number on screen that stopped moving, which looks the same as a network with nothing to report. It is the structural reason a broken node here has always been indistinguishable from an idle one. Append nBestHeight to the version message and keep what a peer announced in CNode::nStartingHeight. The field goes last, so a node that predates it stops reading after addr and treats the rest as trailing bytes, which cost it one log line. Reading it is guarded by vRecv.empty() and never required: peers that do not send it keep nStartingHeight at -1 and are left out of the median. Every release so far sends the short form. GetPeerMedianHeight() takes the median rather than the maximum so a single peer claiming an absurd height cannot move it. Then use it. After five minutes more than one block behind the peer median, the node says so in the log -- not behind a debug category, because an operator who already enabled -debug is not the one who needs telling -- and the GUI carries the same sentence in its status area. "8 blocks behind for 27 minutes" is something a user can act on; a frozen counter is not. Verified against two nodes on the live network: the one starting from an empty datadir read its peer at 3797, reported 2636 then 1274 then 433 blocks behind as it caught up, and went quiet on its own once level. The other logged "peer height = -1" for the older nodes it met on the real network, which is the compatibility path doing its job. Known limit: nStartingHeight is a snapshot from handshake time and goes stale on a long-lived connection, so this catches a node that is receiving something but not blocks, not one that has gone completely deaf. The inactivity timeout from #55 covers that case and refreshes these heights when it forces the reconnect.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follows #55. That one made a node notice a peer it cannot hear. This one lets it notice that it is behind everyone.
The gap
The version message carries no height. Bitcoin 0.1.0 never had the field and this fork inherited the gap, so a node has no idea where the rest of the network is. The only evidence of trouble is a number on screen that stopped moving — which looks exactly like a network with nothing to report.
That is the structural reason a broken node here has always been indistinguishable from an idle one. It is the shape of #42, of #51, and of the seed sitting 8 blocks behind for 27 minutes this morning with all three relays reachable the whole time.
What this does
nBestHeightgoes on the end of the version message and lands inCNode::nStartingHeight.addrand treats the rest as trailing bytes, which costs it one log line.vRecv.empty(); peers that do not send it stay at-1and are left out of the median. Every release so far sends the short form.Then it gets used: after five minutes more than one block behind the peer median, the node says so. In the log, not behind a debug category — an operator who already enabled
-debugis not the one who needs telling — and in the GUI status area.Testing
Two nodes on the live network, connected over
.btf:peer height = -1for the older nodes it met on the real network — the compatibility path doing its job, unpromptedKnown limit
nStartingHeightis a snapshot from handshake time and goes stale on a long-lived connection. So this catches a node that is receiving something but not blocks; it does not catch one that has gone completely deaf, because then every peer's remembered height is old too.The inactivity timeout from #55 is what covers that case, and it refreshes these numbers when it forces the reconnect. Carrying the height in
pingwould make it live — worth doing, but on its own evidence rather than bundled here.