wallet: fix when sufficient preset inputs and subtractFeeFromOutputs by achow101 · Pull Request #17568 · bitcoin/bitcoin

achow101 · 2019-11-23T03:35:01Z

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

meshcollider · 2019-11-23T03:46:09Z

src/wallet/wallet.cpp

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

Oops, forgot to delete that.

DrahtBot · 2019-11-23T05:01:32Z

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 · 2019-11-23T05:19:15Z

The new test passes on current master without the code change

achow101 · 2019-11-23T05:27:49Z

The new test passes on current master without the code change

Not for me:

2019-11-23T05:27:14.580000Z TestFramework (INFO): Test fundrawtxn subtract fee from outputs with preset inputs that are sufficient
2019-11-23T05:27:14.637000Z TestFramework (ERROR): JSONRPC error
Traceback (most recent call last):
  File "/home/andy/bitcoin/bitcoin/test/functional/test_framework/test_framework.py", line 111, in main
    self.run_test()
  File "test/functional/rpc_fundrawtransaction.py", line 95, in run_test
    self.test_subtract_fee_with_presets()
  File "test/functional/rpc_fundrawtransaction.py", line 753, in test_subtract_fee_with_presets
    fundedtx = self.nodes[0].fundrawtransaction(rawtx, {'subtractFeeFromOutputs': [0]})
  File "/home/andy/bitcoin/bitcoin/test/functional/test_framework/coverage.py", line 47, in __call__
    return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
  File "/home/andy/bitcoin/bitcoin/test/functional/test_framework/authproxy.py", line 141, in __call__
    raise JSONRPCException(response['error'], status)
test_framework.authproxy.JSONRPCException: Fee exceeds maximum configured by -maxtxfee (-4)
2019-11-23T05:27:14.694000Z TestFramework (INFO): Stopping nodes
2019-11-23T05:27:14.849000Z TestFramework (WARNING): Not cleaning up dir /tmp/bitcoin_func_test_rpwug9f9
2019-11-23T05:27:14.849000Z TestFramework (ERROR): Test failed. Test logging available at /tmp/bitcoin_func_test_rpwug9f9/test_framework.log
2019-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 · 2019-11-23T09:20:29Z

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 · 2019-11-23T12:37:27Z

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

meshcollider · 2019-11-24T05:02:29Z

samuel@hansome-pc:~/git/bitcoin$ git diff master
diff --git a/test/functional/rpc_fundrawtransaction.py b/test/functional/rpc_fundrawtransaction.py
index 693051edc..6f1ae0d3b 100755
--- a/test/functional/rpc_fundrawtransaction.py
+++ b/test/functional/rpc_fundrawtransaction.py
@@ -92,6 +92,7 @@ class RawTransactionsTest(BitcoinTestFramework):
         self.test_option_feerate()
         self.test_address_reuse()
         self.test_option_subtract_fee_from_outputs()
+        self.test_subtract_fee_with_presets()

     def test_change_position(self):
         """Ensure setting changePosition in fundraw with an exact match is handled properly."""
@@ -741,5 +742,17 @@ class RawTransactionsTest(BitcoinTestFramework):
         # The total subtracted from the outputs is equal to the fee.
         assert_equal(share[0] + share[2] + share[3], result[0]['fee'])

+    def test_subtract_fee_with_presets(self):
+        self.log.info("Test fundrawtxn subtract fee from outputs with preset inputs that are sufficient")
+
+        addr = self.nodes[0].getnewaddress()
+        txid = self.nodes[0].sendtoaddress(addr, 10)
+        vout = find_vout_for_address(self.nodes[0], txid, addr)
+
+        rawtx = self.nodes[0].createrawtransaction([{'txid': txid, 'vout': vout}], [{self.nodes[0].getnewaddress(): 5}])
+        fundedtx = self.nodes[0].fundrawtransaction(rawtx, {'subtractFeeFromOutputs': [0]})
+        signedtx = self.nodes[0].signrawtransactionwithwallet(fundedtx['hex'])
+        self.nodes[0].sendrawtransaction(signedtx['hex'])
+
 if __name__ == '__main__':
     RawTransactionsTest().main()
