Skip to content

Encode key type as u64 instead of u8#90

Merged
tcharding merged 1 commit intorust-bitcoin:masterfrom
nymius:push-pzszypvvpkux
Apr 1, 2026
Merged

Encode key type as u64 instead of u8#90
tcharding merged 1 commit intorust-bitcoin:masterfrom
nymius:push-pzszypvvpkux

Conversation

@nymius
Copy link
Copy Markdown
Collaborator

@nymius nymius commented Mar 27, 2026 •
edited
Loading

I used the changes in rust-bitcoin/rust-bitcoin#5625 rust-bitcoin/rust-bitcoin#2906 to create this patch.

Partially fixes #78

@nymius nymius force-pushed the push-pzszypvvpkux branch from 721028c to e1e4933 Compare March 27, 2026 22:33
tcharding
tcharding reviewed Mar 29, 2026
View reviewed changes
tcharding
tcharding reviewed Mar 29, 2026
View reviewed changes
tcharding
tcharding reviewed Mar 29, 2026
View reviewed changes
src/consts.rs
pub(crate) const PSBT_GLOBAL_XPUB: u64 = 0x01;
/// Type: Transaction Version PSBT_GLOBAL_TX_VERSION = 0x02
pub(crate) const PSBT_GLOBAL_TX_VERSION: u8 = 0x02;
pub(crate) const PSBT_GLOBAL_TX_VERSION: u64 = 0x02;
Copy link
Copy Markdown
Member

@tcharding tcharding Mar 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All these changes, while correct, are unrelated to this PR, right? Can we have them either as a separate patch (with explanation) or separate PR.

Copy link
Copy Markdown
Collaborator Author

@nymius nymius Mar 30, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these are related, these changes are the ones that produced the issue that the rust-bitcoin linked issue in #78 was fixing. So I'm going to keep these changes here, and I opened #94 to address the #78 motivation.

Copy link
Copy Markdown
Member

@tcharding tcharding Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh sorry mate, I missed that these const were the keytype values. My bad.

tcharding
tcharding reviewed Mar 29, 2026
View reviewed changes
@tcharding tcharding mentioned this pull request Mar 29, 2026
Merged
@nymius nymius force-pushed the push-pzszypvvpkux branch from e1e4933 to 8fca8d6 Compare March 30, 2026 17:30
@nymius nymius mentioned this pull request Mar 30, 2026
Merged
@nymius nymius marked this pull request as ready for review March 30, 2026 17:44
@nymius nymius changed the title WIP: encode key type as u64 instead of u8 Encode key type as u64 instead of u8 Mar 30, 2026
@tcharding
Copy link
Copy Markdown
Member

tcharding commented Mar 31, 2026 •
edited
Loading

EDIT: Oh you fix this in #94

I could be confused again but I think this is buggy mate.

        let key_byte_size: u64 = byte_size - 1;

Needs to be - type_size because the 1 is the byte size of the u8 before we changed it to u64. Its in src/raw.rs and also src/v0/bitcoin/raw.rs.

@nymius
Copy link
Copy Markdown
Collaborator Author

nymius commented Mar 31, 2026 •
edited
Loading

I split this PR based on this comment:

All these changes, while correct, are unrelated to this PR, right? Can we have them either as a separate patch (with explanation) or separate PR.

That's why I opened #94, working on top of these changes, and expecting to merge them as stacked PRs.
Do you still want to merge them as separated changes? I can rebase #94 on master once this one is merged
Or should I close this one and have all merged with #94?

@nymius nymius force-pushed the push-pzszypvvpkux branch from 8fca8d6 to 9eab32f Compare March 31, 2026 18:35
@nymius
fix: encode keytype as u64 instead of u8
d47ab6d 
`type_value` represents the `keytype` of a Key. The BIP 174
specifies that the `keytype` field is a compact size unsigned integer,
i.e., it can be as large as u64::MAX. There hasn't been any larger
keytypes specified, so u8 has been fine so far, but it isn't correct.

This change enlarges the `type_value` field to hold the full compact
size unsigned integer.
@nymius nymius force-pushed the push-pzszypvvpkux branch from 9eab32f to d47ab6d Compare March 31, 2026 20:24
@nyonson
Copy link
Copy Markdown
Collaborator

nyonson commented Mar 31, 2026

FWIW the 2 patches in #94 make sense to me, think this one can just be closed in favor of #94.

tcharding added a commit that referenced this pull request Apr 1, 2026
@tcharding
Merge #94: Handle multi byte keytypes in Key::decode and Key::encode
9ea3c28 
3d23615 fix: handle multi byte keytypes in Key::decode and Key::encode (nymius)
d47ab6d fix: encode keytype as u64 instead of u8 (nymius)

Pull request description:

  `<keytype>` as specified by BIP 174 may be represented with up to 8 bytes, even if minimally encoded.

  Because the currently specified fields weren't using as many bytes, we were only considering the single byte case in the `Key::decode` and `Key::encode` logic.

  Here we change this logic to address larger `<keytypes>` and follow the specification.

  Stacks on top of: #90, #93.
  Fixes #78.

ACKs for top commit:
  nyonson:
    ACK 3d23615
  tcharding:
    ACK 3d23615

Tree-SHA512: f6d12532eabd702979020835f23f7e588620014f8850e15d02dcbea20f253eea894f5a9f36b95d66a55f0e38a3b6a6701e8ca06aff88f6c70eb57600d59c9ef1
@tcharding tcharding merged commit d47ab6d into rust-bitcoin:master Apr 1, 2026
10 checks passed
@tcharding
Copy link
Copy Markdown
Member

tcharding commented Apr 1, 2026

Went in in #94

@nymius nymius deleted the push-pzszypvvpkux branch April 1, 2026 12:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tcharding tcharding tcharding left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Key type should be able to capture any CompactSize/VarInt value

3 participants

@nymius @tcharding @nyonson