-
Notifications
You must be signed in to change notification settings - Fork 16
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
create-testnet-data: Make --stake-delegators write credentials to disk. Keep current behavior with new --transient-stake-delegators. #512
Conversation
173f335
to
44a808b
Compare
7130444
to
2a61253
Compare
2a61253
to
628a6e7
Compare
OnDisk -> do | ||
let delegates = concat $ repeat stakeDelegatorsDirs | ||
delegatesAndPools = zip delegates distribution | ||
-- We don't need to be attentive to laziness here, because anyway this |
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's the context for this comment 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.
The benchmarking team asked us to stop writing credentials to disk (they were generating massive numbers of keys and I believe it was slowing down their tests). In the case where we want to write keys to disk we basically accept that if we are using large numbers of keys there will be a performance degradation.
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.
I left a few nitpicky comments. Otherwise, LGTM 🎉!
-- | The output format used all along this file | ||
desiredKeyOutputFormat :: KeyOutputFormat | ||
desiredKeyOutputFormat = KeyOutputFormatTextEnvelope |
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.
In the best of all possible worlds, this should be a field of the GenesisCreateTestnetDataCmdArgs
, no?
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.
Yes, but I've decided to leave it as future work, until this is requested 👍
payVK <- firstExceptT GenesisCmdFileInputDecodeError | ||
$ newExceptT $ Keys.readVerificationKeyOrFile AsPaymentKey (VerificationKeyFilePath payVKF) | ||
stakeVK <- firstExceptT GenesisCmdFileInputDecodeError |
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.
can be joined in a single ExceptT
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.
I went for some code sharing instead, to make this more digestible:
- payVK <- firstExceptT GenesisCmdFileInputDecodeError
- $ newExceptT $ Keys.readVerificationKeyOrFile AsPaymentKey (VerificationKeyFilePath payVKF)
- stakeVK <- firstExceptT GenesisCmdFileInputDecodeError
- $ newExceptT $ Keys.readVerificationKeyOrFile AsStakeKey (VerificationKeyFilePath stakeVKF)
+ payVK <- readVKeyFromDisk AsPaymentKey payVKF
+ stakeVK <- readVKeyFromDisk AsStakeKey stakeVKF
let paymentCredential = PaymentCredentialByKey $ verificationKeyHash payVK
stakeAddrRef = StakeAddressByValue $ StakeCredentialByKey $ verificationKeyHash stakeVK
dInitialUtxoAddr = makeShelleyAddressInEra ShelleyBasedEraShelley nw paymentCredential stakeAddrRef
@@ -514,6 +512,9 @@ computeDelegation nw delegDir dPoolParams = do
where
payVKF = File @(VerificationKey ()) $ delegDir </> "payment.vkey"
stakeVKF = File @(VerificationKey ()) $ delegDir </> "staking.vkey"
+ readVKeyFromDisk role file =
+ firstExceptT GenesisCmdFileInputDecodeError $ newExceptT $
+ Keys.readVerificationKeyOrFile role (VerificationKeyFilePath file)
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.
Looks good but a couple comments
OnDisk -> do | ||
let delegates = concat $ repeat stakeDelegatorsDirs | ||
delegatesAndPools = zip delegates distribution | ||
-- We don't need to be attentive to laziness here, because anyway this |
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.
The benchmarking team asked us to stop writing credentials to disk (they were generating massive numbers of keys and I believe it was slowing down their tests). In the case where we want to write keys to disk we basically accept that if we are using large numbers of keys there will be a performance degradation.
628a6e7
to
a4a8fc7
Compare
a4a8fc7
to
a5d8ae8
Compare
…ior with --transient-stake-delegators.
a5d8ae8
to
31c3e6d
Compare
Changelog
Context
How to trust this PR
delegations
is computed in the newOnDisk
case. Squint really hard.Checklist