96da68a38f qa: functional test a transaction running into the legacy sigop limit (Antoine Poinsot)
367147954d qa: unit test standardness of inputs packed with legacy sigops (Antoine Poinsot)
5863315e33 policy: make pathological transactions packed with legacy sigops non-standard. (Antoine Poinsot)
Pull request description:
The Consensus Cleanup soft fork proposal includes a limit on the number of legacy signature
operations potentially executed when validating a transaction. If this change is to be implemented
here and activated by Bitcoin users in the future, we should make transactions that are not valid
according to the new rules non-standard first because it would otherwise be a trivial DoS to
potentially unupgraded miners after the soft fork activates.
ML post: https://gnusha.org/pi/bitcoindev/49dyqqkf5NqGlGdinp6SELIoxzE_ONh3UIj6-EB8S804Id5yROq-b1uGK8DUru66eIlWuhb5R3nhRRutwuYjemiuOOBS2FQ4KWDnEh0wLuA=@protonmail.com/T/#u
ACKs for top commit:
instagibbs:
reACK 96da68a38f
maflcko:
review ACK 96da68a38f🚋
achow101:
ACK 96da68a38f
glozow:
light code review ACK 96da68a38f, looks correct to me
Tree-SHA512: 106ffe62e48952affa31c5894a404a17a3b4ea8971815828166fba89069f757366129f7807205e8c6558beb75c6f67d8f9a41000be2f8cf95be3b1a02d87bfe9
It's useful to have an end-to-end test in addition to the unit test to sanity check the RPC error as
well as making sure the transaction is otherwise fully standard.
useful to easily create transactions with same txid, different
wtxid and valid witness for testing scenarios in other places
(ex: private broadcast connections)
The helper assumes that the n and k values have to be provided as a
single byte push operation, which is only possible for values up to 16.
Fix that by passing the numbers directly to the CScript list, where it's
automatically converted to minimally-encoded pushes (see class
method `CScript.__coerce_instance`, branch `isinstance(other, int)`).
In case of 17..20, this means that the data-pushes are done with two
bytes using OP_PUSH1 (0x01), e.g. for n=20: 0x01,0x14
Since the original fix was set to be a "reasonable" transaction
to reduce allocations and the true motivation later revealed,
it makes sense to relax this check to something more principled.
There are more exotic transaction patterns that could take advantage
of a relaxed requirement, such as 1 input, 1 output OP_RETURN to burn
a utxo to fees for CPFP purposes when change isn't practical.
Two changes could be accomplished:
1) Anything not 64 bytes could be allowed
2) Anything above 64 bytes could be allowed
In the Great Consensus Cleanup, suggestion (2) was the route taken.
It would not allow an "empty" OP_RETURN
but would reduce the required padding from 22 bytes to 5.
The functional test is also modified to test the actual case
we care about: 64 bytes
The previous diff touched most files in ./test/, so bump the headers to
avoid having to touch them again for a bump later.
-BEGIN VERIFY SCRIPT-
./contrib/devtools/copyright_header.py update ./test/
-END VERIFY SCRIPT-
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
