Use 72 WU instead of 73 WU for signature weight by jkczyz · Pull Request #4208 · lightningdevkit/rust-lightning

jkczyz · 2025-11-05T18:54:24Z

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 · 2025-11-05T18:54:27Z

👋 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 · 2025-11-05T21:45:11Z

Codecov Report

❌ Patch coverage is 92.92929% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.33%. Comparing base (08c2236) to head (2330ed9).
⚠️ Report is 22 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/funding.rs	42.85%	4 Missing ⚠️
lightning/src/ln/channel.rs	94.11%	0 Missing and 2 partials ⚠️
lightning/src/ln/interactivetxs.rs	97.72%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #4208 +/- ##
==========================================
+ Coverage 89.30% 89.33% +0.03% 
==========================================
Files 180 180 Lines 137913 138176 +263 Branches 137913 138176 +263 ==========================================
+ Hits 123159 123445 +286 + Misses 12142 12118 -24 - Partials 2612 2613 +1

Flag	Coverage Δ
fuzzing	`33.58% <0.00%> (+0.06%)`	⬆️
tests	`88.73% <92.92%> (+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.

wpaulino

LGTM after squash

ldk-reviews-bot · 2025-11-05T22:19:07Z

👋 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

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 · 2025-11-06T15:09:00Z

Squahsed

jkczyz · 2025-11-07T16:40:42Z

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

ldk-reviews-bot · 2025-11-08T15:10:11Z

🔔 1st Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

TheBlueMatt

Basically LGTM but I'd kinda rather not use the very confusing secp256k1 constants.

TheBlueMatt · 2025-11-09T13:48:04Z

+bitcoin::secp256k1::constants::PUBLIC_KEY_SIZE as u64 + // pubkey1
 	1 + // data len
-33 + // pubkey2
+bitcoin::secp256k1::constants::PUBLIC_KEY_SIZE as u64 + // pubkey2


Honestly I find these constants kinda unreadable - a public key's "size" really should be 64 bytes (ish) cause its two coordinates. The fact that PUBLIC_KEY_SIZE refers to the serialized size when serialized in compressed form is very surprising. Same goes for MAX_SIGNATURE_SIZE, which confusingly refers to the maximum size of a signature we generate (depending on settings, often its not even that), not the actual maximum signature size.

Honestly I find these constants kinda unreadable - a public key's "size" really should be 64 bytes (ish) cause its two coordinates. The fact that PUBLIC_KEY_SIZE refers to the serialized size when serialized in compressed form is very surprising.
Hmm... there is also UNCOMPRESSED_PUBLIC_KEY_SIZE, so seems this complaint is more directed at secp256k1 crate not using a similar prefix for PUBLIC_KEY_SIZE?
Same goes for MAX_SIGNATURE_SIZE, which confusingly refers to the maximum size of a signature we generate (depending on settings, often its not even that), not the actual maximum signature size.
It's the maximum that secp256k1 will generate, though, which is what we use.

Hmm... there is also UNCOMPRESSED_PUBLIC_KEY_SIZE, so seems this complaint is more directed at secp256k1 crate not using a similar prefix for PUBLIC_KEY_SIZE?
I mean, yes, but that doesn't mean we have to use confusing constants :).
It's the maximum that secp256k1 will generate, though, which is what we use.
But its not, at least not with the grind signatures default-feature. The use here in many places is actually the maximum standard signature size, which is the default the secp will generate but that's a different concept.

Added some internal constants. FWIW, we are already using PUBLIC_KEY_SIZE throughout the codebase.

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.

jkczyz · 2025-11-10T23:41:26Z

While looking to fix the MAX_STANDARD_TX_WEIGHT check, I noticed that we aren't adjusting FundingTxInput::new_p2wpkh for grind_signatures. Gonna push the fix in this PR rather than a follow-up since it is related.

TheBlueMatt

LGTM, feel free to squash.

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.

The interactive-tx construction protocol needs to make sure the constructed transaction does not exceed MAX_STANDARD_TX_WEIGHT. A naive estimate of the transaction weight after signing was used, but was not accurate. Specifically, it double-counted EMPTY_SCRIPT_SIG_WEIGHT and didn't include SEGWIT_MARKER_FLAG_WEIGHT.

Useful for re-using code producing a FundingTxInput set to implement CoinSelectionSource.

jkczyz · 2025-11-11T15:10:50Z

Squashed

TheBlueMatt

Your turn @wpaulino

TheBlueMatt · 2025-11-12T13:08:10Z

Backported in #4221

jkczyz · 2025-11-05T18:54:24Z

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 · 2025-11-05T18:54:27Z

👋 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 · 2025-11-05T21:45:11Z

Codecov Report

❌ Patch coverage is 92.92929% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.33%. Comparing base (08c2236) to head (2330ed9).
⚠️ Report is 22 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/funding.rs	42.85%	4 Missing ⚠️
lightning/src/ln/channel.rs	94.11%	0 Missing and 2 partials ⚠️
lightning/src/ln/interactivetxs.rs	97.72%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #4208 +/- ##
==========================================
+ Coverage 89.30% 89.33% +0.03% 
==========================================
Files 180 180 Lines 137913 138176 +263 Branches 137913 138176 +263 ==========================================
+ Hits 123159 123445 +286 + Misses 12142 12118 -24 - Partials 2612 2613 +1

Flag	Coverage Δ
fuzzing	`33.58% <0.00%> (+0.06%)`	⬆️
tests	`88.73% <92.92%> (+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.

wpaulino

LGTM after squash

ldk-reviews-bot · 2025-11-05T22:19:07Z

👋 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

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 · 2025-11-06T15:09:00Z

Squahsed

jkczyz · 2025-11-07T16:40:42Z

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

ldk-reviews-bot · 2025-11-08T15:10:11Z

🔔 1st Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

TheBlueMatt

Basically LGTM but I'd kinda rather not use the very confusing secp256k1 constants.

TheBlueMatt · 2025-11-09T13:48:04Z

+bitcoin::secp256k1::constants::PUBLIC_KEY_SIZE as u64 + // pubkey1
 	1 + // data len
-33 + // pubkey2
+bitcoin::secp256k1::constants::PUBLIC_KEY_SIZE as u64 + // pubkey2


Honestly I find these constants kinda unreadable - a public key's "size" really should be 64 bytes (ish) cause its two coordinates. The fact that PUBLIC_KEY_SIZE refers to the serialized size when serialized in compressed form is very surprising. Same goes for MAX_SIGNATURE_SIZE, which confusingly refers to the maximum size of a signature we generate (depending on settings, often its not even that), not the actual maximum signature size.

Honestly I find these constants kinda unreadable - a public key's "size" really should be 64 bytes (ish) cause its two coordinates. The fact that PUBLIC_KEY_SIZE refers to the serialized size when serialized in compressed form is very surprising.
Hmm... there is also UNCOMPRESSED_PUBLIC_KEY_SIZE, so seems this complaint is more directed at secp256k1 crate not using a similar prefix for PUBLIC_KEY_SIZE?
Same goes for MAX_SIGNATURE_SIZE, which confusingly refers to the maximum size of a signature we generate (depending on settings, often its not even that), not the actual maximum signature size.
It's the maximum that secp256k1 will generate, though, which is what we use.

Hmm... there is also UNCOMPRESSED_PUBLIC_KEY_SIZE, so seems this complaint is more directed at secp256k1 crate not using a similar prefix for PUBLIC_KEY_SIZE?
I mean, yes, but that doesn't mean we have to use confusing constants :).
It's the maximum that secp256k1 will generate, though, which is what we use.
But its not, at least not with the grind signatures default-feature. The use here in many places is actually the maximum standard signature size, which is the default the secp will generate but that's a different concept.

Added some internal constants. FWIW, we are already using PUBLIC_KEY_SIZE throughout the codebase.

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.

jkczyz · 2025-11-10T23:41:26Z

While looking to fix the MAX_STANDARD_TX_WEIGHT check, I noticed that we aren't adjusting FundingTxInput::new_p2wpkh for grind_signatures. Gonna push the fix in this PR rather than a follow-up since it is related.

TheBlueMatt

LGTM, feel free to squash.

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.

The interactive-tx construction protocol needs to make sure the constructed transaction does not exceed MAX_STANDARD_TX_WEIGHT. A naive estimate of the transaction weight after signing was used, but was not accurate. Specifically, it double-counted EMPTY_SCRIPT_SIG_WEIGHT and didn't include SEGWIT_MARKER_FLAG_WEIGHT.

Useful for re-using code producing a FundingTxInput set to implement CoinSelectionSource.

jkczyz · 2025-11-11T15:10:50Z

Squashed

TheBlueMatt

Your turn @wpaulino

TheBlueMatt · 2025-11-12T13:08:10Z

Backported in #4221

jkczyz · 2025-11-05T18:54:24Z

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 · 2025-11-05T18:54:27Z

👋 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 · 2025-11-05T21:45:11Z

Codecov Report

❌ Patch coverage is 92.92929% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.33%. Comparing base (08c2236) to head (2330ed9).
⚠️ Report is 22 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/funding.rs	42.85%	4 Missing ⚠️
lightning/src/ln/channel.rs	94.11%	0 Missing and 2 partials ⚠️
lightning/src/ln/interactivetxs.rs	97.72%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #4208 +/- ##
==========================================
+ Coverage 89.30% 89.33% +0.03% 
==========================================
Files 180 180 Lines 137913 138176 +263 Branches 137913 138176 +263 ==========================================
+ Hits 123159 123445 +286 + Misses 12142 12118 -24 - Partials 2612 2613 +1

Flag	Coverage Δ
fuzzing	`33.58% <0.00%> (+0.06%)`	⬆️
tests	`88.73% <92.92%> (+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.

wpaulino

LGTM after squash

ldk-reviews-bot · 2025-11-05T22:19:07Z

👋 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

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 · 2025-11-06T15:09:00Z

Squahsed

jkczyz · 2025-11-07T16:40:42Z

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

ldk-reviews-bot · 2025-11-08T15:10:11Z

🔔 1st Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

TheBlueMatt

Basically LGTM but I'd kinda rather not use the very confusing secp256k1 constants.

TheBlueMatt · 2025-11-09T13:48:04Z

+bitcoin::secp256k1::constants::PUBLIC_KEY_SIZE as u64 + // pubkey1
 	1 + // data len
-33 + // pubkey2
+bitcoin::secp256k1::constants::PUBLIC_KEY_SIZE as u64 + // pubkey2


Honestly I find these constants kinda unreadable - a public key's "size" really should be 64 bytes (ish) cause its two coordinates. The fact that PUBLIC_KEY_SIZE refers to the serialized size when serialized in compressed form is very surprising. Same goes for MAX_SIGNATURE_SIZE, which confusingly refers to the maximum size of a signature we generate (depending on settings, often its not even that), not the actual maximum signature size.

Honestly I find these constants kinda unreadable - a public key's "size" really should be 64 bytes (ish) cause its two coordinates. The fact that PUBLIC_KEY_SIZE refers to the serialized size when serialized in compressed form is very surprising.
Hmm... there is also UNCOMPRESSED_PUBLIC_KEY_SIZE, so seems this complaint is more directed at secp256k1 crate not using a similar prefix for PUBLIC_KEY_SIZE?
Same goes for MAX_SIGNATURE_SIZE, which confusingly refers to the maximum size of a signature we generate (depending on settings, often its not even that), not the actual maximum signature size.
It's the maximum that secp256k1 will generate, though, which is what we use.

Hmm... there is also UNCOMPRESSED_PUBLIC_KEY_SIZE, so seems this complaint is more directed at secp256k1 crate not using a similar prefix for PUBLIC_KEY_SIZE?
I mean, yes, but that doesn't mean we have to use confusing constants :).
It's the maximum that secp256k1 will generate, though, which is what we use.
But its not, at least not with the grind signatures default-feature. The use here in many places is actually the maximum standard signature size, which is the default the secp will generate but that's a different concept.

Added some internal constants. FWIW, we are already using PUBLIC_KEY_SIZE throughout the codebase.

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.

jkczyz · 2025-11-10T23:41:26Z

While looking to fix the MAX_STANDARD_TX_WEIGHT check, I noticed that we aren't adjusting FundingTxInput::new_p2wpkh for grind_signatures. Gonna push the fix in this PR rather than a follow-up since it is related.

TheBlueMatt

LGTM, feel free to squash.

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.

The interactive-tx construction protocol needs to make sure the constructed transaction does not exceed MAX_STANDARD_TX_WEIGHT. A naive estimate of the transaction weight after signing was used, but was not accurate. Specifically, it double-counted EMPTY_SCRIPT_SIG_WEIGHT and didn't include SEGWIT_MARKER_FLAG_WEIGHT.

Useful for re-using code producing a FundingTxInput set to implement CoinSelectionSource.

jkczyz · 2025-11-11T15:10:50Z

Squashed

TheBlueMatt

Your turn @wpaulino

TheBlueMatt · 2025-11-12T13:08:10Z

Backported in #4221

jkczyz · 2025-11-05T18:54:24Z

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 · 2025-11-05T18:54:27Z

👋 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 · 2025-11-05T21:45:11Z

Codecov Report

❌ Patch coverage is 92.92929% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.33%. Comparing base (08c2236) to head (2330ed9).
⚠️ Report is 22 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/funding.rs	42.85%	4 Missing ⚠️
lightning/src/ln/channel.rs	94.11%	0 Missing and 2 partials ⚠️
lightning/src/ln/interactivetxs.rs	97.72%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #4208 +/- ##
==========================================
+ Coverage 89.30% 89.33% +0.03% 
==========================================
Files 180 180 Lines 137913 138176 +263 Branches 137913 138176 +263 ==========================================
+ Hits 123159 123445 +286 + Misses 12142 12118 -24 - Partials 2612 2613 +1

Flag	Coverage Δ
fuzzing	`33.58% <0.00%> (+0.06%)`	⬆️
tests	`88.73% <92.92%> (+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.

wpaulino

LGTM after squash

ldk-reviews-bot · 2025-11-05T22:19:07Z

👋 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

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 · 2025-11-06T15:09:00Z

Squahsed

jkczyz · 2025-11-07T16:40:42Z

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

ldk-reviews-bot · 2025-11-08T15:10:11Z

🔔 1st Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

TheBlueMatt

Basically LGTM but I'd kinda rather not use the very confusing secp256k1 constants.

TheBlueMatt · 2025-11-09T13:48:04Z

+bitcoin::secp256k1::constants::PUBLIC_KEY_SIZE as u64 + // pubkey1
 	1 + // data len
-33 + // pubkey2
+bitcoin::secp256k1::constants::PUBLIC_KEY_SIZE as u64 + // pubkey2


Honestly I find these constants kinda unreadable - a public key's "size" really should be 64 bytes (ish) cause its two coordinates. The fact that PUBLIC_KEY_SIZE refers to the serialized size when serialized in compressed form is very surprising. Same goes for MAX_SIGNATURE_SIZE, which confusingly refers to the maximum size of a signature we generate (depending on settings, often its not even that), not the actual maximum signature size.

Honestly I find these constants kinda unreadable - a public key's "size" really should be 64 bytes (ish) cause its two coordinates. The fact that PUBLIC_KEY_SIZE refers to the serialized size when serialized in compressed form is very surprising.
Hmm... there is also UNCOMPRESSED_PUBLIC_KEY_SIZE, so seems this complaint is more directed at secp256k1 crate not using a similar prefix for PUBLIC_KEY_SIZE?
Same goes for MAX_SIGNATURE_SIZE, which confusingly refers to the maximum size of a signature we generate (depending on settings, often its not even that), not the actual maximum signature size.
It's the maximum that secp256k1 will generate, though, which is what we use.

Hmm... there is also UNCOMPRESSED_PUBLIC_KEY_SIZE, so seems this complaint is more directed at secp256k1 crate not using a similar prefix for PUBLIC_KEY_SIZE?
I mean, yes, but that doesn't mean we have to use confusing constants :).
It's the maximum that secp256k1 will generate, though, which is what we use.
But its not, at least not with the grind signatures default-feature. The use here in many places is actually the maximum standard signature size, which is the default the secp will generate but that's a different concept.

Added some internal constants. FWIW, we are already using PUBLIC_KEY_SIZE throughout the codebase.

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.

jkczyz · 2025-11-10T23:41:26Z

While looking to fix the MAX_STANDARD_TX_WEIGHT check, I noticed that we aren't adjusting FundingTxInput::new_p2wpkh for grind_signatures. Gonna push the fix in this PR rather than a follow-up since it is related.

TheBlueMatt

LGTM, feel free to squash.

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.

The interactive-tx construction protocol needs to make sure the constructed transaction does not exceed MAX_STANDARD_TX_WEIGHT. A naive estimate of the transaction weight after signing was used, but was not accurate. Specifically, it double-counted EMPTY_SCRIPT_SIG_WEIGHT and didn't include SEGWIT_MARKER_FLAG_WEIGHT.

Useful for re-using code producing a FundingTxInput set to implement CoinSelectionSource.

jkczyz · 2025-11-11T15:10:50Z

Squashed

TheBlueMatt

Your turn @wpaulino

TheBlueMatt · 2025-11-12T13:08:10Z

Backported in #4221

jkczyz · 2025-11-05T18:54:24Z

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 · 2025-11-05T18:54:27Z

👋 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 · 2025-11-05T21:45:11Z

Codecov Report

❌ Patch coverage is 92.92929% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.33%. Comparing base (08c2236) to head (2330ed9).
⚠️ Report is 22 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/funding.rs	42.85%	4 Missing ⚠️
lightning/src/ln/channel.rs	94.11%	0 Missing and 2 partials ⚠️
lightning/src/ln/interactivetxs.rs	97.72%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #4208 +/- ##
==========================================
+ Coverage 89.30% 89.33% +0.03% 
==========================================
Files 180 180 Lines 137913 138176 +263 Branches 137913 138176 +263 ==========================================
+ Hits 123159 123445 +286 + Misses 12142 12118 -24 - Partials 2612 2613 +1

Flag	Coverage Δ
fuzzing	`33.58% <0.00%> (+0.06%)`	⬆️
tests	`88.73% <92.92%> (+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.

wpaulino

LGTM after squash

ldk-reviews-bot · 2025-11-05T22:19:07Z

👋 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

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 · 2025-11-06T15:09:00Z

Squahsed

jkczyz · 2025-11-07T16:40:42Z

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

ldk-reviews-bot · 2025-11-08T15:10:11Z

🔔 1st Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

TheBlueMatt

Basically LGTM but I'd kinda rather not use the very confusing secp256k1 constants.

TheBlueMatt · 2025-11-09T13:48:04Z

+bitcoin::secp256k1::constants::PUBLIC_KEY_SIZE as u64 + // pubkey1
 	1 + // data len
-33 + // pubkey2
+bitcoin::secp256k1::constants::PUBLIC_KEY_SIZE as u64 + // pubkey2


Honestly I find these constants kinda unreadable - a public key's "size" really should be 64 bytes (ish) cause its two coordinates. The fact that PUBLIC_KEY_SIZE refers to the serialized size when serialized in compressed form is very surprising. Same goes for MAX_SIGNATURE_SIZE, which confusingly refers to the maximum size of a signature we generate (depending on settings, often its not even that), not the actual maximum signature size.

Honestly I find these constants kinda unreadable - a public key's "size" really should be 64 bytes (ish) cause its two coordinates. The fact that PUBLIC_KEY_SIZE refers to the serialized size when serialized in compressed form is very surprising.
Hmm... there is also UNCOMPRESSED_PUBLIC_KEY_SIZE, so seems this complaint is more directed at secp256k1 crate not using a similar prefix for PUBLIC_KEY_SIZE?
Same goes for MAX_SIGNATURE_SIZE, which confusingly refers to the maximum size of a signature we generate (depending on settings, often its not even that), not the actual maximum signature size.
It's the maximum that secp256k1 will generate, though, which is what we use.

Hmm... there is also UNCOMPRESSED_PUBLIC_KEY_SIZE, so seems this complaint is more directed at secp256k1 crate not using a similar prefix for PUBLIC_KEY_SIZE?
I mean, yes, but that doesn't mean we have to use confusing constants :).
It's the maximum that secp256k1 will generate, though, which is what we use.
But its not, at least not with the grind signatures default-feature. The use here in many places is actually the maximum standard signature size, which is the default the secp will generate but that's a different concept.

Added some internal constants. FWIW, we are already using PUBLIC_KEY_SIZE throughout the codebase.

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.

jkczyz · 2025-11-10T23:41:26Z

While looking to fix the MAX_STANDARD_TX_WEIGHT check, I noticed that we aren't adjusting FundingTxInput::new_p2wpkh for grind_signatures. Gonna push the fix in this PR rather than a follow-up since it is related.

TheBlueMatt

LGTM, feel free to squash.

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.

The interactive-tx construction protocol needs to make sure the constructed transaction does not exceed MAX_STANDARD_TX_WEIGHT. A naive estimate of the transaction weight after signing was used, but was not accurate. Specifically, it double-counted EMPTY_SCRIPT_SIG_WEIGHT and didn't include SEGWIT_MARKER_FLAG_WEIGHT.

Useful for re-using code producing a FundingTxInput set to implement CoinSelectionSource.

jkczyz · 2025-11-11T15:10:50Z

Squashed

TheBlueMatt

Your turn @wpaulino

TheBlueMatt · 2025-11-12T13:08:10Z

Backported in #4221

jkczyz · 2025-11-05T18:54:24Z

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 · 2025-11-05T18:54:27Z

👋 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 · 2025-11-05T21:45:11Z

Codecov Report

❌ Patch coverage is 92.92929% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.33%. Comparing base (08c2236) to head (2330ed9).
⚠️ Report is 22 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/funding.rs	42.85%	4 Missing ⚠️
lightning/src/ln/channel.rs	94.11%	0 Missing and 2 partials ⚠️
lightning/src/ln/interactivetxs.rs	97.72%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #4208 +/- ##
==========================================
+ Coverage 89.30% 89.33% +0.03% 
==========================================
Files 180 180 Lines 137913 138176 +263 Branches 137913 138176 +263 ==========================================
+ Hits 123159 123445 +286 + Misses 12142 12118 -24 - Partials 2612 2613 +1

Flag	Coverage Δ
fuzzing	`33.58% <0.00%> (+0.06%)`	⬆️
tests	`88.73% <92.92%> (+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.

wpaulino

LGTM after squash

ldk-reviews-bot · 2025-11-05T22:19:07Z

👋 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

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 · 2025-11-06T15:09:00Z

Squahsed

jkczyz · 2025-11-07T16:40:42Z

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

ldk-reviews-bot · 2025-11-08T15:10:11Z

🔔 1st Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

TheBlueMatt

Basically LGTM but I'd kinda rather not use the very confusing secp256k1 constants.

TheBlueMatt · 2025-11-09T13:48:04Z

+bitcoin::secp256k1::constants::PUBLIC_KEY_SIZE as u64 + // pubkey1
 	1 + // data len
-33 + // pubkey2
+bitcoin::secp256k1::constants::PUBLIC_KEY_SIZE as u64 + // pubkey2


Honestly I find these constants kinda unreadable - a public key's "size" really should be 64 bytes (ish) cause its two coordinates. The fact that PUBLIC_KEY_SIZE refers to the serialized size when serialized in compressed form is very surprising. Same goes for MAX_SIGNATURE_SIZE, which confusingly refers to the maximum size of a signature we generate (depending on settings, often its not even that), not the actual maximum signature size.

Honestly I find these constants kinda unreadable - a public key's "size" really should be 64 bytes (ish) cause its two coordinates. The fact that PUBLIC_KEY_SIZE refers to the serialized size when serialized in compressed form is very surprising.
Hmm... there is also UNCOMPRESSED_PUBLIC_KEY_SIZE, so seems this complaint is more directed at secp256k1 crate not using a similar prefix for PUBLIC_KEY_SIZE?
Same goes for MAX_SIGNATURE_SIZE, which confusingly refers to the maximum size of a signature we generate (depending on settings, often its not even that), not the actual maximum signature size.
It's the maximum that secp256k1 will generate, though, which is what we use.

Hmm... there is also UNCOMPRESSED_PUBLIC_KEY_SIZE, so seems this complaint is more directed at secp256k1 crate not using a similar prefix for PUBLIC_KEY_SIZE?
I mean, yes, but that doesn't mean we have to use confusing constants :).
It's the maximum that secp256k1 will generate, though, which is what we use.
But its not, at least not with the grind signatures default-feature. The use here in many places is actually the maximum standard signature size, which is the default the secp will generate but that's a different concept.

Added some internal constants. FWIW, we are already using PUBLIC_KEY_SIZE throughout the codebase.

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.

jkczyz · 2025-11-10T23:41:26Z

While looking to fix the MAX_STANDARD_TX_WEIGHT check, I noticed that we aren't adjusting FundingTxInput::new_p2wpkh for grind_signatures. Gonna push the fix in this PR rather than a follow-up since it is related.

TheBlueMatt

LGTM, feel free to squash.

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.

The interactive-tx construction protocol needs to make sure the constructed transaction does not exceed MAX_STANDARD_TX_WEIGHT. A naive estimate of the transaction weight after signing was used, but was not accurate. Specifically, it double-counted EMPTY_SCRIPT_SIG_WEIGHT and didn't include SEGWIT_MARKER_FLAG_WEIGHT.

Useful for re-using code producing a FundingTxInput set to implement CoinSelectionSource.

jkczyz · 2025-11-11T15:10:50Z

Squashed

TheBlueMatt

Your turn @wpaulino

TheBlueMatt · 2025-11-12T13:08:10Z

Backported in #4221

jkczyz · 2025-11-05T18:54:24Z

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 · 2025-11-05T18:54:27Z

👋 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 · 2025-11-05T21:45:11Z

Codecov Report

❌ Patch coverage is 92.92929% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.33%. Comparing base (08c2236) to head (2330ed9).
⚠️ Report is 22 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/funding.rs	42.85%	4 Missing ⚠️
lightning/src/ln/channel.rs	94.11%	0 Missing and 2 partials ⚠️
lightning/src/ln/interactivetxs.rs	97.72%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #4208 +/- ##
==========================================
+ Coverage 89.30% 89.33% +0.03% 
==========================================
Files 180 180 Lines 137913 138176 +263 Branches 137913 138176 +263 ==========================================
+ Hits 123159 123445 +286 + Misses 12142 12118 -24 - Partials 2612 2613 +1

Flag	Coverage Δ
fuzzing	`33.58% <0.00%> (+0.06%)`	⬆️
tests	`88.73% <92.92%> (+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.

wpaulino

LGTM after squash

ldk-reviews-bot · 2025-11-05T22:19:07Z

👋 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

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 · 2025-11-06T15:09:00Z

Squahsed

jkczyz · 2025-11-07T16:40:42Z

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

ldk-reviews-bot · 2025-11-08T15:10:11Z

🔔 1st Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

TheBlueMatt

Basically LGTM but I'd kinda rather not use the very confusing secp256k1 constants.

TheBlueMatt · 2025-11-09T13:48:04Z

+bitcoin::secp256k1::constants::PUBLIC_KEY_SIZE as u64 + // pubkey1
 	1 + // data len
-33 + // pubkey2
+bitcoin::secp256k1::constants::PUBLIC_KEY_SIZE as u64 + // pubkey2


Honestly I find these constants kinda unreadable - a public key's "size" really should be 64 bytes (ish) cause its two coordinates. The fact that PUBLIC_KEY_SIZE refers to the serialized size when serialized in compressed form is very surprising. Same goes for MAX_SIGNATURE_SIZE, which confusingly refers to the maximum size of a signature we generate (depending on settings, often its not even that), not the actual maximum signature size.

Honestly I find these constants kinda unreadable - a public key's "size" really should be 64 bytes (ish) cause its two coordinates. The fact that PUBLIC_KEY_SIZE refers to the serialized size when serialized in compressed form is very surprising.
Hmm... there is also UNCOMPRESSED_PUBLIC_KEY_SIZE, so seems this complaint is more directed at secp256k1 crate not using a similar prefix for PUBLIC_KEY_SIZE?
Same goes for MAX_SIGNATURE_SIZE, which confusingly refers to the maximum size of a signature we generate (depending on settings, often its not even that), not the actual maximum signature size.
It's the maximum that secp256k1 will generate, though, which is what we use.

Hmm... there is also UNCOMPRESSED_PUBLIC_KEY_SIZE, so seems this complaint is more directed at secp256k1 crate not using a similar prefix for PUBLIC_KEY_SIZE?
I mean, yes, but that doesn't mean we have to use confusing constants :).
It's the maximum that secp256k1 will generate, though, which is what we use.
But its not, at least not with the grind signatures default-feature. The use here in many places is actually the maximum standard signature size, which is the default the secp will generate but that's a different concept.

Added some internal constants. FWIW, we are already using PUBLIC_KEY_SIZE throughout the codebase.

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.

jkczyz · 2025-11-10T23:41:26Z

While looking to fix the MAX_STANDARD_TX_WEIGHT check, I noticed that we aren't adjusting FundingTxInput::new_p2wpkh for grind_signatures. Gonna push the fix in this PR rather than a follow-up since it is related.

TheBlueMatt

LGTM, feel free to squash.

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.

The interactive-tx construction protocol needs to make sure the constructed transaction does not exceed MAX_STANDARD_TX_WEIGHT. A naive estimate of the transaction weight after signing was used, but was not accurate. Specifically, it double-counted EMPTY_SCRIPT_SIG_WEIGHT and didn't include SEGWIT_MARKER_FLAG_WEIGHT.

Useful for re-using code producing a FundingTxInput set to implement CoinSelectionSource.

jkczyz · 2025-11-11T15:10:50Z

Squashed

TheBlueMatt

Your turn @wpaulino

TheBlueMatt · 2025-11-12T13:08:10Z

Backported in #4221

jkczyz · 2025-11-05T18:54:24Z

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 · 2025-11-05T18:54:27Z

👋 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 · 2025-11-05T21:45:11Z

Codecov Report

❌ Patch coverage is 92.92929% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.33%. Comparing base (08c2236) to head (2330ed9).
⚠️ Report is 22 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/funding.rs	42.85%	4 Missing ⚠️
lightning/src/ln/channel.rs	94.11%	0 Missing and 2 partials ⚠️
lightning/src/ln/interactivetxs.rs	97.72%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #4208 +/- ##
==========================================
+ Coverage 89.30% 89.33% +0.03% 
==========================================
Files 180 180 Lines 137913 138176 +263 Branches 137913 138176 +263 ==========================================
+ Hits 123159 123445 +286 + Misses 12142 12118 -24 - Partials 2612 2613 +1

Flag	Coverage Δ
fuzzing	`33.58% <0.00%> (+0.06%)`	⬆️
tests	`88.73% <92.92%> (+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.

wpaulino

LGTM after squash

ldk-reviews-bot · 2025-11-05T22:19:07Z

👋 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

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 · 2025-11-06T15:09:00Z

Squahsed

jkczyz · 2025-11-07T16:40:42Z

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

ldk-reviews-bot · 2025-11-08T15:10:11Z

🔔 1st Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

TheBlueMatt

Basically LGTM but I'd kinda rather not use the very confusing secp256k1 constants.

TheBlueMatt · 2025-11-09T13:48:04Z

+bitcoin::secp256k1::constants::PUBLIC_KEY_SIZE as u64 + // pubkey1
 	1 + // data len
-33 + // pubkey2
+bitcoin::secp256k1::constants::PUBLIC_KEY_SIZE as u64 + // pubkey2


Honestly I find these constants kinda unreadable - a public key's "size" really should be 64 bytes (ish) cause its two coordinates. The fact that PUBLIC_KEY_SIZE refers to the serialized size when serialized in compressed form is very surprising. Same goes for MAX_SIGNATURE_SIZE, which confusingly refers to the maximum size of a signature we generate (depending on settings, often its not even that), not the actual maximum signature size.

Honestly I find these constants kinda unreadable - a public key's "size" really should be 64 bytes (ish) cause its two coordinates. The fact that PUBLIC_KEY_SIZE refers to the serialized size when serialized in compressed form is very surprising.
Hmm... there is also UNCOMPRESSED_PUBLIC_KEY_SIZE, so seems this complaint is more directed at secp256k1 crate not using a similar prefix for PUBLIC_KEY_SIZE?
Same goes for MAX_SIGNATURE_SIZE, which confusingly refers to the maximum size of a signature we generate (depending on settings, often its not even that), not the actual maximum signature size.
It's the maximum that secp256k1 will generate, though, which is what we use.

Hmm... there is also UNCOMPRESSED_PUBLIC_KEY_SIZE, so seems this complaint is more directed at secp256k1 crate not using a similar prefix for PUBLIC_KEY_SIZE?
I mean, yes, but that doesn't mean we have to use confusing constants :).
It's the maximum that secp256k1 will generate, though, which is what we use.
But its not, at least not with the grind signatures default-feature. The use here in many places is actually the maximum standard signature size, which is the default the secp will generate but that's a different concept.

Added some internal constants. FWIW, we are already using PUBLIC_KEY_SIZE throughout the codebase.

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.

jkczyz · 2025-11-10T23:41:26Z

While looking to fix the MAX_STANDARD_TX_WEIGHT check, I noticed that we aren't adjusting FundingTxInput::new_p2wpkh for grind_signatures. Gonna push the fix in this PR rather than a follow-up since it is related.

TheBlueMatt

LGTM, feel free to squash.

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.

The interactive-tx construction protocol needs to make sure the constructed transaction does not exceed MAX_STANDARD_TX_WEIGHT. A naive estimate of the transaction weight after signing was used, but was not accurate. Specifically, it double-counted EMPTY_SCRIPT_SIG_WEIGHT and didn't include SEGWIT_MARKER_FLAG_WEIGHT.

Useful for re-using code producing a FundingTxInput set to implement CoinSelectionSource.

jkczyz · 2025-11-11T15:10:50Z

Squashed

TheBlueMatt

Your turn @wpaulino

TheBlueMatt · 2025-11-12T13:08:10Z

Backported in #4221

jkczyz requested a review from wpaulino November 5, 2025 18:54

jkczyz force-pushed the 2025-11-signature-weight branch from edd3851 to 39fc8fbCompare November 5, 2025 20:10

morehouse mentioned this pull request Nov 5, 2025
Use 73 bytes der-encoded signatures in weight estimation ACINQ/eclair#3210
Merged

wpaulino reviewed Nov 5, 2025
View reviewed changes

Comment threadlightning/src/sign/mod.rs Outdated

jkczyz force-pushed the 2025-11-signature-weight branch from 39fc8fb to 54bcda8Compare November 6, 2025 15:02

jkczyz commented Nov 6, 2025
View reviewed changes

jkczyz force-pushed the 2025-11-signature-weight branch from 54bcda8 to 63bb5edCompare November 6, 2025 15:08

jkczyz requested a review from TheBlueMatt November 6, 2025 15:09

jkczyz self-assigned this Nov 6, 2025

jkczyz added this to Weekly Goals Nov 6, 2025

jkczyz force-pushed the 2025-11-signature-weight branch from 63bb5ed to d88395fCompare November 7, 2025 16:38

TheBlueMatt reviewed Nov 9, 2025
View reviewed changes

TheBlueMatt added the backport 0.2 label Nov 9, 2025

TheBlueMatt added this to the 0.2 milestone Nov 9, 2025

jkczyz force-pushed the 2025-11-signature-weight branch from d88395f to 6778f24Compare November 10, 2025 16:21

wpaulino previously approved these changes Nov 10, 2025
View reviewed changes

jkczyz dismissed wpaulino’s stale review via 4334722November 10, 2025 23:46

jkczyz force-pushed the 2025-11-signature-weight branch from 6778f24 to 4334722Compare November 10, 2025 23:46

jkczyz requested review from TheBlueMatt and wpaulino November 10, 2025 23:46

jkczyz force-pushed the 2025-11-signature-weight branch 2 times, most recently from 2daeaf2 to a2a2cfcCompare November 11, 2025 00:29

TheBlueMatt reviewed Nov 11, 2025
View reviewed changes

jkczyz added 4 commits November 11, 2025 09:10

Account for grind_signatures in splice funding tx
4698026
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.

Add method to convert FundingTxInput to Utxo
2330ed9
Useful for re-using code producing a FundingTxInput set to implement CoinSelectionSource.

jkczyz force-pushed the 2025-11-signature-weight branch from a2a2cfc to 2330ed9Compare November 11, 2025 15:10

jkczyz requested a review from TheBlueMatt November 11, 2025 15:10

TheBlueMatt approved these changes Nov 11, 2025
View reviewed changes

wpaulino approved these changes Nov 11, 2025
View reviewed changes

wpaulino merged commit 9150bc8 into lightningdevkit:mainNov 11, 2025
26 checks passed

github-project-automationBot moved this to Done in Weekly Goals Nov 11, 2025

TheBlueMatt mentioned this pull request Nov 12, 2025
0.2 backports #4221
Merged

TheBlueMatt removed the backport 0.2 label Nov 12, 2025