Use correct KES construction #2092
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mrBliss
reviewed
May 14, 2020
ouroboros-consensus-shelley/src/Ouroboros/Consensus/Shelley/Protocol.hs
Outdated
Show resolved
Hide resolved
Comment on lines
+430
to
+435
| , stabilityWindow = ceiling $ 3 * (toRational f / fromIntegral k) | ||
| , randomnessStabilisationWindow = ceiling $ 4 * (toRational f / fromIntegral k) |
There was a problem hiding this comment.
We also have this in Ouroboros.Consensus.Shelley.Ledger.Ledger, but slightly differently, e.g., with a link to the doc. It would be good to only do this in a single place.
There was a problem hiding this comment.
Agree, but let's do in a separate PR
Comment on lines
+63
to
+64
| -- TODO This is somewhat annoying, since we want it to be based upon a runtime | ||
| -- value. |
There was a problem hiding this comment.
Hmm, is there a ticket for this in cardano-ledger-specs?
There was a problem hiding this comment.
No ticket yet, just a branch with me playing around and a conversation on slack... I'll make one
| instance TPraosCrypto TPraosStandardCrypto | ||
|
|
||
| -- | A hot KES key. We store alonside the key the KES period for which it is |
There was a problem hiding this comment.
Suggested change
| -- | A hot KES key. We store alonside the key the KES period for which it is | |
| -- | A hot KES key. We store alongside the key the KES period for which it is valid (?) |
mrBliss
reviewed
May 14, 2020
| @@ -112,8 +103,25 @@ instance DSIGNAlgorithm ByronDSIGN where | |||
| then Right () | |||
| else Left "Verification failed" | |||
|
|
|||
| abstractSizeVKey _ = error "Ouroboros.Consensus.Byron.Crypto.DSIGN: DSIGNAlgorithm ByronDSIGN" | |||
| abstractSizeSig _ = error "Ouroboros.Consensus.Byron.Crypto.DSIGN: DSIGNAlgorithm ByronDSIGN" | |||
| seedSizeDSIGN _ = 32 | |||
There was a problem hiding this comment.
It would be nice if we could run testDSIGNAlgorithm on this instance, but that's not so easy to accomplish (expose a test lib in cardano-base?)
mrBliss
approved these changes
May 14, 2020
nc6
force-pushed
the
nc/kes
branch
3 times, most recently
from
28025bb to
b1fc4d3
Compare
|
bors merge |
iohk-bors bot
added a commit
that referenced
this pull request
May 14, 2020
2092: Use correct KES construction r=nc6 a=nc6 This PR updates ouroboros-network to use the newest KES constructions from `cardano-base`, as well as fixing some bugs pertaining to how we signed with and evolved our KES keys. Each commit can be reviewed independently. Co-authored-by: Nicholas Clarke <nick@topos.org.uk>
Build failed |
- Use SUM KES scheme - KES updating must now take the current period, rather than the target period. - The KES class now no longer tracks its current period. As such, we introduce an abstraction `HotKey` which collects the current period with the KES key.
Previously KES keys would be evolved to the _current_ KES period, and would sign at that period. But this is incorrect; we should be caring about the number of evolutions since the first period, defined by the operational certificate. As such, we now sign with the _relative_ KES period (e.g. the number of evolutions), and evolve the keys accordingly.
This commit adds a verification, during our leader check, that we have an operational cert which is valid for the current KES period. Previously, if the OCert was not valid, two things could happen (depending on the crypto implementation): - An error would be thrown during signing, or - A block would be generated and subsequently rejected by the validation logic. Now instead we skip any slots where we would otherwise be the leader but for an invalid operational certificiate. It would probably be nice to log this in the future.
|
bors retry |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR updates ouroboros-network to use the newest KES constructions from
cardano-base, as well as fixing some bugs pertaining to how we signed with and evolved our KES keys.Each commit can be reviewed independently.