Page MenuHomePhabricator

make pad_tx produce the correct size when possible
ClosedPublic

Authored by PiRK on Thu, Oct 7, 10:04.

Details

Reviewers
Fabien
Group Reviewers
Restricted Project
Commits
rABCfbb20474303b: make pad_tx produce the correct size when possible
Summary

Previously, pad_tx was overpadding transactions by at least 1 byte. This was caused by at least 2 reasons:

  • the byte encoding the data length after the OP_RETURN was not taken into
  • the additional byte required by OP_PUSHDATA1 when the data length exceeded 0x4c was not taken into account

This is an attempt to make the resulting length match the required length in most situation.
Now, it should only differ in cases where the request size is less than one empty vout (10 bytes) above the current tx size.

Test Plan

ninja check-functional-extended

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

PiRK requested review of this revision.Thu, Oct 7, 10:04

refactor the unit test, less code repetition and additional sizes covered

PiRK planned changes to this revision.Thu, Oct 7, 14:28

Let's try to make it even more correct :)

PiRK edited the summary of this revision. (Show Details)

Make it even more correct.

The only sizes I cannot solve exactly is the first 10 bytes above the initial transaction size (the first empty OP_RETURN).

Remove unused constant
Use random.randbytes instead of building the bytes from a list of random random.randrange(0xff) integers. This is a significant performance improvement from my previous code, that brings the test run time down from 0.133 s to 0.036 s.

Tail of the build log:

[735/788] bitcoin-upgrade-activated: testing activation_tests
[736/788] Running utility command for check-bitcoin-upgrade-activated-getarg_tests
[737/788] Running utility command for check-bitcoin-upgrade-activated-activation_tests
[738/788] bitcoin-upgrade-activated: testing cashaddrenc_tests
[739/788] bitcoin: testing crypto_tests
[740/788] Running utility command for check-bitcoin-upgrade-activated-cashaddrenc_tests
[741/788] bitcoin: testing versionbits_tests
[742/788] Running utility command for check-bitcoin-crypto_tests
[743/788] bitcoin-upgrade-activated: testing bswap_tests
[744/788] bitcoin: testing script_tests
[745/788] Running utility command for check-bitcoin-versionbits_tests
[746/788] Running utility command for check-bitcoin-upgrade-activated-bswap_tests
[747/788] Running utility command for check-bitcoin-script_tests
[748/788] bitcoin-upgrade-activated: testing rpc_server_tests
[749/788] bitcoin-upgrade-activated: testing base58_tests
[750/788] Running utility command for check-bitcoin-upgrade-activated-rpc_server_tests
[751/788] bitcoin-upgrade-activated: testing checkdatasig_tests
[752/788] bitcoin: testing monolith_opcodes_tests
[753/788] leveldb: testing skiplist_test
[754/788] bitcoin: testing cashaddr_tests
[755/788] Running utility command for check-bitcoin-monolith_opcodes_tests
[756/788] Running utility command for check-bitcoin-upgrade-activated-base58_tests
[757/788] bitcoin: testing coins_tests
[758/788] bitcoin-upgrade-activated: testing addrman_tests
[759/788] leveldb: testing arena_test
[760/788] bitcoin-upgrade-activated: testing crypto_tests
[761/788] Running utility command for check-bitcoin-upgrade-activated-checkdatasig_tests
[762/788] Running utility command for check-bitcoin-cashaddr_tests
[763/788] Running utility command for check-bitcoin-upgrade-activated-addrman_tests
[764/788] Running utility command for check-bitcoin-coins_tests
[765/788] Running bitcoin test suite
PASSED: bitcoin test suite
[766/788] bitcoin-upgrade-activated: testing intmath_tests
[767/788] Running utility command for check-bitcoin-upgrade-activated-crypto_tests
[768/788] Running utility command for check-bitcoin-upgrade-activated-intmath_tests
[769/788] bitcoin-upgrade-activated: testing flatfile_tests
[770/788] leveldb: testing issue200_test
[771/788] Running leveldb test suite
PASSED: leveldb test suite
[772/788] bitcoin-upgrade-activated: testing checkpoints_tests
[773/788] Running utility command for check-bitcoin-upgrade-activated-flatfile_tests
[774/788] Running utility command for check-bitcoin-upgrade-activated-checkpoints_tests
[775/788] bitcoin-upgrade-activated: testing dbwrapper_tests
[776/788] Running utility command for check-bitcoin-upgrade-activated-dbwrapper_tests
[777/788] bitcoin-upgrade-activated: testing cuckoocache_tests
[778/788] Running utility command for check-bitcoin-upgrade-activated-cuckoocache_tests
[779/788] bitcoin-upgrade-activated: testing wallet_tests
[780/788] Running utility command for check-bitcoin-upgrade-activated-wallet_tests
[781/788] Linking CXX executable src/qt/test/test_bitcoin-qt
[782/788] bitcoin-qt: testing test_bitcoin-qt
[783/788] Running bitcoin-qt test suite
PASSED: bitcoin-qt test suite
[784/788] bitcoin-upgrade-activated: testing coins_tests
[785/788] Running utility command for check-bitcoin-upgrade-activated-coins_tests
[786/788] bitcoin-upgrade-activated: testing checkqueue_tests
[787/788] Running utility command for check-bitcoin-upgrade-activated-checkqueue_tests
[788/788] Running bitcoin-upgrade-activated test suite
PASSED: bitcoin-upgrade-activated test suite
ninja: build stopped: cannot make progress due to previous errors.
Build build-diff failed with exit code 1

random.randbytes was added in Python 3.9.

Use a fallback with random.randrange building a large enough integer, and then formatting that int to hex and then converting to bytes. It is still significantly faster than the previous method of builing a list of single bytes: .0.0402 seconds

Fabien requested changes to this revision.Fri, Oct 8, 08:16
Fabien added a subscriber: Fabien.
Fabien added inline comments.
test/functional/test_framework/txtools.py
59 ↗(On Diff #30378)

This will make the tx non standard, and can cause very difficult to understand bugs in the functional tests

This revision now requires changes to proceed.Fri, Oct 8, 08:16

don't use OP_TRUE to keep the tx standard

Instead, remove 10 bytes from the data for this particular tx size and complete with another empty OP_RETURN.

PiRK retitled this revision from improve the correctness of pad_tx to make pad_tx produce the correct size when possible .Fri, Oct 8, 10:04
Fabien added inline comments.
test/functional/test_framework/txtools.py
121 ↗(On Diff #30380)

extra ;

This revision is now accepted and ready to land.Fri, Oct 8, 19:11
PiRK retitled this revision from make pad_tx produce the correct size when possible to make pad_tx produce the correct size when possible.

trivial changes : remove an extra ";" in a comment, add typehints in pad_tx definition