Add BLS Pubkey managerment to vote program.#9310
Conversation
|
If this PR represents a change to the public RPC API:
Thank you for keeping the RPC clients in sync with the server API @wen-coding. |
|
The Firedancer team maintains a line-for-line reimplementation of the |
buffalojoec
left a comment
There was a problem hiding this comment.
Nice! This is getting really close.
samkim-crypto
left a comment
There was a problem hiding this comment.
Left some small comments, but looking good overall.
| bls_proof_of_possession_compressed_bytes: &[u8; BLS_PROOF_OF_POSSESSION_COMPRESSED_SIZE], | ||
| ) -> Result<(), InstructionError> { | ||
| let bls_pubkey_compressed = BLSPubkeyCompressed(*bls_pubkey_compressed_bytes); | ||
| let bls_pubkey = BLSPubkey::try_from(bls_pubkey_compressed) |
There was a problem hiding this comment.
The new v2.0.0 interface, I'll make the function signature to also accept byte arrays directly, but we should technically still be able to call verify directly on compressed form, so we don't need extra conversion here and for proof of possession.
bls_proof_of_possession.verify(&bls_pubkey_compressed, Some(&message))
buffalojoec
left a comment
There was a problem hiding this comment.
Looking great! Test coverage looks good, but I left some suggestions on factoring the tests so we get even more coverage and we can also more clearly see what's going on.
The program side looks good from my end. Next we have to get the vote-interface release out and get this branch updated.
53e98f4 to
713402e
Compare
1679a50 to
7a9af4f
Compare
|
If this PR represents a change to the public RPC API:
Thank you for keeping the RPC clients in sync with the server API @wen-coding. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #9310 +/- ##
========================================
Coverage 82.6% 82.6%
========================================
Files 902 902
Lines 323512 324444 +932
========================================
+ Hits 267318 268261 +943
+ Misses 56194 56183 -11 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
How did these changes end up here?
There was a problem hiding this comment.
Because it's the only way to make tests pass once we upgrade flate2, please review #9557 and I can then rebase.
There was a problem hiding this comment.
If we bump flate2 we need to check we are not breaking epochslots.
There was a problem hiding this comment.
Yeah definitely, even without flate2 risk, this change itself is dangerous enough, I probably should start test validator after the review goes through
b9300d2 to
f2f46e1
Compare
| let test_authorize_voter = if with_bls { | ||
| test_commands.clone().get_matches_from(vec![ | ||
| "test", | ||
| "vote-authorize-voter-with-bls", |
There was a problem hiding this comment.
@samkim-crypto @zz-sol I'm a bit confused how to make things work here. I believe this one is for offline signing. In our case I'm doing BLS keypair derivation inside cli parser, so I always need the vote authority keypair passed in. How do we make offline signing work in this case? Is it:
- do
vote-authorize-voter-with-blswithsign-onlyto generate the whole message encrypted - So when this thing is called we should have BLS pubkey and PoP without the keypair online
Does that mean I should somehow pass in the BLS pubkey and PoP?
There was a problem hiding this comment.
@grod220 this is the part I'm not sure about. The ones with "--sign-only" should work, because I still have ed25519 vote authority keypair there, but how does this piece fit in the picture?
There was a problem hiding this comment.
(paste from slack message)
I've done some offline signing CLI work in the past in the context of a multisig transaction. Step 1 was the offline --sign-only mode which output PUBKEY=SIGNATURE and Step 2 had the same parameters, but also with the multiple --signer PUBKEY=SIGNATURE args and broadcasted the tx.
If the tx message includes bls_pubkey and bls_proof_of_possession , but step 2 can't rebuild that same message---you need to pass those as optional args in step 2:
--bls-pubkey <base58>
--bls-proof-of-possession <base58>
And those computed values need to be an output of step 1.
There was a problem hiding this comment.
Thanks a lot! I fixed the authorized_voter_with_bls, making the new_authorized either pubkey or keypair, depending on whether --bls-pubkey or --bls-proof-of-possession are given.
Somehow I don't think we need similar structure (writing to output and pass in bls-pubkey/bls-proof-of-possession) for create-vote-account-v2, is that expected?
buffalojoec
left a comment
There was a problem hiding this comment.
It looks like there are a few things in-flight still (BLS SDK bump, CLI steps), so I just reviewed the Vote program.
Looking good! I left a few minor test requests, but overall the program implementation looks solid to me.
There was a problem hiding this comment.
How come these three ABI digests changed?
There was a problem hiding this comment.
I think this is due to us linking in new solana-bls-signatures package. These messages aren't used in production yet.
There was a problem hiding this comment.
frozen-abi does not check for ABI, it checks for API compatibility in a very paranoid way. Wire format may not have changed at all here.
Our contributor @puhtaytow is working on getting these glitches separated from actual ABI changes. Stay tuned.
buffalojoec
left a comment
There was a problem hiding this comment.
Lgtm! The last piece is the BLS dependency and getting another sign-off from Sam about the PoP validation. Once that's all set, I'll approve.
@samkim-crypto is on vacation. I chatted with @zz-sol :
|
| BLSProofOfPossession::try_from(bls_proof_of_possession_compressed) | ||
| .map_err(|_| InstructionError::InvalidArgument)?; | ||
| let message = generate_pop_message(vote_account_pubkey, bls_pubkey_compressed_bytes); | ||
| if Ok(true) == bls_proof_of_possession.verify(&bls_pubkey, Some(&message)) { |
There was a problem hiding this comment.
Oh, i just recalled this
iiuc this API change will be included in 2.0 @samkim-crypto
There was a problem hiding this comment.
Yep this can be simplified with the official release of bls-signatures v2.0, but the current version should be fine for this PR!
There was a problem hiding this comment.
Sorry for the delay on this. All the PoP and BLS parts look fine to me. When bls-signatures v2.0.0 is cut, I'll create a small PR to simplify some parts, but this shouldn't block this PR from merging (correctness will remain the same). Really great job @wen-coding!
Per SIMD 387, add the following: