wallet_bumpfee.py: Make sure coin selection produces change #15538

pull instagibbs wants to merge 1 commits into bitcoin:master from instagibbs:bumpfee_change_test changing 1 files +3 −1

instagibbs commented at 4:14 pm on March 5, 2019: member

I was hitting the case where change-less transactions were being made.
wallet_bumpfee.py: Make sure coin selection produces change 276972cb95
in test/functional/wallet_bumpfee.py:263 in 276972cb95
259@@ -260,7 +260,9 @@ def test_unconfirmed_not_spendable(rbf_node, rbf_node_address): 260 261 262 def test_bumpfee_metadata(rbf_node, dest_address): 263- rbfid = rbf_node.sendtoaddress(dest_address, Decimal("0.00100000"), "comment value", "to value") 264+ assert(rbf_node.getbalance() < 49)
ryanofsky commented at 4:49 pm on March 5, 2019:

This is pretty different from the other tests, and it took me a minute to figure out how it worked.

Would add comment that spend_one_input can’t be used here because it doesn’t create metadata, so getbalance check and generate call are needed to create a transaction with metadata that is guaranteed to have a change output.

MarcoFalke commented at 5:24 pm on March 5, 2019:

I’d prefer to use the assert_bla helpers. Otherwise this is extremely difficult to debug in case of a failure.

ryanofsky commented at 12:28 pm on March 6, 2019:

re: #15538 (review)

I’d prefer to use the assert_bla helpers. Otherwise this is extremely difficult to debug in case of a failure.

This is making an assertion about the test environment, and if it fails it probably means there is a bug in the test setup, not a bug in bitcoin. So I thought plain assert was reasonable here. Could go either way though. (For comparison, google gunit test framework has separate ASSERT_TRUE, ASSERT_EQ macros you’re supposed to use for test assertions, and CHECK, CHECK_EQ macros you’re supposed to use for normal code and test setup assertions).
ryanofsky approved
ryanofsky commented at 5:04 pm on March 5, 2019: member

utACK 276972cb95ba944a7a4b1858a08d333962261396
MarcoFalke commented at 5:23 pm on March 5, 2019: member

Have you hit this issue in the wild? If so, mind to include a traceback?
fanquake added the label Tests on Mar 5, 2019
promag commented at 12:11 pm on March 6, 2019: member

utACK after @MarcoFalke #15538 (review).

IIRC it was already suggested bump fee could add an input to support the increased fee - which could result in a change output. Is there something wrong with this approach?
ryanofsky commented at 12:18 pm on March 6, 2019: member

IIRC it was already suggested bump fee could add an input to support the increased fee - which could result in a change output. Is there something wrong with this approach?

It’s a good approach, but this PR is making a simple 3 line change to fix an invalid assumption made by a python test. Changing bumpfee to add inputs would be adding a new feature and would be a pretty indirect way of fixing a broken test.
instagibbs commented at 12:27 pm on March 6, 2019: member

It’s a feature I’ve wanted to do for a while but it required significant refactoring to do it the Right Way previously. Hoping someone in the residency can tackle it maybe.

On Wed, Mar 6, 2019, 7:20 AM Russell Yanofsky notifications@github.com wrote:

IIRC it was already suggested bump fee could add an input to support the increased fee - which could result in a change output. Is there something wrong with this approach?

It’s a good approach, but this PR is making a simple 3 line change to fix an invalid assumption made by a python test. Changing bumpfee to add inputs would be adding a new feature and would be a pretty indirect way of fixing a broken test.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/15538#issuecomment-470086701, or mute the thread https://github.com/notifications/unsubscribe-auth/AFgC0w0dNJtuEYV3Xl3yIYJAz4eUcbAYks5vT7JygaJpZM4be90y .
promag commented at 12:27 pm on March 6, 2019: member

@ryanofsky sure, not saying otherwise!
laanwj merged this on Jul 2, 2019
laanwj closed this on Jul 2, 2019
laanwj referenced this in commit 6c21a801f3 on Jul 2, 2019
sidhujag referenced this in commit 4b40a16010 on Jul 3, 2019
DrahtBot locked this on Dec 16, 2021

Contributors
instagibbs ryanofsky MarcoFalke promag

Labels
Tests