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

Updating cardano-ledger #2455

Closed
wants to merge 1 commit into from
Closed

Conversation

JaredCorduan
Copy link
Contributor

I have updated the cardano ledger package to the current master, and ouroboros-network to my last commit in IntersectMBO/ouroboros-network#2971.

  • The ledger state now stores an PulsingRewUpdate instead of a
    RewardUpdate. The PulsingRewUpdate allows us to compute the staking
    rewards incrementally instead of all at once. This involves a new
    serialization format for the reward updates.
  • Annotated decoders are now used for PParams and PParamsDelta (the
    data structures now store their own serialization). Additionally, the
    LedgerState also now uses an annotated decoder. The serialization in
    consensus has been changed to use the cbor-in-cbor encoding for these
    types (including in the queries).
  • A bug was fixed in the reward calculation which was introduced in
    commit 31083fc5cecbe17f3628393072094b130058edd9.
  • The ledger repo now uses ghc-8.10.4 and cabal-3.4.0.0.

Question is it okay that I have removed the FromCBOR instances of the protocol parameter updates and the ledger state? I did add the necessary functions to serialize them.

@@ -247,6 +248,10 @@ instance Crypto.Crypto crypto => ToJSON (Shelley.RewardUpdate crypto) where
, "nonMyopic" .= Shelley.nonMyopic rUpdate
]

instance Crypto.Crypto crypto => ToJSON (Shelley.PulsingRewUpdate crypto) where
toJSON (Shelley.Pulsing _ _) = "calculating"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

is this alright?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think if we're not interested in the intermediate calculation Aeson.Null would probably be more appropriate.

Copy link
Contributor

Choose a reason for hiding this comment

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

There is only a ToJSON instance and no FromJSON instance. Is this because the output is only intended as debugging output for consumption by 3rd party tools? Or something along those lines?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's my understanding @DavidEichmann , but @Jimbo4350 would know better.

Copy link
Contributor

@gitmachtl gitmachtl Mar 8, 2021

Choose a reason for hiding this comment

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

There is only a ToJSON instance and no FromJSON instance. Is this because the output is only intended as debugging output for consumption by 3rd party tools? Or something along those lines?

yes. almost every SPO is using the JSON output for daily operation, calculations, leaderlogs, etc... or are we talking about another ToJSON here?

@JaredCorduan JaredCorduan force-pushed the jc/update-ledger-to-ba74779256 branch 2 times, most recently from 0abbdd9 to 9bd83f0 Compare March 5, 2021 19:06
( ShelleyLedgerEra era ~ ledgerera,
Consensus.ShelleyBasedEra ledgerera
) =>
Decoder s (LBS.ByteString -> LedgerState era)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am now considering if it would be better to do the cbor-in-cbor encoding in the ledger package itself, so that there could be a FromCBOR instance of LedgerState and thus break less downstream. I'll maybe tackle that on Monday if the others agree.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've talked with @nc6 about this, and we decided that not having these odd cbor-in-cbor FromCBOR instances is probably best. So now I just need to make sure that node is adapted to handle the Annotator (LedgerState era).

* The ledger state now stores an PulsingRewUpdate instead of a
  RewardUpdate. The PulsingRewUpdate allows us to compute the staking
  rewards incrementally instead of all at once. This involves a new
  serialization format for the reward updates.
* Annotated decoders are now used for PParams and PParamsDelta (the
  data structures now store their own serialization). Additionally, the
  LedgerState also now uses an annotated decoder. The serialization in
  consensus has been changed to use the cbor-in-cbor encoding for these
  types (including in the queries).
* A bug was fixed in the reward calculation which was introduced in
  commit 31083fc5cecbe17f3628393072094b130058edd9.
* The ledger repo now uses ghc-8.10.4 and cabal-3.4.0.0.
@JaredCorduan JaredCorduan force-pushed the jc/update-ledger-to-ba74779256 branch from 465db5e to b6320e0 Compare March 8, 2021 22:03
@JaredCorduan JaredCorduan marked this pull request as ready for review March 8, 2021 22:04
@JaredCorduan JaredCorduan marked this pull request as draft March 9, 2021 04:00
@JaredCorduan
Copy link
Contributor Author

I converted this back to draft, since the cbor-in-cbor encoding in the consensus query needs to be reverted.

@gufmar
Copy link
Contributor

gufmar commented Mar 9, 2021

As someone who is not directly involved, I would like to add a thought:
The reward calculation takes place with a safety margin of 48 hours after the last epoch change. the payment takes place days later.
So there should be no scientific or technical reason why this calculation is carried out by all nodes at exactly the same time. Theoretically, it should be sufficient to define that the calculation must be completed within a time window of e.g. 4 hours from the 48h point. With the current or even lower saturation point, a large pool currently generates about 55 blocks per epoch, i.e. one every 2 hours on average. With the current fixed and uncoordinated 48h starting point, however, it is still certain to hit about 20-40 block generators.
What is the objection to the fact that for this time frame of the rewards calculation, an advance determination of the allocated slots is made internally in the node, e.g. for all slots of the next 20 minutes. The reward calculation only takes place if it becomes apparent that no slot is pending anyway. Otherwise, a new check is carried out after this block, up to a maximum of e.g. +4 hours. Then the reward calculation is carried out even if a block is pending, which will be a practically very rare case.

@JaredCorduan
Copy link
Contributor Author

That is correct @gufmar , the nodes can do the calculation at any point in the epoch after the stability window. We have a fix in the ledger code that essentially just computes the rewards for a a small number of pools (probably one given the current number of registered stake pools) at a time. see IntersectMBO/cardano-ledger#2142

We are just trying to handle all the dependencies now.

@JaredCorduan
Copy link
Contributor Author

Replaced by #2474

jc/update-ledger-to-ba74779256 was an overly-ambitious (and now incorrect) branch name 😄

iohk-bors bot added a commit that referenced this pull request Mar 13, 2021
2474: Updating cardano-ledger r=JaredCorduan a=JaredCorduan

I have updated the cardano ledger package to the current master, and ouroboros-network to my last commit in IntersectMBO/ouroboros-network#2984

This replaces #2455, since `jc/update-ledger-to-ba74779256` was an overly-ambitious (and now incorrect) branch name. 😄 

* The ledger state now stores an PulsingRewUpdate instead of a
  RewardUpdate. The PulsingRewUpdate allows us to compute the staking
  rewards incrementally instead of all at once. This involves a new
  serialization format for the reward updates.
* A bug was fixed in the reward calculation which was introduced in
  commit 31083fc5cecbe17f3628393072094b130058edd9.
  (the bug was not a part of any release.)
* The MIR certificates now have additional functionality, but such
  functionality is prohibited until protocol major version 5. Binary
  compatibility is preserved for the original functionality. The new
  functionality is the ability to transfer lovelace between the reserves
  and the treasury.
* The ledger repo now uses ghc-8.10.4 and cabal-3.4.0.0.

Co-authored-by: Jared Corduan <jaredcorduan@gmail.com>
@JaredCorduan JaredCorduan deleted the jc/update-ledger-to-ba74779256 branch April 16, 2021 11:28
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.

5 participants