test: relax bumpfee dust_to_fee txsize an extra vbyte #18516

jonatack commented at 3:26 PM on April 3, 2020: member

Hopefully closes #18511 by allowing the transaction size to be 140-141 vbytes rather than strictly 141, and bumps with a slightly larger fee to ensure dust in the 140 vbyte case.

DrahtBot added the label Tests on Apr 3, 2020

in test/functional/wallet_bumpfee.py:256 in e7a1154d7c outdated

 252 | @@ -253,12 +253,13 @@ def test_dust_to_fee(self, rbf_node, dest_address):
 253 |      self.log.info('Test that bumped output that is dust is dropped to fee')
 254 |      rbfid = spend_one_input(rbf_node, dest_address)
 255 |      fulltx = rbf_node.getrawtransaction(rbfid, 1)
 256 | -    # size of transaction (p2wpkh, 1 input, 2 outputs): 141 vbytes
 257 | -    assert_equal(fulltx["vsize"], 141)
 258 | -    # bump with fee_rate of 0.00350000 BTC per 1000 vbytes
 259 | -    # expected bump fee of 141 vbytes * fee_rate 0.00350000 BTC / 1000 vbytes = 0.00049350 BTC
 260 | +    # expected size of transaction (p2wpkh, 1 input, 2 outputs): 140-141 vbytes

instagibbs commented at 4:34 PM on April 3, 2020:

please explain why the slack(is it the DER encoded sig length difference in witness?)

jonatack commented at 10:57 AM on April 6, 2020:

good idea -- done

in test/functional/wallet_bumpfee.py:257 in e7a1154d7c outdated

 252 | @@ -253,12 +253,13 @@ def test_dust_to_fee(self, rbf_node, dest_address):
 253 |      self.log.info('Test that bumped output that is dust is dropped to fee')
 254 |      rbfid = spend_one_input(rbf_node, dest_address)
 255 |      fulltx = rbf_node.getrawtransaction(rbfid, 1)
 256 | -    # size of transaction (p2wpkh, 1 input, 2 outputs): 141 vbytes
 257 | -    assert_equal(fulltx["vsize"], 141)
 258 | -    # bump with fee_rate of 0.00350000 BTC per 1000 vbytes
 259 | -    # expected bump fee of 141 vbytes * fee_rate 0.00350000 BTC / 1000 vbytes = 0.00049350 BTC
 260 | +    # expected size of transaction (p2wpkh, 1 input, 2 outputs): 140-141 vbytes
 261 | +    assert(140 <= fulltx["vsize"] <= 141)

instagibbs commented at 4:34 PM on April 3, 2020:

assert 140 <= fulltx["vsize"] <= 141 I think is the style used

jonatack commented at 10:58 AM on April 6, 2020:

done

jonatack force-pushed on Apr 6, 2020

jonatack commented at 11:04 AM on April 6, 2020: member

Thanks for reviewing, @instagibbs. Updated with your feedback.

in test/functional/wallet_bumpfee.py:257 in b631556d65 outdated

 258 | -    # bump with fee_rate of 0.00350000 BTC per 1000 vbytes
 259 | -    # expected bump fee of 141 vbytes * fee_rate 0.00350000 BTC / 1000 vbytes = 0.00049350 BTC
 260 | -    # but dust is dropped, so actual bump fee is 0.00050000
 261 | -    bumped_tx = rbf_node.bumpfee(rbfid, {"fee_rate": 0.0035})
 262 | +    # The DER formatting used by Bitcoin to serialize ECDSA signatures means signatures can have
 263 | +    # a variable size of 70-72 bytes, with most being 71 or 72 bytes. The signature in the witness

jnewbery commented at 1:59 PM on April 6, 2020:

Pedantically, the signature size can be smaller than 70 bytes, but with very small probability :)

jonatack commented at 3:09 PM on April 6, 2020:

changed to "70-72 bytes (or possibly even less),"

in test/functional/wallet_bumpfee.py:258 in b631556d65 outdated

 259 | -    # expected bump fee of 141 vbytes * fee_rate 0.00350000 BTC / 1000 vbytes = 0.00049350 BTC
 260 | -    # but dust is dropped, so actual bump fee is 0.00050000
 261 | -    bumped_tx = rbf_node.bumpfee(rbfid, {"fee_rate": 0.0035})
 262 | +    # The DER formatting used by Bitcoin to serialize ECDSA signatures means signatures can have
 263 | +    # a variable size of 70-72 bytes, with most being 71 or 72 bytes. The signature in the witness
 264 | +    # is divided by 4 for the vsize, so this variance can take it across a vbyte boundary. Thus

