Codecov Report

Merging #2435 into develop will increase coverage by 0.07%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop    #2435      +/-   ##
===========================================
+ Coverage    61.28%   61.35%   +0.07%     
===========================================
  Files          198      198              
  Lines        16383    16376       -7     
===========================================
+ Hits         10040    10048       +8     
+ Misses        5508     5494      -14     
+ Partials       835      834       -1

Another question here is how, if at all, we inform peers that our AppVersion has upgraded (this could happen without a Tendermint restart according to current design)

Another question here is how, if at all, we inform peers that our AppVersion has upgraded (this could happen without a Tendermint restart according to current design)

What behaviour would result from this? Are we disconnecting from peers with incompatible app versions? Are we foresee incompatible app versions? If it is a p2p concern it might be worth introduce that as part of the messages exchanged by the PEXs and consequently deciding if the peer should be kept around.

Are we disconnecting from peers with incompatible app versions?

Current thinking is no, under the assumption that we want to be able to sync a peer across multiple app versions. But this also means we don't know if we're actually compatible with a peer when we connect to them, since they could be on an old height/version, and we won't find out that we forked away from eachother until we actually disagree on blocks somewhere.

More prudent might be to consider tracking the correct version for each height, and enforcing that on peers, and having peers update each other when their version changes, and disconnecting if their version is wrong for their height.

Even better might be to include the list of (height, version) pairs as part of the NodeInfo, so you know if you have the same history of upgrades and can disconnect right away if not. In ADR-017 I suggest incorporating all of this into the ChainID, so a ChainID becomes a description of the chain's forks and of the peer compatibility, regardless of what height the peers are on (!)

@ebuchman Tried to capture the follow-up questions in #2448, please ammend/correct if necessary.