dont use sweeper unconfirmed utxos #8545
Conversation
|
Important Auto Review SkippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis set of changes focuses on enhancing the UTXO selection process for channel funding within a cryptocurrency network, specifically targeting the exclusion of unconfirmed and labeled UTXOs by the sweeper subsystem. It introduces new testing for channel funding with labeled transactions, updates error messaging for clarity on UTXO status, and adds functionality to fetch transaction labels. These improvements aim to prevent the use of potentially compromised UTXOs in critical operations, thereby increasing the robustness and reliability of channel funding. Changes
Assessment against linked issues
Possibly related issues
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 (
|
ziggie1984
force-pushed
the
dont-use-sweeper-unconfirmed-utxos
branch
2 times, most recently
from
d7b91d0 to
546e369
Compare
|
Nope it's not part of the sweeper scope, and concept ACK. |
ziggie1984
force-pushed
the
dont-use-sweeper-unconfirmed-utxos
branch
from
b289f05 to
86d8d12
Compare
ziggie1984
added
the
funding
ziggie1984
force-pushed
the
dont-use-sweeper-unconfirmed-utxos
branch
2 times, most recently
from
1010d7f to
404f2bd
Compare
ziggie1984
force-pushed
the
dont-use-sweeper-unconfirmed-utxos
branch
from
404f2bd to
9c6b9f9
Compare
|
Concept ACK, looking forward to this fix! |
ProofOfKeags
requested review from
ProofOfKeags,
a team and
bhandras
and removed request for
a team
ziggie1984
force-pushed
the
dont-use-sweeper-unconfirmed-utxos
branch
from
9c6b9f9 to
de9b38c
Compare
|
@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: 2
Configuration used: CodeRabbit UI
Commits
Files that changed from the base of the PR and between d9887f3 and de9b38c0e15022f8c1b438c25f9c7e69f657a940.Files selected for processing (10)
- docs/release-notes/release-notes-0.18.0.md (1 hunks)
- itest/list_on_test.go (1 hunks)
- itest/lnd_channel_funding_utxo_selection_test.go (1 hunks)
- itest/lnd_funding_test.go (2 hunks)
- lntest/mock/walletcontroller.go (1 hunks)
- lnwallet/btcwallet/btcwallet.go (1 hunks)
- lnwallet/chanfunding/wallet_assembler.go (1 hunks)
- lnwallet/interface.go (1 hunks)
- lnwallet/mock.go (1 hunks)
- lnwallet/wallet.go (4 hunks)
Additional comments: 9
lntest/mock/walletcontroller.go (1)
- 290-292: The implementation of
FetchTxLabelin the mockWalletControllerreturns a hardcoded empty string and nil error, which is appropriate for a mock. Ensure that this behavior aligns with the intended test scenarios where transaction labels are involved.lnwallet/mock.go (1)
- 300-303: The implementation of
FetchTxLabelin themockWalletControllerreturns a hardcoded empty string and nil error, which is appropriate for a mock. Ensure that this behavior aligns with the intended test scenarios where transaction labels are involved.itest/lnd_channel_funding_utxo_selection_test.go (1)
- 333-334: The updated error message provides clearer information about why an outpoint cannot be reused for channel funding, specifically mentioning its lock by the sweeper subsystem. This enhancement improves the clarity and actionability of error messages in test scenarios.
itest/list_on_test.go (1)
- 120-123: The addition of the new test case "open channel with labeled txns" is correctly implemented and follows the existing pattern for defining test cases. This test case is crucial for verifying the new behavior related to channel funding with labeled UTXOs, aligning with the PR's objectives.
docs/release-notes/release-notes-0.18.0.md (1)
- 93-96: The summary of the fix for utxo selection in the internal channel funding flow is clear and concise. However, it would be beneficial to include a brief explanation of why using unconfirmed and labeled UTXOs by the sweeper subsystem poses a risk. This additional context can help users understand the importance of the fix and the potential issues it prevents.
lnwallet/chanfunding/wallet_assembler.go (1)
- 306-307: The updated error message now explicitly mentions the possibility of an outpoint being locked by the sweeper subsystem. This enhancement improves clarity for the user, helping them understand the specific reason why an outpoint cannot be used for channel funding.
lnwallet/btcwallet/btcwallet.go (1)
- 1926-1948: The implementation of
FetchTxLabelcorrectly fetches the label of a transaction, handling cases where no label bucket exists or the transaction does not have a label by silencing the error. This approach is well-documented and aligns with the intended behavior.lnwallet/wallet.go (2)
- 2549-2581: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [2537-2578]
The logic to exclude unconfirmed UTXOs marked by the sweeper subsystem from coin selection is sound and aligns with the goal of preventing the use of potentially replaceable transactions in critical operations like channel funding. However, it's important to ensure that this exclusion logic is consistently applied across all relevant parts of the wallet to maintain the integrity of the funding process.
- 2552-2578: The implementation of excluding unconfirmed UTXOs marked by the sweeper subsystem is correctly done by checking the transaction label against the sweepLabel. This approach effectively prevents the use of such UTXOs in channel funding, which is crucial for avoiding issues related to transaction malleability and replace-by-fee (RBF) scenarios. The use of
strings.Containsfor label comparison is appropriate here, considering the label format.
ziggie1984
force-pushed
the
dont-use-sweeper-unconfirmed-utxos
branch
from
de9b38c to
969ffae
Compare
There was a problem hiding this comment.
This seems to me to be a layer violation. I could be wrong here and would love others to weigh in but the way I see it is that lnwallet sits below both the sweeper and the channel open facility. As such, we should deal with this elsewhere. We should have the sweeper and other various subsystems to enumerate the utxos that it is responsible for creating and whether or not they are stable. We can assume that if they are unconfirmed that they are unstable.
From here, the OpenChannel/AcceptChannel code should ask the lnwallet to list out the outputs it is aware of, and then perform a set difference, removing the outputs that the sweeper (or other systems) says are currently unstable.
Interesting idea, so the point is, that we do the coin-selection (checking if we have enough funds for the channel) in the Happy to discuss other ways to redesign it, will also think about alternatives. |
I'll admit I'm not super familiar with the code that implements the funding manager. We probably can't do coin select "up to amount" prior to this filter so maybe we would need to accept a filter function like this CC @Roasbeef @yyforyongyu for their opinions here. |
|
Thanks to your valuable feedback @ProofOfKeags, so I agree with you and will diverge from this approach. Instead of filtering for the label lets just filter the wallet transaction for RBF signaling. Although current bitcoind implementations disregard it and will still allow RBFs for non signaling transactions (full-rbf) we can still use this signaling to decide whether we consider this utxo safe to use for further transactions ? Will work on the new design and show a draft soonish. |
|
Last push only update of the latest btcwallet lib. |
|
Sorry for the accidental close. GitHub tried to be smart, the PR btcsuite/btcwallet#919 has the word "fix" in front of this PR's link... |
| for _, utxo := range utxos { | ||
| // If there is a filter function supplied all utxos not adhering | ||
| // to these conditions will be discared. | ||
| if c.allowUtxo != nil && !c.allowUtxo(*utxo) { |
There was a problem hiding this comment.
why not also just add a WithUtxoFilter option to ListUnspentWitnessFromDefaultAccount in btcwallet? just wondering if we could do the filtering in a single place instead
There was a problem hiding this comment.
Yes agree we might consider doing this. I liked doing it way higher up the design because I realized I would need to funnel the argument into every function call, so because I started with the single funding flow I was happy to introduce the change at a higher level. Only later I found out that the psbt part can only be done by funneling the argument all the way down to the btcwallet lib. Maybe you are right having the filtering all in one place might be more cleaner. I open to rework it. Keep in mind that the filter logic will live in two different functions tho in the btcwallet lib. (findEligibleOutputs and ListUnspent). The latter is also exported so it might cause a bigger API change. Wdyt ?
There was a problem hiding this comment.
The psbt case had already options in the call CreateSimpleTx so it was easy to introduce.
| // | ||
| // NOTE: This is very useful when opening channels with unconfirmed | ||
| // inputs to make sure stable non-replaceable inputs are used. | ||
| AllowUtxoForFunding func(Utxo) bool |
There was a problem hiding this comment.
hmm ok one style thing that stands out to me here is that we are making this a request specific field but it is not something that changes per request. So why not instead have this on the main lnwallet.Config? Then it is just defined once and can be re-used over and over again per channel funding request.
There was a problem hiding this comment.
ie, to me, anything in InitFundingReserveMsg should be specific to that request
There was a problem hiding this comment.
if we wanna make things way more generic (so that this is a request specific thing) then how about making the field on InitFundingReserveMsg something like UtxoFilter func(utxo) bool and then pass that straight as a functional option to btcwallet.
That way this does make sense on a per-request level here.
There was a problem hiding this comment.
i know there was potentially already some off-line discussion about this between other reviewers & the author - so apologies if this has already been covered - just want to make sure I understand the decision properly
There was a problem hiding this comment.
ok upon further inspection, I see that that would quite a bit of refactor since the lnwallet.Config gets create way before the sweeper is created. So perhaps not worth the effort to change this 👍
There was a problem hiding this comment.
I agree your proposal seems definitely better design wise, but I don't think it's worth the effort going down this route.
| @@ -1417,21 +1418,80 @@ func (w *WalletKit) fundPsbtInternalWallet(account string, | |||
| return err | |||
| } | |||
|
|
|||
| // filterFn makes sure utxos which are unconfirmed and | |||
| // still used by the sweeper are not used. | |||
| filterFn := func(u *lnwallet.Utxo) bool { | |||
There was a problem hiding this comment.
same question as prev commit - could we not expand ListUnspentWitness to take the UtxoFilter functional option?
ziggie1984
force-pushed
the
dont-use-sweeper-unconfirmed-utxos
branch
from
d424906 to
3573bab
Compare
|
I think we should pause on merging this until the sweeper series is in. There'll be quite a lot of conflicts within the itests, and given all the work put into that series so far, I think we want to minimize extra work created for the OP. |
|
@ellemouton: review reminder |
|
Just needs a rebase 🙏 |
ziggie1984
force-pushed
the
dont-use-sweeper-unconfirmed-utxos
branch
from
3573bab to
558d7ee
Compare
|
Looks like a couple of the unit tests are failing. Afaict they seem unrelated, do we have flakes again? |
| // the restictions of unstable utxos. | ||
| // We create the unconfirmed sweeper originating utxo just like before | ||
| // by force-closing a channel from dave's side. | ||
| ht.CloseChannelAssertPending(dave, chanPoint3, true) |
There was a problem hiding this comment.
just FYI we can easily create an unconfirmed utxo using BumpFee to avoid force closing channels now - sth like calling BumpFee with a node's wallet utxo and using Immediate: true.
There was a problem hiding this comment.
Using the BumpFee RPC, is more involved then just FCing the Channel here which immediately creates the sweep-output. To use the BumpFee RPC we need an unconfirmed transaction in the first place.
itest/lnd_psbt_test.go
Outdated
| // Fund Carol's wallet with an unconfirmed utxo. | ||
| ht.FundCoinsUnconfirmed(fundingAmt, carol) | ||
|
|
||
| ht.AssertNumUTXOsUnconfirmed(carol, 1) |
There was a problem hiding this comment.
again think the test can be simplified by using the BumpFee - we then no longer need to create FC to generate a sweeper-used utxo and can exit the test earlier.
There was a problem hiding this comment.
see comment above, because we are opening channels anyway in this test to prove that one can open channels with unconfirmed utxos, we can just FC the channel and immediately have a sweeper related utxo for further testing.
| AliasManager: s.aliasMgr, | ||
| DeleteAliasEdge: deleteAliasEdge, | ||
| AliasManager: s.aliasMgr, | ||
| IsSweeperOutpoint: s.sweeper.IsSweeperOutpoint, |
There was a problem hiding this comment.
not a big fan of passing func as config params but I guess the alternative would require a larger change.
ziggie1984
force-pushed
the
dont-use-sweeper-unconfirmed-utxos
branch
from
558d7ee to
bb7b108
Compare
ziggie1984
force-pushed
the
dont-use-sweeper-unconfirmed-utxos
branch
from
bb7b108 to
8c44c67
Compare
|
itest failed 😢 |
|
Looking into it. |
ziggie1984
force-pushed
the
dont-use-sweeper-unconfirmed-utxos
branch
from
8c44c67 to
a14f48e
Compare
Restrict the utxo selection when opening a single internal wallet funded backed channel.
Add utxo restrictions for psbt internal wallet funded lightning channels. This also includes batchopening channels backed by the internal wallet.
Allow unconfirmed inputs to be used when funding a channel via the psbt channel opening flow. We do now check for unstable utxos.
Itests were added to the normal channel funding flow and the psbt funding flow using unstable (unconfirmed sweeper inputs).
ziggie1984
force-pushed
the
dont-use-sweeper-unconfirmed-utxos
branch
from
a14f48e to
58e1288
Compare
Fixes #8543
Fixes #8540
We will not use any unconfirmed funds which are labeled by the sweeper subsystem to open internal wallet funded channels.
Seems to be a quick win comparing the change scope ?
@yyforyongyu or is this fix already part of your sweeper series ?
Added an itest to verify this behavior.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Tests