jnewbery commented at 2:24 PM on April 6, 2020:

nit: this variance can take ~it~ the weight across a 4-byte boundary.

The witness discount is applied to all bytes that don't appear in the non-segwit serialization:

1 byte (marker 0x00)
1 byte (flag 0x01)
1 byte (number of stack items 0x02)
71/72 bytes (sig length + signature 0x46..../0x47....)
34 bytes (pubkey length _ pubkey 0x21....)

TOTAL: 108/109 bytes weight => 27/28 vbytes vsize

ref: https://github.com/bitcoin/bips/blob/master/bip-0141.mediawiki#transaction-id

jonatack commented at 3:09 PM on April 6, 2020:

good point, updated!

in test/functional/wallet_bumpfee.py:260 in b631556d65 outdated

 261 | -    bumped_tx = rbf_node.bumpfee(rbfid, {"fee_rate": 0.0035})
 262 | +    # The DER formatting used by Bitcoin to serialize ECDSA signatures means signatures can have
 263 | +    # a variable size of 70-72 bytes, with most being 71 or 72 bytes. The signature in the witness
 264 | +    # is divided by 4 for the vsize, so this variance can take it across a vbyte boundary. Thus
 265 | +    # the expected transaction size (p2wpkh, 1 input, 2 outputs) is 140-141 vbytes -- usually 141.
 266 | +    assert 140 <= fulltx["vsize"] <= 141

jnewbery commented at 2:25 PM on April 6, 2020:

If this fails, we don't get any useful output (the assert return value is False). If you wanted to be really verbose, you could do something like:

if fulltx['vsize'] not in [140, 141]:
    print("Invalid tx size: {}, full tx: {}".format(fulltx['vsize'], fulltx))
    raise AssertionError

That's perhaps overkill, but more verbose logging makes debugging intermittent failures much easier.

jonatack commented at 3:12 PM on April 6, 2020:

Good point; thanks for the suggestion. Updated (with the x <= y <= z operator as it apparently runs faster?)

jnewbery commented at 2:26 PM on April 6, 2020: member

ACK b631556d654c53281098aa62b2318181b0f592be

Good find, Jon. I have a few comments inline. Feel free to take or leave them. None of them should stop this improvement being merged.

test: relax bumpfee dust_to_fee txsize an extra vbyte

and add explanatory documentation for the reasoning.

25e03ba1ff

jonatack force-pushed on Apr 6, 2020

jonatack commented at 3:13 PM on April 6, 2020: member

Thanks for reviewing, @jnewbery! Great points; updated.

jnewbery commented at 3:33 PM on April 6, 2020: member

utACK 25e03ba1ff18ca06954786e512000648941b4dfb

MarcoFalke merged this on Apr 6, 2020

MarcoFalke closed this on Apr 6, 2020

in test/functional/wallet_bumpfee.py:262 in 25e03ba1ff

 263 | +    # variable size of 70-72 bytes (or possibly even less), with most being 71 or 72 bytes. The signature
 264 | +    # in the witness is divided by 4 for the vsize, so this variance can take the weight across a 4-byte
 265 | +    # boundary. Thus expected transaction size (p2wpkh, 1 input, 2 outputs) is 140-141 vbytes, usually 141.
 266 | +    if not 140 <= fulltx["vsize"] <= 141:
 267 | +        print("Error: Invalid tx vsize of {} (140-141 expected), full tx: {}".format(fulltx["vsize"], fulltx))
 268 | +        raise AssertionError

MarcoFalke commented at 3:40 PM on April 6, 2020:

        raise AssertionError("Error: Invalid tx vsize of {} (140-141 expected), full tx: {}".format(fulltx["vsize"], fulltx))

Generally we put the fail reason in the assertion instead of printing it to stdout, but I'll got ahead and merge this now, to get some more green in the ci.

jonatack commented at 3:56 PM on April 6, 2020:

Generally we put the fail reason in the assertion instead of printing it to stdout, but I'll got ahead and merge this now, to get some more green in the ci.

Thanks -- noting that for the future.

jnewbery commented at 4:00 PM on April 6, 2020:

oops, sorry - yes this should have been a log (or as Marco says, in the assertion)

MarcoFalke commented at 4:19 PM on April 6, 2020:

Maybe a small fixup pr would be appropriate?

jonatack commented at 4:41 PM on April 6, 2020:

Ok -- done in #18540

jonatack deleted the branch on Apr 6, 2020

MarcoFalke referenced this in commit 54d5ba3d9c on Apr 6, 2020

DrahtBot locked this on Feb 15, 2022