Skip to content

BIP324: Fix features bitmask for decoding-case selection#1969

Closed
phrwlk wants to merge 1 commit into
bitcoin:masterfrom
phrwlk:Boban
Closed

BIP324: Fix features bitmask for decoding-case selection#1969
phrwlk wants to merge 1 commit into
bitcoin:masterfrom
phrwlk:Boban

Conversation

@phrwlk

@phrwlk phrwlk commented Sep 16, 2025

Copy link
Copy Markdown
Contributor

Replace (features & 4) with (features & 3) in the four decoding-case checks. The docstring specifies that the lower two bits (f & 3) select which of x1/x2/x3 are valid. Using (features & 4) limited the value to {0,4}, making cases 1 and 2 unreachable and bypassing proper gating when bit 4 (u >= p) was set. This aligns implementation with the documented behavior and ensures all four modes are exercised correctly.

@jonatack jonatack added Pending acceptance This BIP modification requires sign-off by the champion of the BIP being modified Bug fix labels Sep 17, 2025

@jonatack jonatack left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ACK e78b332

cc BIP authors @real-or-random @jonasschnelli @sipa @dhruv for sign-off

@real-or-random

Copy link
Copy Markdown
Contributor

I think that change is correct, but then this PR will also need to update the pregenerated packet_encoding_test_vectors.csv.

@jonatack jonatack added the PR Author action required Needs updates, has unaddressed review comments, or is otherwise waiting for PR author label Sep 23, 2025
@real-or-random

Copy link
Copy Markdown
Contributor

I think that change is correct, but then this PR will also need to update the pregenerated packet_encoding_test_vectors.csv.

See #2016

@jonatack

Copy link
Copy Markdown
Member

Ok, closing in favor of #2016 that cherry-picks this commit and then updates the generated files per #1969 (comment) above.

@jonatack jonatack closed this Oct 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bug fix Pending acceptance This BIP modification requires sign-off by the champion of the BIP being modified PR Author action required Needs updates, has unaddressed review comments, or is otherwise waiting for PR author

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants