This repository was archived by the owner on Nov 15, 2023. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Babe VRF Signing in keystore #6225
Merged
Merged
Changes from 1 commit
Commits
Show all changes
36 commits
Select commit
Hold shift + click to select a range
6a6b33f
Introduce trait
363bb02
Implement VRFSigner in keystore
45c7582
Use vrf_sign from keystore
4fcf607
Convert output to VRFInOut
3c148b7
Simplify conversion
3ca49f6
vrf_sign secondary slot using keystore
f292623
Fix RPC call to claim_slot
df6063d
Use Public instead of Pair
76bcb2c
Check primary threshold in signer
81e31c5
Fix interface to return error
1d64e56
Move vrf_sign to BareCryptoStore
95a7b93
Merge remote-tracking branch 'upstream/master' into vrf-signing
f62b1d7
Fix authorship_works test
04ec073
Fix BABE logic leaks
6c6f380
Acquire a read lock once
c572d13
Also fix RPC acquiring the read lock once
b5415f6
Merge remote-tracking branch 'upstream/master' into vrf-signing
e4db9bf
Implement a generic way to construct VRF Transcript
b581c6c
Use make_transcript_data to call sr25519_vrf_sign
022b706
Make sure VRFTranscriptData is serializable
0c23a48
Cleanup
c3f17de
Move VRF to it's own module
a691f24
Implement & test VRF signing in testing module
a5821f0
Merge remote-tracking branch 'upstream/master' into vrf-signing
b5322c3
Remove leftover
53bed39
Fix feature requirements
43596d7
Revert removing vec macro
61e3b8d
Drop keystore pointer to prevent deadlock
383bf44
Nitpicks
2508594
Add test to make sure make_transcript works
1fd6936
Fix mismatch in VRF transcript
b45a1ca
Add a test to verify transcripts match in babe
df7dd65
Merge remote-tracking branch 'upstream/master' into vrf-signing
67b1c03
Return VRFOutput and VRFProof from keystore
e46467b
Merge remote-tracking branch 'upstream/master' into vrf-signing
ecd7cf1
Merge remote-tracking branch 'upstream/master' into vrf-signing
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Check primary threshold in signer
- Loading branch information
There are no files selected for viewing
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
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
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
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.
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.
nit: this API has way too many arguments and I'd prefer it to be extracted to a
VrfSigningRequeststruct which is the single parameter to the function.Uh oh!
There was an error while loading. Please reload this page.
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 result of
make_transcripthas the typemerlin::Transcriptwhich is very broad so I would expect only that the transcript needs to be passed to a signing request.I guess that this is because the async signing request is meant to go to a remote signer that has more information on what it is signing. In that case, the API design presented in this PR does not accurately represent the goals or contract of the features you are intending to eventually deliver.
And without knowing the context of Substrate's roadmap, it's impossible to make any case in favor of this change. Knowing all the features we intend to build should not be a requirement to understand the code and why decisions are made.
I recommend starting again with better APIs that avoid leaking BABE consensus logic out to the signer. And if BABE data (not logic!) needs to be sent to a signing service, the API should be clear that the API is for BABE, not any VRF signature as
vrf_signwould imply.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 totally agree that it wasn't a good design. I have now changed things so that the function gets
transcriptwhich is passed from BABE as suggested.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.
hmm, after actually implementing it... i now realize that the there was actually a reason behind passing all those parameters to
vrf_sign, mainly that thetranscriptneeds to somehow be serialized so that it can be sent over the wire to the remote signer. However, doesn't seem that this is possible, which is why sending the parameters to the remote signer to build the transcript there was my go-to solution.What do you think about this?
Uh oh!
There was an error while loading. Please reload this page.
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'm OK with that under these two conditions:
Keystorenot the mainKeystoretraits as it ruins clean abstraction otherwise.However I still find that approach far from ideal because it is probably hard to do the necessary trait-juggling to impose a clean boundary between Keystore & BABE. And we would be entering into this approach with the understanding that every consensus engine will now need its own extension trait for the Keystore. And it is a little harder to answer the question "how would a user implement their own consensus engine with raw signing?". IMO that is how we should view consensus crates and core code anyway - core interfaces should be general enough that altering consensus engine behavior doesn't require changing anything in the core. So I'm "OK" with that approach but I think we can do better...
It would better to do a deeper refactoring so we can send the raw components of the transcript over the wire. Then you could have an actual
vrf_signfunction which accepted transcript components in a general way.maybe set up
vrf_signas a generic function like this:transcript_data,key_type,publiccan all go over the wire, even though the transcript doesn't. But the details of what the transcript data is and how it is transformed into a transcript are 100% up to the caller, as it should be.Now in practice in BABE,
transcript_datawould be astruct BabeAuthorshipVrfTranscriptData { randomness, slot_number, epoch_index, }, all parameters tosp_babe::make_transcript, which themake_transcriptpassed tovrf_signwould be a thin wrapper around.And I can see that the remote signer may want access to
threshold, so we can also putthresholdin theBabeAuthorshipVrfTranscriptDataso it is sent over the wire.Uh oh!
There was an error while loading. Please reload this page.
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.
Also I vastly prefer
sr25519_vrf_signas a name because we might add another VRF implementation to Substrate at some point and our API should accommodate that without requiring a major version bump.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.
Thank you for such a detailed explanations. I tried to go with your suggested approach, but apparently this would cause the following issue:
To build up on your idea, i will introduce
BabeAuthorshipVrfTranscriptDatabut i will change thevrf_signmethod interface to accept the data pre-encoded so that they are sent over the wire and on the call-site, the result would be decoded accordingly.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 generic parameter issue is caused by both
<VRFData: Encode>and the fact that one of method parameters is a generic (make_transcript)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 have implemented an alternative that i think makes
sr25519_vrf_signgeneric enough. Could you please let me know your thoughts again?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 issue is that it causes the trait to become non object-safe. I didn't realize that it was used as a trait object somewhere, which complicates things...