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.

Sign up for GitHub

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

Jump to bottom

Integrate cardano-ledger for node-8.11.0 #1069

Merged
merged 5 commits into from
May 13, 2024
Merged

Integrate cardano-ledger for node-8.11.0 #1069

merged 5 commits into from
May 13, 2024

Conversation

Lucsanszky
Copy link
Collaborator

@Lucsanszky Lucsanszky commented Apr 18, 2024 •
edited by amesgen
Loading

@Lucsanszky
Copy link
Collaborator Author

Obviously, this is far from done but I opted to open a draft early so everyone can track the progress of the integration.

One thing that is worth mentioning right away: This removes the Crypto c constraint from exampleConwayGenesis and uses StandardCrypto instead. I made some changes to the affected test to make it compile but I haven't ran it yet. It is now using StandardCrypto instead of MockCryptoCompatByron. Is this okay or should we figure out something else?

cc: @dnadales

@Lucsanszky Lucsanszky force-pushed the lucsanszky/integration-8.11 branch 3 times, most recently from db59b48 to 6e9fca6 Compare April 24, 2024 01:26
@Lucsanszky Lucsanszky mentioned this pull request Apr 24, 2024
Closed
@Lucsanszky Lucsanszky force-pushed the lucsanszky/integration-8.11 branch 3 times, most recently from ad3720a to 45e379c Compare April 26, 2024 01:24
@amesgen

This comment was marked as resolved.

Sign in to view
@Lucsanszky Lucsanszky force-pushed the lucsanszky/integration-8.11 branch 2 times, most recently from e87e596 to 0a1a154 Compare April 30, 2024 23:23
@Lucsanszky Lucsanszky force-pushed the lucsanszky/integration-8.11 branch 2 times, most recently from 5530023 to 3f873d6 Compare May 9, 2024 01:56
@amesgen amesgen force-pushed the lucsanszky/integration-8.11 branch from 3f873d6 to cc97ed1 Compare May 13, 2024 12:29
@amesgen amesgen changed the title Integrate cardano-ledger and ouroboros-network for node-8.11.0 (SRP) Integrate cardano-ledger for node-8.11.0 May 13, 2024
@amesgen amesgen changed the base branch from main to coot/ouroboros-network-0.16 May 13, 2024 12:29
@amesgen amesgen force-pushed the lucsanszky/integration-8.11 branch from cc97ed1 to 3afee0a Compare May 13, 2024 12:34
Base automatically changed from coot/ouroboros-network-0.16 to main May 13, 2024 16:19
disassembler and others added 5 commits May 13, 2024 13:08
@disassembler
CHaP: bump index state
1fc53f3
@Lucsanszky @disassembler
Account for cardano-ledger changes
2353df1 
  * Update `index-state` and affected `cardano-ledger` version bounds:
    `cardano-ledger` requires `plutus-ledger-api ^>=1.26.0`,
    hence the `index-state` update

  * Use `StandardCrypto` in protocol info tests:
    `exampleConwayGenesis` was made monomorphic here:
    IntersectMBO/cardano-ledger#4252
    The `Crypto c` constraint was removed and now it uses
    `StandardCrypto`.

  * Add `plutusV3CostModel` to `Conway` genesis file

  * Update `SupportsTwoPhaseValidation` instance for `Conway`:
    `ConwayUtxowFailure` was fixed to get rid of the possibility
    of injecting `UtxoFailure` in two separate ways
    (as seen in the code removed by this commit)
@Lucsanszky @disassembler
Utilize new PParams prediction functionality for HFC
18f186d 
  * Use the updated behavior
  * Fixup haddock to reflect generality of protocol version update
