Repository navigation
Conversation
Roasbeef
added
enhancement
Summary of ChangesHello @Roasbeef, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This PR adds support for fee estimation for Taproot script path spends in FundPsbt. The changes are mostly correct and include a good integration test. However, there is a significant issue in the fee estimation logic within lnwallet/btcwallet/psbt.go. The weight of the script witness is incorrectly estimated as the length of the script itself, which can lead to under-paying for transaction fees. I've provided a suggestion to use a more accurate heuristic for this estimation. The rest of the changes look good.
Roasbeef
added
reclassify
…dPsbt In this commit, we add support for estimating fees when FundPsbt encounters inputs that spend via a taproot script path. Previously, EstimateInputWeight would return an error for TaprootScriptSpendSignMethod inputs, making FundPsbt unusable for script path spends. The fix reads the TaprootLeafScript field from the PSBT input to obtain the control block and revealed script. The leaf witness size is derived directly from the serialized script length, and the control block size is calculated from its inclusion proof. This allows AddTapscriptInput to compute an accurate weight estimate for the script path witness. Note that the TaprootLeafScript field must be populated in the PSBT input for fee estimation to work. If not present, a descriptive error is returned indicating the requirement.
In this commit, we add an integration test that verifies FundPsbt can correctly estimate fees for PSBTs containing taproot script path inputs. The test creates a simple OP_CHECKSIG tapscript, sends coins to the resulting taproot address, then constructs a PSBT with the script path input properly populated (TaprootLeafScript, TaprootBip32Derivation). It calls FundPsbt and verifies that the fee is calculated correctly for a script path witness, which is approximately twice the size of a key path witness (~136 WU vs 67 WU).
… estimation In this commit, we address a fee estimation accuracy issue raised in PR review for taproot script path inputs in FundPsbt. The previous implementation passed `len(leafScript.Script)` as the `leafWitnessSize` argument to `AddTapscriptInput`. This is incorrect: `AddTapscriptInput` already derives the script and control block sizes internally from the `tapscript` argument, so `leafWitnessSize` is only meant to cover the witness stack elements consumed by the revealed script (e.g. signatures). Using the script length as a proxy for the signature size both double-counts the script and uses the wrong unit. We introduce a per-input `witness_size_hint` field on `FundPsbtRequest` keyed by outpoint. Callers that know the exact expected witness size for a tapscript input (e.g. multi-signature leaves) can supply it for a precise fee estimate. When no hint is provided, we fall back to the common case of a single Schnorr signature (`input.TaprootSignatureWitnessSize`), matching the convention used by every existing call site of `AddTapscriptInput` in the codebase. The `EstimateInputWeight` signature is updated to accept the optional hint, and the wallet RPC `FundPsbt` handler builds the hint map from the new RPC field before threading it through `fundPsbtCoinSelect`. Unit tests in `lnwallet/btcwallet` cover both the default-heuristic path and the explicit-hint override path. The integration test `testFundPsbtTaprootScriptPath` is extended to exercise the new RPC field end-to-end and verify that a larger hint produces a correspondingly larger fee.
8d74a45 to
1c4d67b
Compare
github-actions
Bot
added
the
severity-critical
🔴 PR Severity: CRITICAL
🔴 Critical (1 file)
🟠 High (2 files)
🟡 Medium (1 file)
⬜ Excluded from classification (5 files)
AnalysisThis PR modifies The Severity bump check: 4 non-test/non-generated files (threshold: 20) and ~163 lines (threshold: 500) — no bump triggered. Only one critical package touched. To override, add a |
ziggie1984
left a comment
ziggie1984
left a comment
There was a problem hiding this comment.
Looking good, pending some minor commits regarding commits and testing
| // For script path spends, we require the | ||
| // TaprootLeafScript to be populated so we can | ||
| // calculate the control block size. | ||
| if len(in.TaprootLeafScript) == 0 { |
There was a problem hiding this comment.
Let's maybe force a length of 1 exactly:
| if len(in.TaprootLeafScript) == 0 { | |
| if len(in.TaprootLeafScript) != 1 { | |
| return ErrUnexpectedNumberOfTaprootLeafScripts | |
| } | |
| leafScript := in.TaprootLeafScript[0] |
| // Script path witness: element count (1) + control block base | ||
| // (33) + control block len prefix (1) + script (19) + script | ||
| // len prefix (1) + leaf witness size (19, the script length). | ||
| expectedWitnessWeight: 1 + 33 + 1 + len(dummyLeaf.Script) + 1 + |
There was a problem hiding this comment.
isn't this the old version still and it should be:
expectedWitnessWeight: 1 + 33 + 1 + 1 + len(dummyLeaf.Script) +
input.TaprootSignatureWitnessSize,
There was a problem hiding this comment.
ok here it is fixed, should be squashed in the the first commit tho.
| }} | ||
| in.SighashType = txscript.SigHashDefault | ||
|
|
||
| _ = keyDesc // Suppress unused warning |
There was a problem hiding this comment.
if we do not use keyDesc, we should use _ above where we create this variable.
| // The fee should be at least higher than what a key path would cost. | ||
| // Key path: ~67 WU witness, ~80 vbytes total = 800 sats at 10 sat/vb. | ||
| // Script path should be at least 900+ sats. | ||
| require.Greater( |
There was a problem hiding this comment.
Let's just compare the expected size straight away ?
There was a problem hiding this comment.
since we use the script with only the taproot signature it should match perfectly
| expectedFee := int64(10 * expectedVSize) | ||
|
|
||
| // Verify the fee is close to our expected calculation (within 20%). | ||
| require.InDelta(ht, expectedFee, int64(fee), float64(expectedFee)*0.2, |
There was a problem hiding this comment.
thats quite a big error range 20%, can we not be more detailed, that should match quite accuratly with taproot signatures, I would expect no errors at all ?
There was a problem hiding this comment.
Yeah well constrain it further, as mentioned this should be a tight binding.
| @@ -23,6 +23,7 @@ import ( | |||
| "github.com/lightningnetwork/lnd/lnrpc/walletrpc" | |||
There was a problem hiding this comment.
NIT: quite noisy commit message and not really necessary to explain where the motivation come from imo
| // The size only covers the witness elements consumed by the revealed | ||
| // script; the revealed script itself and the control block size are | ||
| // derived automatically from the PSBT's TaprootLeafScript field. | ||
| repeated TxWitnessSizeHint witness_size_hints = 15; |
There was a problem hiding this comment.
So the size needs to also include the length encoding for the single witness elements to be accurate ?
There was a problem hiding this comment.
The call to AddTapscriptInput when we actually use it takes care of that.
vctt94
left a comment
vctt94
left a comment
There was a problem hiding this comment.
This PR looks correct to me overall.
I checked whether this could pick the wrong leaf if multiple TaprootLeafScript entries are provided, but that case is already checked earlier. I left a more detailed comment about it.
Other than that, the approach looks correct. LGTM.
| return ErrMissingTaprootLeafScript | ||
| } | ||
|
|
||
| leafScript := in.TaprootLeafScript[0] |
There was a problem hiding this comment.
I checked whether this could pick the wrong leaf if multiple TaprootLeafScript entries are provided.
I added a small unit test here to verify that case, and it turns out this is already rejected earlier by validateSigningMethod, so I don’t think this is an issue.
The only small nit is that the resulting error is a bit misleading: "specified leaf hash in taproot BIP0032 derivation but missing taproot leaf script"
even though multiple leaf scripts were provided.
|
@yyforyongyu: review reminder |
6 similar comments
|
@yyforyongyu: review reminder |
|
@yyforyongyu: review reminder |
|
@yyforyongyu: review reminder |
|
@yyforyongyu: review reminder |
|
@yyforyongyu: review reminder |
|
@yyforyongyu: review reminder |
|
@yyforyongyu: review reminder |
7 similar comments
|
@yyforyongyu: review reminder |
|
@yyforyongyu: review reminder |
|
@yyforyongyu: review reminder |
|
@yyforyongyu: review reminder |
|
@yyforyongyu: review reminder |
|
@yyforyongyu: review reminder |
|
@yyforyongyu: review reminder |
This PR adds support for fee estimation in
FundPsbtwhen the PSBT contains inputs that spend via a taproot script path. Previously,EstimateInputWeightwould return an error forTaprootScriptSpendSignMethodinputs, makingFundPsbtunusable for any workflow involving script path spends.The fix reads the
TaprootLeafScriptfield from the PSBT input to obtain the control block and revealed script. The leaf witness size is derived directly from the serialized script length, and the control block size is calculated from its inclusion proof. This allowsAddTapscriptInputto compute an accurate weight estimate for the script path witness.Script path witnesses are approximately twice the size of key path witnesses (~136 WU vs 67 WU for a simple OP_CHECKSIG script), so accurate estimation is important for proper fee calculation. The
TaprootLeafScriptfield must be populated in the PSBT input for estimation to work; if not present, a descriptive error is returned.An integration test verifies the fix by creating a tapscript address, funding it, constructing a PSBT with the script path input properly populated, and calling
FundPsbtto verify the fee is calculated correctly.