Make bumpfee tests less fragile #9701

ryanofsky commented at 11:55 PM on February 6, 2017: member

Fix test fragility described in #8456 (comment)

I ran into a lot of spurious errors related to fundrawtransaction in the bumpfee tests while trying to add a simple test for this. fundrawtransaction would sometimes chose different sized inputs than the small_output_fails and dust_to_fee tests were expecting, so I rewrote these tests in 13960f870c1ba94f391012f9c9243c59951ea6ab to manually create RBF transactions with the right inputs.

I also ran into problems with fundrawtransaction using up all the right-sized inputs before these tests could use them, so I changed the order of the tests so they would run before any fundrawtransaction calls.

I also ran into problems with fundrawtransaction sometimes chosing a higher fee for the RBF transaction than the test_rebumping test expected, causing the bumpfee command in the test to fail with an "Invalid totalFee, must be at least ..." error. I did not figure out what was causing this, but it seemed to somehow be related to the number of peer_node.sendtoaddress(rbf_node_address, 0.001) calls made during the test setup.

fanquake added the label Tests on Feb 7, 2017

sdaftuar commented at 7:45 PM on February 23, 2017: member

utACK 145b702

in qa/rpc-tests/bumpfee.py:None in 145b702e5a outdated

 275 | -
 276 | -def spend_one_input(node, input_amount, outputs):
 277 | -    input = dict(sequence=BIP125_SEQUENCE_NUMBER, **next(u for u in node.listunspent() if u["amount"] == input_amount))
 278 | -    rawtx = node.createrawtransaction([input], outputs)
 279 | +def spend_one_input(node, dest_address):
 280 | +    input = dict(

jnewbery commented at 3:57 PM on March 9, 2017:

I know this pre-dates your PR, but since you're changing the line, can you change this to not use input as a variable name. input is a built-in Python function. Perhaps txin is better?

ryanofsky commented at 2:22 PM on March 16, 2017:

Renamed input->tx_input in 8ecec5f75a8b527194275835cbc0177a6ae99b3a.

in qa/rpc-tests/bumpfee.py:None in 145b702e5a outdated

 212 | +    feerate = Decimal("0.00025000")
 213 | +    rbf_node.settxfee(feerate)
 214 |      bumped_tx = rbf_node.bumpfee(rbfid)
 215 | -    assert bumped_tx["fee"] > 2 * abs(rbftx["fee"])
 216 | +    bumped_len = len(rbf_node.getrawtransaction(bumped_tx["txid"])) // 2
 217 | +    assert_greater_than(Decimal("0.00001000"), abs(feerate - bumped_tx["fee"] * 1000 / bumped_len))

jnewbery commented at 4:41 PM on March 9, 2017:

I see that you've tried to clarify what this assert is doing, but it's still pretty obscure to me. Why should the difference between the desired feerate and the actual feerate be at least 1000 satoshis/byte? Can you add a comment explaining?

I also think that this inequality is more clearly expressed as actual_feerate >= desired_feerate + 1000 (ie the thing we're asserting is on the left side of the inequality).

ryanofsky commented at 2:22 PM on March 16, 2017:

It's checking that difference between requested and actual feerate is less than 1000, not greater than 1000. Replaced 1000 with epsilon variable to maybe make comparison clearer in 779ea9c214c1cf508efea59b8ebaaec424fdac44.

in qa/rpc-tests/bumpfee.py:None in 145b702e5a outdated

 211 | -    rbf_node.settxfee(Decimal("0.00002500"))
 212 | +    feerate = Decimal("0.00025000")
 213 | +    rbf_node.settxfee(feerate)
 214 |      bumped_tx = rbf_node.bumpfee(rbfid)
 215 | -    assert bumped_tx["fee"] > 2 * abs(rbftx["fee"])
 216 | +    bumped_len = len(rbf_node.getrawtransaction(bumped_tx["txid"])) // 2

jnewbery commented at 4:45 PM on March 9, 2017:

I think this is more clearly expressed as:

bumped_len = rbf_node.getrawtransaction(bumped_tx["txid"], True)["size"] (ie not relying on size being the length of the serialized string // 2)

MarcoFalke commented at 12:10 PM on March 12, 2017:

There is a count_bytes helper function in util, that also checks that the bytes are hex encoded.

ryanofsky commented at 2:22 PM on March 16, 2017:

Thanks, switched to count_bytes in 779ea9c214c1cf508efea59b8ebaaec424fdac44.

jnewbery commented at 4:53 PM on March 9, 2017: member

Looks great. Definitely an improvement.

I've added a few nits inline. My only other comment is that it'd be great if you could update the module docstring (after rebasing onto master) to say:

what the individual subtests are doing
the subtests consume discrete inputs, and there should be no shared state between subtests. They should be able to be run in any order (and if anyone adds additional subtests, they should follow that convention).

Tested ACK https://github.com/bitcoin/bitcoin/pull/9701/commits/145b702e5a2e2f6cf8766e070e25ee9a24ee931d

MarcoFalke commented at 12:05 PM on March 12, 2017: member

utACK 145b702. Needs rebase.

laanwj commented at 11:09 AM on March 14, 2017: member

utACK, needs rebase.

laanwj assigned laanwj on Mar 14, 2017

ryanofsky force-pushed on Mar 16, 2017

ryanofsky referenced this in commit 779ea9c214 on Mar 16, 2017

ryanofsky force-pushed on Mar 16, 2017

ryanofsky commented at 3:00 PM on March 16, 2017: member

Rebased 145b702e5a2e2f6cf8766e070e25ee9a24ee931d -> 68a49603eea561d7c38c4d7fc7d7550e8622e1dd (pr/bumpfee-fragile.0 -> pr/bumpfee-fragile.1) Squashed 8ecec5f75a8b527194275835cbc0177a6ae99b3a -> 45b060ef9d00f17e639c0e5d76500e5587bcc6d0 (pr/bumpfee-fragile.2 -> pr/bumpfee-fragile.3)

jnewbery commented at 9:45 PM on March 16, 2017: member

tested ACK. I'd still prefer to see:

a comment in the docstring about the subtests consuming discrete inputs, and that there should be no shared state between subtests.
a comment explaining why epsilon is 1000. What is this testing?
the call to count_bytes removed and using the RPC to get size directly. The test here is for the feerate, not that getrawtranasaction can correctly serialize into a hex string.

ryanofsky referenced this in commit 5876a4da9b on Mar 20, 2017

ryanofsky force-pushed on Mar 20, 2017

ryanofsky commented at 9:21 PM on March 20, 2017: member

Dropped count_bytes and clarified the settxfee assertion in 5876a4da9bf93c32efc42ebaf3c11f639608f8ec.
Expanded docstring in 74d74d9f68ed3a5dd9fd268fdb0ef5985ae670a0.
Squashed 74d74d9f68ed3a5dd9fd268fdb0ef5985ae670a0 -> a641eba17ae97ff80d7d1c272b7f86f94dfc70fd (pr/bumpfee-fragile.4 -> pr/bumpfee-fragile.5)

[qa] Remove bumpfee.py get_change_address hack 94b528bb0c

[qa] Get rid of nondeterminism in bumpfee.py

Change bumpfee tests to use the spend_one_input function instead of the
create_fund_sign_send function. The latter function would choose transaction
inputs and fees in unpredictable ways depending on the order that tests ran,
which meant that adding new tests could cause old tests to fail, and in general
made bumpfee.py fragile and unpleasant to work with.

e6b2963241

[qa] Make bumpfee.py test function order consistent

Run bumpfee tests in top-down order, now that the test fragility is fixed, and
they can actually run in order.

1dfd64fadc

[qa] Rename python input variable to tx_input

input() is actually the name of a python built in function

0b94e49831

[qa] Expand bumpfee test docstring f85ac54e24

jnewbery commented at 1:52 PM on March 21, 2017: member

retested ACK https://github.com/bitcoin/bitcoin/pull/9701/commits/a641eba17ae97ff80d7d1c272b7f86f94dfc70fd. Changes look good. Thanks!

ryanofsky force-pushed on Mar 21, 2017

ryanofsky commented at 3:36 PM on March 21, 2017: member

Rebased a641eba17ae97ff80d7d1c272b7f86f94dfc70fd -> f85ac54e2429ad70c8bd4b2dbdf9d4107fd81f3f (pr/bumpfee-fragile.5 -> pr/bumpfee-fragile.6) because of renames in #9956.

MarcoFalke merged this on Mar 23, 2017

MarcoFalke closed this on Mar 23, 2017

MarcoFalke referenced this in commit a230b05887 on Mar 23, 2017

DrahtBot locked this on Sep 8, 2021