Return a more informative error message when trying to spend coinbase; select non-coinbase inputs when sending to a transparent output if needed #1431

bitcartel · 2016-09-22T18:46:33Z

Code change:

Extra parameter added to AvailableCoins to include or exclude Coinbase coins. Default value of parameter is 'true' as current behaviour is to include Coinbase coins.
SelectCoins, used for sending taddr->taddr, will now exclude Coinbase coins.

Unit test:
Tried to write a test to focus on the extra parameter added to AvailableCoins but could not.

Empirical testing on Testnet:
Current behaviour is that upstream RPC commands sendfrom and sendtoaddress try to spend coinbase coins returned by AvailableCoins. So the user will see:

./zcash-cli sendtoaddress mrEGRmGJhmwAa4MQjzGd86ry63vrvovu9b 1000.0
error: {"code":-6,"message":"Insufficient funds"}

./zcash-cli sendtoaddress mrEGRmGJhmwAa4MQjzGd86ry63vrvovu9b 0.00003000
error: {"code":-4,"message":"Error: The transaction was rejected! This might happen if some of the coins in your wallet were already spent, such as if you used a copy of wallet.dat and coins were spent in the copy but not marked as spent here."}

./zcash-cli sendfrom "" mrEGRmGJhmwAa4MQjzGd86ry63vrvovu9b 0.00003000
error: {"code":-4,"message":"Error: The transaction was rejected! This might happen if some of the coins in your wallet were already spent, such as if you used a copy of wallet.dat and coins were spent in the copy but not marked as spent here."}

After fix is applied:

./zcash-cli sendtoaddress mrEGRmGJhmwAa4MQjzGd86ry63vrvovu9b 1000.0
error: {"code":-6,"message":"Insufficient funds"}

./zcash-cli sendtoaddress mrEGRmGJhmwAa4MQjzGd86ry63vrvovu9b 0.00003000
error: {"code":-4,"message":"Coinbase funds can only be sent to a zaddr"}

When non-coinbase UTXOs exist, they will now be selected and used:

./zcash-cli z_sendmany tnPJZHeVxegCg91utaquBRPEDBGjozfz9iLDHt7zvphFbZdspNgkTVLCGjDcadQBKNyUwKs8pNjDXuEZKrE1aNLpFwHgz4t '[{"address":"mx5fTRhLZwbYE7ZqhAPueZgQGSnwTbdvKU", "amount":0.01}]'

./zcash-cli sendtoaddress mrEGRmGJhmwAa4MQjzGd86ry63vrvovu9b 1000.0
error: {"code":-6,"message":"Insufficient funds"}

./zcash-cli sendtoaddress mrEGRmGJhmwAa4MQjzGd86ry63vrvovu9b 0.00003000
9818e543ac2f689d4ce8b52087607d73fecd771d45d316a1d9db092f0485aff2

./zcash-cli sendfrom "" mrEGRmGJhmwAa4MQjzGd86ry63vrvovu9b 0.00003000
899f2894823f51f15fc73b5e0871ac943edbe0ff88e1635f86906087b72caf30

daira · 2016-09-27T11:23:44Z

src/wallet/wallet.cpp

+        vector<COutput> vAllCoins;
+        AvailableCoins(vAllCoins, true, coinControl, false, true);
+        fOnlyCoinbaseCoinsRet = vAllCoins.size()>0;
+    }


The code is correct but confusing. I suggest adding a comment:

// fOnlyCoinbaseCoinsRet should be true when there are coinbase coins // available, but no non-coinbase coins.

If I read the code correctly it has this behavior:

If all my coins are coinbases, I'll get the error message "Coinbase funds can only be sent to a zaddr".

Otherwise, (when I have some coinbases and some non-coinbases, but the non-coinbases are insufficient) I'll get the error "Insufficient funds."

This means that in the case where my non-coinbases aren't enough, but my non-coinbases plus my coinbases would be, then I still get the error "Insufficient funds." Should that be a separate case, i.e. "You have sufficient funds, but coinbases must be spent to a zaddr first"?

