walletrpc: unify coin selection, allow coin selection for PSBT with pre-defined inputs #8378
Conversation
guggero
added
wallet
| ChainIO: walletController, | ||
| DefaultConstraints: partialChainControl.ChannelConstraints, | ||
| NetParams: *walletConfig.NetParams, | ||
| CoinSelectionStrategy: walletConfig.CoinSelectionStrategy, |
There was a problem hiding this comment.
Perhaps this is something that we want to tie to a single send/fund session rather than having it be a static value set once the wallet has been initialized? So we'd accept it as an arg in the send/fund calls, using a functional option if we don't want to break some of the APIs across the code base.
There was a problem hiding this comment.
Yeah, I think that makes sense. Though it feels like that would be a different PR, as we'd probably also want to add such new parameters to the existing RPCs that currently use the config-level strategy (FundPsbt, EstimateFee, SendCoins, SendOutputs).
There was a problem hiding this comment.
SGTM, I think can make a tracking issue for the feature cc @saubyk.
input/size.go
Outdated
| // AddOutputWithScriptLength estimates the weight of an output based on the | ||
| // pkScript length. | ||
| func (twe *TxWeightEstimator) AddOutputWithScriptLength( | ||
| scriptLen int) *TxWeightEstimator { |
There was a problem hiding this comment.
In what instance would we be using a script that isn't already one of the standard pkScript templates?
There was a problem hiding this comment.
This probably won't make it into the final PR. This is needed for adding the change output weight. And because we want to avoid deriving a change address before we actually need one, I just pass around the expected change script size. But I'll probably replace that with an actual change address type and then use that to estimate the weight.
lnrpc/walletrpc/walletkit_server.go
Outdated
| changeOutput := packet.UnsignedTx.TxOut[changeIndex] | ||
| changeScriptLen = len(changeOutput.PkScript) | ||
|
|
||
| case changeType == ChangeAddressType_CHANGE_ADDRESS_TYPE_P2TR: |
There was a problem hiding this comment.
Will P2TR be the default change type here?
There was a problem hiding this comment.
I would say no. Because the default for the normal FundPsbt is P2WKH (which was kept to not break backward compatibility with earlier RPC users, see more context in discussion here: #7348).
And since the change type is on the parent message, I don't think that using the new third funding option should introduce a different behavior of a top-level parameter.
But since we can specify the change type to be P2TR, I don't think the default is a big deal.
guggero
force-pushed
the
fundpsbt-coin-select
branch
2 times, most recently
from
b661a49 to
11f3fde
Compare
|
Note Reviews PausedUse the following commands to manage reviews:
WalkthroughThe Lightning Network wallet's configuration has been expanded with a new Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
guggero
force-pushed
the
fundpsbt-coin-select
branch
from
11f3fde to
3b72edc
Compare
|
Fixed up a bunch of things and added unit and integration tests. |
|
@coderabbitai review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Configuration used: CodeRabbit UI
Commits
Files that changed from the base of the PR and between 4315e80 and 3b72edcfa5338b846e2c6f5871098b6f68601965.Files ignored due to path filters (4)
go.modis excluded by:!**/*.modgo.sumis excluded by:!**/*.sumlnrpc/walletrpc/walletkit.pb.gois excluded by:!**/*.pb.golnrpc/walletrpc/walletkit.swagger.jsonis excluded by:!**/*.json
Files selected for processing (25)
- config_builder.go (2 hunks)
- docs/release-notes/release-notes-0.18.0.md (2 hunks)
- funding/manager_test.go (2 hunks)
- input/size.go (4 hunks)
- itest/list_on_test.go (1 hunks)
- itest/lnd_psbt_test.go (5 hunks)
- lnrpc/walletrpc/config_active.go (2 hunks)
- lnrpc/walletrpc/psbt.go (2 hunks)
- lnrpc/walletrpc/walletkit.proto (4 hunks)
- lnrpc/walletrpc/walletkit_server.go (8 hunks)
- lnrpc/walletrpc/walletkit_server_test.go (2 hunks)
- lntest/mock/walletcontroller.go (1 hunks)
- lnwallet/btcwallet/btcwallet.go (1 hunks)
- lnwallet/btcwallet/psbt.go (4 hunks)
- lnwallet/btcwallet/psbt_test.go (2 hunks)
- lnwallet/chanfunding/assembler.go (2 hunks)
- lnwallet/chanfunding/coin_select.go (9 hunks)
- lnwallet/chanfunding/coin_select_test.go (30 hunks)
- lnwallet/chanfunding/wallet_assembler.go (10 hunks)
- lnwallet/config.go (2 hunks)
- lnwallet/interface.go (2 hunks)
- lnwallet/mock.go (1 hunks)
- lnwallet/test/test_interface.go (3 hunks)
- lnwallet/wallet.go (5 hunks)
- subrpcserver_config.go (1 hunks)
Files skipped from review due to trivial changes (1)
- lnwallet/btcwallet/psbt_test.go
Additional comments: 89
lnrpc/walletrpc/psbt.go (1)
- 51-60: The
lockInputsfunction has been refactored to accept a slice ofwire.OutPointinstead of apsbt.Packet. This change simplifies the function's interface by directly working with the necessary data structure. The logic within the function has been updated accordingly to iterate over theoutpointsslice and lock each output using theLeaseOutputmethod from theWalletControllerinterface. The error handling within the loop correctly attempts to release any locks that were acquired before an error occurred, ensuring that resources are not left in an inconsistent state.lnwallet/config.go (1)
- 66-68: The
Configstruct has been extended with a new fieldCoinSelectionStrategyof typewallet.CoinSelectionStrategy. This addition is part of the broader changes to the coin selection process, allowing for a configurable strategy to be used when funding transactions. The field is well-named and follows the existing coding conventions within the file.lnrpc/walletrpc/config_active.go (1)
- 76-78: Similar to the previous file, the
Configstruct inconfig_active.gohas been updated to include theCoinSelectionStrategyfield. This change ensures that the new coin selection strategy is configurable within thewalletrpcpackage as well, maintaining consistency across the codebase.lntest/mock/walletcontroller.go (2)
- 230-233: The
DecorateInputsmethod has been added to theWalletControllermock with a placeholder implementation that currently does nothing. This method is likely intended to be used in future tests that require input decoration. It's important to ensure that the method is implemented correctly when the functionality is added.- 234-234: The
PublishTransactionmethod has been modified to send the transaction to thePublishedTransactionschannel. This change allows for testing of transaction publication logic by observing the channel, which is a common pattern in asynchronous testing.lnwallet/chanfunding/assembler.go (2)
- 15-15: The
ListCoinsmethod of theCoinSourceinterface has been updated to return a slice ofwallet.Coininstead of the previousCointype. This change aligns the coin source with the updated types from thebtcwalletpackage, ensuring compatibility.- 20-20: The
CoinFromOutPointmethod now returns a pointer towallet.Coin, which is consistent with the changes made to theListCoinsmethod. This ensures that the interface's methods are using the same updated types.lnwallet/mock.go (1)
- 238-241: The
DecorateInputsmethod has been added to themockWalletControllerwith a placeholder implementation that currently does nothing. This method is likely intended to be used in future tests that require input decoration. It's important to ensure that the method is implemented correctly when the functionality is added.itest/list_on_test.go (1)
- 280-283: A new test case named "fund psbt" has been added to the
allTestCaseslist with the associated test functiontestFundPsbt. This addition is part of expanding the integration test coverage to include the new PSBT funding functionality.docs/release-notes/release-notes-0.18.0.md (2)
- 104-106: The description of the
coin-selection-strategyconfig option is clear and concise, explaining its new applicability to channel funding operations and thePsbtCoinSelectoption of theFundPsbtRPC.- 124-131: The explanation of the new template option for the
FundPsbtRPC method is well-documented, detailing the ability to perform coin selection with pre-defined inputs and the flexibility in handling change outputs.subrpcserver_config.go (1)
- 200-204: Using reflection to set the
CoinSelectionStrategyfield in thesubRPCServerConfigsmethod is a valid approach, but it's important to ensure that thecc.Wallet.Cfg.CoinSelectionStrategyfield is properly initialized before this method is called to avoid any potentialnilpointer dereferences.lnwallet/chanfunding/coin_select.go (4)
- 41-60: The introduction of the
ChangeAddressTypeenum-like type is a good practice for code clarity and maintainability. It clearly defines the possible types of change addresses that can be generated.- 68-78: The
selectInputsfunction has been updated to use thewallet.Cointype and a coin selection strategy. Ensure that all callers of this function are updated to pass the correct strategy and handle the new return type.Verification successful
The search results from
ripgrepconfirm that theselectInputsfunction is only called within thelnwallet/chanfunding/coin_select.gofile. There are no other calls to this function in the codebase, which means that all known calls toselectInputshave been updated to use the new signature with the correct parameters.Based on this information, the review comment has been verified, and all callers of the
selectInputsfunction within the codebase are using the updated function signature.* 114-139: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [94-136]Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that all callers of selectInputs are updated to use the new signature. ast-grep --lang go --pattern $'selectInputs($_, $_, $_, $_)'Length of output: 488
Script:
#!/bin/bash # Search for calls to selectInputs across the entire codebase to ensure all instances are updated. ast-grep --lang go --pattern $'selectInputs($_, $_, $_, $_)'Length of output: 488
Script:
#!/bin/bash # Use ripgrep to search for the string pattern of selectInputs function calls across the entire codebase. rg 'selectInputs\('Length of output: 283
The
calculateFeesfunction now takes an additionalChangeAddressTypeparameter and returns an extra value. This change increases the complexity of the function, and it's crucial to ensure that all callers are updated to handle the new return values correctly.Verification successful
Based on the script output, all callers of the
calculateFeesfunction have been updated to handle the new return values correctly. The function is consistently called with four parameters, and the three return values are being handled in each case.* 164-207: The `CoinSelect` function has been significantly modified to include additional parameters and a new return value. This change affects the function's contract and may impact all areas of the code where this function is used. It's important to verify that all such instances are updated accordingly.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that all callers of calculateFees are updated to handle the new return values. ast-grep --lang go --pattern $'calculateFees($_, $_, $_, $_)'Length of output: 893
Verification successful
The verification process has confirmed that the
CoinSelectfunction calls found in the codebase match the updated function signature with the correct number of parameters. The instances inwallet_assembler.go,coin_select_test.go, andcoin_select.goare consistent with the new signature.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that all instances of CoinSelect are updated to match the new function signature. ast-grep --lang go --pattern $'CoinSelect($_, $_, $_, $_, $_, $_, $_)'Length of output: 1179
Script:
#!/bin/bash # Verify that the instances of CoinSelect found are the only ones in the codebase. # This will ensure that there are no other calls to CoinSelect that have not been updated. rg "CoinSelect" --countLength of output: 895
Script:
#!/bin/bash # Check each file where "CoinSelect" appears to confirm that the function calls match the new signature. # We will use ast-grep to match the pattern of the function call with the correct number of parameters. # List of files to check, as per the previous output files_to_check=( "subrpcserver_config.go" "config_builder.go" "sweep/interface.go" "sweep/tx_input_set.go" "sweep/walletsweep_test.go" "sweep/walletsweep.go" "sweep/sweeper.go" "sweep/backend_mock_test.go" "rpcserver.go" "lnwallet/config.go" "lnwallet/wallet.go" "lnwallet/test/test_interface.go" "lnwallet/chanfunding/assembler.go" "lnwallet/chanfunding/coin_select_test.go" "lnwallet/chanfunding/coin_select.go" "lnwallet/chanfunding/wallet_assembler.go" "lnwallet/btcwallet/btcwallet.go" "lnwallet/btcwallet/config.go" "lnwallet/btcwallet/psbt.go" "lnrpc/walletrpc/config_active.go" "lnrpc/walletrpc/walletkit_server.go" "lnrpc/walletrpc/walletkit.proto" "lnrpc/walletrpc/walletkit.swagger.json" "lnrpc/walletrpc/walletkit.pb.go" "lnrpc/walletrpc/walletkit_server_test.go" "itest/lnd_psbt_test.go" "funding/manager_test.go" "docs/release-notes/release-notes-0.18.0.md" "config.go" ) # Generate the ast-grep command to check the function call pattern in each file for file in "${files_to_check[@]}"; do echo "Checking $file for CoinSelect function calls..." ast-grep --lang go --pattern $'CoinSelect($_, $_, $_, $_, $_, $_, $_)' --file "$file" doneLength of output: 14999
Script:
#!/bin/bash # Search for the updated CoinSelect function calls across the entire codebase. ast-grep --lang go --pattern $'CoinSelect($_, $_, $_, $_, $_, $_, $_)'Length of output: 1179
lnwallet/chanfunding/wallet_assembler.go (8)
- 12-12: The import of the
walletpackage is correct and necessary due to the usage ofwallet.Coinandwallet.CoinSelectionStrategyin the file.- 34-34: The type of
InputCoinshas been changed from[]Cointo[]wallet.Coin. This change is consistent with the PR's objective to replace theCointype withwallet.Coin.- 211-213: The addition of the
CoinSelectionStrategyfield to theWalletConfigstruct aligns with the PR's objective to introduce a coin selection strategy for the wallet. This field is necessary for the new functionality.- 271-276: The variables
allCoinsandmanuallySelectedCoinshave been changed to use thewallet.Cointype. This change is consistent with the rest of the file and the PR's objectives.- 405-406: The usage of
w.cfg.CoinSelectionStrategywithin theCoinSelectUpToAmountfunction call is correct and reflects the new coin selection strategy feature being implemented.- 440-441: The usage of
w.cfg.CoinSelectionStrategywithin theCoinSelectSubtractFeesfunction call is correct and reflects the new coin selection strategy feature being implemented.- 454-455: The usage of
w.cfg.CoinSelectionStrategywithin theCoinSelectfunction call is correct and reflects the new coin selection strategy feature being implemented.- 528-529: The function
outpointsToCoinsnow returns[]wallet.Coininstead of[]Coin, which is consistent with the changes made throughout the file.lnrpc/walletrpc/walletkit_server_test.go (3)
- 7-24: The addition of multiple imports is necessary for the new test cases being added to the file. These imports provide the required dependencies for the test functions.
- 67-82: The
mockCoinSelectionLockerstruct and its methodWithCoinSelectLockare correctly defined and used for testing purposes. The method's logic is simple and correctly returns an error if thefailflag is set.- 84-631: The test function
TestFundPsbtCoinSelectis comprehensive and covers various scenarios for the coin selection process in PSBT funding. The test cases are well-structured, and the use of helper functions likemakePacket,updatePacket, andcalcFeeimproves readability and maintainability. The assertions are correctly using therequirepackage to ensure that the conditions are met.lnwallet/btcwallet/psbt.go (6)
- 35-49: Error variables are correctly defined and follow Go conventions.
- 47-47: The
FundPsbtfunction's documentation is clear and explains the behavior well. It handles different account types and sets the key scope accordingly.- 47-47: The
SignPsbtfunction checks for the readiness of inputs to sign and attempts to sign all unsigned inputs. It handles different signing methods and tweaks private keys if necessary. The function's logic is sound, but ensure that thevalidateSigningMethodandmaybeTweakPrivKeyPsbtfunctions are thoroughly tested due to their complexity.- 372-435: The
EstimateInputWeightfunction correctly estimates the weight of various PSBT inputs and adds them to the transaction weight estimator. It handles different script types and returns appropriate errors for unsupported or unknown types.- 609-617: The
DecorateInputsfunction is a simple pass-through to the underlying wallet'sDecorateInputsmethod. It's straightforward and does not introduce additional logic.- 618-618: The
lookupFirstCustomAccountfunction handles the retrieval of custom account properties. It iterates over default key scopes and returns the first match. Ensure that error handling is robust and that the function behaves correctly when multiple custom accounts exist.lnwallet/chanfunding/coin_select_test.go (7)
- 10-10: The addition of the
github.com/btcsuite/btcwallet/walletimport is necessary for the updated tests to use thewallet.Cointype.- 29-29: The
defaultChanFundingChangeTypeglobal variable is set toP2TRChangeAddress. Ensure this default aligns with the intended change address strategy for the tests.Verification successful
The
defaultChanFundingChangeTypeglobal variable being set toP2TRChangeAddressis consistent with the project's change address strategy, as evidenced by its usage across the codebase, including in wallet functions and tests. This suggests that the default is aligned with the intended change address strategy for the tests.* 67-67: The `TestCalculateFees` function has been updated to use `wallet.Coin` instead of `Coin`. This change is consistent with the updated import and usage of the `wallet` package. * 153-153: The `TestCoinSelect` function has been updated to use `wallet.Coin` and the `defaultChanFundingChangeType` variable. The changes are consistent and necessary for the updated logic. * 351-351: The `TestCalculateChangeAmount` function's changes are consistent with the new logic for calculating change amounts. The tests appear to cover various scenarios adequately. * 466-466: The `TestCoinSelectSubtractFees` function has been updated to use `wallet.Coin` and the `defaultChanFundingChangeType` variable. The changes are consistent and necessary for the updated logic. * 700-700: The `TestCoinSelectUpToAmount` function has been updated to use `wallet.Coin` and the `defaultChanFundingChangeType` variable. The changes are consistent and necessary for the updated logic.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the defaultChanFundingChangeType is consistent with the project's change address strategy. grep -R "defaultChanFundingChangeType" .Length of output: 808
Script:
#!/bin/bash # Search for instances where change address types are defined or used throughout the codebase to determine if P2TRChangeAddress is a standard default. rg "ChangeAddress" --context 5Length of output: 51897
lnwallet/interface.go (2)
- 100-101: The addition of the
Indexfield to theAddressPropertystruct is clear and follows the existing structure of the code.- 507-511: The addition of the
DecorateInputsmethod to theWalletControllerinterface is well-documented and the method signature is consistent with the existing interface design.input/size.go (7)
- 52-55: The introduction of the
BaseOutputSizeconstant is a good refactor, reducing magic numbers and improving maintainability.- 64-64: Refactoring
P2PKHOutputSizeto useBaseOutputSizeis correct and improves code readability.- 70-70: Refactoring
P2WKHOutputSizeto useBaseOutputSizeis correct and improves code readability.- 76-76: Refactoring
P2WSHOutputSizeto useBaseOutputSizeis correct and improves code readability.- 85-85: Refactoring
P2SHOutputSizeto useBaseOutputSizeis correct and improves code readability.- 97-97: Refactoring
P2TROutputSizeto useBaseOutputSizeis correct and improves code readability.- 1021-1027: The new
AddOutputmethod correctly estimates the weight of an output based on the providedpkScript. This method enhances modularity by allowing the addition of arbitrary output types to the weight estimator.lnrpc/walletrpc/walletkit.proto (4)
- 510-512: The addition of the
indexfield to theAddressPropertymessage is consistent with the need to track the derivation index of addresses. This is a useful addition for clients that need to correlate addresses with their index in the HD wallet structure.- 1152-1171: The
FundPsbtRequestmessage has been extended with a newPsbtCoinSelectmessage type for thecoin_selectfield. This change allows for more flexible coin selection, even when some inputs are pre-defined. The documentation comments are clear and provide good context for the change.- 1243-1273: The
PsbtCoinSelectmessage has been added, which includes apsbtfield and achange_outputoneof that allows specifying an existing output index or adding a new change output. This aligns with the PR's objective to enhance coin selection capabilities. The comments are detailed and explain the purpose and usage well.- 1219-1220: The comment for
locked_utxosin theFundPsbtResponsemessage correctly explains the behavior of locking new inputs added by theFundPsbtRPC. It's important for clients to understand which inputs will be locked as a result of the operation.config_builder.go (2)
- 689-698: The
lnwallet.Configstruct has been updated to include theCoinSelectionStrategyfield. This change is consistent with the PR objectives and allows for specifying the strategy for coin selection. Ensure that all parts of the codebase that instantiatelnwallet.Configare updated to handle this new field.- 805-814: Similarly, the
lnwallet.Configstruct within theRPCSignerWalletImplhas been updated to include theCoinSelectionStrategyfield. This is also consistent with the PR objectives and ensures that the remote signing wallet implementation can specify the coin selection strategy.itest/lnd_psbt_test.go (10)
- 13-22: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [5-19]
Imports
encoding/hex,fmt,chainhash, andspewhave been added. Ensure these are used within the file and are not superfluous.
- 1065-1169: The new test function
testFundPsbthas been added to test the FundPsbt RPC use cases. Ensure that the test covers all necessary scenarios and that assertions are correctly checking the expected outcomes.- 1171-1181: The helper function
addressToPkScripthas been added. Verify that it correctly converts an address to a pkScript and that error handling is appropriate.- 1183-1235: The helper function
getAddressBip32Derivationhas been added. Verify that it correctly retrieves BIP-0032 derivation info for an address and that error handling is appropriate.- 1237-1267: The helper function
getAddressDerivationPathhas been added. Verify that it correctly retrieves the derivation path for an address and that error handling is appropriate.- 1269-1306: The helper function
fundPsbtCoinSelecthas been added. Verify that it correctly calls the FundPsbt RPC using the coin selection with template PSBT mode and that error handling is appropriate.- 1308-1327: The helper function
signPackethas been added. Verify that it correctly calls the SignPsbt RPC and that error handling is appropriate.- 1329-1354: The helper function
extractPublishAndMinehas been added. Verify that it correctly finalizes a PSBT, extracts the final transaction, publishes it, mines a block, and asserts the transaction was mined successfully.- 1537-1550: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [1426-1542]
The function
assertPsbtSpendhas been modified. Verify that the changes correctly create an output, attempt to create a sweep transaction, and sign it using the SignPsbt RPC.
- 1540-1542: The function
assertPsbtFundSignSpendhas been modified. Verify that the changes correctly fund a PSBT from the internal wallet and attempt to sign it using the SignPsbt or FinalizePsbt method.lnwallet/btcwallet/btcwallet.go (1)
- 823-828: The changes to the
ListAddressesfunction include the addition of derivation path index retrieval from the public key and assignment to theIndexfield of theAddressPropertystruct. This is a logical extension of theAddressPropertyto include more detailed information about each address, which can be useful for certain applications. Ensure that all calls toListAddressesare aware of the new return type[]lnwallet.AddressPropertyinstead of[]lnwallet.Address.lnrpc/walletrpc/walletkit_server.go (12)
- 41-41: The import of the
chanfundingpackage is new and is expected as per the PR's objectives to enhance the coin selection process.- 1206-1211: The handling of the account name for the PSBT funding is correct, defaulting to the
lnwallet.DefaultAccountNameif no specific account is provided.- 1162-1309: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [1213-1368]
The logic for handling the different types of PSBT templates (PSBT directly, PSBT with coin selection, or raw TX template) is well-structured and covers all cases. Each case is handled separately with appropriate error handling.
- 1406-1444: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [1370-1454]
The
fundPsbtInternalWalletfunction is correctly implemented, locking the UTXOs after funding the PSBT and handling errors appropriately. The use ofWithCoinSelectLockensures that coin selection and UTXO locking are done atomically, which is good for consistency.
- 1460-1671: The
fundPsbtCoinSelectfunction correctly implements the new coin selection logic, including the handling of change outputs. The use ofWithCoinSelectLockis again appropriate here.- 1673-1712: The
handleChangefunction is well-implemented, correctly handling the addition of change to an existing output or creating a new change output as needed.- 1747-1759: The
addrTypeFromChangeAddressTypefunction correctly maps thechanfunding.ChangeAddressTypeto thelnwallet.AddressType. This is a utility function that will be used in multiple places, and it's implemented correctly.- 1500-1513: The switch statement for setting the
changeDustLimitis correct and accounts for different change address types. It uses the appropriate dust limit calculation for each type.- 1515-1573: The logic to determine if the existing inputs are sufficient to cover the transaction and potentially create a change output is correct. The function handles the case where no additional inputs are needed and when a change output is required.
- 1575-1658: The coin selection process within the
WithCoinSelectLockfunction is correctly implemented. It selects coins, handles change, and locks the inputs. The error handling and serialization of the PSBT are also done correctly.- 1663-1670: The conversion of lock leases to the RPC format in
marshallLeasesis implemented correctly, ensuring that the data is presented in a format that can be used by RPC clients.- 1747-1759: The
addrTypeFromChangeAddressTypefunction is correctly implemented, mapping the internal change address type to the wallet's address type.lnwallet/wallet.go (6)
- 23-23: The import of the
github.com/btcsuite/btcwallet/walletpackage is added. This is necessary for the new functionality related to coin selection and PSBTs.- 856-859: The
DustLimitfield in theInitFundingReserveMsgstruct is updated to useDustLimitForSize(input.P2WSHSize). This change ensures that the dust limit is calculated based on the size of a P2WSH output, which is larger than a P2WPKH output. This is a logical change given that P2WSH outputs are used for multi-sig scripts, which are typically larger than single-sig P2WPKH scripts.- 2542-2544: The conversion from
[]*Utxoto[]wallet.Coinin theListCoinsmethod is correct. Thewallet.Coinstruct is being properly populated with theTxOutandOutPointfrom eachUtxo.- 2559-2565: The
CoinFromOutPointmethod in theCoinSourcestruct correctly fetches the UTXO information for the given outpoint and returns awallet.Coininstance with theTxOutandOutPointfields populated.- 2542-2544: The conversion from
[]*Utxoto[]wallet.Coinin theListCoinsmethod is correct. Thewallet.Coinstruct is being properly populated with theTxOutandOutPointfrom eachUtxo.- 2559-2565: The
CoinFromOutPointmethod in theCoinSourcestruct correctly fetches the UTXO information for the given outpoint and returns awallet.Coininstance with theTxOutandOutPointfields populated.lnwallet/test/test_interface.go (3)
- 29-29: The import of the
walletpackage is added to support the newCoinSelectionStrategyfield.- 366-367: The
CoinSelectionStrategyfield is set towallet.CoinSelectionLargest. Ensure that this strategy is consistently used across all relevant configurations and that thewalletpackage defines this strategy as expected.- 2971-2978: The
CoinSelectionStrategyfield is added to theWalletConfigstruct passed toNewWalletAssembler. Ensure that theCoinSelectionStrategyis properly handled within theNewWalletAssemblerfunction and that the strategy is applied correctly during the coin selection process.funding/manager_test.go (2)
- 23-23: The import
github.com/btcsuite/btcwallet/wallethas been added.- 373-373: Added
CoinSelectionStrategyfield tolnwallet.Configwith the valuewallet.CoinSelectionLargest. Ensure that this strategy is compatible with the rest of the wallet's configuration and that all necessary updates to handle this strategy are in place.
guggero
force-pushed
the
fundpsbt-coin-select
branch
from
3b72edc to
8565b8d
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Commits
Files that changed from the base of the PR and between ba4021c and 8565b8df5944432731e36b8d966b5f313febe3da.Files ignored due to path filters (4)
go.modis excluded by:!**/*.modgo.sumis excluded by:!**/*.sumlnrpc/walletrpc/walletkit.pb.gois excluded by:!**/*.pb.golnrpc/walletrpc/walletkit.swagger.jsonis excluded by:!**/*.json
Files selected for processing (25)
- config_builder.go (2 hunks)
- docs/release-notes/release-notes-0.18.0.md (2 hunks)
- funding/manager_test.go (2 hunks)
- input/size.go (4 hunks)
- itest/list_on_test.go (1 hunks)
- itest/lnd_psbt_test.go (6 hunks)
- lnrpc/walletrpc/config_active.go (2 hunks)
- lnrpc/walletrpc/psbt.go (2 hunks)
- lnrpc/walletrpc/walletkit.proto (4 hunks)
- lnrpc/walletrpc/walletkit_server.go (8 hunks)
- lnrpc/walletrpc/walletkit_server_test.go (2 hunks)
- lntest/mock/walletcontroller.go (1 hunks)
- lnwallet/btcwallet/btcwallet.go (1 hunks)
- lnwallet/btcwallet/psbt.go (4 hunks)
- lnwallet/btcwallet/psbt_test.go (2 hunks)
- lnwallet/chanfunding/assembler.go (2 hunks)
- lnwallet/chanfunding/coin_select.go (9 hunks)
- lnwallet/chanfunding/coin_select_test.go (30 hunks)
- lnwallet/chanfunding/wallet_assembler.go (10 hunks)
- lnwallet/config.go (2 hunks)
- lnwallet/interface.go (2 hunks)
- lnwallet/mock.go (1 hunks)
- lnwallet/test/test_interface.go (3 hunks)
- lnwallet/wallet.go (5 hunks)
- subrpcserver_config.go (1 hunks)
Files skipped from review as they are similar to previous changes (22)
- config_builder.go
- docs/release-notes/release-notes-0.18.0.md
- funding/manager_test.go
- input/size.go
- itest/list_on_test.go
- itest/lnd_psbt_test.go
- lnrpc/walletrpc/config_active.go
- lnrpc/walletrpc/psbt.go
- lnrpc/walletrpc/walletkit_server.go
- lntest/mock/walletcontroller.go
- lnwallet/btcwallet/btcwallet.go
- lnwallet/btcwallet/psbt.go
- lnwallet/btcwallet/psbt_test.go
- lnwallet/chanfunding/assembler.go
- lnwallet/chanfunding/coin_select_test.go
- lnwallet/chanfunding/wallet_assembler.go
- lnwallet/config.go
- lnwallet/interface.go
- lnwallet/mock.go
- lnwallet/test/test_interface.go
- lnwallet/wallet.go
- subrpcserver_config.go
Additional comments: 14
lnwallet/chanfunding/coin_select.go (8)
- 36-36: The error message for
errUnsupportedInputis clear and provides the necessary information to identify the unsupported script type.- 41-61: The
ChangeAddressTypeenum-like type is well-defined and the comments explain the purpose of each value clearly.- 68-84: The
selectInputsfunction has been updated to use thewallet.Cointype and a new coin selection strategy parameter. Ensure that all calls to this function have been updated to pass the correct arguments.Verification successful
The verification process has confirmed that the
selectInputsfunction is being called with four arguments in two different places within thecoin_select.gofile. The argumentsamtNeededoramt,coins,strategy, andfeeRateare passed in the correct order, matching the updated function signature. This suggests that the calls toselectInputshave been correctly updated to pass the new arguments as required by the modified function definition.* 114-139: > 📝 **NOTE** > This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [94-136]Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for calls to selectInputs to ensure they pass the correct arguments. ast-grep --lang go --pattern $'selectInputs($_, $_, $_, $_)'Length of output: 488
Script:
#!/bin/bash # Verify the types and order of the arguments passed to selectInputs in the provided calls. ast-grep --lang go --pattern $'selectInputs($amtNeeded, $coins, $strategy, $feeRate)' ast-grep --lang go --pattern $'selectInputs($amt, $coins, $strategy, $feeRate)'Length of output: 167
Script:
#!/bin/bash # Attempt a more general pattern to verify the arguments passed to selectInputs. ast-grep --lang go --pattern $'selectInputs($$$)'Length of output: 477
The
calculateFeesfunction now takes an additionalChangeAddressTypeparameter and returns two fee estimates. Ensure that the logic for fee calculation with and without a change output is correct and that the new parameter is used appropriately.Verification successful
The
calculateFeesfunction correctly takes an additionalChangeAddressTypeparameter and returns two fee estimates. The logic for fee calculation with and without a change output is implemented as intended, using theChangeAddressTypeparameter appropriately.* 164-207: The `CoinSelect` function has been significantly updated. It now includes a loop for multiple rounds of coin selection if necessary, and it uses the new `ChangeAddressType`. Ensure that the logic for selecting coins, calculating fees, and determining the need for additional rounds of coin selection is correct.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that calculateFees is called with the correct ChangeAddressType argument. ast-grep --lang go --pattern $'calculateFees($_, $_, $_, $_)'Length of output: 893
Script:
#!/bin/bash # View the implementation of the calculateFees function to verify the logic. rg "func calculateFees" lnwallet/chanfunding/coin_select.goLength of output: 133
Script:
#!/bin/bash # Extract the full implementation of the calculateFees function to verify the logic. rg -A 20 "func calculateFees" lnwallet/chanfunding/coin_select.goLength of output: 704
Script:
#!/bin/bash # Retrieve more lines following the function signature to see the complete logic of the calculateFees function. rg -A 40 "func calculateFees" lnwallet/chanfunding/coin_select.goLength of output: 1235
Verification successful
The
CoinSelectfunction's implementation incoin_select.goaligns with the PR objectives and the review comment. It includes a loop for multiple rounds of coin selection, correctly handles the calculation of fees, and uses the newChangeAddressType. The logic for selecting coins, calculating fees, and determining the need for additional rounds of coin selection appears to be correct.* 209-274: The `CalculateChangeAmount` function has been updated to handle cases where an existing change output is used. Ensure that the logic for calculating the change amount and determining if more coins need to be selected is correct, especially when dealing with dust limits. * 161-302: > 📝 **NOTE** > This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [281-384]Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that CoinSelect is called with the correct arguments and that its logic is sound. ast-grep --lang go --pattern $'CoinSelect($_, $_, $_, $_, $_, $_, $_)'Length of output: 1179
Script:
#!/bin/bash # Inspect the CoinSelect function implementation in coin_select.go rg "func CoinSelect" lnwallet/chanfunding/coin_select.go -A 30Length of output: 3147
The
CoinSelectSubtractFeesfunction is a new addition. It attempts to select coins such that the total spent after fees is up to the specified amount. Ensure that the logic for coin selection, fee calculation, and output amount determination is correct.Verification successful
The
CoinSelectSubtractFeesfunction has been verified, and its implementation aligns with the objectives of enhancing the coin selection process. It includes logic for initial coin selection, fee estimation, output amount determination, and sanity checks for fees, which are consistent with the expected behavior for spending up to a specified amount after fees. The function also appropriately considers the dust limit and includes a decision-making process for adding a change output.* 344-354: > 📝 **NOTE** > This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [347-422]Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that CoinSelectSubtractFees is called with the correct arguments and that its logic is sound. ast-grep --lang go --pattern $'CoinSelectSubtractFees($_, $_, $_, $_, $_, $_, $_)'Length of output: 1311
Script:
#!/bin/bash # Inspect the implementation of the CoinSelectSubtractFees function to verify its logic. ast-grep --lang go --pattern $'func CoinSelectSubtractFees($_, $_, $_, $_, $_, $_, $_) { $$$ }'Length of output: 96
Script:
#!/bin/bash # Attempt to find the definition of the CoinSelectSubtractFees function using a broader pattern. ast-grep --lang go --pattern $'func CoinSelectSubtractFees($$$)'Length of output: 4618
The
CoinSelectUpToAmountfunction attempts to select coins up to a maximum amount, considering fees and optional reserve. Ensure that the logic for coin selection, reserve consideration, and fee sanity checks is correct.lnrpc/walletrpc/walletkit_server_test.go (3)
- 7-24: The imports have been updated to include additional packages required for the tests. Ensure that these new imports are used appropriately in the test cases.
Verification successful
The provided script output shows that the new imports in the
walletkit_server_test.gofile are indeed used within the test cases. The context includes usage of:
bytesfor buffer operations.fmtfor formatting error messages.testingfor running tests.btcecfor operations related to private keys.schnorrfor Schnorr signature-related operations.btcutilfor Bitcoin utility functions.psbtfor Partially Signed Bitcoin Transaction operations.chainhashfor hash operations.txscriptfor Bitcoin script operations.wirefor Bitcoin wire protocol structures.walletfrombtcwalletfor wallet-related operations.input,mock,lnwallet,chainfee, andchanfundingfromlightningnetwork/lndfor various Lightning Network-related operations.requirefromtestifyfor assertions in tests.Each import is utilized in the test functions, indicating that they are necessary for the test cases to function correctly. There are no signs of unused imports or incorrect usage based on the context provided.
* 67-82: The `mockCoinSelectionLocker` struct and its method `WithCoinSelectLock` are used for testing the coin selection lock functionality. Ensure that the mock behaves as expected and that the error condition is tested.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the new imports are used correctly in the test cases. rg --type go --files-with-matches 'bytes|fmt|testing|btcec|schnorr|btcutil|psbt|chainhash|txscript|wire|wallet|input|mock|lnwallet|chainfee|chanfunding|require'Length of output: 33430
Script:
#!/bin/bash # Verify that the new imports are used correctly in the test cases within walletkit_server_test.go. rg --type go --files-with-matches 'bytes|fmt|testing|btcec|schnorr|btcutil|psbt|chainhash|txscript|wire|wallet|input|mock|lnwallet|chainfee|chanfunding|require' lnrpc/walletrpc/walletkit_server_test.goLength of output: 244
Script:
#!/bin/bash # Inspect the occurrences of the new imports within the walletkit_server_test.go file to ensure they are used correctly. rg 'bytes|fmt|testing|btcec|schnorr|btcutil|psbt|chainhash|txscript|wire|wallet|input|mock|lnwallet|chainfee|chanfunding|require' lnrpc/walletrpc/walletkit_server_test.go -C 3Length of output: 16030
Verification successful
The
mockCoinSelectionLockeris used correctly in tests, and its error condition is tested insweep/walletsweep_test.go. Inwalletkit_server_test.go, the mock is part of the test setup, suggesting that the coin selection lock functionality is indeed being tested, although thefailcondition is not explicitly triggered in this file.* 84-632: The test function `TestFundPsbtCoinSelect` has been updated with new test cases. Ensure that the test cases cover the expected behavior of the `FundPsbt` function, including error conditions and successful coin selection with various types of change addresses.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the mockCoinSelectionLocker is used correctly and that its error condition is tested. ast-grep --lang go --pattern $'mockCoinSelectionLocker{$$$}'Length of output: 482
Script:
#!/bin/bash # Examine the test in sweep/walletsweep_test.go where mockCoinSelectionLocker.fail is set to true rg "coinSelectLocker := &mockCoinSelectionLocker{[\s\S]*?fail: true" sweep/walletsweep_test.go -C 10Length of output: 293
Script:
#!/bin/bash # Search for the test case in sweep/walletsweep_test.go where mockCoinSelectionLocker.fail is set to true rg "fail: true" sweep/walletsweep_test.go -C 10Length of output: 1571
Script:
#!/bin/bash # Examine the test cases in walletkit_server_test.go where mockCoinSelectionLocker is used rg "mockCoinSelectionLocker" lnrpc/walletrpc/walletkit_server_test.go -C 10Length of output: 1078
lnrpc/walletrpc/walletkit.proto (3)
- 510-512: The addition of the
indexfield to theAddressPropertymessage is consistent with the protobuf syntax and adds useful information about the derivation path index of an address. This change should be cross-verified with the implementation to ensure that the index is being correctly set and used.- 1152-1171: The
FundPsbtRequestmessage has been updated with a newPsbtCoinSelectmessage type for thecoin_selectfield. This change allows for more flexible coin selection when funding a PSBT, even when some inputs are predefined. Ensure that the corresponding server-side implementation correctly handles the newcoin_selectfield and that it integrates properly with the existing coin selection logic.Verification successful
The search with ripgrep has successfully identified the existence of the
FundPsbtmethod in various files. Specifically, it is present inlnwallet/mock.go,lnwallet/btcwallet/psbt.go,lntest/mock/walletcontroller.go,lntest/rpc/wallet_kit.go,lnrpc/walletrpc/walletkit_grpc.pb.go, andlnrpc/walletrpc/walletkit_server.go.The
FundPsbtmethod inlnrpc/walletrpc/walletkit_server.gois of particular interest because it is likely to be the server-side implementation that corresponds to thewalletkit.protochanges. The method signature inwalletkit_server.godoes not match the previous patterns used in the ast-grep searches, which explains why those searches did not yield results.To verify that the
FundPsbtmethod inwalletkit_server.gocorrectly handles the newPsbtCoinSelectmessage, I will now generate a script to examine the contents of this method inwalletkit_server.go.
The output from the
rgcommand provides a snippet of theFundPsbtmethod implementation inwalletkit_server.go. However, the snippet does not include the handling of thePsbtCoinSelectmessage. The provided lines focus on determining the transaction fee based on the target number of blocks to confirmation or the specified sat/vByte.To verify the handling of the
PsbtCoinSelectmessage, we need to see more of the method's implementation. I will generate a script to retrieve additional lines following the ones we have already seen.
The additional lines provided from the
FundPsbtmethod inwalletkit_server.gocontinue to describe the setup for the transaction fee and the extraction of the minimum number of confirmations for outputs used to fund the transaction. The snippet also shows that the PSBT will be funded by the default account unless otherwise specified.However, the snippet still does not include the handling of the
PsbtCoinSelectmessage. To fully verify the handling of thePsbtCoinSelectmessage, we need to see the part of the method where this message is processed.Given the length of the method, it may be more efficient to retrieve the entire
FundPsbtmethod to ensure we capture the handling of thePsbtCoinSelectmessage. I will generate a script to retrieve the entire method.
The extended output from the
FundPsbtmethod inwalletkit_server.gonow includes the handling of thePsbtCoinSelectmessage. The code snippet shows that the method checks ifreq.GetCoinSelect()is not nil, which indicates that thePsbtCoinSelectmessage is being used. It then proceeds to parse the PSBT from thecoinSelectRequest.Psbtand performs various checks, such as ensuring that outputs are specified and that the output sum is positive.This confirms that the server-side implementation of the
FundPsbtmethod has been updated to handle the newPsbtCoinSelectmessage, as indicated by the PR objectives and the review comment. The method performs additional logic when thecoin_selectfield is provided, including parsing the PSBT and preparing for coin selection even if inputs are already present.Based on the information gathered, the review comment is consistent with the changes in the codebase, and the
FundPsbtmethod correctly handles the newPsbtCoinSelectmessage.* 1243-1273: The `PsbtCoinSelect` message is well-defined and includes a `oneof` for the `change_output`, which allows for specifying an existing output index or adding a new change output. This change should be carefully reviewed in the implementation to ensure that the logic for handling the change output is correctly applied and that any existing output index specified is valid.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the FundPsbt RPC method correctly handles the new PsbtCoinSelect message. ast-grep --lang go --pattern $'func (w WalletKit) FundPsbt($_, $_) { $$$ case $_.coin_select { $$$ } $$$ }'Length of output: 120
Script:
#!/bin/bash # First, let's verify the existence of the FundPsbt method in the Go codebase. ast-grep --lang go --pattern $'func (w WalletKit) FundPsbt($_, $_) { $$$ }'Length of output: 78
Script:
#!/bin/bash # If the FundPsbt method exists, let's check for the handling of the coin_select field without assuming a case statement. ast-grep --lang go --pattern $'func (w WalletKit) FundPsbt($_, $_) { $$$ $_.coin_select { $$$ } }'Length of output: 109
Script:
#!/bin/bash # Broaden the search to find any FundPsbt method in the Go codebase. ast-grep --lang go --pattern $'func $_ FundPsbt($_, $_) { $$$ }'Length of output: 67
Script:
#!/bin/bash # Verify if WalletKit is the correct receiver for the FundPsbt method or if there is a different receiver. ast-grep --lang go --pattern $'func ($_ WalletKit) $_($_, $_) { $$$ }'Length of output: 73
Script:
#!/bin/bash # Use ripgrep to search for the string "FundPsbt" within the Go files to confirm the existence of the method and identify its receiver. rg 'func .+ FundPsbt' --type goLength of output: 841
Script:
#!/bin/bash # Examine the contents of the FundPsbt method in walletkit_server.go to verify handling of the PsbtCoinSelect message. rg 'func \(w \*WalletKit\) FundPsbt' -A 30 lnrpc/walletrpc/walletkit_server.goLength of output: 896
Script:
#!/bin/bash # Retrieve more lines following the initial snippet from the FundPsbt method in walletkit_server.go to verify handling of the PsbtCoinSelect message. rg 'func \(w \*WalletKit\) FundPsbt' -A 50 lnrpc/walletrpc/walletkit_server.goLength of output: 1466
Script:
#!/bin/bash # Retrieve the entire FundPsbt method from walletkit_server.go to verify handling of the PsbtCoinSelect message. rg 'func \(w \*WalletKit\) FundPsbt' -A 100 lnrpc/walletrpc/walletkit_server.goLength of output: 3075
guggero
force-pushed
the
fundpsbt-coin-select
branch
from
8565b8d to
4dfffa7
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Commits
Files that changed from the base of the PR and between ba4021c and 4dfffa7d64676740f3b7a6a8b120219fec8e2e38.Files ignored due to path filters (4)
go.modis excluded by:!**/*.modgo.sumis excluded by:!**/*.sumlnrpc/walletrpc/walletkit.pb.gois excluded by:!**/*.pb.golnrpc/walletrpc/walletkit.swagger.jsonis excluded by:!**/*.json
Files selected for processing (26)
- config_builder.go (2 hunks)
- docs/release-notes/release-notes-0.18.0.md (2 hunks)
- funding/manager_test.go (2 hunks)
- input/size.go (4 hunks)
- itest/list_on_test.go (1 hunks)
- itest/lnd_channel_funding_utxo_selection_test.go (10 hunks)
- itest/lnd_psbt_test.go (6 hunks)
- lnrpc/walletrpc/config_active.go (2 hunks)
- lnrpc/walletrpc/psbt.go (2 hunks)
- lnrpc/walletrpc/walletkit.proto (4 hunks)
- lnrpc/walletrpc/walletkit_server.go (8 hunks)
- lnrpc/walletrpc/walletkit_server_test.go (2 hunks)
- lntest/mock/walletcontroller.go (1 hunks)
- lnwallet/btcwallet/btcwallet.go (1 hunks)
- lnwallet/btcwallet/psbt.go (4 hunks)
- lnwallet/btcwallet/psbt_test.go (2 hunks)
- lnwallet/chanfunding/assembler.go (2 hunks)
- lnwallet/chanfunding/coin_select.go (9 hunks)
- lnwallet/chanfunding/coin_select_test.go (30 hunks)
- lnwallet/chanfunding/wallet_assembler.go (10 hunks)
- lnwallet/config.go (2 hunks)
- lnwallet/interface.go (2 hunks)
- lnwallet/mock.go (1 hunks)
- lnwallet/test/test_interface.go (3 hunks)
- lnwallet/wallet.go (5 hunks)
- subrpcserver_config.go (1 hunks)
Files skipped from review as they are similar to previous changes (23)
- config_builder.go
- docs/release-notes/release-notes-0.18.0.md
- funding/manager_test.go
- input/size.go
- itest/list_on_test.go
- itest/lnd_psbt_test.go
- lnrpc/walletrpc/config_active.go
- lnrpc/walletrpc/psbt.go
- lnrpc/walletrpc/walletkit.proto
- lnrpc/walletrpc/walletkit_server_test.go
- lntest/mock/walletcontroller.go
- lnwallet/btcwallet/btcwallet.go
- lnwallet/btcwallet/psbt.go
- lnwallet/btcwallet/psbt_test.go
- lnwallet/chanfunding/assembler.go
- lnwallet/chanfunding/coin_select_test.go
- lnwallet/chanfunding/wallet_assembler.go
- lnwallet/config.go
- lnwallet/interface.go
- lnwallet/mock.go
- lnwallet/test/test_interface.go
- lnwallet/wallet.go
- subrpcserver_config.go
Additional comments: 18
itest/lnd_channel_funding_utxo_selection_test.go (8)
- 14-14: The addition of the
requirepackage fromgithub.com/stretchr/testifyis appropriate for assertions in tests.- 135-135: The calculation of
remainingWalletBalanceshould be verified to ensure that the subtraction offundingFee(2, true)is correct and does not result in an underflow or incorrect balance calculation.Verification successful
The verification process has confirmed that the
fundingFeefunction is correctly implemented and used within theremainingWalletBalancecalculation in theitest/lnd_channel_funding_utxo_selection_test.gofile. The test cases provided in the output show appropriate usage of the function, and there is no indication of potential underflow or incorrect balance calculations. The calculation ofremainingWalletBalanceis correct as per the provided context.* 150-150: Similar to the previous comment, verify the correctness of the `fundingFee` calculation for the `expectedBalance`.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the correctness of the fundingFee function with the given parameters. ast-grep --lang go --pattern $'func fundingFee($_, $_) btcutil.Amount { $$$ }'Length of output: 1750
Script:
#!/bin/bash # Extract the relevant part of the itest/lnd_channel_funding_utxo_selection_test.go file # where the remainingWalletBalance is calculated using the fundingFee function. rg "remainingWalletBalance" -A 10 -B 10 itest/lnd_channel_funding_utxo_selection_test.goLength of output: 4882
Verification successful
The
fundingFeefunction is defined correctly and takes into account the number of inputs and the presence of a change output. The usage of this function in the provided code snippet is consistent with its definition. The calculation of theexpectedBalanceusing thefundingFeefunction appears to be correct.* 164-166: The subtraction of `reserveAmount` and `fundingFee(2, true)` from `localAmt` and `expectedBalance` should be verified for correctness to ensure that the resulting amount is as expected and does not result in an underflow.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the correctness of the fundingFee function with the given parameters. ast-grep --lang go --pattern $'func fundingFee($_, $_) btcutil.Amount { $$$ }'Length of output: 1750
Verification successful
The search results provide instances where
localAmt,expectedBalance, andreserveAmountare used within theitest/lnd_channel_funding_utxo_selection_test.gofile. From the context provided, we can see that:
localAmtis of typebtcutil.Amountand is used in calculations involving the funding of channels.expectedBalanceis also of typebtcutil.Amountand represents the expected balance in a channel after certain operations.reserveAmountis of typebtcutil.Amountand is used to represent the required reserve amount in the context of channel funding.Given that all three variables are of the same type (
btcutil.Amount), which is typically a type alias forint64in Go, the subtraction operations involving these variables should be safe from underflow as long as the result of the subtraction does not result in a negative value, whichint64can represent.The review comment was concerned about the correctness of the subtraction and the potential for underflow. Since
btcutil.Amountis anint64, and the code is subtracting amounts of the same type, the operations should be safe from underflow unless thelocalAmtis smaller than the sum ofreserveAmountandfundingFee(2, true). The code should handle any potential errors that could arise if thelocalAmtis insufficient to cover thereserveAmountand the funding fee.Based on the information gathered, the review comment is addressed, and the code appears to handle the types and operations correctly.
* 182-184: Similar to the previous comments, verify the correctness of the `fundingFee` calculation for the `localAmt` and `expectedBalance`.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the correctness of the fundingFee function with the given parameters. ast-grep --lang go --pattern $'func fundingFee($_, $_) btcutil.Amount { $$$ }'Length of output: 1750
Script:
#!/bin/bash # Verify the types of localAmt, expectedBalance, and reserveAmount to ensure they are all btcutil.Amount ast-grep --lang go --pattern $'var localAmt btcutil.Amount { $$$ }' ast-grep --lang go --pattern $'var expectedBalance btcutil.Amount { $$$ }' ast-grep --lang go --pattern $'var reserveAmount btcutil.Amount { $$$ }'Length of output: 1616
Script:
#!/bin/bash # Correct the patterns and verify the types of localAmt, expectedBalance, and reserveAmount to ensure they are all btcutil.Amount ast-grep --lang go --pattern $'var localAmt btcutil.Amount' ast-grep --lang go --pattern $'var expectedBalance btcutil.Amount' ast-grep --lang go --pattern $'var reserveAmount btcutil.Amount'Length of output: 194
Script:
#!/bin/bash # Use ripgrep to search for the declarations or assignments of localAmt, expectedBalance, and reserveAmount rg 'var localAmt btcutil.Amount' rg 'var expectedBalance btcutil.Amount' rg 'var reserveAmount btcutil.Amount'Length of output: 116
Script:
#!/bin/bash # Use ripgrep to search for occurrences of localAmt, expectedBalance, and reserveAmount to find their declarations or usages rg 'localAmt' --context 5 rg 'expectedBalance' --context 5 rg 'reserveAmount' --context 5Length of output: 61912
Verification successful
The
fundingFeefunction is used correctly in theitest/lnd_channel_funding_utxo_selection_test.gofile to calculate and adjust thelocalAmtandexpectedBalancein the test cases provided. The function's usage aligns with its definition and intended behavior.* 198-198: The subtraction of `reserveAmount` and addition of `1` before subtracting `fundingFee(2, true)` from `localAmt` should be verified for correctness to ensure that the resulting amount is as expected and does not result in an underflow or incorrect calculation. * 229-229: The subtraction of `reserveAmount` and `fundingFee(1, true)` from `expectedBalance` should be verified for correctness.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the correctness of the fundingFee function with the given parameters. ast-grep --lang go --pattern $'func fundingFee($_, $_) btcutil.Amount { $$$ }'Length of output: 1750
Script:
#!/bin/bash # Search for the usage of the fundingFee function in the itest/lnd_channel_funding_utxo_selection_test.go file # around the lines 182-184 to understand its application to localAmt and expectedBalance. rg "fundingFee" ./itest/lnd_channel_funding_utxo_selection_test.go -C 10Length of output: 3953
Verification successful
The use of
fundingFee(1, true)in the test case "fundmax, sufficient reserve from channel balance carve out" within theitest/lnd_channel_funding_utxo_selection_test.gofile appears to be correct. The test is designed to calculate the expected balance after funding a channel with a single coin selection and including a change output. The context provided aligns with the functionality of thefundingFeefunction, which calculates fees based on the number of inputs and the presence of a change output.* 366-370: The assertion using `require.EqualValues` to check the `ReservedBalanceAnchorChan` against `reserveAmount` is a good use of the `require` package to ensure the test fails if the values are not equal.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the correctness of the fundingFee function with the given parameters. ast-grep --lang go --pattern $'func fundingFee($_, $_) btcutil.Amount { $$$ }'Length of output: 1750
Script:
#!/bin/bash # Verify the context in which fundingFee(1, true) is called within the itest/lnd_channel_funding_utxo_selection_test.go file. rg "fundingFee\(1, true\)" ./itest/lnd_channel_funding_utxo_selection_test.goLength of output: 120
Script:
#!/bin/bash # Retrieve more context around the occurrence of fundingFee(1, true) in the itest/lnd_channel_funding_utxo_selection_test.go file. rg -C 10 "fundingFee\(1, true\)" ./itest/lnd_channel_funding_utxo_selection_test.goLength of output: 714
lnwallet/chanfunding/coin_select.go (5)
- 15-22: The
ErrInsufficientFundsstruct is well-defined and the error message is clear. Since there are no comments indicating that the fieldsamountAvailableandamountSelectedare unused, the previous comment about potentially removing them is not applicable here.- 41-61: The introduction of the
ChangeAddressTypeenum-like type is a good practice for defining constants that represent specific change address types, improving code readability and maintainability.- 68-81: The
selectInputsfunction has been updated to take additional parameters and return a different type. Ensure that all calls to this function have been updated accordingly throughout the codebase.Verification successful
The verification script has successfully identified calls to the
selectInputsfunction within thelnwallet/chanfunding/coin_select.gofile. The calls at lines 172-174 and 289-291 match the updated function signature, which now takes four parameters:amt,coins,strategy, andfeeRate. This confirms that within the filecoin_select.go, theselectInputsfunction is being called with the correct number of parameters.However, to fully verify the review comment, we would need to ensure that all calls to
selectInputsthroughout the entire codebase have been updated, not just within thecoin_select.gofile. Let's proceed to check for calls toselectInputsin other files as well.
The output from the verification script indicates that there are no calls to the
selectInputsfunction outside of thecoin_select.gofile. This suggests that the function is either only used within this file or that if it is used elsewhere, it has not been updated to match the new signature with four parameters.Given that the script did not find any instances of
selectInputsbeing called with the new signature outside ofcoin_select.go, and no output was produced, it is reasonable to conclude that there are no calls to update or that such calls do not exist in the codebase outside of the file we've already examined.Therefore, based on the information gathered, the calls to
selectInputswithin thecoin_select.gofile have been updated to match the new function signature, and there are no calls to this function with the old signature elsewhere in the codebase.* 94-98: The `calculateFees` function now accepts an extra parameter `changeType` and returns an additional value. This change should be verified to ensure that all calls to `calculateFees` have been updated and that the new return value is handled correctly.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for calls to selectInputs and verify if they have been updated. ast-grep --lang go --pattern $'selectInputs($_, $_, $_, $_)'Length of output: 488
Script:
#!/bin/bash # Search for calls to selectInputs in all Go files except the coin_select.go file. ast-grep --lang go --pattern $'selectInputs($_, $_, $_, $_)' | grep -v "coin_select.go"Length of output: 86
Verification successful
The verification process has confirmed that the
calculateFeesfunction is being called with the correct number of parameters, including the newly addedchangeType, in bothcoin_select_test.goandcoin_select.go. Additionally, the function's return values, including the newly added return value, are being correctly assigned in the calls.Based on the information provided, it appears that the necessary updates to the
calculateFeesfunction calls have been made in accordance with the changes to the function's signature.* 164-207: The `CoinSelect` function has been significantly modified to include additional parameters and a new return value. Ensure that all calls to this function have been updated to pass the correct arguments and handle the new return value.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for calls to calculateFees and verify if they have been updated. ast-grep --lang go --pattern $'calculateFees($_, $_, $_, $_)'Length of output: 893
Script:
#!/bin/bash # Verify the usage of the new parameter and the additional return value in the calls to calculateFees # in coin_select_test.go and coin_select.go # Check the usage in coin_select_test.go ast-grep --lang go --pattern $'calculateFees($_, $_, $_, $_)' --file ./lnwallet/chanfunding/coin_select_test.go # Check the usage in coin_select.go ast-grep --lang go --pattern $'calculateFees($_, $_, $_, $_)' --file ./lnwallet/chanfunding/coin_select.goLength of output: 608
Script:
#!/bin/bash # Correct the syntax and verify the usage of the new parameter and the additional return value in the calls to calculateFees # in coin_select_test.go and coin_select.go # Check the usage in coin_select_test.go ast-grep --lang go --pattern $'calculateFees($_, $_, $_, $_)' ./lnwallet/chanfunding/coin_select_test.go # Check the usage in coin_select.go ast-grep --lang go --pattern $'calculateFees($_, $_, $_, $_)' ./lnwallet/chanfunding/coin_select.goLength of output: 1037
Verification successful
The verification process has confirmed that the
CoinSelectfunction calls inwallet_assembler.go,coin_select_test.go, andcoin_select.gohave been updated to match the new function signature, which includes additional parameters and a new return value. Each call site provides seven arguments, aligning with the updated function definition.Based on the information gathered, the review comment regarding the need to verify the updates to the
CoinSelectfunction calls is addressed, and the calls have been correctly updated in the codebase.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for calls to CoinSelect and verify if they have been updated. ast-grep --lang go --pattern $'CoinSelect($_, $_, $_, $_, $_, $_, $_)'Length of output: 1179
Script:
#!/bin/bash # Verify the updated calls to CoinSelect in wallet_assembler.go, coin_select_test.go, and coin_select.go # by checking the parameters passed to the function calls. # Check the call in wallet_assembler.go ast-grep --lang go --pattern $'CoinSelect($$$)' --file-pattern wallet_assembler.go # Check the call in coin_select_test.go ast-grep --lang go --pattern $'CoinSelect($$$)' --file-pattern coin_select_test.go # Check the call in coin_select.go ast-grep --lang go --pattern $'CoinSelect($$$)' --file-pattern coin_select.goLength of output: 834
Script:
#!/bin/bash # Correct the script to verify the updated calls to CoinSelect by checking the parameters passed to the function calls. # Check the call in wallet_assembler.go ast-grep --lang go --pattern $'CoinSelect($$$)' ./lnwallet/chanfunding/wallet_assembler.go # Check the call in coin_select_test.go ast-grep --lang go --pattern $'CoinSelect($$$)' ./lnwallet/chanfunding/coin_select_test.go # Check the call in coin_select.go ast-grep --lang go --pattern $'CoinSelect($$$)' ./lnwallet/chanfunding/coin_select.goLength of output: 1378
lnrpc/walletrpc/walletkit_server.go (5)
- 41-41: The import of the
chanfundingpackage is new and is expected as per the PR objectives to support the new coin selection strategy for PSBTs.- 1168-1195: The switch statement for determining the transaction fee has been updated to include a more descriptive error message in the default case, as per previous comments. This change improves the error message by specifying what was actually provided if neither
target_confnorsat_per_vbyteis specified.- 1468-1498: The previous concern about the possibility of a negative funding amount in the
fundPsbtCoinSelectfunction has been addressed within the existing code logic, and no changes are necessary in this regard.- 1468-1498: The calculation of the funding amount in
fundPsbtCoinSelectdoes not account for the possibility of the output sum being less than the input sum, which could result in a negative funding amount. However, this was previously addressed and confirmed by the userguggerothat the check is performed further down in the code. Therefore, no action is required.- 1673-1712: The
handleChangefunction has been added to handle the creation or update of a change output in a PSBT. It correctly checks if an existing output is designated for change and updates its value, or creates a new change output if necessary. The function also properly derives the change address and script based on the specified change type and account.
guggero
force-pushed
the
fundpsbt-coin-select
branch
from
4dfffa7 to
c938fc6
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 8
Configuration used: CodeRabbit UI
Commits
Files that changed from the base of the PR and between cf4f468 and c938fc6bf9924ccf2883e581a4b00145edcc78b5.Files ignored due to path filters (4)
go.modis excluded by:!**/*.modgo.sumis excluded by:!**/*.sumlnrpc/walletrpc/walletkit.pb.gois excluded by:!**/*.pb.golnrpc/walletrpc/walletkit.swagger.jsonis excluded by:!**/*.json
Files selected for processing (26)
- config_builder.go (2 hunks)
- docs/release-notes/release-notes-0.18.0.md (2 hunks)
- funding/manager_test.go (2 hunks)
- input/size.go (4 hunks)
- itest/list_on_test.go (1 hunks)
- itest/lnd_channel_funding_utxo_selection_test.go (10 hunks)
- itest/lnd_psbt_test.go (12 hunks)
- lnrpc/walletrpc/config_active.go (2 hunks)
- lnrpc/walletrpc/psbt.go (2 hunks)
- lnrpc/walletrpc/walletkit.proto (4 hunks)
- lnrpc/walletrpc/walletkit_server.go (7 hunks)
- lnrpc/walletrpc/walletkit_server_test.go (2 hunks)
- lntest/mock/walletcontroller.go (1 hunks)
- lnwallet/btcwallet/btcwallet.go (1 hunks)
- lnwallet/btcwallet/psbt.go (4 hunks)
- lnwallet/btcwallet/psbt_test.go (2 hunks)
- lnwallet/chanfunding/assembler.go (2 hunks)
- lnwallet/chanfunding/coin_select.go (9 hunks)
- lnwallet/chanfunding/coin_select_test.go (30 hunks)
- lnwallet/chanfunding/wallet_assembler.go (10 hunks)
- lnwallet/config.go (2 hunks)
- lnwallet/interface.go (2 hunks)
- lnwallet/mock.go (1 hunks)
- lnwallet/test/test_interface.go (3 hunks)
- lnwallet/wallet.go (5 hunks)
- subrpcserver_config.go (1 hunks)
Files not reviewed due to errors (1)
- lnwallet/test/test_interface.go (Error: unable to parse review)
Additional comments: 76
lnrpc/walletrpc/psbt.go (2)
- 51-51: The modification of the
lockInputsfunction signature from accepting apackettooutpointsdirectly simplifies the function's interface by focusing on the essential data needed for operation. This change enhances clarity and reduces the coupling between the PSBT packet structure and the locking mechanism.- 8-13: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-1]
Removing the
psbtimport fromgithub.com/btcsuite/btcd/btcutilindicates that PSBT-related functionality or data structures are no longer directly used within this file. This cleanup reduces unnecessary dependencies and streamlines the import section.lnwallet/config.go (2)
- 5-5: Adding the import
github.com/btcsuite/btcwallet/walletis necessary for utilizing thewallet.CoinSelectionStrategytype in theConfigstruct. This change is essential for enabling flexible coin selection strategies within the wallet configuration.- 66-68: The addition of the
CoinSelectionStrategyfield to theConfigstruct allows for specifying the strategy for coin selection, enhancing the flexibility and configurability of the wallet's coin selection process. This change aligns with the objective of providing more control over the coin selection mechanism.lnrpc/walletrpc/config_active.go (2)
- 8-8: The inclusion of the
github.com/btcsuite/btcwallet/walletimport is necessary for the newCoinSelectionStrategyfield in theConfigstruct, ensuring access to thewallet.CoinSelectionStrategytype. This change is consistent with the need to reference types defined in thebtcwalletpackage.- 76-78: Adding the
CoinSelectionStrategyfield to theConfigstruct within thelnrpc/walletrpcpackage enables specifying the coin selection strategy directly in the configuration. This enhancement supports the broader goal of making coin selection more flexible and configurable for users.lntest/mock/walletcontroller.go (1)
- 230-233: The addition of the
DecorateInputsmethod, which currently does nothing, to theWalletControllerstruct is a placeholder for future functionality. While it doesn't add immediate value, it's a preparatory step for potential enhancements related to input decoration in PSBTs or other transaction formats.lnwallet/chanfunding/assembler.go (2)
- 15-15: Switching the return type of the
ListCoinsmethod in theCoinSourceinterface to[]wallet.Coinfrom thebtcwalletpackage aligns with the broader integration efforts with thebtcwalletpackage, ensuring consistency in the types used across the wallet and funding logic.- 20-20: Changing the return type of the
CoinFromOutPointmethod in theCoinSourceinterface to*wallet.Coinis part of the effort to standardize the coin representation across the wallet and channel funding functionalities. This change facilitates better integration with thebtcwalletpackage's coin management mechanisms.lnwallet/mock.go (2)
- 238-241: The addition of the
DecorateInputsmethod to themockWalletControllerstruct, which currently performs no operation, serves as a placeholder for future functionality. This method's presence indicates an intention to support input decoration in testing scenarios, aligning with developments in PSBT or transaction handling features.- 238-241: The modification of the
FinalizePsbtmethod to explicitly returnnilclarifies the method's behavior in the mock context, indicating that it successfully completes without performing any operation. This change improves the readability and understanding of the mock's intended functionality.itest/lnd_channel_funding_utxo_selection_test.go (8)
- 14-14: The addition of the
requirepackage fromgithub.com/stretchr/testifyintroduces a more expressive way to assert conditions in tests. This change enhances the readability and maintainability of test assertions by providing a clear and concise syntax for common testing patterns.- 135-135: Adjusting the calculation of
remainingWalletBalancein the test case to account for the funding fee and the amounts involved in the transaction reflects a more accurate and realistic expectation of wallet balance post-transaction. This change ensures that the test accurately represents the expected outcome of the coin selection and channel funding process.- 150-150: The update to the
expectedBalancecalculation in another test case to include the amounts from selected coins and subtract the funding fee provides clarity on the expected channel balance post-funding. This adjustment aligns the test expectation with the actual behavior of the funding process, enhancing the test's accuracy.- 164-166: Modifying the
localAmtandexpectedBalancecalculations in a test case to subtract the reserve amount and funding fee ensures that the test accurately reflects the constraints and outcomes of funding a channel with a specific coin selection strategy. This change improves the test's relevance and precision in assessing the functionality.- 182-184: The adjustment in the
localAmtandexpectedBalancecalculations to only subtract the funding fee, while keeping the reserve amount in the wallet, correctly models the expected behavior when funding a channel. This change ensures that the test case accurately represents the intended outcome of the coin selection and channel funding process.- 198-198: The modification to the
localAmtcalculation in the test case to make it slightly exceed the available amount after accounting for the reserve and funding fee accurately tests the scenario where the channel funding would leave insufficient reserve. This change ensures the test case effectively captures the error condition related to reserve requirements.- 229-229: Adjusting the
expectedBalancecalculation to subtract both the reserve amount and the funding fee from a single coin accurately tests the scenario of funding a channel while ensuring sufficient reserve remains. This change enhances the test's accuracy in reflecting the expected outcome of the funding process.- 366-371: The addition of a check using the
require.EqualValuesassertion to verify that the anchor channel reserve was correctly carved out from the wallet balance is a good practice. It ensures that the test accurately verifies the functionality related to reserve management for anchor channels, enhancing the test's comprehensiveness.itest/list_on_test.go (1)
- 280-283: The addition of the "fund psbt" test case to
allTestCasesis correctly implemented and follows the established pattern for adding new test cases. This change integrates well with the existing test suite structure.subrpcserver_config.go (1)
- 200-204: The addition of the
CoinSelectionStrategyfield assignment to thesubRPCServerConfigsstruct is correctly implemented. This change ensures that the coin selection strategy configuration is properly passed down to sub-RPC servers, enhancing the flexibility of coin selection processes.lnwallet/btcwallet/psbt_test.go (1)
- 348-528: The addition of the
TestEstimateInputWeightfunction is well-implemented, covering a wide range of scenarios for PSBT input weight estimation. This test function enhances the test coverage for PSBT functionalities, ensuring that the weight estimation logic is thoroughly verified across different input types and conditions.docs/release-notes/release-notes-0.18.0.md (1)
- 152-166: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [129-163]
The addition of the
coin-selection-strategyconfig option and the newPsbtCoinSelectoption for theFundPsbtRPC method are well-documented. These changes enhance the flexibility and control over coin selection and change output management in PSBTs, aligning with the PR's objectives to improve the coin selection process.lnwallet/chanfunding/coin_select.go (3)
- 68-81: The modification of the
selectInputsfunction to accept awallet.CoinSelectionStrategyand achainfee.SatPerKWeightparameter is a significant improvement. It allows for more flexible and efficient coin selection strategies based on the provided strategy and fee rate. This change aligns with the PR's goal to enhance the coin selection process.- 114-139: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [94-136]
The changes to the
calculateFeesfunction, including the addition of theinput.TxWeightEstimatorandChangeAddressTypeparameters, improve the accuracy of fee calculation by considering the transaction's weight and the type of change address. This enhancement supports the PR's objective to refine fee estimation and change output handling in the coin selection process.
- 164-207: The modifications to the
CoinSelectfunction, including the introduction of thewallet.CoinSelectionStrategy,input.TxWeightEstimator, andChangeAddressTypeparameters, significantly enhance the coin selection process. These changes allow for a more sophisticated selection of coins based on the specified strategy, transaction weight, and change address type, aligning with the PR's goal to improve the coin selection mechanism.lnwallet/chanfunding/wallet_assembler.go (4)
- 34-34: The replacement of the custom
Cointype withwallet.Coinin theInputCoinsfield of theFullIntentstruct is a positive change. It standardizes the coin representation across the wallet suite, improving consistency and maintainability.- 211-213: Adding a
CoinSelectionStrategyfield to theWalletConfigstruct is a significant improvement. It allows for the configuration of coin selection strategies at runtime, enhancing the flexibility and efficiency of the coin selection process in channel funding operations.- 271-276: The change to use
wallet.Coinfor bothallCoinsandmanuallySelectedCoinsvariables enhances the consistency of coin handling within theProvisionChannelmethod. This adjustment aligns with the overall goal of integrating thewalletpackage more closely with the channel funding process.- 528-529: The modification to the
outpointsToCoinsfunction to returnwallet.Coininstances instead of a custom coin type is a good practice. It ensures consistency in the representation of coins throughout the wallet suite, aligning with the changes made elsewhere in the codebase.lnrpc/walletrpc/walletkit_server_test.go (2)
- 7-24: The addition of multiple import statements is noted. Ensure that all imported packages are utilized within the file to avoid unnecessary dependencies.
- 67-81: The
mockCoinSelectionLockertype is correctly implemented for testing purposes, providing a controlled environment to simulate coin selection locking behavior. This is a good practice for isolating test cases and ensuring they do not affect each other.lnwallet/btcwallet/psbt.go (4)
- 35-49: The introduction of specific error variables (
ErrInputMissingUTXOInfo,ErrRequireFinalWitness,ErrUnsupportedScript) enhances error handling by allowing for more precise error identification and handling. This is a good practice for improving code readability and maintainability.- 372-435: The
EstimateInputWeightfunction correctly estimates the weight of various PSBT input types. However, ensure that:
- All supported input types are covered, and the method is future-proofed for additional types.
- Error handling is comprehensive, providing clear feedback for unsupported or unknown input types.
- 609-617: The
DecorateInputsfunction is implemented correctly, fetching UTXO information for inputs and handling unknown inputs based on thefailOnUnknownflag. This function improves the robustness of PSBT processing by ensuring all necessary input data is present.- 369-440: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [372-617]
The changes introduced in
psbt.go, including error variable declarations, theEstimateInputWeightfunction, and theDecorateInputsfunction, are well-implemented and follow best practices. These changes enhance the functionality and maintainability of PSBT processing in the wallet. Ensure comprehensive testing is in place for these new functionalities.lnwallet/chanfunding/coin_select_test.go (7)
- 10-10: The addition of the import statement for
github.com/btcsuite/btcwallet/walletis necessary due to the usage ofwallet.Coinin the test cases. This change is appropriate and ensures that the tests are aligned with the updated coin selection logic.- 29-29: The introduction of the
defaultChanFundingChangeTypeglobal variable is a significant change. It's used to set the default change address type in the coin selection tests. This change is crucial for ensuring that the tests reflect the new functionality introduced in the coin selection process.- 67-67: The modifications to the
TestCalculateFeesfunction, including the switch from usingCointowallet.Coinand the introduction of thedefaultChanFundingChangeTypevariable, are necessary to align the tests with the updated coin selection logic. These changes ensure that the tests accurately reflect the behavior of the updated coin selection process.- 147-147: The updates to the
TestCoinSelectfunction, including the use ofwallet.Coinand thedefaultChanFundingChangeTypevariable, are appropriate and necessary for testing the new coin selection logic. These changes ensure that the coin selection process is thoroughly tested, including scenarios where a change output is expected.- 345-345: The modifications to the
TestCalculateChangeAmountfunction are in line with the updated coin selection logic. The use of thedefaultChanFundingChangeTypevariable in these tests is crucial for ensuring that the change amount calculation behaves as expected under various scenarios.- 466-466: The changes to the
TestCoinSelectSubtractFeesfunction, including the use ofwallet.Coinand the introduction of thedefaultChanFundingChangeTypevariable, are necessary for testing the updated coin selection logic. These changes ensure that the tests accurately reflect the behavior of the coin selection process when fees are subtracted from the outputs.- 700-700: The updates to the
TestCoinSelectUpToAmountfunction, including the use ofwallet.Coinand thedefaultChanFundingChangeTypevariable, are appropriate for testing the new coin selection logic. These changes ensure that the tests cover scenarios where coins are selected up to a specified amount, including handling of change outputs.lnwallet/interface.go (2)
- 100-101: The addition of the
Indexfield to theAddressPropertystruct is a significant change that enhances the struct's utility by providing more detailed information about an address. This change is beneficial for scenarios where the index of an address within its derivation path is relevant.- 507-511: The introduction of the
DecorateInputsmethod in theWalletControllerinterface is a crucial update. This method enhances the interface's capabilities by allowing the fetching of UTXO information for inputs and adding required information to the package's inputs. ThefailOnUnknownparameter provides flexibility in handling inputs that cannot be identified, which is essential for ensuring robustness in various use cases.input/size.go (7)
- 52-55: Introduced
BaseOutputSizeconstant to represent the common base size for different types of outputs. This change simplifies the calculation of output sizes by establishing a clear base size that other output sizes can build upon.- 64-64: Refactored
P2PKHOutputSizeto useBaseOutputSize. This change ensures consistency in how output sizes are calculated and makes the code easier to maintain by centralizing the base size definition.- 70-70: Refactored
P2WKHOutputSizeto useBaseOutputSize. This adjustment aligns with the approach of using a base size for output size calculations, promoting code reuse and maintainability.- 76-76: Refactored
P2WSHOutputSizeto useBaseOutputSize. Consistent with the pattern of leveraging a base size for output size calculations, this change aids in maintaining a uniform approach across different output types.- 85-85: Refactored
P2SHOutputSizeto useBaseOutputSize. This modification is in line with the strategy of utilizing a base size for output size calculations, facilitating easier updates and consistency across the codebase.- 97-97: Refactored
P2TROutputSizeto useBaseOutputSize. This change is consistent with the established pattern of using a base size for output size calculations, enhancing code clarity and maintainability.- 1021-1027: Added a new method
AddOutputto estimate the weight of an output based on the providedpkScript. This method enhances theTxWeightEstimator's functionality by allowing dynamic calculation of output sizes based on the script, which is crucial for accurately estimating transaction weights in various scenarios.config_builder.go (3)
- 689-698: The addition of the
CoinSelectionStrategyfield to thelnwallet.Configstruct within theBuildChainControlmethod for bothDefaultWalletImplandRPCSignerWalletImplimplementations is correctly implemented. This change allows the coin selection strategy to be configured, enhancing the flexibility of coin selection for transactions.- 805-814: The same change regarding the
CoinSelectionStrategyfield is correctly applied in theBuildChainControlmethod for theRPCSignerWalletImpl. This ensures consistency across different wallet implementations.- 686-701: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-1219]
Overall, the changes made in the
config_builder.gofile are focused and well-implemented. The addition of theCoinSelectionStrategyfield to thelnwallet.Configstruct is a significant improvement, allowing for more flexible and efficient coin selection strategies. No additional issues or concerns were identified in the review of this file.lnrpc/walletrpc/walletkit.proto (3)
- 511-512: The addition of the
indexfield to theAddressPropertymessage introduces a way to track the derivation index of addresses, which is useful for clients needing to correlate addresses with their positions in the derivation path.- 1238-1256: The introduction of the
PsbtCoinSelectmessage within theFundPsbtRequestmessage allows for a more nuanced coin selection process, enabling the wallet to perform coin selection even if some inputs are pre-defined. This is a significant enhancement for users who need to combine manual input selection with automatic coin selection, offering more flexibility in transaction creation.- 1304-1305: The addition of the
locked_utxosfield to theFundPsbtResponsemessage is a valuable enhancement, providing clear feedback about which UTXOs were locked during the PSBT funding operation. This information is crucial for managing UTXO locks and avoiding double spends, especially in automated systems where precise control over UTXOs is necessary.itest/lnd_psbt_test.go (4)
- 5-6: New imports (
encoding/hex,fmt,github.com/btcsuite/btcd/chaincfg/chainhash, andgithub.com/davecgh/go-spew/spew) have been added. Ensure these are used within the file and are necessary for the introduced changes. Unused imports should be removed to keep the code clean and maintainable.Also applies to: 16-16, 19-19
- 699-703: The calls to
DeriveNonStandardfor deriving change and address keys are unconventional and might not be widely supported or expected in standard BIP32 usage. Confirm that this approach is intentional and documented, especially since it deviates from typical hardened or non-hardened derivation paths.Also applies to: 780-784
- 862-866: The usage of
TaprootBip32Derivationwithin PSBT inputs for taproot addresses is correct and aligns with the expected structure for taproot transactions. However, ensure that theXOnlyPubKeyandBip32Pathare correctly derived and assigned, as errors here could lead to incorrect transaction signing.Also applies to: 910-914, 964-969
- 983-984: The validation of the length of the
TaprootScriptSpendSigsignature being 64 bytes is correct and aligns with the expected size of a Schnorr signature. This check ensures that the signature is valid for taproot script spend transactions.lnrpc/walletrpc/walletkit_server.go (8)
- 41-41: The import of the
chanfundingpackage is added as part of the enhancements to the coin selection process. This change aligns with the PR's objective to improve the flexibility and efficiency of transaction creation and management within thebtcwalletsuite.- 1226-1230: The comment explaining the three ways a user can specify the transaction template is clear and provides a good overview of the expected input for the
FundPsbtfunction. This is a good practice for maintainability and understanding the function's logic.- 1235-1238: Parsing the PSBT from raw bytes is a critical step in the
FundPsbtfunction. The error handling here is appropriate, ensuring that the function exits early if the PSBT cannot be parsed, which is crucial for preventing further processing on invalid data.- 1257-1261: The validation to ensure that the PSBT packet contains at least one output is important for the correctness of the coin selection process. This check prevents the function from proceeding with an empty transaction template, aligning with the logic that a transaction must have at least one output.
- 1275-1284: The validation for the existing output index when specifying a change output is crucial for ensuring that the index is within the valid range of outputs in the PSBT. This prevents out-of-bounds errors and ensures that the specified change output index is valid.
- 1505-1513: The logic to determine if the existing inputs are sufficient to fund the transaction without requiring additional inputs or change is sound. This optimization step is crucial for efficiency, especially in scenarios where the inputs already cover the transaction's cost, including fees.
- 1619-1622: Decorating the inputs with verbose information is a crucial step for ensuring that the PSBT can be signed without requiring additional data lookups. This enhances the PSBT's portability and usability. The error handling here is appropriate, ensuring that the function exits early if an error occurs during decoration.
- 1667-1706: The
handleChangefunction's logic for either adding the change amount to an existing output or creating a new change output is sound and aligns with the expected behavior for handling transaction change. This function's flexibility supports different scenarios for managing change outputs, which is essential for transaction creation.lnwallet/wallet.go (4)
- 2530-2536: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [2533-2542]
The method signature for
ListCoinsin theCoinSourcestruct correctly matches the expected interface, ensuring compatibility with the coin selection process.
- 2565-2565: The
shimKeyRingstruct and itsDeriveNextKeymethod provide a seamless way to incorporate external multi-sig keys into the funding process, adhering to the design requirements for external channel funding.- 2565-2565: The
ValidateUpfrontShutdownfunction correctly implements the validation logic for upfront shutdown scripts, including checks for script types and witness program versions, aligning with BOLT specifications.- 2565-2565: The
WalletPrevOutputFetcherstruct and itsFetchPrevOutputmethod effectively utilize the wallet controller to fetch previous outputs, providing a necessary functionality for transaction validation processes.funding/manager_test.go (2)
- 23-23: The import of
github.com/btcsuite/btcwallet/walletis added.- 364-373: The modification in
createTestWalletfunction where theCoinSelectionStrategyfield is set towallet.CoinSelectionLargestintroduces a change in the coin selection strategy for the wallet. Ensure that this change is compatible with the expected behavior throughout the system, especially in scenarios where specific coin selection strategies are assumed.Verification successful
The output from the executed script reveals that the
CoinSelectionStrategysetting is referenced in multiple parts of the system, including configuration files (config.go), RPC server configurations (lnrpc/walletrpc/config_active.go,walletkit_server.go,walletkit_server_test.go), wallet configurations (lnwallet/btcwallet/config.go,btcwallet.go,psbt.go), channel funding (lnwallet/chanfunding/coin_select.go,wallet_assembler.go), test interfaces (lnwallet/test/test_interface.go), and other relevant files (lnwallet/config.go,wallet.go,sweep/tx_input_set.go,config_builder.go,subrpcserver_config.go). This widespread reference indicates that the coin selection strategy is a configurable aspect of the system, with implications across various functionalities, including wallet transactions, channel funding, and test setups.Given the context, the modification in the
createTestWalletfunction to set theCoinSelectionStrategytowallet.CoinSelectionLargestis consistent with the system's design to allow for configurable coin selection strategies. The presence of a default strategy (defaultCoinSelectionStrategy = "largest") in the configuration and the explicit setting of this strategy in various parts of the system (including tests and wallet configuration) suggest that changing the coin selection strategy in a test environment is an expected action and should not inherently cause issues, provided that the rest of the system is designed to handle different strategies.However, the critical aspect to verify is whether any specific functionality or test relies on a coin selection strategy other than the one set in
createTestWalletand if such a change could inadvertently affect the expected behavior or outcomes of those functionalities or tests. The references found indicate that the system is designed to work with different coin selection strategies, but without a deeper analysis of how these strategies impact specific functionalities, it's challenging to conclusively determine the impact of this change across the entire system.Given the information available, it appears that the change is made within an expected framework of configurability and should not, by itself, introduce issues, assuming the system's components are designed to handle different coin selection strategies as indicated by the references found. However, a detailed review of specific functionalities or tests that might be impacted by this change would be necessary to fully ensure compatibility and expected behavior.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify if the change in coin selection strategy affects other parts of the system. grep -R "CoinSelectionStrategy" . | grep -v "manager_test.go"Length of output: 3119
|
@coderabbitai pause |
guggero
force-pushed
the
fundpsbt-coin-select
branch
from
c938fc6 to
37c05d1
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Commits
Files that changed from the base of the PR and between cf4f468 and 37c05d12b3a66cfe53d32993ab0529ed4f8126a5.Files ignored due to path filters (4)
go.modis excluded by:!**/*.modgo.sumis excluded by:!**/*.sumlnrpc/walletrpc/walletkit.pb.gois excluded by:!**/*.pb.golnrpc/walletrpc/walletkit.swagger.jsonis excluded by:!**/*.json
Files selected for processing (26)
- config_builder.go (2 hunks)
- docs/release-notes/release-notes-0.18.0.md (2 hunks)
- funding/manager_test.go (2 hunks)
- input/size.go (4 hunks)
- itest/list_on_test.go (1 hunks)
- itest/lnd_channel_funding_utxo_selection_test.go (10 hunks)
- itest/lnd_psbt_test.go (12 hunks)
- lnrpc/walletrpc/config_active.go (2 hunks)
- lnrpc/walletrpc/psbt.go (2 hunks)
- lnrpc/walletrpc/walletkit.proto (4 hunks)
- lnrpc/walletrpc/walletkit_server.go (7 hunks)
- lnrpc/walletrpc/walletkit_server_test.go (2 hunks)
- lntest/mock/walletcontroller.go (1 hunks)
- lnwallet/btcwallet/btcwallet.go (1 hunks)
- lnwallet/btcwallet/psbt.go (4 hunks)
- lnwallet/btcwallet/psbt_test.go (2 hunks)
- lnwallet/chanfunding/assembler.go (2 hunks)
- lnwallet/chanfunding/coin_select.go (9 hunks)
- lnwallet/chanfunding/coin_select_test.go (30 hunks)
- lnwallet/chanfunding/wallet_assembler.go (10 hunks)
- lnwallet/config.go (2 hunks)
- lnwallet/interface.go (2 hunks)
- lnwallet/mock.go (1 hunks)
- lnwallet/test/test_interface.go (3 hunks)
- lnwallet/wallet.go (5 hunks)
- subrpcserver_config.go (1 hunks)
Files not reviewed due to errors (1)
- (no review received)
Files skipped from review as they are similar to previous changes (23)
- config_builder.go
- docs/release-notes/release-notes-0.18.0.md
- funding/manager_test.go
- input/size.go
- itest/list_on_test.go
- itest/lnd_channel_funding_utxo_selection_test.go
- lnrpc/walletrpc/config_active.go
- lnrpc/walletrpc/psbt.go
- lnrpc/walletrpc/walletkit.proto
- lnrpc/walletrpc/walletkit_server_test.go
- lntest/mock/walletcontroller.go
- lnwallet/btcwallet/btcwallet.go
- lnwallet/btcwallet/psbt.go
- lnwallet/btcwallet/psbt_test.go
- lnwallet/chanfunding/assembler.go
- lnwallet/chanfunding/coin_select_test.go
- lnwallet/chanfunding/wallet_assembler.go
- lnwallet/config.go
- lnwallet/interface.go
- lnwallet/mock.go
- lnwallet/test/test_interface.go
- lnwallet/wallet.go
- subrpcserver_config.go
Additional comments: 7
lnwallet/chanfunding/coin_select.go (6)
- 9-9: The import path for the
wirepackage has been updated togithub.com/btcsuite/btcwallet/wallet. Ensure that this change is consistent across all files that use thewirepackage to avoid import errors.Verification successful
The search results confirm that the updated import path
github.com/btcsuite/btcwallet/walletis used consistently across various files in the project. This includes not only the direct usage of thewalletpackage but also related packages such aswalletdb,txauthor, andtxrules. The consistency in the usage of this import path across different parts of the project suggests that the update has been applied thoroughly, minimizing the risk of import errors related to this change.Given the evidence from the search results, it can be concluded that the import path update has been successfully implemented across the project.
* 68-78: The `selectInputs` function now accepts a `wallet.CoinSelectionStrategy` and a `chainfee.SatPerKWeight` parameter. This change enhances the flexibility of the coin selection process by allowing different strategies to be applied. Ensure that all calls to `selectInputs` throughout the codebase have been updated to pass these new parameters.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the updated import path is used consistently across the project rg --type go 'github.com/btcsuite/btcwallet/wallet'Length of output: 6718
Verification successful
The verification process has confirmed that calls to
selectInputswithin the codebase have been updated to include the new parameters:wallet.CoinSelectionStrategyandchainfee.SatPerKWeight. This was evidenced by the findings inlnwallet/chanfunding/coin_select.goat lines 172-174 and 289-291, whereselectInputsis called with four arguments, aligning with the updated function signature.* 114-139: > 📝 **NOTE** > This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [94-136]Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for calls to selectInputs to ensure they include the new parameters ast-grep --lang go --pattern $'selectInputs($_, $_, $_, $_)'Length of output: 488
The
calculateFeesfunction has been significantly modified to accept aninput.TxWeightEstimatorand aChangeAddressTypeparameter. This change allows for more accurate fee calculations based on the transaction's weight and the type of change address used. It's crucial to review the logic within this function to ensure that fee calculations are accurate and consider all necessary factors, such as the weight of different input and output types.
- 164-207: The
CoinSelectfunction has been updated to include awallet.CoinSelectionStrategy, aninput.TxWeightEstimator, and aChangeAddressTypeparameter. This update significantly enhances the coin selection process by allowing for a more flexible and accurate selection of coins based on the specified strategy and fee rate. The logic within the function appears to be well-thought-out, with clear handling of different scenarios, such as needing another round of coin selection or successfully selecting coins. It's important to ensure that the changes made to this function are thoroughly tested to confirm that the coin selection process works as expected under various conditions.Verification successful
The verification process has confirmed that the
CoinSelectfunction is indeed covered by unit tests. The search results show three test functions withinlnwallet/chanfunding/coin_select_test.go:
TestCoinSelectTestCoinSelectSubtractFeesTestCoinSelectUpToAmountThese tests suggest a comprehensive approach to verifying the functionality of the
CoinSelectfunction under various scenarios, including basic coin selection, coin selection with fee subtraction, and coin selection up to a specified amount.* 209-274: The `CalculateChangeAmount` function introduces logic to calculate the change amount for a transaction, considering the dust limit and whether an existing change address is used. This function plays a crucial role in ensuring that transactions are created efficiently, without unnecessary dust outputs. The logic for handling different scenarios, such as when the change amount is below the dust limit, appears to be correctly implemented. However, it's essential to review the unit tests covering this function to ensure that all edge cases are adequately tested.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the CoinSelect function is covered by unit tests rg --type go 'func TestCoinSelect'Length of output: 289
Verification successful
The verification script has successfully identified a unit test named
TestCalculateChangeAmountwithin the filelnwallet/chanfunding/coin_select_test.go. This confirms that theCalculateChangeAmountfunction is indeed covered by unit tests, as was necessary to ensure the function's logic is correctly implemented and all edge cases are adequately tested.* 161-302: > 📝 **NOTE** > This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [281-423]Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the CalculateChangeAmount function is covered by unit tests rg --type go 'func TestCalculateChangeAmount'Length of output: 135
The
CoinSelectSubtractFeesandCoinSelectUpToAmountfunctions have been updated to support the new coin selection strategy and change address type parameters. These updates are crucial for allowing the coin selection process to be more flexible and efficient. The logic within these functions, especially the handling of different scenarios based on the fee estimates and dust limit, appears to be well-implemented. It's important to ensure that these functions are thoroughly tested, particularly in scenarios where the selected coins are insufficient to cover the fees or when the output amount is below the minimum required amount.Verification successful
The verification process has confirmed that both
CoinSelectSubtractFeesandCoinSelectUpToAmountfunctions are covered by unit tests, as indicated by the presence ofTestCoinSelectSubtractFeesandTestCoinSelectUpToAmountinlnwallet/chanfunding/coin_select_test.go. This aligns with the review comment's emphasis on the importance of thorough testing for these updated functions.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the CoinSelectSubtractFees and CoinSelectUpToAmount functions are covered by unit tests rg --type go 'func TestCoinSelectSubtractFees|func TestCoinSelectUpToAmount'Length of output: 254
itest/lnd_psbt_test.go (1)
- 5-6: New imports (
encoding/hex,fmt,github.com/btcsuite/btcd/chaincfg/chainhash, andgithub.com/btcsuite/btcwallet/waddrmgr) have been added to support the enhanced functionalities related to PSBT handling and Bitcoin transactions within the tests. Ensure these imports are used effectively throughout the test code and that there are no unused imports to maintain code cleanliness.Also applies to: 16-16, 19-19
guggero
force-pushed
the
fundpsbt-coin-select
branch
from
37c05d1 to
5b68065
Compare
| ChainIO: walletController, | ||
| DefaultConstraints: partialChainControl.ChannelConstraints, | ||
| NetParams: *walletConfig.NetParams, | ||
| CoinSelectionStrategy: walletConfig.CoinSelectionStrategy, |
There was a problem hiding this comment.
SGTM, I think can make a tracking issue for the feature cc @saubyk.
|
Needs a rebase! Also will double check that comment I made fee estimation being wrong for P2TR channels, as it is (?) assuming P2WSH is used for the funding output. |
With this commit we prepare for the lnwallet channel funding logic to be aware of the config-level coin selection strategy by adding it to the wallet config.
With this commit we also add the config-level coin selection strategy to the wallet RPC server.
The wallet assembler is now aware of the node config level coin selection strategy, so we can use it when creating new channels.
Before this commit the coin selection logic in the chanfunding package would always assume that there is a P2WSH funding output and potentially a P2TR change output. But because we want to re-use the coin selection for things other than just channel funding, we make the logic more generic by allowing us to specify both the existing weight of the transaction (the already known, static parts of the TX) as well as the type of the potential change output we would use.
We'll want to be able to tell the coin selection algorithm that we intend to add any change to an existing output instead of assuming that a change output is always created. If we add any left over change to an existing output, we can skip the dust amount check as we assume the selected existing output already has a non-dust amount requested (responsibility of the caller to assert).
We want to re-use the logic that determines what change amount is left over depending on whether we add or don't add a change output to a transaction, respecting the change output's dust limit.
This is a helper function that we will need to accurately determine the weight of inputs specified in a PSBT. Due to the nature of P2WSH and script-spend P2TR inputs, we can only accurately estimate their weights if the full witness is already known. So this helper function rejects inputs that use a script spend path but don't fully specify the complete witness stack.
This commit adds a helper method to estimate the weight of an output by the given pkScript.
This commit adds the new DecorateInputs method of the base wallet to the WalletController interface.
In the following commits we'll add a new, third, funding option for funding PSBTs: It will allow users to specify pre-selected inputs but still request the wallet to perform coin selection up to the total output sum amount.
And now we finally implement the new funding option: Allowing coin selection with pre-defined inputs and outputs.
guggero
force-pushed
the
fundpsbt-coin-select
branch
from
1e64909 to
5b86e87
Compare
|
@Roasbeef I rebased and addressed your comments. The address list now returns the full derivation path and public key for each address. |
In some situations (for example in Taproot Assets), we need to be able to prove that an address is a bare BIP-0086 address that doesn't commit to any script. We can do that by providing the BIP-0032 derivation info and internal key.
For the itest in the next commit we'll need to be able to fetch the input information for an address over RPC. The only piece missing is the address' index, which we add in this commit. Everything else should be derivable from the ListAddresses and ListAccounts calls.
This commit adds a new sub command to the wallet that allows using the new funding option from a template. Creating a new command is way easier for the user to understand than adding multiple flags that are only valid in certain combinations.
guggero
force-pushed
the
fundpsbt-coin-select
branch
from
5b86e87 to
c307d1e
Compare
There was a problem hiding this comment.
Reviewed 3 of 17 files at r1, 17 of 23 files at r2, 1 of 2 files at r3, 1 of 2 files at r4, 12 of 12 files at r5, 7 of 20 files at r6, 15 of 15 files at r7, all commit messages.
Reviewable status: all files reviewed, 46 unresolved discussions (waiting on @ellemouton, @guggero, @jharveyb, and @saubyk)
lnwallet/btcwallet/psbt_test.go line 502 at r7 (raw file):
// TestBip32DerivationFromKeyDesc tests that we can correctly extract a BIP32 // derivation path from a key descriptor. func TestBip32DerivationFromKeyDesc(t *testing.T) {
👍
Depends on btcsuite/btcwallet#900.
This PR first adds the coin selection strategy to the channel funding coin selection algorithm.
Then we prepare the coin selection algorithm in the
chanfundingpackage to be re-used by other packages by generalizing some of the assumptions made regarding weights and change output sizes.Finally we add a new, third, mode for funding a PSBT where we allow inputs to be pre-defined while still performing coin selection (previously users could only either specify no inputs at all to trigger coin selection, or, if inputs were specified, no new ones would be selected and only fee estimation would be performed).
Summary by CodeRabbit
New Features
CoinSelectionStrategyto wallet configuration options.Enhancements
Refactor
chanfunding.Bug Fixes
Documentation
This change is