Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

wallet: Return an error if select utxos in txtoOutputs contain duplicates. #928

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Chinwendu20
Copy link

As detailed here, the sendcoins command in lnd does not fail if a user supplies duplicate select utxos:
lightningnetwork/lnd#8516 (comment)

This means that we could potentially end up crafting invalid transactions as a result of this bug. This PR fixes that by returning an error in such scenarios. Testcases are also added to test for this.

@Chinwendu20
Copy link
Author

Chinwendu20 commented May 13, 2024

This PR is dependent on a new release of the fn package. To be updated

@@ -179,6 +180,10 @@ func (w *Wallet) txToOutputs(outputs []*wire.TxOut,

var inputSource txauthor.InputSource
if len(selectedUtxos) > 0 {
if !fn.IsUniqueVals(selectedUtxos) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it makes sense to just inline this function here, since it's literally a single line, instead of waiting for another PR to be merged and tag to be pushed.

@@ -179,6 +180,12 @@ func (w *Wallet) txToOutputs(outputs []*wire.TxOut,

var inputSource txauthor.InputSource
if len(selectedUtxos) > 0 {
if len(fn.NewSet(selectedUtxos...)) !=
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: (if line length permits)

			dedupUtxos := fn.NewSet(selectedUtxos...)
			if len(dedupUtxos) != len(selectedUtxos) {

}
tx1, err := w.txToOutputs(
[]*wire.TxOut{targetTxOut}, nil, nil, 0, 1, 1000,
CoinSelectionLargest, true, tc.selectUtxos, alwaysAllowUtxo,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please set up your IDE in a way that you get a visual indication of where the 80 characters cutoff limit is (and setting the tabulator width correctly too)?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies about this, as I just copied and put it in a for block, I did not recheck for the effect on the line length.

@Chinwendu20 Chinwendu20 force-pushed the duplicates branch 2 times, most recently from 8606565 to e42363c Compare May 17, 2024 11:00
Signed-off-by: Ononiwu Maureen <[email protected]>
Copy link
Collaborator

@guggero guggero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🎉

@guggero
Copy link
Collaborator

guggero commented May 22, 2024

cc @sputn1ck, can't request review through GitHub.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants