-
Notifications
You must be signed in to change notification settings - Fork 724
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
Updated stake-snapshot and pool-params CLI commands #2560
Conversation
How does this differ from #2536 ? Is that PR not addressing the same issue as this one? Just creating a new PR creates more work for the people who have to review it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moving in the right direction 👍
@@ -80,6 +89,7 @@ renderShelleyQueryCmdError err = | |||
ShelleyQueryCmdHelpersError helpersErr -> renderHelpersError helpersErr | |||
ShelleyQueryCmdAcquireFailure aqFail -> Text.pack $ show aqFail | |||
ShelleyQueryCmdByronEra -> "This query cannot be used for the Byron era" | |||
ShelleyQueryCmdPoolIdError -> "The pool id does not exist" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would be very useful to the user would be to include the PoolId
and render it here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kevinhammond We have a problem with these commands. They both depend on the ledger state query which currently hangs when used on mainnet. We don't have immediate plans to fix this now as the focus is eUTxO things. Context: https://input-output-rnd.slack.com/archives/CC0CLNEH1/p1617065989042000
Please avoid in future any production use of the debug state dump features.It's there for debug and hacking / prototyping purposes. But it's not a stable format. It's too big. It's not designed for production use.
The way forward would likely be bugging the consensus folk to expose a suitable query
Here is the evidence that they do not hang (run live on mainnet). The commands deliberately select a subset of the ledger state information, so reducing time and memory requirements for SPOs (who are currently using the ledger-state dump for production reasons). The commands can, of course, be changed if the ledger state format changes or the consensus team exposes the required state. The SPOs are very keen to have these available - they will help reduce memory requirements when provisioning a stake pool.
time cardano-cli query stake-snapshot --stake-pool-id 00beef0a9be2f6d897ed24a613cf547bb20cd282a04edfc53d477114 --mainnet
{
"poolStakeGo": 40278547538358,
"activeStakeGo": 22753958467474959,
"poolStakeMark": 40424218559492,
"activeStakeMark": 22670949084364797,
"poolStakeSet": 39898761956772,
"activeStakeSet": 22488877070796904
}
real 0m20.085s
user 0m8.874s
sys 0m2.585s
time cardano-cli query pool-params --stake-pool-id 00beef0a9be2f6d897ed24a613cf547bb20cd282a04edfc53d477114 --mainnet
{
"poolStakeGo": 40278547538358,
"activeStakeGo": 22753958467474959,
"poolStakeMark": 40424218559492,
"activeStakeMark": 22670949084364797,
"poolStakeSet": 39898761956772,
"activeStakeSet": 22488877070796904
}
real 0m20.085s
user 0m8.874s
sys 0m2.585s
@kevinhammond We have a problem with these commands. They both depend on the ledger state query which currently hangs when used on mainnet. We don't have immediate plans to fix this now as the focus is eUTxO things. Context: https://input-output-rnd.slack.com/archives/CC0CLNEH1/p1617065989042000
The way forward would likely be bugging the consensus folk to expose a suitable query |
The other PR was created as a draft (I had in fact only intended Jordan to look at it). I did try updating it, but the code has moved on, and I was seeing many irrelevant and confusing changes to configuration files etc I'll delete #2536 |
After speaking with Kevin, his queries don't decode the entire ledger state and therefore have a smaller memory footprint. |
Git provides all the functionality needed to over come issues like this. |
Yes, but it's notorious for having corners that are difficult/impossible to move out of. In this case, I followed the recommended approach and hit one of those corners. This avoided the risk of messing up the repo (the draft PR can be deleted now) |
As a community tool builder, I would just like to add that this PR is very welcome and will help make our tools better and the end-user experience much improved. So anything that can be done to help push this over the finish line and get it included in an upcoming release is appreciated. |
Let's please get this through. We're currently taking 15 minutes in some cases to dump the entire ledger state to json (2gb) and takes even longer to decode it and sum up the active stake and total active stake for our pools. This has become so painful that we have an external API built so not everybody has to go through this decoding process. This PR, while not perfect, gets us what we need in around 20 seconds. It's head and shoulders over what we do today and we won't have to rely on an external API to get the info needed for cncli leaderlogs. |
Yes, seems to be 2.5GB for these commands versus 12GB for I think the memory cost is essentially all "sunk cost" (for the ledger state), since it's only necessary to extract maps etc whose results are then folded over to calculate the sums etc. In contrast, the full ledger state dump needs to traverse the whole structure and convert everything into JSON before it is output (it looks like it might even be producing the output strictly?). In the long term, we can look at exposing more things through the consensus interface, but this should relieve the short term pressure to do that (and help SPOs reduce their memory requirements and reliance on external code) |
bors r+ |
👎 Rejected by code reviews |
@kevinhammond This PR is not yet ready to be merged, There are numerous "fixup" commits that from what I can see, should all be squashed down to a single commit. If you don't know how to do that, please ask one of the other devs. |
0b033ea
to
4b41c2d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking a lot better, final changes requested and then I'll be happy to approve. The docs will also need updating, specifically: doc/reference/cardano-node-cli-reference.md
👍
left $ ShelleyQueryCmdPoolIdError poolId | ||
else | ||
liftIO . LBS.putStrLn $ encodePretty $ Stakes {markpool = markStake, setpool = setStake, gopool = goStake, marktot = markTotal, settot = setTotal, gotot = goTotal} | ||
where |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's move the functions/values from this where clause to the outer where clause. No need to have two.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, this is not possible, so I have pushed the function definitions into the inner scope
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Scrap that. decodeLedgerState is used to generate the Right so can't be pushed inwards. I've lifted it to the module level instead
a391698
to
17aa27d
Compare
After doing some testing, it looks like only |
6b4456c
to
e65393c
Compare
Summary of changes:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good, a few minor comments
|
||
Right ledgerState -> do | ||
-- Convert the hash string into a KeyHash for use by the ledger | ||
hk <- KeyHash <$> hashFromStringAsHex (filter (/= '"') (show poolId)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should be able to use serialiseToRawBytesHexText
here instead of hashFromStringAsHex (filter (/= '"') (show poolId)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to not work as serialiseToRawBytesHexText
returns Text
but I need a Maybe (Hash (ADDRHASH (Crypto ledgerera)) (VerKeyDSIGN (DSIGN (Crypto ledgerera))))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies, this is the change I had in mind.
diff --git a/cardano-cli/src/Cardano/CLI/Shelley/Run/Query.hs b/cardano-cli/src/Cardano/CLI/Shelley/Run/Query.hs
index 10c695228..a61afcf84 100644
--- a/cardano-cli/src/Cardano/CLI/Shelley/Run/Query.hs
+++ b/cardano-cli/src/Cardano/CLI/Shelley/Run/Query.hs
@@ -438,22 +438,17 @@ writeLedgerState mOutFile qState@(SerialisedLedgerState serLedgerState) =
writeStakeSnapshot :: forall era ledgerera. ()
=> ShelleyLedgerEra era ~ ledgerera
- => Era.Era ledgerera
+ => Era.Crypto ledgerera ~ StandardCrypto
=> FromCBOR (LedgerState era)
=> PoolId
-> SerialisedLedgerState era
-> ExceptT ShelleyQueryCmdError IO ()
-writeStakeSnapshot poolId qState =
+writeStakeSnapshot (StakePoolKeyHash hk) qState =
case decodeLedgerState qState of
-- In the event of decode failure print the CBOR instead
Left bs -> firstExceptT ShelleyQueryCmdHelpersError $ pPrintCBOR bs
Right ledgerState -> do
- let x = hashFromStringAsHex (filter (/= '"') (show poolId))
- -- Convert the hash string into a KeyHash for use by the ledger
- hk <- KeyHash <$> x
- & hoistMaybe (ShelleyQueryCmdPoolIdError poolId)
-
-- Ledger State
let (LedgerState snapshot) = ledgerState
@@ -764,6 +759,7 @@ obtainLedgerEraClassConstraints
-> ((Ledger.ShelleyBased ledgerera
, ToJSON (LedgerState era)
, FromCBOR (LedgerState era)
+ , Era.Crypto ledgerera ~ StandardCrypto
) => a) -> a
obtainLedgerEraClassConstraints ShelleyBasedEraShelley f = f
obtainLedgerEraClassConstraints ShelleyBasedEraAllegra f = f
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep. Worked. Thanks!
@@ -0,0 +1,107 @@ | |||
# Querying a Stake Pool | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should add something to the effect of "this command if for debugging purposes only and is not guaranteed to succeed due to the size of the ledger state"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
d2f214e
to
eca22fc
Compare
I've updated and retested. Looks good. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! Thanks @kevinhammond & @newhoggy. Just squash the commits before merging & notify QA
bors merge |
Merge conflict. |
92d6f49
to
f173850
Compare
bors merge |
2560: Updated stake-snapshot and pool-params CLI commands r=newhoggy a=kevinhammond This PR provides two new query commands that should be useful to SPOs who are using the CNCLI set of tools, for example (as well as for internal IOG purposes): `cardano-cli query stake-snapshot --stake-pool-id <poolid>` `cardano-cli query pool-params --stake-pool-id <poolid>` These commands allow users to access two pieces of information from the ledger state 1. stake snapshots 2. pool parameters, including retirements The stake snapshot returns information about the mark, set, go ledger snapshots for a pool, plus the total active stake for each snapshot that can be used in a 'sigma' calculation: ``` cardano-cli query stake-snapshot --stake-pool-id 00beef0a9be2f6d897ed24a613cf547bb20cd282a04edfc53d477114 --mainnet { "poolStakeGo": 40278547538358, "activeStakeGo": 22753958467474959, "poolStakeMark": 40424218559492, "activeStakeMark": 22670949084364797, "poolStakeSet": 39898761956772, "activeStakeSet": 22488877070796904 } ``` Each snapshot is taken at the end of a different era. The `go` snapshot is the current one and was taken two epochs earlier, `set` was taken one epoch ago, and `mark` was taken immediately before the start of the current epoch. The pool parameters command returns three pieces of information: current parameters, future parameters and retiring information. They may be `null` if eg the parameters are not changing. ``` cardano-cli query pool-params --stake-pool-id d785ff6a030ae9d521770c00f264a2aa423e928c85fc620b13d46eda --mainnet { "poolParams": { "publicKey": "d785ff6a030ae9d521770c00f264a2aa423e928c85fc620b13d46eda", "cost": 340000000, "metadata": { "hash": "b150b12a1301c4b1510ac8b9f53f7571cabb43455f6fd244cd8fd97504b1c869", "url": "https://adalite.io/ADLT4-metadata.json" }, "owners": [ "463a9695c9222183ee6e1523478722bebcb332fa3769f1d8ef40c7d0", "5049c1dac0e597ee902f27a74a167cf135ae7c1717b0d3a417cd6c67" ], "vrf": "0a21e37b1917ce37a897eb2a8dc6715973a18d0586f7ab4962e3975561151348", "pledge": 30000000000, "margin": 3.0e-2, "rewardAccount": { "network": "Mainnet", "credential": { "key hash": "b1bc146a5fb0683c4e3836712d115b98619048bc307cc059b6adc76e" } }, "relays": [ { "single host address": { "IPv6": null, "port": 3003, "IPv4": "54.228.75.154" } }, { "single host address": { "IPv6": null, "port": 3001, "IPv4": "54.228.75.154" } }, { "single host address": { "IPv6": null, "port": 3003, "IPv4": "34.249.11.89" } }, { "single host address": { "IPv6": null, "port": 3001, "IPv4": "34.249.11.89" } } ] }, "futurePoolParams": null, "retiring": null } ``` The main advantage of these commands over using `query ledger-state` is that they avoid the need to dump the full ledger state (which is both time consuming and memory intensive - meaning they reduce the total system demands for SPOs), and will make it easier to support CNCLI and other tools. They also use existing internal operations (such as the ledger pool stake and active stake calculations), meaning that the information is guaranteed to be identical to that which the ledger is using (and without having to write scripts to extract/correlate the information). I have decided to put these under 'query' rather than 'pool' since they are extracting info from the ledger state. Co-authored-by: Kevin Hammond <12563287+kevinhammond@users.noreply.github.com>
Build failed: |
bors r+ |
Build succeeded: |
Whoo Hoo! |
This is a huge and very welcome step. Thank you very much @kevinhammond for working to get this included natively in the node! |
This PR provides two new query commands that should be useful to SPOs who are using the CNCLI set of tools, for example (as well as for internal IOG purposes):
cardano-cli query stake-snapshot --stake-pool-id <poolid>
cardano-cli query pool-params --stake-pool-id <poolid>
These commands allow users to access two pieces of information from the ledger state
The stake snapshot returns information about the mark, set, go ledger snapshots for a pool, plus the total active stake for each snapshot that can be used in a 'sigma' calculation:
Each snapshot is taken at the end of a different era. The
go
snapshot is the current one and was taken two epochs earlier,set
was taken one epoch ago, andmark
was taken immediately before the start of the current epoch.The pool parameters command returns three pieces of information: current parameters, future parameters and retiring information. They may be
null
if eg the parameters are not changing.The main advantage of these commands over using
query ledger-state
is that they avoid the need to dump the full ledger state (which is both time consuming and memory intensive - meaning they reduce the total system demands for SPOs), and will make it easier to support CNCLI and other tools. They also use existing internal operations (such as the ledger pool stake and active stake calculations), meaning that the information is guaranteed to be identical to that which the ledger is using (and without having to write scripts to extract/correlate the information).I have decided to put these under 'query' rather than 'pool' since they are extracting info from the ledger state.