-
Notifications
You must be signed in to change notification settings - Fork 135
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
Factor out get_signature and add it to canister_sig_util #2134
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.
LGTM, thanks!
|
||
let witness_hash = witness.digest(); | ||
let root_hash = sig_map.root_hash(); | ||
if witness_hash != root_hash { |
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.
Does this check (still) make sense? We rehash the pruned witness tree every time, but if it did not match the (unpruned) signature map root hash, we would simply have a bug in our implementation. I wonder if this is really best addressed by wasting resources in production, or if we should rather have better tests (unless we're already sufficiently covered).
Honestly, I think we should remove it here.
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 witness_hash != root_hash { | ||
return Err(format!( | ||
"internal error: signature map computed an invalid hash tree, witness hash is {}, root hash is {}", | ||
hex::encode(witness_hash), |
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 could also get rid of the hex
dependency if you didn't have this check 😉
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, I've filed a ticket for the potential code simplification.
|
||
let witness_hash = witness.digest(); | ||
let root_hash = sig_map.root_hash(); | ||
if witness_hash != root_hash { |
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.
Factor out
get_signature
, which is used in canisters that create canister signatures, and add the corresponding functionality tocanister_sig_util
-crate (as functionget_signature_as_cbor
).🟡 Some screens were changed