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

Improve resilience of future PParams #4433

Merged
merged 2 commits into from
Jun 27, 2024

Conversation

lehins
Copy link
Collaborator

@lehins lehins commented Jun 20, 2024

Description

This PR adds a test for futurePParams prediction and fixes a problem where it would not work correctly when there was no TICKs before the point of no return (first 4k/f slots)

This isn't really a problem in practice, because of this property that Sam pointed out:

if there isn't a single block in 3k/f slots, the ledger state goes out of view and no further blocks can be made

But it is nice to fix it regardless, because it simplifies the futurePParams state transition a bit

Checklist

  • Commit sequence broadly makes sense and commits have useful messages
  • New tests are added if needed and existing tests are updated
  • When applicable, versions are updated in .cabal and CHANGELOG.md files according to the
    versioning process.
  • The version bounds in .cabal files for all affected packages are updated. If you change the bounds in a cabal file, that package itself must have a version increase. (See RELEASING.md)
  • All visible changes are prepended to the latest section of a CHANGELOG.md for the affected packages. New section is never added with the code changes. (See RELEASING.md)
  • Code is formatted with fourmolu (use scripts/fourmolize.sh)
  • Cabal files are formatted (use scripts/cabal-format.sh)
  • hie.yaml has been updated (use scripts/gen-hie.sh)
  • Self-reviewed the diff

@lehins lehins changed the title Improve resilience of future p params Improve resilience of future PParams Jun 20, 2024
@lehins lehins force-pushed the lehins/improve-resilience-of-futurePParams branch from 52eae67 to 86ae8a3 Compare June 20, 2024 22:39
@lehins lehins requested a review from teodanciu June 22, 2024 02:18
@teodanciu
Copy link
Contributor

I suppose we don't have this hypothetical problem for Shelley, because there PotentialPParamsUpdate gets directly computed based on the votes rather than on the predicted-solidifed values, right?

@teodanciu
Copy link
Contributor

If you want to keep the documentation up to date, you could add this to the paragraph explaining the changes at the epoch boundary:
We also predict the parameters after resetting the pulser, in order to cover - for completion - the case in which there is noTICKbefore the first4k/f slots.

Copy link
Contributor

@teodanciu teodanciu left a comment

Choose a reason for hiding this comment

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

Looks good to me, just some questions for my own understanding.

@lehins lehins force-pushed the lehins/improve-resilience-of-futurePParams branch from 86ae8a3 to 322bf0a Compare June 25, 2024 00:18
@lehins lehins enabled auto-merge June 25, 2024 00:21
@lehins lehins force-pushed the lehins/improve-resilience-of-futurePParams branch from 322bf0a to 9cf6e66 Compare June 27, 2024 03:23
lehins added 2 commits June 26, 2024 22:52
Ensure that every Conway Epoch begins with a correct prediction for
`futurePParams`. The problem that this commit fixes has to do with the
fact that `futurePParams` where correctly predicted on the very first
`TICK`. However, if there are no blocks prior to point-of-no-return,
which can't happen on a normal running chain, there would be no `TICK`,
thus there would be no prediction. So, regardless if there was a correct
prediciton after the point-of-no-return, `PParamUpdate` would be simply
ignored, since `futurePParams` would be already set to `NoPParamUpdate`.

Despite the fact that this can't really happen on mainnet, we can fix
this quite easily by predicting `futurePParams` during DRepPulser
initialization, which is exactly what being imlemented in this commit.
@lehins lehins force-pushed the lehins/improve-resilience-of-futurePParams branch from 9cf6e66 to b7b9e8d Compare June 27, 2024 04:52
@lehins lehins merged commit 16f57e4 into master Jun 27, 2024
125 checks passed
@lehins lehins deleted the lehins/improve-resilience-of-futurePParams branch June 27, 2024 06:31
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.

2 participants