samuel@hansome-pc:~/git/bitcoin$ make -j5
Making all in src
make[1]: Entering directory '/home/samuel/git/bitcoin/src'
make[2]: Entering directory '/home/samuel/git/bitcoin/src'
  CXX      libbitcoinconsensus_la-arith_uint256.lo
  CXX      libbitcoinconsensus_la-hash.lo
  CXX      libbitcoinconsensus_la-pubkey.lo
  CXX      libbitcoinconsensus_la-uint256.lo
...  
  CXX      qt/qt_libbitcoinqt_a-moc_walletview.o
  CXX      qt/qt_libbitcoinqt_a-qrc_bitcoin.o
  CXX      qt/qt_libbitcoinqt_a-qrc_bitcoin_locale.o
  CXXLD    bitcoind
  CXXLD    test/test_bitcoin
  CXXLD    bench/bench_bitcoin
  AR       qt/libbitcoinqt.a
  CXXLD    qt/bitcoin-qt
  CXXLD    qt/test/test_bitcoin-qt
make[2]: Leaving directory '/home/samuel/git/bitcoin/src'
make[1]: Leaving directory '/home/samuel/git/bitcoin/src'
Making all in doc/man
make[1]: Entering directory '/home/samuel/git/bitcoin/doc/man'
make[1]: Nothing to be done for 'all'.
make[1]: Leaving directory '/home/samuel/git/bitcoin/doc/man'
make[1]: Entering directory '/home/samuel/git/bitcoin'
make[1]: Nothing to be done for 'all-am'.
make[1]: Leaving directory '/home/samuel/git/bitcoin'
samuel@hansome-pc:~/git/bitcoin$ ./test/functional/test_runner.py rpc_fundrawtransaction.py
Temporary test directory at /tmp/test_runner_?_??_20191124_180027
Remaining jobs: [rpc_fundrawtransaction.py]
1/1 - rpc_fundrawtransaction.py passed, Duration: 13 s

TEST                      | STATUS    | DURATION

rpc_fundrawtransaction.py | ? Passed  | 13 s

ALL                       | ? Passed  | 13 s (accumulated)
Runtime: 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 · 2019-11-25T07:26:17Z

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 · 2019-11-25T08:56:03Z

Ah, good find.

Code review ACK d3ca0b4a9621bdab47d22229adbf6f81c0d11384

darosior

ACK 4b0d19babf20291fc8c3dadad106367b06446e9d

Sjors · 2019-11-25T18:42:04Z

@practicalswift spotted a similar issue in #13546. Let's also actually initialize this thing, as @promag suggested:

bitcoin/src/wallet/wallet.cpp

Line 2681 in 2a97d2b

bool bnb_used;

instagibbs

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

…used

…actFeeFromOutputs

achow101 · 2019-11-26T18:03:09Z

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

instagibbs · 2019-11-26T18:05:57Z

utACK eadd130

Sjors · 2019-11-27T16:12:34Z

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

ACK eadd130

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:

// Choose coins to use
bool bnb_used = false;
if (pick_new_inputs) {
    <snip>
} else {
    bnb_used = false;
}

and are doing similar things with coin_selection_params.use_bnb.

…eeFromOutputs eadd130 tests: Add a test for funding with sufficient preset inputs and subtractFeeFromOutputs (Andrew Chow) ff330ba Default to bnb_used = false as there are many cases where BnB is not used (Andrew Chow) Pull request description: #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. ACKs for top commit: Sjors: 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: ACK eadd130 instagibbs: utACK eadd130 Tree-SHA512: 7286c321f78666eea558cc591174630d210263594df41cab1065417510591ee514ade0e1d0cec8af09a785757da68de82592b013e8fe8d4966cec3254368706e

…btractFeeFromOutputs eadd130 tests: Add a test for funding with sufficient preset inputs and subtractFeeFromOutputs (Andrew Chow) ff330ba Default to bnb_used = false as there are many cases where BnB is not used (Andrew Chow) Pull request description: bitcoin#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. ACKs for top commit: Sjors: 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: ACK eadd130 instagibbs: utACK bitcoin@eadd130 Tree-SHA512: 7286c321f78666eea558cc591174630d210263594df41cab1065417510591ee514ade0e1d0cec8af09a785757da68de82592b013e8fe8d4966cec3254368706e

promag · 2019-12-01T21:12:21Z

@Sjors thanks for the mention.

Code review ACK eadd130.

