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
Fix witness display bug #1999
Fix witness display bug #1999
Conversation
Currently if the witness has zero elements or any of the individual witnesses is empty we panic. Panic is caused by subtracting 1 from a zero length. Check the length is non-zero before subtracting 1, print `[]` if empty.
We recently fixed a bug that causes a panic if a `Witness` contains an empty instruction. Add a unit test to verify it.
e3624b8
to
84614d9
Compare
Looks good! I guess there are technically two bugs, since there are two |
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.
ACK 84614d9
The |
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.
ACK 84614d9
Since this is a bug fix better to merge and in case the comment is found appropriate do another MR
if instructions.len() > 0 { | ||
let last_instruction = instructions.len() - 1; |
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 think
match instructions.len().checked_sub(1) {
Some(last_instruction) => ...
None => ..
}
would be more refactor-solid and better convey the underflow risk
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 suggestion, I'll do up another PR and let this one merge. Thanks!
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.
Done in #2035
When we introduce a custom
Debug
implementation for theWitness
we introduced a bug that causes code to panic if the witness contains an empty instruction.The bug can be verified by putting patch 2 first or by running
cargo run --example sighash
on master.