Use selected Utxos while crafting txn #912

Chinwendu20 · 2024-03-04T05:51:34Z

Related issue: lightningnetwork/lnd#6949 (comment)
and it is needed for lightningnetwork/lnd#8516

This pull request enables SendOutput in the wallet to use passed UTXOS as inputs for transactions. All passed UTXOS would be used as inputs.

sputn1ck

Thanks for the PR @Chinwendu20 !
I left some comments. I think we should definitely cover the new code in some test cases.

wallet/wallet.go

wallet/createtx.go

sputn1ck · 2024-03-04T08:34:39Z

wallet/createtx.go

-		if err != nil {
-			return err
+
+		arrangedCoins := wrappedEligible


We're using the arrangedCoins later in inputSource := makeInputSource(arrangedCoins). I wonder what happens if we're using an output amount much small than the combined inputs, I think it would be expected to use all the provided input, but I'm not sure if makeInputSource will do that. I think some test coverage is definitely needed here.

If I understand correctly, you are asking what would happen if a user wants to send an amount way smaller than the combined amount present in the select utxos?
I do not think that scenario is a cause for alarm or violates anything?

What I'm trying to convey is that not the arrangedCoins, but the inputSource is the final instance for selecting the inputs. In the current approach if we have preselected 2 inputs[1btc, 1btc] and want to send to an output with 0.5btc we would only select the first input. I don't think this is the intention of this PR, as we want to use all the inputs we select.

See:

btcwallet/wallet/createtx.go

Line 25 in 1f3534b

func makeInputSource(eligible []Coin) txauthor.InputSource {

btcwallet/wallet/txauthor/author.go

Line 103 in 1f3534b

inputAmount, inputs, inputValues, scripts, err := fetchInputs(targetAmount + targetFee)

Ohh I must have missed this requirement, my understanding was that we needed to use the utxos provided by the users only to make a decision on what to send to an address. I did not realize we had to use all the inputs. I would have to close this PR then to use the psbt API on lnd side.

Signed-off-by: Ononiwu Maureen <[email protected]>

Chinwendu20 · 2024-03-05T02:18:01Z

I think we can continue with this approach, more straightforward than the psbt API, having to fund, finalize and all that, this is more direct. Made some changes with respect to the requirement as well.

sputn1ck

Approach seems correct now! Just left some comments that would clean up the pr a bit

wallet/wallet.go

wallet/createtx.go

wallet/createtx_test.go

Chinwendu20 · 2024-03-07T14:42:11Z

wallet/createtx_test.go

@@ -399,3 +402,96 @@ func TestCreateSimpleCustomChange(t *testing.T) {
 		require.Equal(t, scriptType, txscript.WitnessV0PubKeyHashTy)
 	}
 }
+
+// TestCreateSimpleCustomChange tests that it's possible to let the
+// CreateSimpleTx use all coins for coin selection, but specify a custom scope


Fix comment.

guggero

Nice work! I like that we can re-use the constant input source for this.
Code looks good apart from a few code style nits.

wallet/wallet.go

wallet/createtx.go

guggero · 2024-03-13T15:50:34Z

wallet/createtx.go

+				}
+			}
+
+			arrangedCoins, err := coinSelectionStrategy.


Maybe we should just rename coinSelectionStrategy to simply strategy to avoid this weird folding. The type contains enough information to warrant this shorter name IMO.

I would change the name to that. But I am curious in the case we are not able to change the name, what would be the best way to handle this situation?

Then the way you formatted it is would have been correct. I just really personally don't like that style, so I always try to avoid it if possible. There are a couple of tricks you can use to achieve that: Shorten the name of the struct the method is called on, shorten the method name, extract the code into its own function/method to reduce indentation, reduce indentation by avoiding else blocks where it makes sense (e.g. return in the if, so the else block can continue un-indented) and so on.

wallet/createtx_test.go

Signed-off-by: Ononiwu Maureen <[email protected]>

guggero

Final nits, otherwise LGTM 🎉

guggero · 2024-03-14T07:58:58Z

wallet/wallet.go

+		coinSelectionStrategy, label, selectedUtxos...)
+}
+
+// sendOutputs creates and sends payment transactions.It returns the transaction


nit: missing space after full stop.

guggero · 2024-03-14T07:59:21Z

wallet/createtx.go

