-
Notifications
You must be signed in to change notification settings - Fork 170
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
zoneconcierge: verifying BLS multisig in ProofEpochSealed
#241
Conversation
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.
Nice work! No blocker but you may need to address some conflicts before merging. Sorry about that. I also wish to see more cases of checkpoints being verified if possible. Comments:
@@ -27,6 +27,12 @@ func (m RawCheckpoint) Hash() RawCkptHash { | |||
return hash(fields) | |||
} | |||
|
|||
// SignedMsg is the message corresponding to the BLS sig in this raw checkpoint | |||
// Its value should be (epoch_number || last_commit_hash) | |||
func (m RawCheckpoint) SignedMsg() []byte { |
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.
Nice! 👏
for i := 0; i < bm.Len(); i++ { | ||
// ensure bitmap is big enough to contain ks | ||
if bm.Len() < len(ks) { | ||
return valSet, sum, fmt.Errorf("bitmap (with %d bits) is not large enough to contain the validator set with size %d", bm.Len(), len(ks)) |
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.
Nice fix! Maybe you need to address the conflicts when merging with the latest dev
...
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.
Yep sure, done
proof, err := zcKeeper.ProveEpochSealed(ctx, epoch.EpochNumber) | ||
require.NoError(t, err) | ||
// verify | ||
err = zckeeper.VerifyEpochSealed(epoch, rawCkpt, proof) |
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.
assert the err? It seems that only a valid case is tested. Maybe add more cases?
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.
Asserting happens below in L92-L98. It's not straightforward since this fuzz test includes testing a BLS sig with insufficient signed validators. See L92's if statement.
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.
You are right. Didn't notice that it will generate insufficient sigs. Thanks!
// generate a random epoch | ||
epoch := datagen.GenRandomEpoch() | ||
|
||
// generate a random validator set with 100 validators | ||
numVals := 100 | ||
valSet, blsSKs := datagen.GenerateValidatorSetWithBLSPrivKeys(numVals) | ||
|
||
// sample a validator subset, which may or may not reach a quorum | ||
bm, numSubSet := datagen.GenRandomBitmap() | ||
_, subsetPower, err := valSet.FindSubsetWithPowerSum(bm) | ||
require.NoError(t, err) | ||
|
||
// construct the rawCkpt | ||
// Note that the BlsMultiSig will be generated and assigned later | ||
lch := checkpointingtypes.LastCommitHash(epoch.SealerHeader.LastCommitHash) | ||
rawCkpt := &checkpointingtypes.RawCheckpoint{ | ||
EpochNum: epoch.EpochNumber, | ||
LastCommitHash: &lch, | ||
Bitmap: bm, | ||
BlsMultiSig: nil, | ||
} | ||
|
||
// let the subset generate a BLS multisig over sealer header's last_commit_hash | ||
multiSig, err := signBLSWithBitmap(blsSKs, bm, rawCkpt.SignedMsg()) | ||
require.NoError(t, err) | ||
// assign multiSig to rawCkpt | ||
rawCkpt.BlsMultiSig = &multiSig |
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.
Maybe wrap these into a function like this? By doing this, it would be easier to generate checkpoints in varies format (valid and invalid)
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.
That could be possible, but my feeling is that this function will only be used once and the current fuzz test will not be the ultimate form.
Basically this snippet generates a legitimate epoch, valset, bitmap, rawCkpt and multisig altogether, without going through the real epochs (even in the Cosmos layer). Once we have the inclusion proofs, we will need the actual last_commit_hash
and thus go through blocks towards the epoch. After that the current fuzz test will be broken down into the following parts:
- generating an epoch
- generating a validator set
- going towards a random epoch (new)
- generating bitmap, rawCkpt and multisig
- generating inclusion proofs (new)
Till then the generation looks not that straightforward. I will try this again in a subsequent PR that implements the inclusion proofs.
Partially fixes BM-303
This PR implements the BLS multisig part of the verifier for
ProofEpochSealed
. Specifically,ProofEpochSealed
attests that an epoch has a raw checkpoint that gathers a valid BLS signature from its validator set. The verifications include 1) verifying BLS sig, 2) verifying inclusion of the validator set inAppHash
of the sealer header, i.e., the 2nd header in the next epoch, and 3) verifying inclusion of the epoch metadata inAppHash
of the sealer header.Along the way, we also found two bugs in epoching and checkpointing:
FindSubsetWithPowerSum
andFindSubset
functions of validator set originally iterate over the bitmap. However, bitmap size may be slightly larger than the validator set size (e.g., out bitmap has 104 bits while we have 100 validators). Iterating over the last 4 bits leads to out of range errors. This PR replaces the bound of iteration with the validator set size.This PR also comes with a series of datagen functions and a fuzz test on the verifier.
Future works