-
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: verifier for ProofEpochSubmitted
#246
Conversation
bestSubmissionData := k.btccKeeper.GetSubmissionData(ctx, sk) | ||
if bestSubmissionData == nil { | ||
err := fmt.Errorf("the best submission key for epoch has no submission data") | ||
panic(err) // this can only be a programming error |
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.
ProveEpochSubmitted
is public function on zoneconcierge
keeper and without further context, not having SubmissionData
for any SubmissionKey
it is not really an error. I would either return empty slice and have the caller to make the judgement about whether panic or not, or make it private helper as close to call site as possible.
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.
Good point! I have made ProveEpochSubmitted
to return an error, then make the FinalizedChainInfo
API to panic if ProveEpochSubmitted
returns an error.
// basic sanity check | ||
if rawCkpt == nil { | ||
return fmt.Errorf("rawCkpt is nil") | ||
} else if txsInfo == nil { |
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.
if you check len
of slices (like in next line len(txsInfo) != txformat.NumberOfParts
) then this nil check is unnecessary as len
of nil
slice is 0
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.
Thanks, that's good to know. I thought len(nil)
would lead to panic 😢
} | ||
|
||
// verify Merkle proofs for each tx info | ||
powLimit := getPoWLimit(btcNetParams) |
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 I do not really like that this is panicking, maybe pass powLimit
as argument and make the decision about panic closer to call site ?
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 agree, done.
Fixes BM-304
This PR provides the verifier function of
ProofEpochSubmitted
. The verifier takes the expected raw checkpoint, the twoTransactionInfo
s, two BTC headers and some other metadata, then enforces the following verification rules:TransactionInfo
is valid w.r.t. the BTC headerTransactionInfo
s is same as the expected raw checkpoint.This verifier function will be a part of the verifier for the CZ timestamp. This PR also comes with a fuzz test.