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

Add SelectUnspent JSON-RPC method #2079

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

Conversation

beansgum
Copy link
Contributor

This adds the SelectUnspent method to the JSON-RPC server. It is used for fetching unspent wallet transactions that are enough to pay a specified amount. The returned transactions can be filtered using the minAmount, minconf & accountName parameters. The targetAmount parameter specifies the minimum total amount that should be returned and it'll be ignored if the spendAll parameter is true.
Inputs can be selected using the different methods specified by the InputSelectionMethod parameter. These options consist of RandomInputSelection that indicates any random utxo can be selected. RandomAddressInputSelection that indicates only utxos matching a randomly selected address should be selected. OneUTXOInputSelection indicates only one utxo should be selected(and should be enough to pay targetAmount). As well as UniqueTxInputSelection that indicates only utxos with a unique address and hash should be selected.

Copy link
Member

@matheusd matheusd left a comment

Choose a reason for hiding this comment

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

Did a first pass review. TBH, I'm not super sure this should live in the wallet package, since you can effectively do this selection by calling the existing ListUnspent, then using whatever logic you want to select desired outputs.

Do you have a specific reasoning for why this needs to be in the wallet itself?

internal/rpchelp/helpdescs_en_US.go Outdated Show resolved Hide resolved
wallet/wallet.go Outdated
@@ -3716,6 +3717,276 @@ func (w *Wallet) ListUnspent(ctx context.Context, minconf, maxconf int32, addres
return results, nil
}

// SelectUnspent returns a slice of objects representing the unspent wallet
// transactions for the given criteria that are enough to pay the target amount.
// The transaction amount and confirmations will be greater than the amount
Copy link
Member

Choose a reason for hiding this comment

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

"The output amount [...] will be greater than the minAmount"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

minAmount refers to the minimum amount output value of transaction output should have before it is considered

wallet/wallet.go Outdated Show resolved Hide resolved
wallet/wallet.go Outdated Show resolved Hide resolved
wallet/wallet.go Outdated Show resolved Hide resolved
internal/rpchelp/helpdescs_en_US.go Outdated Show resolved Hide resolved
wallet/wallet.go Outdated
case stake.TxTypeSStx:
// Ticket commitment, only spendable after ticket maturity.
if output.Index == 0 {
if !ticketMatured(w.chainParams, details.Height(), tipHeight) {
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, if you're going to use the new selectunspent api to build regular txs, using this output without client-side filtering will break your code (because you can't arbitrarily spend a ticket submission - only votes and revocations can).

Any thoughts on how to handle this (whether clients need to be warned to verify this or have some flag to toggle inclusion of ticket submission outputs)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ticket submissions will be removed by default, I don't think this API will be used to build votes or revocation transactions.

if !coinbaseMatured(w.chainParams, details.Height(), tipHeight) {
continue
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Need to also check if it's a tspend. OP_GEN outputs also need to pass maturity before being spent.

wallet/wallet.go Outdated
Comment on lines 3851 to 3859
acct, err := w.manager.AddrAccount(
addrmgrNs, addrs[0])
if err == nil {
s, err := w.manager.AccountName(
addrmgrNs, acct)
if err == nil {
acctName = s
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Silencing errors in general is not a great idea. Seems you want to specifically handle the case of the address not existing (which as you noted in the comment earlier, shouldn't happen), so you could check for errors.NotExist directly:

acct, err := w.manager.AddrAccount(addrmgrNs, addrs[0])
if err == nil {
	var s string
	s, err = w.manager.AccountName(addrmgrNs, acct)
	if err == nil {
		acctName = s
	}
}
if err != nil && !errors.Is(err, errors.NotExist) {
	return err
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

I'd do that as a separate PR.

wallet/wallet.go Outdated Show resolved Hide resolved
@beansgum
Copy link
Contributor Author

@matheusd the SelectUnspent function needs to be in the wallet package because it's not efficient for wallets with lots of utxos to return the full list.

- Shuffle transactions using wallet package's shuffle() and remove
  redundant sort.
- Change log level to trace
@beansgum beansgum requested a review from matheusd August 30, 2021 14:34
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