test: fix "tx-size-small" errors after default address change #17108

theStack commented at 1:09 PM on October 11, 2019: member

Addresses #17043, affects RBF and BIP68 functional tests.

The "tx-size-small" policy rule rejects transactions with a non-witness size of smaller than 82 bytes (see src/validation.cpp:MemPoolAccept::PreChecks(...)), which corresponds to a transaction with 1 segwit input and 1 P2WPKH output.

Through the default address change, the created test transactions have segwit inputs now and sending to short scriptPubKeys might violate this rule. By bumping the dummy scriptPubKey size to 22 bytes (= the size of a P2WPKH scriptPubKey), on all occurences the problem is solved.

The dummy scriptPubKey has the format: 21 <21-byte-long string of 'a' or 1s>

fanquake added the label Tests on Oct 11, 2019

instagibbs commented at 1:19 PM on October 11, 2019: member

wonder if it makes sense to have a DUMMY_P2WPKH_SCRIPT constant and just stick those everywhere instead?

MarcoFalke commented at 1:22 PM on October 11, 2019: member

By bumping the dummy scriptPubKey size to 22 bytes (= the size of a P2WPKH scriptPubKey), on all occurences the problem is solved.

Are you sure on that? While the scriptPubKey has the same size as a P2WPKH, you don't know the size of the scriptSig (which could be empty, or OP_TRUE).

instagibbs commented at 1:39 PM on October 11, 2019: member

A normal "blank" 1 input, 1 output transaction is 60 bytes exactly. Padding by 22 should be enough to hit 82 byte minimum. This can even be dynamically generated by code to make sure it doesn't regress:

        script = b'a'*22
        txn = CTransaction()
        txn.vin.append(CTxIn())
        txn.vout.append(CTxOut(0, script))
        assert_equal(len(txn.serialize()), 82)

theStack commented at 1:54 PM on October 11, 2019: member

wonder if it makes sense to have a DUMMY_P2WPKH_SCRIPT constant and just stick those everywhere instead?

Sounds good, is this still a small enough change to include it in this same PR as well? I guess it would also make sense then to have a constant for all the CScript([b'a' * 35]) scriptPubkeys which are used a lot in the same files -- even though I am not sure where the magic number 35 comes from in this case, doesn't seem to fit to any address type length.

MarcoFalke commented at 1:55 PM on October 11, 2019: member

@instagibbs Right. Forgot that the outpoint need to be serialized as well :woman_facepalming: @theStack the 35 was invented by me, because I can't do math. Feel free to fix

instagibbs commented at 2:00 PM on October 11, 2019: member

@theStack 22 is more correct, I'd just use that in this same PR(as some sort of constant). Makes this code easier to understand/update in the future.

theStack commented at 2:26 PM on October 11, 2019: member

By bumping the dummy scriptPubKey size to 22 bytes (= the size of a P2WPKH scriptPubKey), on all occurences the problem is solved.

Are you sure on that? While the scriptPubKey has the same size as a P2WPKH, you don't know the size of the scriptSig (which could be empty, or OP_TRUE).

Yes, because the 82-byte limit to trigger the "tx-size-small" error is already calculated with the smallest possible input, i.e. with an empty scriptSig. A dissection on what the limit consists of (maybe interesting for other readers/reviewers):

Tx Header/Skeleton (4 [Version] + 1 [In-Counter] + 1 [Out-Counter] + 4 [LockTime]) => 10 bytes
Input (32 [PrevTxHash] + 4 [Index] + 1 [scriptSigLen] + 4 [SeqNo]) => 41 bytes
Output (8 [Amount] + 1 [scriptPubKeyLen] + 22 [scriptPubKey]) => 31 bytes

Total: 82 bytes

It took me a while to find out that the witness-flag (2 bytes) in the transaction header doesn't count into the non-witness size (quite obvious though to either have both the flag and the witnesses or none of them). @instagibbs @MarcoFalke: Okay, will introduce a new constant DUMMY_P2WPKH_SCRIPT then and use it both for the bumped formerly short scriptPubKeys as well as for the CScript[b'a' * 35]) ones.

MarcoFalke commented at 2:28 PM on October 11, 2019: member

Sounds good!

theStack commented at 3:24 PM on October 11, 2019: member

Done (https://github.com/bitcoin/bitcoin/pull/17108/commits/510a219db8b15729e655a62bedac23f99264f0e3, https://github.com/bitcoin/bitcoin/pull/17108/commits/76f764bc636736b34fb41ed4255000515e0aa990) with two uncertainties though:

Is it worth it to put the common constant DUMMY_P2WPKH_SCRIPT into a shared file? If yes, where would it fit? script.py? (Haven't found any shared constant scripts in the code)
In the function feature_rbf.py:test_simple_doublespend(), two different pubKeyScripts are expected for tx1a.vout and tx1b.vout, otherwise the test fails (by not throwing the expected "unsufficient fee" exception). I solved this by just adding another byte to the script constant in the second case; would it be worth it to have another constant like DUMMY2_P2WPKH_SCRIPT? Or any other suggestion?

in test/functional/feature_rbf.py:16 in 76f764bc63 outdated

  12 | @@ -13,10 +13,12 @@
  13 |  
  14 |  MAX_REPLACEMENT_LIMIT = 100
  15 |  
  16 | +DUMMY_P2WPKH_SCRIPT = CScript([bytes([1]) * 21])

instagibbs commented at 3:25 PM on October 11, 2019:

to be clear: P2WPKH scripts are 22 length scripts: OP_0 <PUSH 20> <20-bytes>

theStack commented at 3:31 PM on October 11, 2019:

Yes, the 21 is chosen by purpose, as the constructor seems to add an implicit push instruction here, resulting in the desired 22 bytes -- printing length and content of DUMMY_P2WPKH_SCRIPT gives:

22

b'\x15\x01\x01\x01\x01\x01\x01\x01\x01\x01\x01\x01\x01\x01\x01\x01\x01\x01\x01\x01\x01\x01'

Should I maybe add a comment noting that the result is 22 bytes?

instagibbs commented at 3:39 PM on October 11, 2019:

I'd be very surprised that the leading byte isn't the serialize byte for the entire script

theStack commented at 4:22 PM on October 11, 2019:

As far as I can see the length of a scriptPubKey is serialized in class TxOut (respectively for scriptSig in class TxIn) rather directly in the class CScript itself, see https://github.com/bitcoin/bitcoin/blob/master/test/functional/test_framework/messages.py#L337 and https://github.com/bitcoin/bitcoin/blob/master/test/functional/test_framework/messages.py#L89.

Quick check in the python console:

>>> from test_framework.messages import *
>>> from test_framework.script import *
>>> txn = CTransaction()
>>> txn.vin.append(CTxIn())
>>> txn.vout.append(CTxOut(0, CScript([b'a' * 21])))
>>> len(txn.serialize())
82
>>> txn.serialize()[47:]
b'\x00\x00\x00\x00\x00\x00\x00\x00\x16\x15aaaaaaaaaaaaaaaaaaaaa\x00\x00\x00\x00'

The output hexdump shows: <8-byte amount> <1-byte length (22)> <PUSH 21> <string of 21 'a's> ....

instagibbs commented at 4:27 PM on October 11, 2019:

+        # Transaction size
+        txn = CTransaction()
+        txn.vin.append(CTxIn())
+        txn.vout.append(CTxOut(0, b''))
+        print("txn size: " +str(len(txn.serialize())))
+        return
------
txn size: 60

:thinking:

instagibbs commented at 4:29 PM on October 11, 2019:

        # Transaction size
        txn = CTransaction()
        txn.vin.append(CTxIn())
        txn.vout.append(CTxOut(0, b'a'*21))
        print("txn size: " +str(len(txn.serialize())))
        return
-----
txn size: 81

instagibbs commented at 4:29 PM on October 11, 2019:

ah! I'm sorry you're using a different constructor, continue :)

theStack commented at 4:43 PM on October 11, 2019:

I actually tried the simpler "pure" construction of the script with just e.g. b'a' * 22 but then the tests would fail because the script contents don't make sense: test_framework.authproxy.JSONRPCException: non-mandatory-script-verify-flag (Data push larger than necessary) (code 64) (-26) or test_framework.authproxy.JSONRPCException: mandatory-script-verify-flag-failed (Script evaluated without error but finished with a false/empty top stack element) (code 16) (-26)

Another possibility would be to explicitely add the push, like DUMMY_P2WPKH_SCRIPT = bytes([21]) + b'a' * 21

instagibbs commented at 4:54 PM on October 11, 2019:

right it's not even a parseable script

in test/functional/feature_bip68_sequence.py:29 in 76f764bc63 outdated

  25 | @@ -26,13 +26,14 @@
  26 |  # RPC error for non-BIP68 final transactions
  27 |  NOT_FINAL_ERROR = "non-BIP68-final (code 64)"
  28 |  
  29 | +DUMMY_P2WPKH_SCRIPT = CScript([b'a' * 21])

MarcoFalke commented at 5:21 PM on October 11, 2019:

Could add a comment here to explain what this is for less gifted people (like me). Maybe take this: #17108 (comment)

MarcoFalke commented at 5:23 PM on October 11, 2019:

Could move this to a common place like ./test/functional/test_framework/util.py and import it from there?

theStack commented at 6:50 PM on October 11, 2019:

Okay, will add an explanation.

theStack commented at 7:02 PM on October 11, 2019:

The misery is that I can't find an appropriate place where to put the constant into. Importing the module script in util.py leads to a circular dependency (script imports messages, which in turn imports util), while declaring the constant in script.py doesn't work as on top CScript is not defined yet. Any other module within test_framework doesn't seem to fit thematically. Any suggestion?

MarcoFalke commented at 7:25 PM on October 11, 2019:

You can create a new file?

in test/functional/feature_bip68_sequence.py:16 in a51fe188e5 outdated

  16 | @@ -17,6 +17,7 @@
  17 |      satoshi_round,

MarcoFalke commented at 8:25 PM on October 11, 2019:

test/functional/feature_bip68_sequence.py:11:1: F401 'test_framework.script.CScript' imported but unused

theStack force-pushed on Oct 11, 2019

theStack force-pushed on Oct 13, 2019

theStack commented at 2:57 PM on October 13, 2019: member

Squashed (https://github.com/bitcoin/bitcoin/commit/f2240dbcdae9c5b9a566a916c0c97ba0ae71a60a).

in test/functional/test_framework/script_util.py:19 in f2240dbcda outdated

  14 | +# Tx Skeleton: 4 [Version] + 1 [InCount] + 1 [OutCount] + 4 [LockTime] = 10 bytes
  15 | +# Blank Input: 32 [PrevTxHash] + 4 [Index] + 1 [scriptSigLen] + 4 [SeqNo] = 41 bytes
  16 | +# Output:      8 [Amount] + 1 [scriptPubKeyLen] = 9 Bytes
  17 | +#
  18 | +# Hence, the scriptPubKey of the single output has to have a size of at
  19 | +# least 22 Bytes, which corresponds to the size of a P2WPKH scriptPubKey.

instagibbs commented at 12:52 PM on October 14, 2019:

s/Byes/bytes/

theStack commented at 1:04 PM on October 14, 2019:

fixed, as well as three lines before at the end of "Output:"

instagibbs approved

instagibbs commented at 12:53 PM on October 14, 2019: member

utACK https://github.com/bitcoin/bitcoin/pull/17108/commits/f2240dbcdae9c5b9a566a916c0c97ba0ae71a60a

test: fix "tx-size-small" errors after default address change

Addresses #17043, affects RBF and BIP68 functional tests.

The "tx-size-small" policy rule rejects transactions with a non-witness size of
smaller than 82 bytes (see src/validation.cpp:MemPoolAccept::PreChecks(...)),
which corresponds to a transaction with 1 segwit input and 1 P2WPKH output.

Through the default address change, the created test transactions have segwit
inputs now and sending to short scriptPubKeys might violate this rule. By
bumping the dummy scriptPubKey size to 22 bytes (= the size of a P2WPKH
scriptPubKey), on all occurences the problem is solved.

The dummy scriptPubKey has the format:
    21 <21-byte-long string of 'a' or 1s>

former commit messages, now squashed:
test: rbf, bip68: use constant DUMMY_P2WPKH_SCRIPT for bumped scriptPubKey
test: rbf, bip68: use constant DUMMY_P2WPKH_SCRIPT for dummy scriptPubKeys (b'a' * 35)
test: rbf, bip68: comment DUMMY_P2WPKH_SCRIPT constant, put into common (new) module

32d665c265

theStack force-pushed on Oct 14, 2019

instagibbs commented at 1:33 PM on October 14, 2019: member

reACK https://github.com/bitcoin/bitcoin/pull/17108/commits/32d665c2657793c8b2cc7248d26d80a940acfe20 just s/Bytes/bytes/

MarcoFalke commented at 2:14 PM on October 14, 2019: member

ACK 32d665c2657793c8b2cc7248d26d80a940acfe20

<details><summary>Show signature and timestamp</summary>

Signature:

Timestamp of file with hash 9069f393b525b1426fa16021a537bf8f61b38e8f25dd9ab80d5975a19fa053d0 -

</details>

MarcoFalke referenced this in commit 6c7da0736d on Oct 14, 2019

MarcoFalke merged this on Oct 14, 2019

MarcoFalke closed this on Oct 14, 2019

theStack deleted the branch on Dec 1, 2020

DrahtBot locked this on Feb 15, 2022