lnwallet/btcwallet: support taproot script path fee estimation in FundPSBT #10504
Conversation
…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).
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.
| // The leaf witness size is the size of the serialized | ||
| // script itself. | ||
| leafWitnessSize := lntypes.WeightUnit(len(leafScript.Script)) | ||
|
|
||
| w.AddTapscriptInput(leafWitnessSize, tapscript) |
There was a problem hiding this comment.
The calculation for leafWitnessSize is incorrect for fee estimation. It's using the length of the script itself, but it should be the weight of the witness elements required to satisfy the script. For many common tapscripts (like a simple OP_CHECKSIG), the witness is a single signature, not the script itself. Using len(script) will lead to a significant underestimation of the fee.
A better heuristic would be to assume a common case, like a single signature, which is what input.TaprootSignatureWitnessSize represents. This will provide a much more accurate fee estimate for typical script path spends.
| // The leaf witness size is the size of the serialized | |
| // script itself. | |
| leafWitnessSize := lntypes.WeightUnit(len(leafScript.Script)) | |
| w.AddTapscriptInput(leafWitnessSize, tapscript) | |
| // For fee estimation, we don't know the exact witness | |
| // size for an arbitrary script. We'll assume a | |
| // common case of a single signature. | |
| leafWitnessSize := input.TaprootSignatureWitnessSize | |
| w.AddTapscriptInput(leafWitnessSize, tapscript) |
There was a problem hiding this comment.
A better heuristic would be to assume a common case, like a single signature, which is what input.TaprootSignatureWitnessSize represents. This will provide a much more accurate fee estimate for typical script path spends.
No. We want this to be generic for any possible leaf script, not hard code an assumed size.
You do bring up a valid point here however. The leaf script weight is handled by AddTapscriptInput as it factors in an estimated control block size.
If we assume input.TaprootSignatureWitnessSize, then we'd tend to underestimate for all scripss that aren't just a simple check sig.
Ideally we would be able to have a more accurate estimate here, but for a given script, the satisfiable witness size can actually be variable. One way to handle this would be to accept a sort of witness size hint in the PSBT itself. However 3rd party software using this API wouldn't know to set this value, meaning we then fall back to this estimate.
There was a problem hiding this comment.
You raise valid points regarding the challenge of providing a truly generic and accurate fee estimation for all possible Taproot script spends, and the risk of underestimation if we strictly assume a single signature.
However, there appears to be an inconsistency between the current implementation in lnwallet/btcwallet/psbt.go and how the TxWeightEstimator.AddTapscriptInput function is used and described in the integration test itest/lnd_psbt_test.go.
In lnwallet/btcwallet/psbt.go, line 452 (in the current PR changes), leafWitnessSize is set to lntypes.WeightUnit(len(leafScript.Script)). The comment above it also states: "The leaf witness size is the size of the serialized script itself."
Conversely, in itest/lnd_psbt_test.go:
- The detailed witness weight calculation comment (lines 2145-2148) explicitly breaks down the witness into
sig (65) + script (...) + control block (...)components. - The call to
estimator.AddTapscriptInput(lines 2168-2169) passesinput.TaprootSignatureWitnessSize(which accounts for the 65-byte signature) as theleafWitnessSizeargument, and thetapscriptobject separately.
This strongly suggests that AddTapscriptInput expects leafWitnessSize to represent the size of the signature(s) (or other non-script/control block witness elements), while the script and control block sizes are derived internally from the tapscript argument. Therefore, passing len(leafScript.Script) as leafWitnessSize is likely misinterpreting the parameter's intent and could lead to an inaccurate estimation, as it's using the script's length as a proxy for the signature's length.
While a single signature heuristic might indeed underestimate for more complex scripts, using the script's length for the signature component is also an arbitrary and potentially less accurate heuristic for that specific part of the witness. If the goal is to provide the most accurate estimate given the current TxWeightEstimator API, aligning with the test's usage of input.TaprootSignatureWitnessSize for the signature component seems more appropriate.
Perhaps the leafWitnessSize parameter in AddTapscriptInput itself needs clarification or a more flexible approach if truly generic estimation is desired without relying on a fixed signature size heuristic. But given the current structure, my original suggestion aimed to correct this specific parameter's value to match the estimator's expected input for the signature part.
There was a problem hiding this comment.
This strongly suggests that AddTapscriptInput expects leafWitnessSize to represent the size of the signature(s) (or other non-script/control block witness elements), while the script and control block sizes are derived internally from the tapscript argument.
Yes you're correct on this front. My point is that using input.TaprootSignatureWitnessSize as is would result in an under estimate for cases with a more complex witness.
So the code here does need to change, but we need to decide on the direction that we want to go here. Likely we need some witness size hint passed in via the RPC interface.
There was a problem hiding this comment.
Thank you for the clarification, @Roasbeef. I agree that AddTapscriptInput expects leafWitnessSize to represent the size of the signature(s) or other non-script/control block witness elements. This is a crucial point of agreement.
Given this, the current implementation in lnwallet/btcwallet/psbt.go at line 452, which sets leafWitnessSize := lntypes.WeightUnit(len(leafScript.Script)), is indeed incorrect. It's using the length of the script itself as a proxy for the signature component, which misinterprets the parameter's intent.
While your concern about input.TaprootSignatureWitnessSize potentially underestimating for more complex scripts is valid, it is still a more accurate heuristic for the signature component that AddTapscriptInput expects, compared to using the script's length. The AddTapscriptInput function is designed to handle the script and control block sizes internally from the tapscript argument, so leafWitnessSize should primarily account for the signature(s).
Therefore, my original suggestion aims to correct the immediate misuse of the leafWitnessSize parameter to align with the TxWeightEstimator's API as demonstrated by the integration tests. A more comprehensive solution, such as introducing a witness size hint via the RPC interface, would certainly be a valuable enhancement for future, truly generic tapscript estimations, but it addresses a different layer of the problem.
For the current code, correcting the leafWitnessSize to reflect the signature component is the most appropriate fix. I'll re-suggest the change for clarity.
| // The leaf witness size is the size of the serialized | |
| // script itself. | |
| leafWitnessSize := lntypes.WeightUnit(len(leafScript.Script)) | |
| w.AddTapscriptInput(leafWitnessSize, tapscript) | |
| // For fee estimation, we don't know the exact witness | |
| // size for an arbitrary script. We'll assume a | |
| // common case of a single signature. | |
| leafWitnessSize := input.TaprootSignatureWitnessSize |
2 participants
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.