There might be TOCTTOU bug: the wallet lock is acquired inside AvailableCoins() so between the first call and the second, and the wallet state might change in between. fOnlyCoinbaseCoinsRet would be set incorrectly if new non-coinbase coins were acquired. I'm not sure if that matters.

daira · 2016-09-27T11:26:56Z

src/wallet/wallet.cpp

                {
-                    strFailReason = _("Insufficient funds");
+                    if (fOnlyCoinbaseCoins) {
+                        strFailReason =_("Coinbase funds can only be sent to a zaddr");


Nit: space after =

daira · 2016-09-27T11:30:14Z

Code is okay, but it definitely needs a unit test of AvailableCoins covering both cases for fOnlyCoinbaseCoins.

bitcartel · 2016-09-27T18:41:27Z

@daira I spent fair bit of time trying to add a unit test for AvailableCoins. Created a chain of transactions, with valid UTXOs, adding them to a wallet, but could not get AvailableCoins to see those UTXOs as being available for selection. At this moment in time I can only provide results from empirical testing on testnet.

str4d · 2016-09-29T11:38:17Z

@nathan-at-least please comment re: testing strategy.

nathan-at-least · 2016-09-30T01:42:34Z

@bitcartel: the fact that your unit test isn't working could tell us something interesting about the wallet, so keep that code around. (In fact, if the DISABLED gtest functionality is already landed, simply DISABLE that test and merge it, with an appropriate comment to the fact that we're not sure why it fails and that we want it to pass.)

I'd accept any other kind of automated test, such as a case in ./qa/rpc-tester/ if those are more feasible.

I'm not comfortable merging this with only manual testing.

bitcartel · 2016-09-30T18:41:17Z

@daira @nathan @str4d I'm fairly comfortable to have this merged because:

Running tests via qa/pull-tester/rpc-tests.sh passes (does not break existing expected behaviour)
qa tests run in regtest mode, so we must manually edit regtest params in chainparams.cpp to enforce coinbase validity, consensus.fCoinbaseMustBeProtected = true;. Now when we run the wallet test, it confirms that AvailableCoins is not selecting coinbase utxos for sending to a taddr.

../qa/pull-tester/rpc-tests.sh wallet
=== Running testscript wallet.py ===
...
  Initializing test directory /tmp/testDvQQ2O
  Mining blocks...
  JSONRPC error: Coinbase funds can only be sent to a zaddr
  Stopping nodes
  Cleaning up
  Failed
!!! FAIL: wallet.py !!!

Note: To write a specific case we will need to create a new qa harness which use new chain parameters, like regtest, but which enforce coinbase validity. If we simply update regtest params, most of the existing qa tests will fail.

bitcartel · 2016-10-03T16:55:31Z

This needs review and see comment above.

bitcartel · 2016-10-03T23:50:13Z

@nathan See review comment above. Ack?

bitcartel · 2016-10-04T03:22:44Z

@ebfull @str4d needs review.

str4d · 2016-10-04T06:47:48Z

Have not had time to review. Up to @nathan-at-least whether he is happy to include this tomorrow if it gets review.

nathan · 2016-10-04T17:24:36Z

@bitcartel wrong nathan ;)

nathan-at-least · 2016-10-04T18:01:36Z

IIUC, there is at least one ./qa/rpc-test case that verifies this error case and the resulting message. If so, then I ACK.

bitcartel · 2016-10-04T18:05:57Z

@nathan-at-least Kind of. I have to manually:

change a boolean in chainparams.cpp for regtest params
run the wallet test in /qa/rpc-test.
We can't leave the change in 1 permanent as that would break all tests.

nathan-at-least · 2016-10-04T18:16:23Z

I see, so the process is still manual but the manual part has been reduced to tweaking a single flag and rebuilding then running just that one test...

Let's postpone this until rc1 and ensure we have a fully automated test of any type running in our CI.