…eeFromOutputs Summary: eadd1304c81e0b89178e4cc7630bd31650850c85 tests: Add a test for funding with sufficient preset inputs and subtractFeeFromOutputs (Andrew Chow) ff330badd45067cb520b1cfa1844f60a4c9f2031 Default to bnb_used = false as there are many cases where BnB is not used (Andrew Chow) Pull request description: #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. Backport of Core [[bitcoin/bitcoin#17568 | PR17568]] Depends on D7688 Test Plan: ``` ninja check test_runner.py rpc_fundrawtransaction ``` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D7691

…btractFeeFromOutputs eadd130 tests: Add a test for funding with sufficient preset inputs and subtractFeeFromOutputs (Andrew Chow) ff330ba Default to bnb_used = false as there are many cases where BnB is not used (Andrew Chow) Pull request description: bitcoin#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. ACKs for top commit: Sjors: 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: ACK eadd130 instagibbs: utACK bitcoin@eadd130 Tree-SHA512: 7286c321f78666eea558cc591174630d210263594df41cab1065417510591ee514ade0e1d0cec8af09a785757da68de82592b013e8fe8d4966cec3254368706e

fanquake added the Wallet label Nov 23, 2019

fanquake requested a review from meshcollider November 23, 2019 03:36

meshcollider reviewed Nov 23, 2019

View reviewed changes

achow101 force-pushed the fix-bnb-preset-sub branch from fd05e41 to 9b40c0d Compare November 23, 2019 04:35

achow101 force-pushed the fix-bnb-preset-sub branch from 9b40c0d to d3ca0b4 Compare November 25, 2019 07:23

elichai mentioned this pull request Nov 25, 2019

Run tests in memory sanitizer #17460

Closed

darosior approved these changes Nov 25, 2019

View reviewed changes

instagibbs reviewed Nov 26, 2019

View reviewed changes

achow101 force-pushed the fix-bnb-preset-sub branch from d3ca0b4 to 16d1e12 Compare November 26, 2019 18:01

achow101 added 2 commits November 26, 2019 13:02

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

ff330ba

…used

tests: Add a test for funding with sufficient preset inputs and subtr…

eadd130

…actFeeFromOutputs

achow101 force-pushed the fix-bnb-preset-sub branch from 16d1e12 to eadd130 Compare November 26, 2019 18:02

Sjors mentioned this pull request Nov 27, 2019

MemorySanitizer complains about unitialized variable in RPC method names #17620

Closed

Sjors mentioned this pull request Nov 29, 2019

Suppress false positive warning about uninitialized entropy buffers #17627

Closed

fanquake mentioned this pull request Nov 30, 2019

tests: Add "make check-valgrind" to run the unit tests under Valgrind #17639

Closed

fanquake approved these changes Dec 1, 2019

View reviewed changes

fanquake merged commit eadd130 into bitcoin:master Dec 1, 2019

MarkLTZ mentioned this pull request Apr 4, 2020

Bitcoin PR tracking litecoinz-core/litecoinz#14

Closed

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

Conversation

achow101 commented Nov 23, 2019

Uh oh!

meshcollider Nov 23, 2019

Choose a reason for hiding this comment

Uh oh!

achow101 Nov 23, 2019

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Nov 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

meshcollider commented Nov 23, 2019

Uh oh!

achow101 commented Nov 23, 2019

Uh oh!

laanwj commented Nov 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

darosior commented Nov 23, 2019

Uh oh!

meshcollider commented Nov 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

achow101 commented Nov 25, 2019

Uh oh!

meshcollider commented Nov 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

darosior left a comment

Choose a reason for hiding this comment

Uh oh!

Sjors commented Nov 25, 2019

Uh oh!

instagibbs left a comment

Choose a reason for hiding this comment

Uh oh!

achow101 commented Nov 26, 2019

Uh oh!

instagibbs commented Nov 26, 2019

Uh oh!

Sjors commented Nov 27, 2019

Uh oh!

fanquake left a comment

Choose a reason for hiding this comment

Uh oh!

promag commented Dec 1, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

DrahtBot commented Nov 23, 2019 •

edited

Loading

laanwj commented Nov 23, 2019 •

edited

Loading

meshcollider commented Nov 24, 2019 •

edited

Loading

meshcollider commented Nov 25, 2019 •

edited

Loading