Page MenuHomePhabricator
Differential D10276

make pad_tx produce the correct size when possible
ClosedPublic
Actions

Authored by PiRK on Oct 7 2021, 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

Event Timeline

PiRK created this revision.Oct 7 2021, 10:04
Herald added a reviewer: Restricted Project. · View Herald TranscriptOct 7 2021, 10:04
PiRK requested review of this revision.Oct 7 2021, 10:04
Harbormaster completed remote builds in B16921: Diff 30366.Oct 7 2021, 10:18
PiRK edited the summary of this revision. (Show Details)Oct 7 2021, 11:52
PiRK updated this revision to Diff 30368.Oct 7 2021, 12:31

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

PiRK mentioned this in D10199: test: Mockwallet.Oct 7 2021, 12:52
Harbormaster completed remote builds in B16923: Diff 30368.Oct 7 2021, 12:57
PiRK planned changes to this revision.Oct 7 2021, 14:28

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

PiRK updated this revision to Diff 30375.Oct 7 2021, 17:47
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).

Harbormaster completed remote builds in B16927: Diff 30375.Oct 7 2021, 18:02
PiRK updated this revision to Diff 30376.EditedOct 8 2021, 06:38

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.

Harbormaster completed remote builds in B16928: Diff 30376.Oct 8 2021, 06:45
teamcity added a comment.Oct 8 2021, 06:45
Build Bitcoin ABC Diffs / Diff Testing (build-diff) failed.

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
PiRK updated this revision to Diff 30378.Oct 8 2021, 07:26

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

Harbormaster completed remote builds in B16929: Diff 30378.Oct 8 2021, 07:41
Fabien requested changes to this revision.Oct 8 2021, 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.Oct 8 2021, 08:16
PiRK updated this revision to Diff 30380.Oct 8 2021, 08:46

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.

Harbormaster completed remote builds in B16930: Diff 30380.Oct 8 2021, 09:02
PiRK retitled this revision from improve the correctness of pad_tx to make pad_tx produce the correct size when possible .Oct 8 2021, 10:04
Fabien accepted this revision.Oct 8 2021, 19:11
Fabien added inline comments.
test/functional/test_framework/txtools.py
121 ↗(On Diff #30380)

extra ;

This revision is now accepted and ready to land.Oct 8 2021, 19:11
PiRK updated this revision to Diff 30400.Oct 11 2021, 06:42
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

Harbormaster completed remote builds in B16950: Diff 30400.Oct 11 2021, 07:19
Closed by commit rABCfbb20474303b: make pad_tx produce the correct size when possible (authored by PiRK). · Explain WhyOct 11 2021, 07:24
This revision was automatically updated to reflect the committed changes.
PiRK added a commit: rABCfbb20474303b: make pad_tx produce the correct size when possible.

Revision Contents
Changeset List

PathSize
test/
functional/
test_framework/
138 lines
1 line

Diff 30366

View Options

test/functional/test_framework/txtools.py

Loading...
View Options

test/functional/test_runner.py

Loading...