[wallet] Fixups from account API deprecation #13498

pull jnewbery wants to merge 2 commits into bitcoin:master from jnewbery:account_deprecation_fixups changing 3 files +18 −7

jnewbery commented at 4:36 PM on June 18, 2018: member
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)
[wallet] deprecate sendfrom RPC method. e209184101
[wallet] Don't use accounts when checking balance in sendmany df10f07db1
fanquake added the label Wallet on Jun 18, 2018
in src/wallet/rpcwallet.cpp:1269 in df10f07db1
```
1262 | @@ -1255,9 +1263,11 @@ static UniValue sendmany(const JSONRPCRequest& request)
1263 |      EnsureWalletIsUnlocked(pwallet);
1264 |  
1265 |      // Check funds
1266 | -    CAmount nBalance = pwallet->GetLegacyBalance(ISMINE_SPENDABLE, nMinDepth, &strAccount);
1267 | -    if (totalAmount > nBalance)
1268 | +    if (IsDeprecatedRPCEnabled("accounts") && totalAmount > pwallet->GetLegacyBalance(ISMINE_SPENDABLE, nMinDepth, &strAccount)) {
1269 |          throw JSONRPCError(RPC_WALLET_INSUFFICIENT_FUNDS, "Account has insufficient funds");
1270 | +    } else if (!IsDeprecatedRPCEnabled("accounts") && totalAmount > pwallet->GetLegacyBalance(ISMINE_SPENDABLE, nMinDepth, nullptr)) {
1271 | +        throw JSONRPCError(RPC_WALLET_INSUFFICIENT_FUNDS, "Wallet has insufficient funds");
```
TheCharlatan commented at 6:26 PM on June 19, 2018:

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 \"\*\"

sipa commented at 6:29 PM on June 19, 2018:

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.

laanwj commented at 8:57 PM on June 19, 2018:

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)

TheCharlatan commented at 9:56 PM on June 19, 2018:

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.

jnewbery commented at 2:01 PM on June 20, 2018:

@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.

TheCharlatan commented at 3:20 PM on June 20, 2018:

Sorry for the ruckus, this patch does exactly what it should do.

jnewbery commented at 4:32 PM on June 20, 2018:

@TheCharlatan no problem. Any test/review of this PR would be greatly appreciated :slightly_smiling_face:

promag commented at 1:40 PM on June 26, 2018:

Commit "[wallet] Don't use accounts when checking balance in sendmany"

Use same error as https://github.com/bitcoin/bitcoin/blob/ee02debb25675542d13a03b65ed49832b79b0db9/src/wallet/rpcwallet.cpp#L466 and https://github.com/bitcoin/bitcoin/blob/ee02debb25675542d13a03b65ed49832b79b0db9/src/wallet/wallet.cpp#L2905

jnewbery commented at 3:16 PM on June 26, 2018:

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.
TheCharlatan approved
TheCharlatan commented at 10:27 AM on June 21, 2018: member

Tested sendmany and sendfrom, seem to work as intended.
meshcollider commented at 1:50 AM on June 22, 2018: contributor

utACK https://github.com/bitcoin/bitcoin/commit/df10f07db178d84b3def9f0b99d9b7c92e4227a9
sipa commented at 2:17 AM on June 22, 2018: member

Concept ACK
laanwj commented at 5:35 PM on June 24, 2018: member

utACK df10f07db178d84b3def9f0b99d9b7c92e4227a9
promag commented at 1:42 PM on June 26, 2018: member

utACK df10f07. One comment though, feel free to ignore since the error code is more important.
sipa merged this on Jun 26, 2018
sipa closed this on Jun 26, 2018
sipa referenced this in commit f54f3738c8 on Jun 26, 2018
xdustinface referenced this in commit 6bb8da2788 on Dec 22, 2020
xdustinface referenced this in commit b84b7da8c3 on Dec 22, 2020
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#12953 Deprecate accounts