Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix InputWeightPrediction::P2WPKH_MAX constant DER sig length #2213

Merged
merged 3 commits into from Nov 22, 2023

Conversation

conduition
Copy link
Contributor

The P2WPKH_MAX constant assumed DER signatures in the witness have a max length of 73. In practice, their maximum length is 72, because BIP62 forbids nodes from relaying transactions which contain non-canonical ECDSA signatures (i.e. TX sigs must have an $s$ value of less than $\frac{n}{2}$).

This means $s$ is never encoded with a leading zero byte, and the signature as a whole never exceeds 72 bytes in total encoded length. The ground_p2wpkh function was already correct; only the constant needed to be corrected.

Technically 73 bytes is the upper limit for signatures, as nothing forbids miners from including such non-standard transaction signatures in blocks, but for the purposes of fee estimation and input weight prediction, 72 is the number which 99.9% of implementations should use as their ceiling. We already use it as the ceiling for the ground_p2wpkh function - ground_p2wpkh(0) returns a prediction which uses a witness signature of length 72.

Reference:

To enable testing, I added a weight() method to InputWeightPrediction and made it public but i'm not sure whether it has a use-case. Let me know if I should make it private instead.

This method computes the weight an InputWeightPrediction
would to a transaction, not including witness flag bytes.
The P2WPKH_MAX constant assumed DER signatures in the witness have
a max length of 73. However, their maximum length in practice is 72,
because BIP62 forbids nodes from relaying transactions whose ECDSA
signatures are not canonical (i.e. all sigs must have an s value of
less than n/2). This means s is never encoded with a leading zero
byte, and the signature as a whole never exceeds 72 bytes in total
encoded length. The ground_p2wpkh function was already correct;
only the constant needed to be corrected.
Sanity checks the InputWeightPrediction against
a transaction which uses P2WPKH inputs.
Copy link
Member

@tcharding tcharding left a comment

Choose a reason for hiding this comment

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

ACK f41ebc2

Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK f41ebc2

@apoelstra apoelstra merged commit 2664f97 into rust-bitcoin:master Nov 22, 2023
29 checks passed
@conduition conduition deleted the fix-p2wpkh-input-max-weight branch November 24, 2023 16:17
@murchandamus
Copy link

murchandamus commented Nov 27, 2023

Concept ACK:
Note that BIP62 was withdrawn, but as you describe high-s signatures are non-standard. This was achieved per BIP66, so using 72 bytes as the upper bound for serialized ECDSA signatures leads to the correct outcome during transaction building.


/// Tallies the total weight added to a transaction by an input with this weight prediction,
/// not counting potential witness flag bytes or the witness count varint.
pub const fn weight(&self) -> Weight {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shit, I only noticed this now. I kinda worry that people might misuse this but the documentation is pretty good so maybe not a big deal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants