TICKF calculatePoolDistr #3141
Closed
TICKF calculatePoolDistr #3141
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
It was only intended for TxBody to be causing a deserialization error on malformed Ptr, not the TxOut in the LedgerState or TxBody in eras prior to Babbage.
…ease-1-0-0 update plutus
Additionally, a new golden test for the alonzo fee calculation has been added, using the block from: IntersectMBO/cardano-node#4228 (comment)
…-release-1-0-0 the alonzo UTxO rule to use alonzo minfee function
I renamed _pstakeMark in the previous message, just so GHC would require me to update every location in which that field was set. I built everything in `ouroboros-network`, but not in `cardano-node` (I don't have the deps set correctly right now). This commit undoes the rename. The long term fix (instead of simply squashing this commit) would be to use PatternSynonyms or some such to ensure the pool distr thunk is updated everytime the mark snapshot changes. (One option is to instead add the thunk to each snapshot.)
Esgen and I poked and prodded the rule with pd' sharing. We do have evidence that it saves time, but: - I have only witnessed that on in the first few Shelley epoch transitions (209 through 211) - It didn't make as big of a difference as I had hoped. This commit is the result of my attempt to truly minimize the amount of calculation in TICKF. I've got its run-time down to about 10x slower than a TICKF that doesn't cross an epoch (for 209 through 2011), but there are significant subtleties to consider, which the comments in this commit only partly discuss. ***This is an exploratory Proof-Of-Concept only.***
aniketd
force-pushed
the
aniketd/nfrisby-share-tickf-calculatepooldistr
branch
from
b247359 to
a7dceba
Compare
…/nfrisby-share-tickf-calculatepooldistr
|
@nfrisby I have just merged your latest changes on to this one, which remains rebased on top of |
lehins
reviewed
Nov 14, 2022
lehins
reviewed
Nov 14, 2022
aniketd
force-pushed
the
aniketd/nfrisby-share-tickf-calculatepooldistr
branch
from
643fc77 to
9bf832d
Compare
aniketd
force-pushed
the
aniketd/nfrisby-share-tickf-calculatepooldistr
branch
from
9bf832d to
321842a
Compare
With a pattern-synonym for SnapShot to match its new data constructor SnapShotRaw. So, point-wise: 1. Move PoolDistr from SnapShots (the collection) to inside each SnapShot 2. Rename the data constructor SnapShot to SnapShotRaw 3. Introduce a pattern-synonym SnapShot with all fields except PoolDistr to match the new data constructor SnapShotRaw, to allow least change at sites where the older SnapShot data constructor was used. 4. Now a SnapShot's PoolDistr field can be accessed (without the pattern-synonym) using ssStakePoolDistr 5. All existing usage of de-constructing SnapShot now uses the pattern-synonym
aniketd
commented
Nov 17, 2022
There was a problem hiding this comment.
@nfrisby this commit moves the PoolDistr from the record SnapShots to inside each individual SnapShot as suggested by @TimSheard. I'd be glad to receive both your reviews on this change.
aniketd
force-pushed
the
aniketd/nfrisby-share-tickf-calculatepooldistr
branch
from
dca3fc1 to
4c46928
Compare
aniketd
force-pushed
the
aniketd/nfrisby-share-tickf-calculatepooldistr
branch
from
4c46928 to
587a306
Compare
Remove SnapShotRaw and corresponding pattern synonym. Particularly, revert 2319c8d. ----------------------------------------------------------------------- Noting its commit message here for posterity: Move PoolDistr from SnapShots to SnapShot. With a pattern-synonym for SnapShot to match its new data constructor SnapShotRaw. So, point-wise: 1. Move PoolDistr from SnapShots (the collection) to inside each SnapShot 2. Rename the data constructor SnapShot to SnapShotRaw 3. Introduce a pattern-synonym SnapShot with all fields except PoolDistr to match the new data constructor SnapShotRaw, to allow least change at sites where the older SnapShot data constructor was used. 4. Now a SnapShot's PoolDistr field can be accessed (without the pattern-synonym) using ssStakePoolDistr 5. All existing usage of de-constructing SnapShot now uses the pattern-synonym
|
@JaredCorduan I have updated this PR to reflect our latest discussion.
This is ready for your review! |
aniketd
requested review from
TimSheard,
teodanciu and
JaredCorduan
and removed request for
JaredCorduan,
nfrisby and
TimSheard
|
replaced by #3209 |
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.
patternimplementation and movecalcfrom withinSnapShotRawto withinSnapShotsas it was earlier.References
Closes #3132