psbt: Encode keytype as a compact size unsigned integer#2906
psbt: Encode keytype as a compact size unsigned integer#2906apoelstra merged 1 commit intorust-bitcoin:masterfrom
Conversation
Kixunil
left a comment
There was a problem hiding this comment.
Just change the constants to be u64. It removes some guarantees but I don't think anyone cares.
|
Oh, and not supporting 128-bit architectures is definitely more reasonable than diverging from the BIP. |
We definitely should not do this. "Our implementation supports only 253 keytypes" is a pretty serious limitation. As for the casts, some are because our constants are Once we have those methods we can do this PR more cleanly. |
Which ones? I only saw one that was there already and that one is fine because it's bounded by |
|
Maybe it's just the one. |
|
Note to self, re-work this after #2972 is done. |
c6df708 to
d5c7122
Compare
|
Please read updated PR description. (I left this not as a draft to signal request for review) |
|
I don't understand how this is related to #3250, can you explain? |
|
I thought #3250 caused fedimint to have database entries that they could no longer parse. Similarly this breaks serde impls so if stuff was written to database using |
|
They can't parse it because they don't know the length upfront and our API stopped providing streamed deserialization. It could in theory happen that someone stored information as PSBT and loaded in the same application and used one of the values: 255, 254, or 253 which are now special and they'd indeed not notice it's broken beforehand if they never accessed it in a different application and it would apparently work and this change would break them. I highly doubt this is the case and if it happened to anyone they can just depend on both the old version and the new one and convert the types. We might provide a library support for this if there is more than one such project because it doesn't make sense to put it into a library if there's only one. Since I believe this is extremely unlikely, we should just wait and see if anyone complains. |
d5c7122 to
c93b6d0
Compare
|
🚨 API BREAKING CHANGE DETECTED To see the changes click details on "Check semver breaks / PR Semver - stable toolchain" job then expand "Run semver checker script" and scroll to the end of the section. |
Currently we use `u8` for key type but it was pointed out that we should be using a `u64` and encoding it as a compact type. The reason our code works now is because the compact type encoding for a `u8` (less than 253) is the same as for a `u8`. This breaks the `serde` impl, as shown by changes to the regression tests.
c93b6d0 to
2f656f7
Compare
|
Quite frustrating about the serde breakage. I kinda think we should drop the derived serde impl and just always encode as base64. But this PR is fine for now. |
|
4 days ack'ed can be merged when you are ready please. |
In rust-bitcoin#2906 we switched from using a `u8` for type keys to using a `u64` and encoding as a compact int (inline with the spec). Note that a `u8` encodes to the same bytes as a `u64` when the value is < 252. In that patch, I introduced a bug because the length returned by `PoprietaryKey::consensus_encode` uses a hard code 1 for the length of the encoding (because of single byte) instead of the variable length for the new compact encoding. Bug showed up in fuzzing, and was isolated by Jamil - mad props.
c89b816 psbt: Fix bug in Subtype consensus_encode (Tobin C. Harding) Pull request description: In #2906 we switched from using a `u8` for type keys to using a `u64` and encoding as a compact int (inline with the spec). Note that a `u8` encodes to the same bytes as a `u64` when the value is < 252. In that patch, I introduced a bug because the length returned by `PoprietaryKey::consensus_encode` uses a hard code 1 for the length of the encoding (because of single byte) instead of the variable length for the new compact encoding. Bug showed up in fuzzing, and was isolated by Jamil - mad props. Fix: #3501 ACKs for top commit: jamillambert: ACK c89b816 apoelstra: ACK c89b816; successfully ran local tests Tree-SHA512: 1b61b6a9ece197d74038ceedb447fd3ca21db8e2a6a96c9281a99ac232c18c3ca55da8e3f46930401714d3575e9a406a36e4f44929ca963208a5df4be6b46cfb
Currently we use
u8for key type but it was pointed out that we should be using au64and encoding it as a compact type. The reason our code works now is because the compact type encoding for au8(less thanThis breaks the
serdeimpl, as shown by changes to the regression tests,Fix: #2891