[wallet] Fixups from account API deprecation #13498

1. 
   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)
2. [wallet] deprecate sendfrom RPC method. e209184101
3. [wallet] Don't use accounts when checking balance in sendmany df10f07db1
4. fanquake added the label Wallet on Jun 18, 2018
5. 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.
6. TheCharlatan approved
7. 
   TheCharlatan commented at 10:27 am on June 21, 2018: member

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

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

   Concept ACK
10. 
    laanwj commented at 5:35 pm on June 24, 2018: member

    utACK df10f07db178d84b3def9f0b99d9b7c92e4227a9
11. 
    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.
12. sipa merged this on Jun 26, 2018
13. sipa closed this on Jun 26, 2018

    ---

14. sipa referenced this in commit f54f3738c8 on Jun 26, 2018
15. xdustinface referenced this in commit 6bb8da2788 on Dec 22, 2020
16. xdustinface referenced this in commit b84b7da8c3 on Dec 22, 2020
17. MarcoFalke locked this on Sep 8, 2021

Contributors
jnewbery TheCharlatan sipa laanwj promag meshcollider

Labels
Wallet