@Lucsanszky @disassembler
Accept new cbor format for golden tests
5958f1c 
Change in serialization is due to the addition of future
`PParams` to the `LedgerState`
@Lucsanszky @disassembler
Update CHANGELOG fragment
0fc34d8
@disassembler disassembler force-pushed the lucsanszky/integration-8.11 branch from 1bcb75e to 0fc34d8 Compare May 13, 2024 17:09
@disassembler disassembler marked this pull request as ready for review May 13, 2024 17:09
@disassembler disassembler requested a review from a team as a code owner May 13, 2024 17:09
amesgen
amesgen approved these changes May 13, 2024
View reviewed changes
ouroboros-consensus-cardano/test/cardano-test/Test/ThreadNet/Cardano.hs
type Crypto = MockCryptoCompatByron
type Crypto = StandardCrypto
Copy link
Member

@amesgen amesgen May 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This seems fine for now given that it is only about tests and those tests pass, but we should look into the exact implications of this change at some point (also see #1069 (comment)).

Copy link
Member

Choose a reason for hiding this comment

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

The only difference between MockCryptoCompatByron and StandardCrypto is in the VRF and KES:

  • MockKES doesn't do any evolution.
  • MockKES and MockVRF are faster (see below).
  • The types involved in MockKES and MockVRF are potentially simpler to construct (though we mostly use Arbitrary instances that are defined for any Crypto).
  • MockKES and MockVRF yield more compact output in QuickCheck counterexamples.

A small benchmark with hyperfine yields that the Cardano ThreadNet is now ~50% slower:

hyperfine -L crypto MockCryptoCompatByron,StandardCrypto './cardano-test-{crypto} -p "Cardano ThreadNet" --quickcheck-tests=2 --quickcheck-replay=0'
Command Mean [s] Min [s] Max [s] Relative
MockCryptoCompatByron 6.319 ± 1.329 5.350 9.020 1.00
StandardCrypto 9.547 ± 2.617 7.489 15.095 1.51 ± 0.52

Overall CI time of the entire cardano-test suite didn't regress by the same factor as it does lots of other things (see Hydra), it seems to be longer by 20s from 5min20s to 5min40s.

Also, I don't think that the usage of StandardCrypto in ledger is absolutely necessary; it could be generalized to any Crypto c where just the hashes/signatures are constrained, but the VRF/KES is left unconstrained.

Given that larger changes in the crypto parameterization are planned1 I am inclined to leave the code as is for now.

Footnotes

  1. See https://github.com/IntersectMBO/cardano-ledger/issues/4223, which would make all ledger definitions crypto-monomorphic, but removing KES/VRF from Crypto to sth like HeaderCrypto, which would solve the issue at hand (as MockCryptoCompatByron and StandardCrypto only differ in header crypto). ↩

ouroboros-consensus-cardano/src/shelley/Ouroboros/Consensus/Shelley/Ledger/Inspect.hs
Comment on lines -164 to +52
inspectLedger tlc before after = do
inspectLedger _tlc before after = do
guard $ updatesBefore /= updatesAfter
return $ LedgerUpdate $ ShelleyUpdatedProtocolUpdates updatesAfter
return $ LedgerUpdate updatesAfter
where
genesis :: SL.ShelleyGenesis (EraCrypto era)
genesis = shelleyLedgerGenesis (configLedger tlc)

updatesBefore, updatesAfter :: [ProtocolUpdate era]
updatesBefore = protocolUpdates genesis before
updatesAfter = protocolUpdates genesis after
updatesBefore, updatesAfter :: ShelleyLedgerUpdate era
updatesBefore = pparamsUpdate before
updatesAfter = pparamsUpdate after
Copy link
Member

Choose a reason for hiding this comment

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

FTR: With the new code, we will log one LedgerUpdate in every epoch (when we first tick across the Shelley voting deadline, ie 6k/f before the end of the epoch), even if nothing changed. This is in contrast to the previous state of the code. This seems fine to me, at least for now.

@amesgen amesgen added this pull request to the merge queue May 13, 2024
Merged via the queue into main with commit 69c7e6d May 13, 2024
15 checks passed
@amesgen amesgen deleted the lucsanszky/integration-8.11 branch May 13, 2024 18:38
@Lucsanszky Lucsanszky mentioned this pull request May 16, 2024
Closed
24 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@amesgen amesgen amesgen approved these changes

@jasagredo jasagredo jasagredo left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Lucsanszky @amesgen @jasagredo @disassembler