[wallet] Fixups from account API deprecation#13498
Conversation
| if (IsDeprecatedRPCEnabled("accounts") && totalAmount > pwallet->GetLegacyBalance(ISMINE_SPENDABLE, nMinDepth, &strAccount)) { | ||
| throw JSONRPCError(RPC_WALLET_INSUFFICIENT_FUNDS, "Account has insufficient funds"); | ||
| } else if (!IsDeprecatedRPCEnabled("accounts") && totalAmount > pwallet->GetLegacyBalance(ISMINE_SPENDABLE, nMinDepth, nullptr)) { | ||
| throw JSONRPCError(RPC_WALLET_INSUFFICIENT_FUNDS, "Wallet has insufficient funds"); |
There was a problem hiding this comment.
Previously sendmany mimicked sendfrom's behaviour by allowing the user to select the addresses to be sent from. By making the label allways nullptr, there is no more fine grained selection. I propose to keep GetLegacyBalance(ISMINE_SPENDABLE, nMinDepth, &strAccount); without checking wether the deprecated flag is active and using the nullptr when the label argument passed in is \"\*\"
There was a problem hiding this comment.
The label in sendfrom/sendmany denotes which account's balance to debit. It does not, and never did, affect which UTXO were being used in transaction creation.
There was a problem hiding this comment.
The label in sendfrom/sendmany denotes which account's balance to debit. It does not, and never did, affect which UTXO were being used in transaction creation.
Indeed, this is one of the biggest confusions around accounts, and reasons why they're being deprecated.
(also: how the balance is computed doesn't affect UTXO selection)
There was a problem hiding this comment.
Yes, both sendfrom and sendmany use the same api of CreateTransaction (where no direct coin control is used in this case) followed by CommitTransaction. I think the wording would be better if "send" from is changed with "debited" from (both in the api call and in my comment, but that is probably not possible anymore).
Currently I have 2 UTXOs with 2 confirmations each in my wallet. One has an address with the label "THIS" and a balance of 0.19 BTC, the other is unlabeled and has 0.009 BTC. If I getbalance the amount is displayed correctly as 0.199BTC. If I now to try to sendmany "" '{"someaddress":0.03}', the Account has insufficient funds (code -6) error is thrown. Naively, I would expect this to work.
There was a problem hiding this comment.
@TheCharlatan - please see my comment here: #12952 (comment).
The account is not used in coin selection, but it is used in calculating the balance and whether there are enough funds to send from. That's what this PR is fixing.
There was a problem hiding this comment.
Sorry for the ruckus, this patch does exactly what it should do.
There was a problem hiding this comment.
@TheCharlatan no problem. Any test/review of this PR would be greatly appreciated 🙂
sedited
left a comment
sedited
left a comment
There was a problem hiding this comment.
Tested sendmany and sendfrom, seem to work as intended.
|
Concept ACK |
|
utACK df10f07 |
| if (IsDeprecatedRPCEnabled("accounts") && totalAmount > pwallet->GetLegacyBalance(ISMINE_SPENDABLE, nMinDepth, &strAccount)) { | ||
| throw JSONRPCError(RPC_WALLET_INSUFFICIENT_FUNDS, "Account has insufficient funds"); | ||
| } else if (!IsDeprecatedRPCEnabled("accounts") && totalAmount > pwallet->GetLegacyBalance(ISMINE_SPENDABLE, nMinDepth, nullptr)) { | ||
| throw JSONRPCError(RPC_WALLET_INSUFFICIENT_FUNDS, "Wallet has insufficient funds"); |
There was a problem hiding this comment.
Commit "[wallet] Don't use accounts when checking balance in sendmany"
Use same error as
bitcoin/src/wallet/rpcwallet.cpp
Line 466 in ee02deb
and
Line 2905 in ee02deb
There was a problem hiding this comment.
Thanks @promag . This code will need to be changed for V0.18 when accounts are definitively removed. Let's save error message cleanup until then and not invalidate previous ACKs.
df10f07 [wallet] Don't use accounts when checking balance in sendmany (John Newbery) e209184 [wallet] deprecate sendfrom RPC method. (John Newbery) Pull request description: A couple of fixups from the accounts API deprecation PR (#12953): - properly deprecate `sendfrom` - don't use accounts when calculating balance in `sendmany` (unless the `-deprecatedrpc=accounts` flag is being used) Tree-SHA512: 1befde055067438c4c3391bbff1aaed0e6249efd708c567db3f1faad40a0f28e64f95e5bad0679ae826d24a0239e4bc8a1c392dc93e2e7502343a7f6b1d1845c
df10f07 [wallet] Don't use accounts when checking balance in sendmany (John Newbery) e209184 [wallet] deprecate sendfrom RPC method. (John Newbery) Pull request description: A couple of fixups from the accounts API deprecation PR (bitcoin#12953): - properly deprecate `sendfrom` - don't use accounts when calculating balance in `sendmany` (unless the `-deprecatedrpc=accounts` flag is being used) Tree-SHA512: 1befde055067438c4c3391bbff1aaed0e6249efd708c567db3f1faad40a0f28e64f95e5bad0679ae826d24a0239e4bc8a1c392dc93e2e7502343a7f6b1d1845c
7 participants
A couple of fixups from the accounts API deprecation PR (#12953):
sendfromsendmany(unless the-deprecatedrpc=accountsflag is being used)