Restrict "prev" test to just the voting path, to allow catchup. #4198
Conversation
This file contains 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
dmkozh
reviewed
Feb 21, 2024
|
r+ 6a33a51 |
latobarita
added a commit
that referenced
this pull request
Feb 21, 2024
…voting Restrict "prev" test to just the voting path, to allow catchup. Reviewed-by: dmkozh
marta-lokhova
approved these changes
Feb 21, 2024
graydon
force-pushed
the
bug-4193-restrict-prev-test-to-voting
branch
from
6a33a51
to
bd01b44
Compare
|
r+ bd01b44 |
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.
This slightly weakens the conditions on which we judge a soroban-era upgrade invalid. It is motivated by bug #4193 in which a ledger that was voted-on in the past as a valid protocol-upgrade ledger, by a node with soroban
prevcompiled-in, becomes no longer replayable during catchup by a node without sorobanprevcompiled-in.This only applies to soroban-era stuff, so without loss of generality we can talk about his only applying to the v20->v21 upgrade (it also applies to v21->v22 and so forth but that's even less relevant currently).
Before this change, a node lacking a
prevbuild will judge a v20->v21 upgrade invalid for voting or applying during catchup.With this change, a node lacking a
prevbuild will judge a v20->v21 upgrade invalid for voting-on only.This is accomplished by moving a conditional from
Upgrades::isValidForApply(which is called by bothUpgrades::isValidas well asLedgerManagerImpl::closeLedger) to its voting-phase callerUpgrades::isValid.While this does technically "change the protocol" in terms of the semantics of a given ledger -- some previously-invalid ledgers are now valid -- it does so in a very limited way that I believe does not pose any risk in the real world. Specifically: no builds currently in the field claim to support v21 yet at all, so all currently-deployed core builds will already judge all such upgrades invalid on the basis of their protocol number, regardless of
prev-presence. So long as this change is made before we release anything claiming to speak v21, there should be no observable change to behaviour.(I also added a bit more commentary to the code exploring what will happen to nodes that fail to satisfy the conditional, but get forced to accept the upgrade by a majority, which I think are actually not in a particularly bad position.)
I'd appreciate folks who know about this subsystem and set of conditions reasoning through the logic I'm presenting here for themselves and confirming that it makes sense!
Resolves #4193