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

Signature weight should be estimated at 73 based on BOLT-3 #1783

Closed

Conversation

devrandom
Copy link
Member

No description provided.

dunxen
dunxen previously approved these changes Oct 19, 2022
Copy link
Contributor

@dunxen dunxen left a comment

Choose a reason for hiding this comment

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

@@ -1676,7 +1676,7 @@ impl<Signer: Sign> Channel<Signer> {
1 + // witness element count
4 + // 4 element lengths (2 sigs, multisig dummy, and witness script)
self.get_funding_redeemscript().len() as u64 + // funding witness script
2*(1 + 71); // two signatures + sighash type flags
2*(1 + 71 + 1); // two signatures + sighash type flags + sig lengths
Copy link
Contributor

Choose a reason for hiding this comment

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

The length is accounted for two lines up. The reason for the difference is that signatures are variable length.

Copy link
Member Author

Choose a reason for hiding this comment

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

ah, I see. so my comment is wrong, but the change is correct, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

i.e. it should be 2*(1 + 72)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yea, correct to both.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@codecov-commenter
Copy link

Codecov Report

Base: 90.69% // Head: 90.71% // Increases project coverage by +0.01% 🎉

Coverage data is based on head (837c568) compared to base (95bb27a).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1783      +/-   ##
==========================================
+ Coverage   90.69%   90.71%   +0.01%     
==========================================
  Files          87       87              
  Lines       47045    47045              
  Branches    47045    47045              
==========================================
+ Hits        42669    42677       +8     
+ Misses       4376     4368       -8     
Impacted Files Coverage Δ
lightning/src/ln/channel.rs 88.66% <100.00%> (ø)
lightning/src/ln/functional_tests.rs 96.94% <0.00%> (ø)
lightning/src/ln/channelmanager.rs 85.16% <0.00%> (+0.02%) ⬆️
lightning/src/ln/monitor_tests.rs 99.55% <0.00%> (+0.11%) ⬆️
lightning/src/util/events.rs 37.40% <0.00%> (+0.25%) ⬆️
lightning-net-tokio/src/lib.rs 77.03% <0.00%> (+0.30%) ⬆️
lightning/src/chain/onchaintx.rs 95.29% <0.00%> (+0.89%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@jkczyz jkczyz requested a review from TheBlueMatt October 19, 2022 17:05
@TheBlueMatt
Copy link
Collaborator

TheBlueMatt commented Oct 20, 2022

I believe this is wrong, going from https://bitcoin.stackexchange.com/questions/77191/what-is-the-maximum-size-of-a-der-encoded-ecdsa-signature/77192#77192

The DER encoding has 6 bytes of headers, plus the sighash type byte makes 7. Then we have max 33 bytes for R and 32 bytes for low-S. The lengths are included two lines up. That gives us 72 bytes plus the length.

Note that the answer there was trying to give an absolute, absolute maximum and answer based on someone thinking the number was very high. In practice we can't use high-S anyway.

Interestingly, because the shutdown negotiation uses absolute fees, not fee rates, I don't believe this can cause us to disagree with our peer, but I may be wrong about this.

@devrandom
Copy link
Member Author

So this is not a protocol level issue, because the fees for mutual-close are negotiated as total fees rather than feerate. The only real concern is that we would have an unrelayable tx, or that fee negotiations will fail, right?

Looking at the actual difference between 72 and 73 - it's all about low-S, and BOLT-3 in general doesn't assume it. Wouldn't it be a bit strange to not assume low-S for other tx types, but assume it for mutual-close?

@devrandom
Copy link
Member Author

or another way to look at it - is BOLT-3 wrong to not assume low-S for everything?

@dunxen
Copy link
Contributor

dunxen commented Oct 21, 2022

or another way to look at it - is BOLT-3 wrong to not assume low-S for everything?

I think this makes sense. Even though it's just standardness, we kind of do need everything to relay in Lightning so we should always be using low-S.

Maybe warrants opening an issue.

@devrandom
Copy link
Member Author

Submitted lightning/bolts#1034

@TheBlueMatt
Copy link
Collaborator

The only real concern is that we would have an unrelayable tx, or that fee negotiations will fail, right?

Indeed, an unrelayable tx would be the issue.

Looking at the actual difference between 72 and 73 - it's all about low-S

Yep, and high-S would be unrelayable, for other reasons :)

I believe this can be closed here?

@devrandom devrandom closed this Oct 21, 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.

6 participants