-
Notifications
You must be signed in to change notification settings - Fork 26
KES agent integration #1487
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
base: main
Are you sure you want to change the base?
KES agent integration #1487
Conversation
, SerDoc.HasInfo (Agent.DirectCodec m) (KES.VerKeyKES (KES c)) | ||
, SerDoc.HasInfo (Agent.DirectCodec m) (KES.SignKeyKES (KES c)) |
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.
Why is this doc stuff needed at all?
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.
SerDoc is a library for semi-automatically generating serialization documentation from types; this can then be used to generate documentation for typed-protocols
. In a perfect world, we wouldn't need this here, but the KES agent protocols (or at least, some of them) are generic in the crypto, and if we want to be able to generate protocol documentation for those, we need to demand those SerDoc
classes on them. This constraint then propagates up to the KES Agent API, and anything that uses it.
If this is a major complaint, I can restructure the code such that the protocols are no longer parametrized over the crypto, but always use the StandardCrypto
. This would make it impossible to test the KES agent protocols against MockCrypto
, but I don't think that would be a major concern to be honest.
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.
My concern here is that we don't use SerDoc anywhere else, so even if it is a nice feature we might not leverage it in the end. Shouldn't some documentation be generated somewhere making use of this feature? Or couldn't this be attached to the kes-agent
executable instead of to the library and used there to generate whatever docs you want, leaving Consensus free of these constraints?
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.
Indeed that's a bit unfortunate. I had envisioned a setup where we could generate human-readable protocol documentation for all protocols via typed-protocols-doc
, but due to recent changes to the typed-protocols
API, and bugs in earlier versions of typed-protocols-doc
, this is only viable for protocols that use the latest typed-protocols
, which we currently don't.
The actual documentation generator would be a separate executable under the kes-agent
project, but in order to do its thing, it does need a few constraints, and due to the way the code abstracts over crypto algorithms etc., some of those constraints "infect" other code that uses the same data structures. If the whole documentation generation thing isn't something we want to pursue, then yes, we can remove all that stuff, but that would also require removing it from kes-agent
.
...os-consensus-diffusion/src/ouroboros-consensus-diffusion/Ouroboros/Consensus/Node/Tracers.hs
Show resolved
Hide resolved
...-protocol/src/ouroboros-consensus-protocol/Ouroboros/Consensus/Protocol/Praos/AgentClient.hs
Outdated
Show resolved
Hide resolved
...ensus-protocol/src/ouroboros-consensus-protocol/Ouroboros/Consensus/Protocol/Praos/Common.hs
Outdated
Show resolved
Hide resolved
...consensus/src/ouroboros-consensus/Ouroboros/Consensus/MiniProtocol/ChainSync/Client/State.hs
Show resolved
Hide resolved
a10939e
to
1cf35e6
Compare
ouroboros-consensus-cardano/src/unstable-shelley-testlib/Test/Consensus/Shelley/MockCrypto.hs
Outdated
Show resolved
Hide resolved
ceb0550
to
4ca03a3
Compare
108bfcb
to
833650d
Compare
833650d
to
4828699
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.
Changelog needs to be fixed, rest looks good.
vrfKey <- genKeyVRF <$> genSeed (seedSizeVRF (Proxy @(VRF c))) | ||
kesKey <- unsoundPureGenKeyKES <$> genSeed (seedSizeKES (Proxy @(KES c))) |
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.
Why the extra spaces?
specifies how to obtain the actual credentials (OpCert and KES SignKey). For | ||
now, the only supported method is passing an OpCert and an | ||
UnsoundPureSignKeyKES, presumably loaded from disk | ||
(`PraosCredentialsUnsound`); future iterations will add support for | ||
connecting to a KES agent. |
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.
specifies how to obtain the actual credentials (OpCert and KES SignKey). For | |
now, the only supported method is passing an OpCert and an | |
UnsoundPureSignKeyKES, presumably loaded from disk | |
(`PraosCredentialsUnsound`); future iterations will add support for | |
connecting to a KES agent. | |
specifies how to obtain the actual credentials (OpCert and KES SignKey). | |
Two methods are supported: | |
- Directly passing an OpCert and an UnsoundPureSignKeyKES, presumably loaded | |
from disk (`PraosCredentialsUnsound`) | |
- Passing a socket address to a KES Agent from which OpCerts and (sound) | |
SignKeyKES can be obtained (`PraosCredentialsAgent`) |
, SerDoc.HasInfo (Agent.DirectCodec m) (KES.VerKeyKES (KES c)) | ||
, SerDoc.HasInfo (Agent.DirectCodec m) (KES.SignKeyKES (KES c)) |
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.
Indeed that's a bit unfortunate. I had envisioned a setup where we could generate human-readable protocol documentation for all protocols via typed-protocols-doc
, but due to recent changes to the typed-protocols
API, and bugs in earlier versions of typed-protocols-doc
, this is only viable for protocols that use the latest typed-protocols
, which we currently don't.
The actual documentation generator would be a separate executable under the kes-agent
project, but in order to do its thing, it does need a few constraints, and due to the way the code abstracts over crypto algorithms etc., some of those constraints "infect" other code that uses the same data structures. If the whole documentation generation thing isn't something we want to pursue, then yes, we can remove all that stuff, but that would also require removing it from kes-agent
.
This changes Consensus such that mlocked KES keys are used internally.
This is important groundwork for supporting KES agents in the future. In this form, the code will still load KES keys from disk, which is unsound, but the internal machinery is ready to also accept KES keys from other sources, and once loaded, KES keys will be handled appropriately (kept in mlocked RAM at all times, securely erased when expired).
This also involves a restructuring of the
HotKey
data structure, which now manages not only a KES SignKey, but also the corresponding OpCert. This is necessary for two reasons:Supersedes #1284.
Issue #558.
This adds KES Agent connectivity to consensus.
To use a KES Agent to source KES SignKeys and OpCerts, the
praosCredentialsSource
in thePraosCanBeLeader
data structure can now be pointed to a domain socket address where it will look for a KES Agent.Also covers #1077.