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

Clean hardfork switches #3162

Merged
merged 3 commits into from
Nov 28, 2022
Merged

Clean hardfork switches #3162

merged 3 commits into from
Nov 28, 2022

Conversation

teodanciu
Copy link
Contributor

@teodanciu teodanciu commented Nov 21, 2022

Closes: #3104

Answers the question: which HardFork switches can we remove from the code without affecting the state

How I checked:

  1. Checked out cardano-node (tag 1.35.3) and synced up with mainnet until slot 76138419 (the tip at the time)
  2. Checked out db-analyser (part of ouroboros-network), at branch release/cardano-node-1.35.x
  3. Took snapshots of the ledger state, using db-analyser, corresponding to the first slots of each era
  4. For each tested switch:
    • set the switch to True in cardano-ledger in a branch off tag node/1.35.3
    • run db-analyzer pointing to the aforementioned cardano-ledger branch:
      • analyzing from a slot that is not affected by the switch (earlier than the protocol version from the condition)
      • taking a snapshot of the ledger state for slots corresponding to the snapshots taken at step 2
    • this db-analyzer had two possible outcomes: an error or a snapshot. The error is an indication that the switch cannot be removed. In case there was no error, the sha256 sum of the produced snapshot was compared with that of the corresponding snapshot produced at step 2 (using the code with the HardFork switch intact)

1. allowMIRTransfer pp = pvMajor (getField @"_protocolVersion" pp) > 4 ❎

Ran:

cabal run ouroboros-consensus-cardano-tools:db-analyser2 -- --db /home/teodora/cardano-mainnet/db/db-mainnet --only-immutable-db --analyse-from 23068800 --store-ledger 39916975  cardano --config /home/teodora/cardano-mainnet/config.json

because 23068800 is the first slot in Mary (version 4) and 39916975 is the first slot in the next era

This resulted in an error: ExtValidationErrorLedger (HardForkLedgerErrorFromEra S (S (S (Z (WrapLedgerErr {unwrapLedgerErr = BBodyError (BlockTransitionError [LedgersFailure (LedgerFailure (DelegsFailure (WithdrawalsNotInRewardsDELEGS (fromList [(RewardAcnt {getRwdNetwork = Mainnet, getRwdCred = KeyHashObj (KeyHash "7f5287ac749c44f8b517165a35ea60a0a165904b25da29749d2366e4")},Coin 220383)]))))])})))))

So I concluded it can't be removed.


2. aggregatedRewards pp = pvMajor (getField @"_protocolVersion" pp) > 2 ❎

set to True, trying to take a snapshot from 4492800 (first Shelley slot(v2)) to 16588800 (first Allegra slot (v3)) resulted in:
ExtValidationErrorLedger (HardForkLedgerErrorFromEra S (Z (WrapLedgerErr {unwrapLedgerErr = BBodyError (BlockTransitionError [LedgersFailure (LedgerFailure (DelegsFailure (WithdrawalsNotInRewardsDELEGS (fromList [(RewardAcnt {getRwdNetwork = Mainnet, getRwdCred = KeyHashObj (KeyHash "24f1b3791037e55363815fc4b788b886cd1d82a2f2d39e17196cd1a4")},Coin 431236474)]))))])})))


3. validatePoolRewardAccountNetID pp = pvMajor (getField @"_protocolVersion" pp) > 4 ❎

set to True, trying to take a snapshot from 23068800 (first Mary slot(v2)) to 39916975 (first Alonzo slot (v3)) resulted in: ExtValidationErrorLedger (HardForkLedgerErrorFromEra S (S (S (Z (WrapLedgerErr {unwrapLedgerErr = BBodyError (BlockTransitionError [LedgersFailure (LedgerFailure (DelegsFailure (DelplFailure (PoolFailure (WrongNetworkPOOL Mainnet Testnet (KeyHash "ae8ee8f5676a3c3aadefd0ee7adb57ebbd39dd7126c8517e718dc4c1"))))))])})))))


