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

Correct htlc witness size calculations #815

Merged
merged 4 commits into from
Dec 7, 2020

Conversation

halseth
Copy link
Contributor

@halseth halseth commented Nov 11, 2020

I noticed a few inconsistencies in the weight constants.

I guess implementations just use the hardcoded constants for HTLC-timeout and HTLC-success when doing tx constructions (this is what lnd does), so I added a note here that we should continue using this value even though it is off by one.

@halseth halseth changed the title Htlc witness size Correct htlc witness size calculations Nov 11, 2020
@TheBlueMatt
Copy link
Collaborator

Is this an artifact of signature size variation? I mean there is no "right" answer after you sign things vary a byte or two, but is the limit wrong because we didn't account for low-S implying signatures are never max size?

@halseth
Copy link
Contributor Author

halseth commented Nov 12, 2020

We always assume they are the worst case size (73). Are you saying signatures never actually get that large? Interesting.

@t-bast
Copy link
Collaborator

t-bast commented Nov 12, 2020

Wow good catch, I don't understand why we considered this cltv_expiry to be encoded with 3 bytes (and how the success_witness turned out to be incorrect...). Time for some git speleology

HTLC-success) results in weights of:

663 (HTLC-timeout) (666 with with option_anchor_outputs))
703 (HTLC-success) (706 with with option_anchor_outputs))
703 (HTLC-success) (706 with with option_anchor_outputs))
- (reeeally 702 and 705, but we use these numbers for historical reasons)
Copy link
Collaborator

Choose a reason for hiding this comment

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

😂

@t-bast
Copy link
Collaborator

t-bast commented Nov 24, 2020

I re-did the calculations and I agree with what you found.
Can someone else check again to keep me honest?

HTLC-success) results in weights of:

663 (HTLC-timeout) (666 with with option_anchor_outputs))
703 (HTLC-success) (706 with with option_anchor_outputs))
703 (HTLC-success) (706 with with option_anchor_outputs))
Copy link

Choose a reason for hiding this comment

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

with with is probably a typo, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

most certainly

Copy link
Collaborator

@t-bast t-bast left a comment

Choose a reason for hiding this comment

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

ACK 2a8da1a

@t-bast t-bast merged commit d0c8385 into lightning:master Dec 7, 2020
halseth added a commit to halseth/lightning-rfc that referenced this pull request Dec 16, 2020
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.

5 participants