(As an aside, I'd much rather have a localized unit test than a new test harness with different chain params... How feasible is that?)

str4d · 2016-10-04T18:23:06Z

ACK, postponing.

bitcartel · 2016-10-04T18:36:42Z

Summary:

We can't unit test because the wallet is not easy to mock out.
The RPC tests operate in regtest mode which does not enforce all testnet rules.
If we run RPC tests in testnet mode rather than regtest mode, we can't just make a call to mine 100 blocks, so coinbase maturity becomes an issue as does how long it takes to set-up and run the tests.

nathan-at-least · 2016-10-04T19:01:32Z

We can't easily unit test. ;-)

Seriously though: it sounds like unit testing would require a lot of work, but it would be simpler to create a new fourth chain params mode, and a new harness just like ./qa/rpc-tests that uses this new fourth mode. I think even if that's simpler, the cognitive load and the technical debt of having yet a fourth mode is onerous. I'd like us to move towards fewer modes.

However, if this is the only thing we can get working for rc1, then that's what we'll have to do.

Longer term, if we pay the admittedly heavy up front cost of refactoring systems so that we can mock them out, then eventually we can have just a single mode. This would replace a bunch of runtime branches in application code with consistent (though perhaps verbose) test mocking. Mocing is the systematic use branches or compile-time conditionals which are quarantined in our unittest binary and not present on mainnet. That's the world I want to move towards.

It may very well be that rewriting the wallet code from scratch would be less effort than refactoring the existing code to be mockable. So if that's the case, let's seriously think about it for our longer term plans.

daira · 2016-10-04T21:19:53Z

-1 on any fourth chainparams mode.

defuse

ACK modulo possible TOCTTOU bug

defuse · 2016-10-08T01:46:51Z

src/wallet/wallet.cpp

+        vector<COutput> vAllCoins;
+        AvailableCoins(vAllCoins, true, coinControl, false, true);
+        fOnlyCoinbaseCoinsRet = vAllCoins.size()>0;
+    }


If I read the code correctly it has this behavior:

If all my coins are coinbases, I'll get the error message "Coinbase funds can only be sent to a zaddr".

Otherwise, (when I have some coinbases and some non-coinbases, but the non-coinbases are insufficient) I'll get the error "Insufficient funds."

This means that in the case where my non-coinbases aren't enough, but my non-coinbases plus my coinbases would be, then I still get the error "Insufficient funds." Should that be a separate case, i.e. "You have sufficient funds, but coinbases must be spent to a zaddr first"?

There might be TOCTTOU bug: the wallet lock is acquired inside AvailableCoins() so between the first call and the second, and the wallet state might change in between. fOnlyCoinbaseCoinsRet would be set incorrectly if new non-coinbase coins were acquired. I'm not sure if that matters.

bitcartel · 2016-10-09T21:26:20Z

Rebased, added test, push -f.

Thanks @defuse for comments. (1) I've added a more fine-grained error message, per your suggestion, of "Insufficient funds, coinbase funds can only be spent after they have been sent to a zaddr". (2) Regarding lock behaviour, its inherited from upstream and changed by this commit. The wallet lock is a recursive lock and the caller, CWallet::CreateTransaction, already obtains it, so there shouldn't be any issue.

@daira @nathan-at-least Added qa rpc test to test all three conditions (1) can't send coinbase utxos to taddr (2) insufficient funds unless coinbase utxos are sent to zaddr (and then to taddr) (3) insufficient funds even when including coinbase utxos.

The test was made possible by getting the qa rpc test to pass a runtime parameter -regtestprotectcoinbase to zcashd, where upon detection zcashd updates the regtest params to enforce the coinbase->zcaddr consensus rule.

bitcartel · 2016-10-14T21:30:07Z

Note that a third party needs this merged asap for their integration work with zcashd.
@nathan-at-least Test wallet_protectcoinbase.py added, so this should turn your provisional ACK into an ACK?
@defuse Do my comments and change turn your provisional ACK into an ACK?
@daira Test added, which is what you wanted in previous review comment.

daira · 2016-10-15T22:15:35Z

