BIP 34: Check last minimal encoding accoriding of heights #1244
Conversation
| // byte is the most significant one. | ||
| if b.last() == Some(&0x00) { | ||
| return Err(Bip34Error::NonMinimalPush(b.to_vec())); | ||
| } |
There was a problem hiding this comment.
Is this correct? Can't a number start with 0080 to force it to be positive if it'd otherwise be negative?
There was a problem hiding this comment.
Good catch! This is incorrect.
There was a problem hiding this comment.
Fixed this in a much cleaner way
|
I'm confused if "accoriding of heights" is just a mistake, or there's some meaning there that I'm missing. |
3b7b1f7 to
8bf53e4
Compare
790ffc8 to
be7985c
Compare
be7985c to
8405200
Compare
|
Sorry about the poor description. Fixed this in two steps:
|
| Err(Bip34Error::UnexpectedPush(b.to_vec())) | ||
| script::Instruction::PushBytes(b) => { | ||
| // Check that the number is encoded in the minimal way. | ||
| let h = script::read_scriptint(b).map_err(|_e| Bip34Error::UnexpectedPush(b.to_vec()))?; |
There was a problem hiding this comment.
I am unsure if I want to propagate the script error here. It has many other invariants that are unreadable from this particular function.
There was a problem hiding this comment.
I think what you've done is probably the right thing.
| @@ -225,9 +225,9 @@ impl From<bitcoinconsensus::Error> for Error { | |||
| } | |||
| } | |||
|
|
|||
| /// Helper to encode an integer in script format. | |||
| /// Helper to encode an integer in script(minimal CScriptNum) format. | |||
There was a problem hiding this comment.
Can I ask a question, why is the open parenthesis used with no space before it in docs? I've seen this elsewhere and wondered why its done like that, this is the first PR I've seen add one - hence my question. Cheers
There was a problem hiding this comment.
I had no idea that we are supposed to put spaces there. Won't be dismissing the two ACKs for this nit
There was a problem hiding this comment.
There should be a space there -- but agreed, we can fix this in a later doc-fixup PR.
There was a problem hiding this comment.
No need to fix it, I can do them when I'm cleaning up docs. I just wanted to make sure it wasn't for a reason I did not know about. Cheers
See: https://github.com/bitcoin/bips/blob/master/bip-0034.mediawiki
The BIP was updated with commit: bitcoin/bips@08844fd
Found while reviewing #1240
Also cleanly deals with negative heights as errors.