Skip to content

Conversation

@jkczyz
Copy link
Contributor

@jkczyz jkczyz commented Nov 5, 2025

When estimating signature weight, 73 WU was used in some places while 72 WU was used in others. Consistently use 72 WU and replace hardcoded values with constants. 73 WU signatures are non-standard and won't be produced by LDK. Additionally, using 73 WU along with grind_signatures adjustment is nonsensical.

@ldk-reviews-bot
Copy link

ldk-reviews-bot commented Nov 5, 2025

👋 Thanks for assigning @TheBlueMatt as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

@codecov
Copy link

codecov bot commented Nov 5, 2025

Codecov Report

❌ Patch coverage is 93.58974% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.29%. Comparing base (08c2236) to head (d88395f).
⚠️ Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
lightning/src/ln/channel.rs 85.29% 0 Missing and 5 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4208      +/-   ##
==========================================
- Coverage   89.30%   89.29%   -0.01%     
==========================================
  Files         180      180              
  Lines      137913   137949      +36     
  Branches   137913   137949      +36     
==========================================
+ Hits       123159   123179      +20     
- Misses      12142    12157      +15     
- Partials     2612     2613       +1     
Flag Coverage Δ
fuzzing 33.56% <0.00%> (+0.04%) ⬆️
tests 88.68% <93.58%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

@wpaulino wpaulino left a comment

Choose a reason for hiding this comment

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

LGTM after squash

@ldk-reviews-bot
Copy link

👋 The first review has been submitted!

Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer.

@jkczyz jkczyz force-pushed the 2025-11-signature-weight branch from 39fc8fb to 54bcda8 Compare November 6, 2025 15:02
Copy link
Contributor Author

@jkczyz jkczyz left a comment

Choose a reason for hiding this comment

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

Add a commit to use a 95% fee rate tolerance when checking the remote, like Eclair. Let me know if you prefer to leave that to a separate PR.

@jkczyz jkczyz force-pushed the 2025-11-signature-weight branch from 54bcda8 to 63bb5ed Compare November 6, 2025 15:08
@jkczyz
Copy link
Contributor Author

jkczyz commented Nov 6, 2025

Squahsed

@jkczyz jkczyz requested a review from TheBlueMatt November 6, 2025 15:09
@jkczyz jkczyz self-assigned this Nov 6, 2025
When estimating signature weight, 73 WU was used in some places while 72
WU was used in others. Consistently use 72 WU and replace hardcoded
values with constants. 73 WU signatures are non-standard and won't be
produced by LDK. Additionally, using 73 WU along with grind_signatures
adjustment is nonsensical.
When estimating the splice funding transaction fees, adjust for
grind_signatures. Since LDK supplies one of the signatures, only adjust
by 1 WU even though spending the shared input requires two signatures.
The interactive-tx construction protocol uses an agreed upon fee rate.
Since the bitcoind coin selection algorithm may underpay fees when no
change output is needed, providing a tolerance when checking if the
remote's fee contribution could avoid some unexpected failures. This
commit introduces a 95% tolerance similar to Eclair.
@jkczyz jkczyz force-pushed the 2025-11-signature-weight branch from 63bb5ed to d88395f Compare November 7, 2025 16:38
@jkczyz
Copy link
Contributor Author

jkczyz commented Nov 7, 2025

Dropped the linter fix as it was merged in #4212.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

3 participants