Although I think there's still no unit test of AvailableCoins covering both cases for fOnlyCoinbaseCoins (please correct me if I missed it), I believe this is now adequately covered by RPC tests. ACK+cov from me (but this is complicated so it should have at least one more ACK+cov).

bitcartel · 2016-10-15T23:03:15Z

@daira Correct, no unit test, but covered by RPC tests.
@str4d Are you able to review? Thanks.

str4d · 2016-10-16T05:27:48Z

I'll review in the morning.

daira · 2016-10-16T15:26:07Z

qa/rpc-tests/wallet_protectcoinbase.py

+        except JSONRPCException,e:
+            errorString = e.error['message']
+        print "error: ", errorString
+        assert_equal("Insufficient funds, coinbase funds can only be spent after they have been sent to a zaddr" in errorString, True)


Does this test need another case in which the send succeeds because there are enough non-coinbase funds, but the coin selection prior to this PR would have chosen a coinbase input?

(Sorry to raise this after already having ACKed; I know we're short on time.)

I force pushed this:

+ # Send will succeed because the balance of non-coinbase utxos is 20.0 + errorString = "" + try: + self.nodes[0].sendtoaddress(self.nodes[2].getnewaddress(), 19.0) + except JSONRPCException,e: + errorString = e.error['message'] + assert_equal("" in errorString, True) + + self.nodes[1].generate(10) + self.sync_all() + + # check balance + assert_equal(self.nodes[2].getbalance(), Decimal('19')) +

"" in errorString is always True. I think you want assert(False) in the except: clause. ACK if that is fixed.

@daira Ok, done.

daira · 2016-10-17T00:12:04Z

qa/rpc-tests/wallet_protectcoinbase.py

+            self.nodes[0].sendtoaddress(self.nodes[2].getnewaddress(), 19.0)
+        except JSONRPCException:
+            assert(False)
+        assert_equal("" in errorString, True)


You can/should also remove errorString = "" and the assert_equal, since they have no effect.

daira · 2016-10-17T00:12:30Z

src/chainparams.cpp

@@ -336,6 +336,11 @@ CChainParams &Params(CBaseChainParams::Network network) {
 void SelectParams(CBaseChainParams::Network network) {
    SelectBaseParams(network);
    pCurrentParams = &Params(network);
+
+    // Some python qa rpc tests need to enforce the coinbase consensue rule


str4d

ut(ACK+cov) mod comment

str4d · 2016-10-17T01:39:09Z

qa/rpc-tests/wallet_protectcoinbase.py

@@ -0,0 +1,126 @@
+#!/usr/bin/env python2
+# Copyright (c) 2014 The Bitcoin Core developers


daira · 2016-10-17T01:46:01Z

ut(ACK+cov)

…ash#1373). Extra parameter added to AvailableCoins to include or exclude Coinbase coins. SelectCoins, used for sending taddr->taddr, will exclude Coinbase coins. Added qa rpc test and a runtime parameter -regtestprotectcoinbase to enforce the coinbase->zaddr consensus rule in regtest mode.

bitcartel · 2016-10-17T02:06:58Z

Thanks @daira and @str4d for review and all other reviewer comments.
@zkbot r+

zkbot · 2016-10-17T02:06:59Z

📌 Commit 2b1cda3 has been approved by bitcartel

zkbot · 2016-10-17T02:07:01Z

⌛ Testing commit 2b1cda3 with merge ec8dc3a...

…itcartel Return a more informative error message when trying to spend coinbase; select non-coinbase inputs when sending to a transparent output if needed For #1373 and #1519 Code change: - Extra parameter added to AvailableCoins to include or exclude Coinbase coins. Default value of parameter is 'true' as current behaviour is to include Coinbase coins. - SelectCoins, used for sending taddr->taddr, will now exclude Coinbase coins. Unit test: Tried to write a test to focus on the extra parameter added to AvailableCoins but could not. Empirical testing on Testnet: Current behaviour is that upstream RPC commands sendfrom and sendtoaddress try to spend coinbase coins returned by AvailableCoins. So the user will see: ``` ./zcash-cli sendtoaddress mrEGRmGJhmwAa4MQjzGd86ry63vrvovu9b 1000.0 error: {"code":-6,"message":"Insufficient funds"} ./zcash-cli sendtoaddress mrEGRmGJhmwAa4MQjzGd86ry63vrvovu9b 0.00003000 error: {"code":-4,"message":"Error: The transaction was rejected! This might happen if some of the coins in your wallet were already spent, such as if you used a copy of wallet.dat and coins were spent in the copy but not marked as spent here."} ./zcash-cli sendfrom "" mrEGRmGJhmwAa4MQjzGd86ry63vrvovu9b 0.00003000 error: {"code":-4,"message":"Error: The transaction was rejected! This might happen if some of the coins in your wallet were already spent, such as if you used a copy of wallet.dat and coins were spent in the copy but not marked as spent here."} ``` After fix is applied: ``` ./zcash-cli sendtoaddress mrEGRmGJhmwAa4MQjzGd86ry63vrvovu9b 1000.0 error: {"code":-6,"message":"Insufficient funds"} ./zcash-cli sendtoaddress mrEGRmGJhmwAa4MQjzGd86ry63vrvovu9b 0.00003000 error: {"code":-4,"message":"Coinbase funds can only be sent to a zaddr"} ``` When non-coinbase UTXOs exist, they will now be selected and used: ``` ./zcash-cli z_sendmany tnPJZHeVxegCg91utaquBRPEDBGjozfz9iLDHt7zvphFbZdspNgkTVLCGjDcadQBKNyUwKs8pNjDXuEZKrE1aNLpFwHgz4t '[{"address":"mx5fTRhLZwbYE7ZqhAPueZgQGSnwTbdvKU", "amount":0.01}]' ./zcash-cli sendtoaddress mrEGRmGJhmwAa4MQjzGd86ry63vrvovu9b 1000.0 error: {"code":-6,"message":"Insufficient funds"} ./zcash-cli sendtoaddress mrEGRmGJhmwAa4MQjzGd86ry63vrvovu9b 0.00003000 9818e543ac2f689d4ce8b52087607d73fecd771d45d316a1d9db092f0485aff2 ./zcash-cli sendfrom "" mrEGRmGJhmwAa4MQjzGd86ry63vrvovu9b 0.00003000 899f2894823f51f15fc73b5e0871ac943edbe0ff88e1635f86906087b72caf30 ```

zkbot · 2016-10-17T02:48:13Z

☀️ Test successful - zcash

defuse · 2016-10-19T21:50:48Z

This was already merged, but yeah my provisional ACK changed into an ACK.

bitcartel added A-wallet Area: Wallet A-rpc-interface Area: RPC interface labels Sep 22, 2016

bitcartel added this to the 1.0.0-beta2 - audit mitigations and critical bug fixes milestone Sep 22, 2016

bitcartel added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 22, 2016

daira reviewed Sep 27, 2016

View reviewed changes

bitcartel mentioned this pull request Oct 1, 2016

Refactor wallet code or write gtest harness to increase test coverage #1329

Open

str4d modified the milestones: 1.0.0-rc1, 1.0.0-beta2 - audit mitigations and critical bug fixes Oct 4, 2016

str4d mentioned this pull request Oct 4, 2016

Make the RPC error clearer when failing to spend coinbase #1373

Closed

bitcartel force-pushed the master_1373_taddr_coinbase_error branch from 121218b to 9bbc4d6 Compare October 7, 2016 19:19

defuse reviewed Oct 8, 2016

View reviewed changes

bitcartel force-pushed the master_1373_taddr_coinbase_error branch from 9bbc4d6 to 3f34d62 Compare October 9, 2016 05:17

bitcartel mentioned this pull request Oct 12, 2016

Avoid selecting coinbase inputs if possible when sending to transparent output(s) #1519

Closed

bitcartel force-pushed the master_1373_taddr_coinbase_error branch from 0d6650b to a2ba409 Compare October 13, 2016 18:58

bitcartel mentioned this pull request Oct 14, 2016

Refactor a few wallet calls for better testability: z_getbalance, z_gettotalbalance #1358

Closed

daira changed the title ~~Return a more informative error message when trying to spend coinbase~~ Return a more informative error message when trying to spend coinbase; select non-coinbase inputs when sending to a transparent output if needed Oct 16, 2016

daira reviewed Oct 16, 2016

View reviewed changes

bitcartel force-pushed the master_1373_taddr_coinbase_error branch 2 times, most recently from a760cc1 to f3240a3 Compare October 17, 2016 00:04

daira reviewed Oct 17, 2016

View reviewed changes

bitcartel force-pushed the master_1373_taddr_coinbase_error branch from f3240a3 to cdf4f98 Compare October 17, 2016 01:20

str4d approved these changes Oct 17, 2016

View reviewed changes

bitcartel force-pushed the master_1373_taddr_coinbase_error branch from cdf4f98 to 2b1cda3 Compare October 17, 2016 02:06

bitcartel removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 17, 2016

zkbot merged commit 2b1cda3 into zcash:master Oct 17, 2016

str4d mentioned this pull request Oct 21, 2016

!!! FAIL: wallet_protectcoinbase.py !!! #1597

Closed

daira mentioned this pull request Jun 29, 2017

Verify that we exclude coinbase UTXOs from selection/use in Bitcoin RPCs. #2449

Closed

chappjc mentioned this pull request Jun 15, 2022

loadbot: Allow use with all markets. decred/dcrdex#1656

Merged

		@@ -0,0 +1,126 @@
		#!/usr/bin/env python2
		# Copyright (c) 2014 The Bitcoin Core developers

Return a more informative error message when trying to spend coinbase; select non-coinbase inputs when sending to a transparent output if needed #1431

Return a more informative error message when trying to spend coinbase; select non-coinbase inputs when sending to a transparent output if needed #1431

Conversation

bitcartel commented Sep 22, 2016 • edited by daira

Choose a reason for hiding this comment

Choose a reason for hiding this comment

defuse Oct 8, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daira commented Sep 27, 2016

bitcartel commented Sep 27, 2016

str4d commented Sep 29, 2016

nathan-at-least commented Sep 30, 2016 • edited

bitcartel commented Sep 30, 2016

bitcartel commented Oct 3, 2016

bitcartel commented Oct 3, 2016

bitcartel commented Oct 4, 2016

str4d commented Oct 4, 2016

nathan commented Oct 4, 2016

nathan-at-least commented Oct 4, 2016

bitcartel commented Oct 4, 2016

nathan-at-least commented Oct 4, 2016

str4d commented Oct 4, 2016

bitcartel commented Oct 4, 2016 • edited

nathan-at-least commented Oct 4, 2016 • edited

daira commented Oct 4, 2016

defuse left a comment

Choose a reason for hiding this comment

defuse Oct 8, 2016 • edited

Choose a reason for hiding this comment

bitcartel commented Oct 9, 2016 • edited

bitcartel commented Oct 14, 2016

daira commented Oct 15, 2016

bitcartel commented Oct 15, 2016

str4d commented Oct 16, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daira Oct 16, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

str4d left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daira commented Oct 17, 2016

bitcartel commented Oct 17, 2016

zkbot commented Oct 17, 2016

zkbot commented Oct 17, 2016

zkbot commented Oct 17, 2016

defuse commented Oct 19, 2016

bitcartel commented Sep 22, 2016 •

edited by daira

defuse Oct 8, 2016 •

edited

nathan-at-least commented Sep 30, 2016 •

edited

bitcartel commented Oct 4, 2016 •

edited

nathan-at-least commented Oct 4, 2016 •

edited

defuse Oct 8, 2016 •

edited

bitcartel commented Oct 9, 2016 •

edited

daira Oct 16, 2016 •

edited