Fix pk_len() for BareCtx
#472
Conversation
The `pk_len()` returned should account for the extra byte for the OP_PUSH opcode. This is true for all the other contexts, but not for `BareCtx`, so this commit fixes it.
| } else { | ||
| 33 | ||
| 34 |
There was a problem hiding this comment.
nit: Would it make sense to have 33 + 1 instead? This way we can explicitly show OP_PUSH... and <pk>.
There was a problem hiding this comment.
Now I'm wondering if it is not better to have the 1 byte of the OP code outside of this function for all contexts. The name of the function pk_len communicates that it is calculating the length of the serialized public key but it does more than that by adding the byte for the PUSH opcode. Or maybe mention this fact in the docs.
There was a problem hiding this comment.
Yeah it's mentioned in the docs:
/// Get the len of public key when serialized based on context
/// Note that this includes the serialization prefix. Returns
/// 34/66 for Bare/Legacy based on key compressedness
/// 34 for Segwitv0, 33 for Tap
There was a problem hiding this comment.
Re: 33 + 1 I'd be ok making that change, but let's see what the maintainers think about it
|
concept ACK this. no preference about Can't test locally because of the |
…022_10 2d113fb Merge rust-bitcoin/rust-miniscript#472: Fix `pk_len()` for `BareCtx`
The
pk_len()returned should account for the extra byte for the OP_PUSH opcode. This is true for all the other contexts, but not forBareCtx, so this commit fixes it.This PR is based on the
7.0.0tag because in my opinion it's worth applying this and releasing as7.0.1without waiting for8.0.0(BDK would naturally benefit from this because we are about to make one new release still based on7.0.0and it's tricky to workaround this issue in our code).I think the patch is small enough that it can easily be applied on older releases as well, if you want to backport it too.