4. forgoRewardPrefilter pp = pvMajor (getField @"_protocolVersion" pp) > 6 ❎

set to True, trying to take a snapshot from 43372972 (v6) to 76138419 resulted in: ExtValidationErrorLedger (HardForkLedgerErrorFromEra S (S (S (S (Z (WrapLedgerErr {unwrapLedgerErr = BBodyError (BlockTransitionError [ShelleyInAlonzoPredFail (LedgersFailure (LedgerFailure (DelegsFailure (DelplFailure (DelegFailure (StakeKeyNonZeroAccountBalanceDELEG (Just (Coin 658031))))))))])}))))))


5. translateTimeForPlutusScripts pp = pvMajor (getField @"_protocolVersion" pp) > 5 ✔️

set to True, a snapshot of 76138419 (latest slot that I had synced with mainnet) was generated with the same sha256 of the snapshot taken with the switch enabled (9aafb9e44500f84d2a59dfd8019934dd42a947edf976c35d90fa6bb4136604dd )


6. allowScriptStakeCredsToEarnRewards pp = pvMajor (getField @"_protocolVersion" pp) > 4 ✔️

set to True, a snapshot of 76138419 with the original sha256 is generated


7. allowOutsideForecastTTL pp =let mv = pvMajor (getField @"_protocolVersion" pp) in mv == 5 || mv == 6 ✔️

set to True, a snapshot of 76138419 with the original sha256 is generated


8. missingScriptsSymmetricDifference ❔

I forgot why we can't switch this off even if the state is the same

@teodanciu teodanciu changed the base branch from master to td/fix-build November 21, 2022 10:38
@teodanciu teodanciu force-pushed the td/clean-hardfork-switches branch from ab049d4 to aad1445 Compare November 21, 2022 10:39
@teodanciu teodanciu changed the base branch from td/fix-build to master November 21, 2022 10:39
@teodanciu teodanciu force-pushed the td/clean-hardfork-switches branch 3 times, most recently from 4981d58 to aea58b9 Compare November 25, 2022 13:47
@teodanciu teodanciu marked this pull request as ready for review November 25, 2022 13:48
@teodanciu teodanciu force-pushed the td/clean-hardfork-switches branch from aea58b9 to b32a155 Compare November 25, 2022 15:17
@teodanciu teodanciu requested a review from a team as a code owner November 25, 2022 15:17
@teodanciu teodanciu force-pushed the td/clean-hardfork-switches branch 2 times, most recently from 6f5510c to 9e86252 Compare November 25, 2022 17:59
Copy link
Contributor

@JaredCorduan JaredCorduan left a comment

Choose a reason for hiding this comment

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

looks great, thank you @teodanciu , wonderful clean up! (my only comment is about the orig file).

@teodanciu teodanciu force-pushed the td/clean-hardfork-switches branch from 9e86252 to bca24e6 Compare November 28, 2022 14:07
since it was verified that removing it creates the same ledger state,
proving the switch unnecessary.
@teodanciu teodanciu force-pushed the td/clean-hardfork-switches branch from bca24e6 to fd3d310 Compare November 28, 2022 15:07
Copy link
Collaborator

@lehins lehins left a comment

Choose a reason for hiding this comment

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

This is a great cleanup!!! Thank you.
Just one minor comment

eras/shelley/impl/src/Cardano/Ledger/Shelley/Rewards.hs Outdated Show resolved Hide resolved
since it was verified that removing it creates the same ledger state,
proving the switch unnecessary.
since it was verified that removing it creates the same ledger state,
proving the switch unnecessary.
@teodanciu teodanciu force-pushed the td/clean-hardfork-switches branch from fd3d310 to ed690e9 Compare November 28, 2022 18:17
@teodanciu teodanciu merged commit 3c602e1 into master Nov 28, 2022
@iohk-bors iohk-bors bot deleted the td/clean-hardfork-switches branch November 28, 2022 22:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clean up protocol version switching
3 participants