+				e, ok := eligibleByOutpoint[outpoint]
+
+				if !ok {
+					return fmt.Errorf("selected outpoint"+


nit: missing space after word "outpoint".

Signed-off-by: Ononiwu Maureen <[email protected]>

sputn1ck

LGTM!

Use selected Utxos while crafting txn

Chinwendu20 mentioned this pull request Mar 4, 2024

Simplify coin selection for sendcoins lightningnetwork/lnd#8516

Closed

8 tasks

sputn1ck reviewed Mar 4, 2024

View reviewed changes

guggero self-requested a review March 4, 2024 08:44

Chinwendu20 marked this pull request as draft March 4, 2024 10:58

Chinwendu20 force-pushed the sendcoin branch from 4687d87 to 4b50bce Compare March 5, 2024 01:56

Ononiwu Maureen added 3 commits March 5, 2024 03:04

wallet: Add selectUtxos to txCreateOptions

b4c1e19

Signed-off-by: Ononiwu Maureen <[email protected]>

wallet: Add selectUtxos to createTxRequest

7ccfd85

Signed-off-by: Ononiwu Maureen <[email protected]>

wallet: move constantInputSource to createtx.go

4c78651

Signed-off-by: Ononiwu Maureen <[email protected]>

Chinwendu20 force-pushed the sendcoin branch from 4b50bce to 6a1f5e3 Compare March 5, 2024 02:13

Chinwendu20 marked this pull request as ready for review March 5, 2024 02:15

sputn1ck reviewed Mar 7, 2024

View reviewed changes

Chinwendu20 commented Mar 7, 2024

View reviewed changes

Chinwendu20 force-pushed the sendcoin branch 6 times, most recently from ed1195c to 508c3a0 Compare March 8, 2024 13:26

Chinwendu20 requested a review from sputn1ck March 8, 2024 13:28

guggero reviewed Mar 13, 2024

View reviewed changes

wallet: Create new function SendOutputWithInput

fe15d37

Signed-off-by: Ononiwu Maureen <[email protected]>

Chinwendu20 force-pushed the sendcoin branch 2 times, most recently from 0964003 to 3a16e1f Compare March 14, 2024 07:19

guggero approved these changes Mar 14, 2024

View reviewed changes

wallet: Add selectedutxos to txToOutputs

d75f2c3

Signed-off-by: Ononiwu Maureen <[email protected]>

Chinwendu20 force-pushed the sendcoin branch from 3a16e1f to d75f2c3 Compare March 14, 2024 08:39

sputn1ck approved these changes Mar 18, 2024

View reviewed changes

guggero merged commit 4342a56 into btcsuite:master Mar 18, 2024
3 checks passed

buck54321 pushed a commit to buck54321/btcwallet that referenced this pull request Apr 21, 2024

Merge pull request btcsuite#912 from Chinwendu20/sendcoin

aaf0090

Use selected Utxos while crafting txn

buck54321 pushed a commit to buck54321/btcwallet that referenced this pull request Apr 21, 2024

Merge pull request btcsuite#912 from Chinwendu20/sendcoin

b628ee2

Use selected Utxos while crafting txn

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use selected Utxos while crafting txn #912

Use selected Utxos while crafting txn #912

Chinwendu20 commented Mar 4, 2024 •

edited

Loading

sputn1ck left a comment •

edited

Loading

sputn1ck Mar 4, 2024 •

edited

Loading

Chinwendu20 Mar 4, 2024

sputn1ck Mar 4, 2024 •

edited

Loading

Chinwendu20 Mar 4, 2024

Chinwendu20 commented Mar 5, 2024

sputn1ck left a comment

Chinwendu20 Mar 7, 2024

guggero left a comment

guggero Mar 13, 2024

Chinwendu20 Mar 14, 2024

guggero Mar 14, 2024

guggero left a comment

guggero Mar 14, 2024

guggero Mar 14, 2024

sputn1ck left a comment

Use selected Utxos while crafting txn #912

Use selected Utxos while crafting txn #912

Conversation

Chinwendu20 commented Mar 4, 2024 • edited Loading

sputn1ck left a comment • edited Loading

Choose a reason for hiding this comment

sputn1ck Mar 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sputn1ck Mar 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Chinwendu20 commented Mar 5, 2024

sputn1ck left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guggero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guggero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sputn1ck left a comment

Choose a reason for hiding this comment

Chinwendu20 commented Mar 4, 2024 •

edited

Loading

sputn1ck left a comment •

edited

Loading

sputn1ck Mar 4, 2024 •

edited

Loading

sputn1ck Mar 4, 2024 •

edited

Loading