Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bugfix in HFC: do not consider the last known era to be eternal #3754

Merged
merged 2 commits into from
May 20, 2022

Conversation

nfrisby
Copy link
Contributor

@nfrisby nfrisby commented May 18, 2022

This supersedes PR #3750. And it unblocks the Vasil HF.

This PR fixes a bug in the Consensus Hard Fork Combinator (HFC). The bug is that certain parts of the HFC before this PR assume that the final era the code is aware of (ie the rightmost era in the xs argument to HardForkBlock xs) will never end. At face value, this assumption seems very reasonable. If the final era could end, then that means we wrote the code that knows how to end the final era but didn't simultaneously add the code for the following era, which is pretty clearly a bad idea unless you indeed want your system-wide chain to stop growing. The patterns we have in protocolInfoCardano and in the related call in input-output-hk/cardano-node ensure that mistake would be quite obvious in review of such a PR. Despite that assumption seeming reasonable, merely adding Babbage in the recent PR #3595 revealed this assumption as a bug: the new code considered some Alonzo transactions on the historical chains to now be invalid. Together, this PR and PR IntersectMBO/cardano-ledger#2785 fix the bug and also allows those Alonzo transactions to remain valid.

The recent PR #3595 added the Babbage era, changing type CardanoBlock = HardForkBlock [ByronBlock, ShelleyBlock, AllegraBlock, MaryBlock, AlonzoBlock] to type CardanoBlock = HardForkBlock [ByronBlock, ShelleyBlock, AllegraBlock, MaryBlock, AlonzoBlock, BabbageBlock]. From that change alone, due to the bug, the code stopped considering the Alonzo era as eternal, since it was no longer the final era. We now classify this assumption as a bug because it's clear that the (inevitable) addition of a new final era causes, via the bug, a non-mononotic change in behavior: for Alonzo (ie before we transition to Babbage), the Babbage-aware HFC now refuses to translate some slot<->times that it happily translated when Alonzo was the final era in the list.

The eras prior to Alonzo are unaffected because Alonzo introduced Plutus scripts and with them the requirement that the validity interval (specified as an interval between two slots) on an Alonzo transaction that contains Plutus scripts must be translatable to POSIX times, because the Plutus interpreter exposes the interval to the script as POSIX times, not as slots. The translation between slots and times is the responsibility of the HFC, because it depends on the slot duration, which is allowed to change during era transitions (eg it changed from 20s to 1s when the chain transitioned from Byron to Shelley; it has not yet changed a second time). The HFC is very careful with that translation, as you can see in the Time chapter in the Hard Fork Combinator section of the The Cardano Consensus and Storage Layer report. In particular, that chapter explains that the HFC refuses to translate slots<->times unless the answer would always remain correct regardless of any possible rollbacks (ie would be the same for any extension of our immutable tip, which is k blocks back from the tip of our currently selected chain).

The assumption that the final era does not end is in direct violation of that rule: if we assume the final era won't end, then we might translate a slot/time that is (currently!) 1000 years into the future -- and it's obvious that future activity on this chain is likely to change the correspondence between slots and times at some point during the next 1000 years! It wasn't until Alonzo's transaction validity interval check that this mattered, because that's the first (and so far only) slot<->time translation in the ledger rules that involves a user-defined slot (ie what they set as the transaction's validity interval bounds) -- all other translations are fixed by the ledger rules and are by design always within the range the HFC will translate (even after this PR's bugfix).

Thus, as a result of this PR, Babbage and subsequent eras will never be considered eternal, thereby satisfying the rule about all successful slot<->time translations being deterministic with respect to the selection's immutable tip. And PR IntersectMBO/cardano-ledger#2785 will intentionally violate that rule only during Alonzo, so that the historical transactions already on-chain remain valid. Because the consequences are currently limited to transaction validity intervals, there's no harm in that.

So-called "clients", such as db-sync, the wallet, the Cardano cli tools, etc may also exhibit a change in behavior due to this PR, but those at worst will be less convenient than they seemed before: any features of those tools that allowed the user to translate slot<->time well into the future will now refuse to do so. This PR changes no types, so that downstream code already has code paths that handle the HFC's refusal to translate a slot/time; they merely weren't being exercised for as many arguments as they should have been.

@nfrisby nfrisby added the consensus issues related to ouroboros-consensus label May 18, 2022
@nfrisby nfrisby requested review from dcoutts, nc6 and amesgen May 18, 2022 22:28
@nfrisby nfrisby force-pushed the nfrisby/reconstructSummary-bugfix branch from b2bdf98 to 5a54d68 Compare May 18, 2022 22:29
@nfrisby nfrisby added the Vasil_blocker Blocks the Vasil HF-ready release label May 18, 2022
Copy link
Contributor

@nc6 nc6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable. I've left one comment about a case which seems to me to be impossible.

@nfrisby nfrisby force-pushed the nfrisby/reconstructSummary-bugfix branch from 00a6987 to c7da969 Compare May 20, 2022 02:37
Copy link
Contributor

@dcoutts dcoutts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree this is the right thing.

There will be an observable change in behaviour for new clients talking to new nodes, where they will no longer be able to translate slot <-> time beyond the usual forecast window. This is of course the change we want, but it's possible that some clients were incorrectly trying to do conversions further into the future than ought to be possible. So we will need to rely on integration testing for those clients against the new node to check that they were not doing this. It ought to be noisy if they were.

@dcoutts dcoutts marked this pull request as ready for review May 20, 2022 13:43
@dcoutts dcoutts requested a review from dnadales as a code owner May 20, 2022 13:43
@nfrisby
Copy link
Contributor Author

nfrisby commented May 20, 2022

bors merge

@nfrisby
Copy link
Contributor Author

nfrisby commented May 20, 2022

bors retry

@iohk-bors
Copy link
Contributor

iohk-bors bot commented May 20, 2022

Already running a review

@nfrisby
Copy link
Contributor Author

nfrisby commented May 20, 2022

bors cancel

@iohk-bors
Copy link
Contributor

iohk-bors bot commented May 20, 2022

Canceled.

@nfrisby
Copy link
Contributor Author

nfrisby commented May 20, 2022

bors merge

@iohk-bors
Copy link
Contributor

iohk-bors bot commented May 20, 2022

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
consensus issues related to ouroboros-consensus Vasil_blocker Blocks the Vasil HF-ready release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants