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

onchaind: Adjust the witness weight estimation to be a bit more conservative #5669

Merged
merged 2 commits into from
Nov 10, 2022

Conversation

cdecker
Copy link
Member

@cdecker cdecker commented Oct 19, 2022

This came up while debugging the VLS signer for Greenlight. In case of the feerate hitting the floor it is important we agree on the size of the transaction, and in particular the witness weight which is estimated, not measured. If the estimation of VLS is larger than the one in CLN we end up with too small a fee according to VLS, and it'll refuse to sign.

The differences we found were due to a) a missing OP_PUSH33 for the pubkeys, and b) the lower expected signature weight of 72 instead of the spec suggestion of 73.

@cdecker
Copy link
Member Author

cdecker commented Oct 21, 2022

As discussed here lightningdevkit/rust-lightning#1783 (comment) the spec is likely a bit too conservative with its 73 byte signatures, given that the low-S rule ensures we don't have 73 bytes, but 72 bytes.

@cdecker cdecker marked this pull request as ready for review November 9, 2022 15:34
@cdecker cdecker added this to the v22.11 milestone Nov 9, 2022
@cdecker cdecker force-pushed the close-fee-calc branch 3 times, most recently from 0893af7 to 97ee6b6 Compare November 9, 2022 19:47
tests/test_closing.py Outdated Show resolved Hide resolved
We were missing the OP_PUSH for the pubkeys, and the spec mentions we
should be using 73 bytes to estimate the witness weight. Effectively
this adds 4 bytes which really just matters in case fees hit the
floor, and computing the weight becomes important.

Changelog-Fixed: onchaind: Witness weight estimations could be slightly lower than the VLS signer
Copy link
Collaborator

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

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

utACK 4cc20e3

@cdecker cdecker merged commit 832b2e5 into ElementsProject:master Nov 10, 2022
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.

3 participants