New query to get snapshots #3898

newhoggy · 2022-07-14T03:33:47Z

Description

This introduce a new query GetStakeSnapshots which extracts the stake snapshots for all pools or for a specified subset of pools. This query is required to improve the CPU and memory efficiency of the query stake-snapshot command in the CLI.

Checklist

Branch
- Commit sequence broadly makes sense
- Commits have useful messages
- New tests are added if needed and existing tests are updated
- If this branch changes Consensus and has any consequences for downstream repositories or end users, said changes must be documented in interface-CHANGELOG.md
- If this branch changes Network and has any consequences for downstream repositories or end users, said changes must be documented in interface-CHANGELOG.md
- If serialization changes, user-facing consequences (e.g. replay from genesis) are confirmed to be intentional.
Pull Request
- Self-reviewed the diff
- Useful pull request description at least containing the following information:
  - What does this PR change?
  - Why these changes were needed?
  - How does this affect downstream repositories and/or end-users?
  - Which ticket does this PR close (if any)? If it does, is it linked?
- Reviewer requested

`cardano-api` changes

The existing query, QueryDebugLedgerState, will continue to exist and be slow and use a lot of memory, however, there will be a new GetStakeSnapshot which uses a lot less CPU and memory:

GetStakeSnapshot
    :: SL.KeyHash 'SL.StakePool (EraCrypto era)
    -> BlockQuery (ShelleyBlock proto era)
                  (StakeSnapshot (EraCrypto era))

This query takes a set of pool ids so it is possible to query multiple pools at once.

`cardano-cli` changes

A existing command query stake-snapshot will be be functionally unchanged, but use much less CPU and memory.

ouroboros-consensus-shelley/src/Ouroboros/Consensus/Shelley/Ledger/Query.hs

nfrisby

I made some comments throughout. HTH. Thanks!

ouroboros-consensus-shelley/src/Ouroboros/Consensus/Shelley/Ledger/Query.hs

nfrisby · 2022-07-14T15:46:04Z

ouroboros-consensus-shelley/src/Ouroboros/Consensus/Shelley/Ledger/Query.hs

+    GetPoolState {}                            -> fromCBOR
+    GetStakeSnapshot {}                        -> fromCBOR
+
+data StakeSnapshot crypto = StakeSnapshot


Please add a Haddock comment explaining what this is.

Also, I think these fields shouldn't be Integers, but rather SL.Coin?

Added documentation.

Why not return SnapShots crypto? It seems like we are losing potentially useful information i.e the delegations and pool params. The consumer can do these calculations (indeed we can expose functions from cardano-api to do this)

It is define like this:

data SnapShots crypto = SnapShots { _pstakeMark :: SnapShot crypto, -- Lazy on purpose _pstakeSet :: !(SnapShot crypto), _pstakeGo :: !(SnapShot crypto), _feeSS :: !Coin } deriving (Show, Eq, Generic) data SnapShot crypto = SnapShot { _stake :: !(Stake crypto), _delegations :: !(VMap VB VB (Credential 'Staking crypto) (KeyHash 'StakePool crypto)), _poolParams :: !(VMap VB VB (KeyHash 'StakePool crypto) (PoolParams crypto)) } deriving (Show, Eq, Generic)

The maps stored are very big, so in the interest of improving memory and cpu efficiency and not transferring more data than necessary over the socket, I do the map lookup on the node side.

Why not return SnapShots crypto?

The maps stored are very big [..., so] I do the map lookup on the node side

On Slack, Jordan also asked: "Should we allow the possibility to get the stake snapshot for multiple pools at once?"

One option occurs to me: we return a SnapShots crypto (so we don't have to define a new type 🙌) but we restrict all the maps inside that data type to just the keys given (so we support a multi-pool query 🙌).

On the slack thread, you agreed that one-pool at a time is fine for the first PR. So that's not urgent.

But avoiding a new data type might be nice.

So: does the resuse of SnapShots crypto but with all the maps restricted down to the pools identified by the query make sense?

One immediate downside: such a restricted SnapShot is probably counter-intuitive: eg it violates some generally-expected invariants of SnapShot (along the lines of completeness)

ouroboros-consensus-shelley/src/Ouroboros/Consensus/Shelley/Ledger/Query.hs

ouroboros-consensus-cardano/src/Ouroboros/Consensus/Cardano/Node.hs

Jimbo4350

https://github.com/input-output-hk/ouroboros-network/pull/3898/files?diff=unified&w=0#r925578039

replied to comments

dnadales · 2022-07-25T14:14:46Z

ouroboros-consensus/docs/interface-CHANGELOG.md

@@ -77,6 +77,7 @@ https://keepachangelog.com/en/1.1.0/, adapted to our plan explained above.

 - New supported node to client version `NodeToClientV_14` with new queries:
  - `GetPoolState`: Get the pool state for the given stake pool ids
+  - `GetStakeSnapshot`: Get the snapshot of stake distribution


Why do we write this entry here? Why not in a section above 2022-07-20?

I've moved it to a new section.

nfrisby

I made some new comments. But everything is relatively minor (though not necessarily cheap), so I'm clicking Approve.

Thanks for responding to the previous comments 🙏

ouroboros-consensus-shelley/src/Ouroboros/Consensus/Shelley/Ledger/Query.hs

nfrisby · 2022-08-01T16:02:20Z

ouroboros-consensus-shelley/src/Ouroboros/Consensus/Shelley/Ledger/Query.hs

+    GetPoolState {}                            -> fromCBOR
+    GetStakeSnapshot {}                        -> fromCBOR
+
+data StakeSnapshot crypto = StakeSnapshot


Why not return SnapShots crypto?

The maps stored are very big [..., so] I do the map lookup on the node side

On Slack, Jordan also asked: "Should we allow the possibility to get the stake snapshot for multiple pools at once?"

One option occurs to me: we return a SnapShots crypto (so we don't have to define a new type 🙌) but we restrict all the maps inside that data type to just the keys given (so we support a multi-pool query 🙌).

On the slack thread, you agreed that one-pool at a time is fine for the first PR. So that's not urgent.

But avoiding a new data type might be nice.

So: does the resuse of SnapShots crypto but with all the maps restricted down to the pools identified by the query make sense?

One immediate downside: such a restricted SnapShot is probably counter-intuitive: eg it violates some generally-expected invariants of SnapShot (along the lines of completeness)

ouroboros-consensus-shelley/src/Ouroboros/Consensus/Shelley/Ledger/Query.hs

newhoggy · 2022-08-02T11:46:23Z

ouroboros-consensus-shelley/src/Ouroboros/Consensus/Shelley/Ledger/Query.hs

+          case mPoolIds of
+            Nothing ->
+              StakeSnapshots
+                { ssStakeSnapshots = getPoolStakes (Set.fromList (VMap.elems (SL._delegations _pstakeMark)))


I'm assuming that if I get the elems of _delegations _pstakeMark that it would give me all of the pool ids.

I'm not confident that this is true. Please validate.

That is correct @newhoggy. _delegations _pstakeMark is a map from stake credentials to stake pool IDs. You can tell from they type, if you happen to know that the stake pool IDs are the hash of the stake pool operator's cold verification key:

https://github.com/input-output-hk/cardano-ledger/blob/93b5d5421a5b5c3a3b6ad89a74e8bdb2ccc9258c/eras/shelley/impl/src/Cardano/Ledger/Shelley/EpochBoundary.hs#L157

nfrisby · 2022-08-02T13:15:06Z

ouroboros-consensus-shelley/src/Ouroboros/Consensus/Shelley/Ledger/Query.hs

+      <*> fromCBOR
+
+data StakeSnapshots crypto = StakeSnapshots
+  { ssStakeSnapshots  :: Map (SL.KeyHash 'SL.StakePool crypto) (StakeSnapshot crypto)


This is the strict Map, so I think this field should also be strict.

newhoggy · 2022-08-05T11:28:01Z

bors r+

iohk-bors · 2022-08-05T12:44:03Z

Build succeeded:

newhoggy commented Jul 14, 2022

View reviewed changes

ouroboros-consensus-shelley/src/Ouroboros/Consensus/Shelley/Ledger/Query.hs Show resolved Hide resolved

newhoggy force-pushed the newhoggy/new-query-to-get-snapshots branch from e8a3fa0 to 24660db Compare July 14, 2022 04:14

nfrisby reviewed Jul 14, 2022

View reviewed changes

newhoggy force-pushed the newhoggy/new-query-to-get-snapshots branch 5 times, most recently from 917b819 to e836ce8 Compare July 19, 2022 13:06

newhoggy changed the title ~~Newhoggy/new query to get snapshots~~ New query to get snapshots Jul 19, 2022

newhoggy marked this pull request as ready for review July 19, 2022 13:13

newhoggy requested review from dnadales and coot as code owners July 19, 2022 13:13

CarlosLopezDeLara mentioned this pull request Jul 19, 2022

High memory usage when running query stake-snapshot and others. IntersectMBO/cardano-node#4140

Closed

4 tasks

newhoggy force-pushed the newhoggy/new-query-to-get-snapshots branch from e836ce8 to eb5e856 Compare July 19, 2022 22:43

newhoggy requested a review from nfrisby July 19, 2022 22:47

newhoggy force-pushed the newhoggy/new-query-to-get-snapshots branch 3 times, most recently from 01549ff to 551db30 Compare July 20, 2022 01:11

newhoggy mentioned this pull request Jul 20, 2022

Optimise query stake snapshot command IntersectMBO/cardano-node#4179

Merged

Jimbo4350 previously requested changes Jul 20, 2022

View reviewed changes

newhoggy requested a review from Jimbo4350 July 21, 2022 02:17

newhoggy force-pushed the newhoggy/new-query-to-get-snapshots branch from 551db30 to b646f87 Compare July 25, 2022 09:57

dnadales reviewed Jul 25, 2022

View reviewed changes

newhoggy force-pushed the newhoggy/new-query-to-get-snapshots branch from b646f87 to 4fcb5c7 Compare July 26, 2022 11:50

newhoggy requested a review from dnadales July 27, 2022 10:34

New query to get SnapShots

172ae40

newhoggy force-pushed the newhoggy/new-query-to-get-snapshots branch from 4fcb5c7 to 172ae40 Compare August 1, 2022 13:24

nfrisby approved these changes Aug 1, 2022

View reviewed changes

newhoggy commented Aug 2, 2022

View reviewed changes

newhoggy force-pushed the newhoggy/new-query-to-get-snapshots branch 2 times, most recently from 92fd09c to a106d1c Compare August 2, 2022 12:52

nfrisby reviewed Aug 2, 2022

View reviewed changes

newhoggy force-pushed the newhoggy/new-query-to-get-snapshots branch 2 times, most recently from b70ba6a to a0d8d63 Compare August 3, 2022 10:45

Make it possible to query stake snapshots for multiple pools.

ff55347

newhoggy force-pushed the newhoggy/new-query-to-get-snapshots branch from a0d8d63 to ff55347 Compare August 3, 2022 12:02

iohk-bors bot merged commit a06a247 into master Aug 5, 2022

iohk-bors bot deleted the newhoggy/new-query-to-get-snapshots branch August 5, 2022 12:44

newhoggy mentioned this pull request Jan 7, 2023

Optimise query stake-snapshot command IntersectMBO/cardano-node#4754

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New query to get snapshots #3898

New query to get snapshots #3898

newhoggy commented Jul 14, 2022 •

edited

Loading

nfrisby left a comment

nfrisby Jul 14, 2022

newhoggy Jul 19, 2022

Jimbo4350 Jul 20, 2022 •

edited

Loading

newhoggy Jul 21, 2022

nfrisby Aug 1, 2022

Jimbo4350 left a comment

dnadales Jul 25, 2022

newhoggy Jul 27, 2022

nfrisby left a comment

nfrisby Aug 1, 2022

newhoggy Aug 2, 2022

JaredCorduan Aug 2, 2022

newhoggy Aug 3, 2022

nfrisby Aug 2, 2022

newhoggy Aug 3, 2022

newhoggy commented Aug 5, 2022

iohk-bors bot commented Aug 5, 2022

New query to get snapshots #3898

New query to get snapshots #3898

Conversation

newhoggy commented Jul 14, 2022 • edited Loading

Description

Checklist

cardano-api changes

cardano-cli changes

nfrisby left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jimbo4350 Jul 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jimbo4350 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nfrisby left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

newhoggy commented Aug 5, 2022

iohk-bors bot commented Aug 5, 2022

newhoggy commented Jul 14, 2022 •

edited

Loading

`cardano-api` changes

`cardano-cli` changes

Jimbo4350 Jul 20, 2022 •

edited

Loading