Addressing post-hoc PR feedback on #174 #197
Conversation
There is a bug in the C++ side where the error is not set correctly on a “high s” signature. The Rust side had mirrored this bug, but this eliminates the bug in the Rust.
This doesn’t seem to have any effect on the semantics, as the DER-formatted signature includes lengths that ensure it will ignore extra bytes, but the C++ code removes the extra byte, so the Rust should as well.
Co-authored-by: Daira-Emma Hopwood <[email protected]>
This now splits slices and returns the remaining pieces rather than modifying the arguments.
| while { | ||
| let res = i > 1; | ||
| i -= 1; | ||
| res | ||
| } { |
There was a problem hiding this comment.
What a weird construction that was. Like, why not just decrement i in the body of the loop, at least?
| @@ -595,10 +547,11 @@ impl Script<'_> { | |||
| let mut pc = self.0; | |||
| let mut last_opcode = Opcode::Operation(OP_INVALIDOPCODE); | |||
| while !pc.is_empty() { | |||
| let opcode = match Self::get_op(&mut pc) { | |||
| let (opcode, new_pc) = match Self::get_op(pc) { | |||
| Ok(o) => o, | |||
| Err(_) => break, | |||
There was a problem hiding this comment.
nonblocking: It's annoying that the error case here is ignored in the original C++ code. It will probably be worth a comment here, since once we transition fully to the Rust implementation this behavior will be puzzling.
There was a problem hiding this comment.
Yeah … I can only guess that this is done to prevent someone from creating script that’s too expensive, but then doing something invalid at the end to prevent this check from catching it. But
- the interpreter also tracks op_count, so it at least wouldn’t get past 201 no matter what; and
- if this did error, then we’d know not to bother running the script. 🤷🏼
So, should the comment just be something like “TODO: Err is ignored to match the C++ behavior. In future, allow this operation to fail.”?
No description provided.