wallet: fix when sufficient preset inputs and subtractFeeFromOutputs

achow101 commented at 3:35 am on November 23, 2019: member

#17290 introduced a bug where, when we had preset inputs that covered the amount being sent and subtractFeeFrromOutputs was being used, transaction funding would result in a Fee exceeds maximum configured by -maxtxfee error. This was happening because we weren’t setting bnb_used = false when the preset inputs were used as it should have been. This resulted in a too high fee because the change would go to fees accidentally.

Apparently this particular case doesn’t have a test, so I’ve added one as well.

fanquake added the label Wallet on Nov 23, 2019

fanquake requested review from meshcollider on Nov 23, 2019

in src/wallet/wallet.cpp:2356 in fd05e41a37 outdated

2348@@ -2349,14 +2349,19 @@ bool CWallet::SelectCoins(const std::vector<COutput>& vAvailableCoins, const CAm
2349     size_t max_descendants = (size_t)std::max<int64_t>(1, limit_descendant_count);
2350     bool fRejectLongChains = gArgs.GetBoolArg("-walletrejectlongchains", DEFAULT_WALLET_REJECT_LONG_CHAINS);
2351 
2352-    bool res = value_to_select <= 0 ||
2353+    bool res = value_to_select <= 0;
2354+    if (res) {
2355+        bnb_used = false;
2356+    } else {
2357+        res = value_to_select <= 0 ||

meshcollider commented at 3:46 am on November 23, 2019:

Why leave this case here when we know it is false?

achow101 commented at 4:35 am on November 23, 2019:

Oops, forgot to delete that.

achow101 force-pushed on Nov 23, 2019

DrahtBot commented at 5:01 am on November 23, 2019: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#17526 (Use Single Random Draw In addition to knapsack as coin selection fallback by achow101)
#17331 (Use effective values throughout coin selection by achow101)
#17211 (Allow fundrawtransaction and walletcreatefundedpsbt to take external inputs by achow101)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

meshcollider commented at 5:19 am on November 23, 2019: contributor

The new test passes on current master without the code change

achow101 commented at 5:27 am on November 23, 2019: member

The new test passes on current master without the code change

Not for me:

 02019-11-23T05:27:14.580000Z TestFramework (INFO): Test fundrawtxn subtract fee from outputs with preset inputs that are sufficient
 12019-11-23T05:27:14.637000Z TestFramework (ERROR): JSONRPC error
 2Traceback (most recent call last):
 3  File "/home/andy/bitcoin/bitcoin/test/functional/test_framework/test_framework.py", line 111, in main
 4    self.run_test()
 5  File "test/functional/rpc_fundrawtransaction.py", line 95, in run_test
 6    self.test_subtract_fee_with_presets()
 7  File "test/functional/rpc_fundrawtransaction.py", line 753, in test_subtract_fee_with_presets
 8    fundedtx = self.nodes[0].fundrawtransaction(rawtx, {'subtractFeeFromOutputs': [0]})
 9  File "/home/andy/bitcoin/bitcoin/test/functional/test_framework/coverage.py", line 47, in __call__
10    return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
11  File "/home/andy/bitcoin/bitcoin/test/functional/test_framework/authproxy.py", line 141, in __call__
12    raise JSONRPCException(response['error'], status)
13test_framework.authproxy.JSONRPCException: Fee exceeds maximum configured by -maxtxfee (-4)
142019-11-23T05:27:14.694000Z TestFramework (INFO): Stopping nodes
152019-11-23T05:27:14.849000Z TestFramework (WARNING): Not cleaning up dir /tmp/bitcoin_func_test_rpwug9f9
162019-11-23T05:27:14.849000Z TestFramework (ERROR): Test failed. Test logging available at /tmp/bitcoin_func_test_rpwug9f9/test_framework.log
172019-11-23T05:27:14.849000Z TestFramework (ERROR): Hint: Call /home/andy/bitcoin/bitcoin/test/functional/combine_logs.py '/tmp/bitcoin_func_test_rpwug9f9' to consolidate all logs

laanwj commented at 9:20 am on November 23, 2019: member

I’ve tested this. When running the new test on current master’s binary, the “rpc_fundrawtransaction.py” test gives me a “test_framework.authproxy.JSONRPCException: Fee exceeds maximum configured by -maxtxfee (-4)” consistently. After recompile with this patch, it passes.

This is really strange, I wonder what causes the difference in behavior for @meshcollider .

darosior commented at 12:37 pm on November 23, 2019: member

FWIW the test pass on master for me as well (without the code change).

meshcollider commented at 5:02 am on November 24, 2019: contributor

 0samuel@hansome-pc:~/git/bitcoin$ git diff master
 1diff --git a/test/functional/rpc_fundrawtransaction.py b/test/functional/rpc_fundrawtransaction.py
 2index 693051edc..6f1ae0d3b 100755
 3--- a/test/functional/rpc_fundrawtransaction.py
 4+++ b/test/functional/rpc_fundrawtransaction.py
 5@@ -92,6 +92,7 @@ class RawTransactionsTest(BitcoinTestFramework):
 6         self.test_option_feerate()
 7         self.test_address_reuse()
 8         self.test_option_subtract_fee_from_outputs()
 9+        self.test_subtract_fee_with_presets()
10
11     def test_change_position(self):
12         """Ensure setting changePosition in fundraw with an exact match is handled properly."""
13@@ -741,5 +742,17 @@ class RawTransactionsTest(BitcoinTestFramework):
14         # The total subtracted from the outputs is equal to the fee.
15         assert_equal(share[0] + share[2] + share[3], result[0]['fee'])
16
17+    def test_subtract_fee_with_presets(self):
18+        self.log.info("Test fundrawtxn subtract fee from outputs with preset inputs that are sufficient")
19+
20+        addr = self.nodes[0].getnewaddress()
21+        txid = self.nodes[0].sendtoaddress(addr, 10)
22+        vout = find_vout_for_address(self.nodes[0], txid, addr)
23+
24+        rawtx = self.nodes[0].createrawtransaction([{'txid': txid, 'vout': vout}], [{self.nodes[0].getnewaddress(): 5}])
25+        fundedtx = self.nodes[0].fundrawtransaction(rawtx, {'subtractFeeFromOutputs': [0]})
26+        signedtx = self.nodes[0].signrawtransactionwithwallet(fundedtx['hex'])
27+        self.nodes[0].sendrawtransaction(signedtx['hex'])
28+
29 if __name__ == '__main__':
30     RawTransactionsTest().main()
31samuel@hansome-pc:~/git/bitcoin$ make -j5
32Making all in src
33make[1]: Entering directory '/home/samuel/git/bitcoin/src'
34make[2]: Entering directory '/home/samuel/git/bitcoin/src'
35  CXX      libbitcoinconsensus_la-arith_uint256.lo
36  CXX      libbitcoinconsensus_la-hash.lo
37  CXX      libbitcoinconsensus_la-pubkey.lo
38  CXX      libbitcoinconsensus_la-uint256.lo
39...  
40  CXX      qt/qt_libbitcoinqt_a-moc_walletview.o
41  CXX      qt/qt_libbitcoinqt_a-qrc_bitcoin.o
42  CXX      qt/qt_libbitcoinqt_a-qrc_bitcoin_locale.o
43  CXXLD    bitcoind
44  CXXLD    test/test_bitcoin
45  CXXLD    bench/bench_bitcoin
46  AR       qt/libbitcoinqt.a
47  CXXLD    qt/bitcoin-qt
48  CXXLD    qt/test/test_bitcoin-qt
49make[2]: Leaving directory '/home/samuel/git/bitcoin/src'
50make[1]: Leaving directory '/home/samuel/git/bitcoin/src'
51Making all in doc/man
52make[1]: Entering directory '/home/samuel/git/bitcoin/doc/man'
53make[1]: Nothing to be done for 'all'.
54make[1]: Leaving directory '/home/samuel/git/bitcoin/doc/man'
55make[1]: Entering directory '/home/samuel/git/bitcoin'
56make[1]: Nothing to be done for 'all-am'.
57make[1]: Leaving directory '/home/samuel/git/bitcoin'
58samuel@hansome-pc:~/git/bitcoin$ ./test/functional/test_runner.py rpc_fundrawtransaction.py
59Temporary test directory at /tmp/test_runner_?_??_20191124_180027
60Remaining jobs: [rpc_fundrawtransaction.py]
611/1 - rpc_fundrawtransaction.py passed, Duration: 13 s
62
63TEST                      | STATUS    | DURATION
64
65rpc_fundrawtransaction.py | ? Passed  | 13 s
66
67ALL                       | ? Passed  | 13 s (accumulated)
68Runtime: 13 s

Clean branch, up to date master, fresh build. Ubuntu 18.04.1. Idk why it passes but yours doesn’t.

Also tested with Travis, (https://travis-ci.org/meshcollider/bitcoin/builds/616192547) - jobs .4 and .8 passed rpc_fundrawtransaction.py

achow101 force-pushed on Nov 25, 2019

achow101 commented at 7:26 am on November 25, 2019: member

I figured out why we are getting different test results on master.

It’s because bnb_used is not initialized, so the access is undefined behavior and changes depending on the optimizations used. I always configured with --enable-debug which disables all optimizations. Presumably this results in bnb_used having some value that corresponds to true. After configuring without --enable-debug and thus allowing optimizations, the test passes on master. I assume this is because some optimization gives bnb_used the false value by default.

I’ve pushed another commit which should completely address this type of issue, at least until we merge #17331 which gets rid of that variable entirely. Instead of setting it at each of the non-BnB cases that can occur, we just default assume bnb_used = false and let it be set to true after BnB selection succeeds.

meshcollider commented at 8:56 am on November 25, 2019: contributor

Ah, good find.

Code review ACK d3ca0b4a9621bdab47d22229adbf6f81c0d11384

darosior approved

darosior commented at 2:43 pm on November 25, 2019: member

ACK 4b0d19babf20291fc8c3dadad106367b06446e9d

Sjors commented at 6:42 pm on November 25, 2019: member

@practicalswift spotted a similar issue in #13546. Let’s also actually initialize this thing, as @promag suggested: https://github.com/bitcoin/bitcoin/blob/2a97d2b1a525dbc8f6e6610fe543e003acfb7fac/src/wallet/wallet.cpp#L2681

instagibbs commented at 2:21 pm on November 26, 2019: member

Second commit needs a rework of the commit message since it’s just an additional test now.

achow101 force-pushed on Nov 26, 2019

Default to bnb_used = false as there are many cases where BnB is not used ff330badd4

tests: Add a test for funding with sufficient preset inputs and subtractFeeFromOutputs eadd1304c8

achow101 force-pushed on Nov 26, 2019

achow101 commented at 6:03 pm on November 26, 2019: member

Changed bnb_used to be initialized to false. Also updated the commit message.

instagibbs commented at 6:05 pm on November 26, 2019: member

utACK https://github.com/bitcoin/bitcoin/pull/17568/commits/eadd1304c81e0b89178e4cc7630bd31650850c85

Sjors commented at 4:12 pm on November 27, 2019: member

ACK eadd130. I can’t get this new test to fail on macOS (without this PR). It passes whether or not I compile with --enable-debug. It does fail on Ubuntu. Yay undefined behavior… Anyway, it’s a useful test.

fanquake approved

fanquake commented at 5:22 pm on December 1, 2019: member

ACK eadd1304c81e0b89178e4cc7630bd31650850c85

SelectCoins() is only called in CreateTransaction(). bnb_used is set to false before that call (and again inside SelectCoins()) which is before any calls to SelectCoinsMinConf().

Looking forward to something like #17331 which not only removes bnb_used but simplifies the wallet.cpp logic. As we’ve currently got code like this:

0// Choose coins to use
1bool bnb_used = false;
2if (pick_new_inputs) {
3    <snip>
4} else {
5    bnb_used = false;
6}

and are doing similar things with coin_selection_params.use_bnb.

fanquake referenced this in commit 19698ac6bc on Dec 1, 2019

fanquake merged this on Dec 1, 2019

fanquake closed this on Dec 1, 2019

sidhujag referenced this in commit f257825d57 on Dec 1, 2019

promag commented at 9:12 pm on December 1, 2019: member

@Sjors thanks for the mention.

Code review ACK eadd1304c81e0b89178e4cc7630bd31650850c85.

jasonbcox referenced this in commit 78322dfdfa on Sep 30, 2020

sidhujag referenced this in commit c9ca316e0d on Nov 10, 2020

MarcoFalke locked this on Dec 16, 2021

wallet: fix when sufficient preset inputs and subtractFeeFromOutputs #17568

Conflicts