Page MenuHomePhabricator

[Consensus] Allow only tx versions 1 and 2 by consensus
ClosedPublic

Authored by tobias_ruck on Feb 22 2023, 14:35.

Details

Summary

After the Wellington upgrade, we disallow txs that don't have nVersion set to 1 or 2.

This is already a policy rule, therefore doesn't require changes by any wallet.

Forbidding the version field from having an arbitrary value (even if just for miners) allows us to use this field for a new transaction format, e.g. Mitra.

The Bitcoin Cash chain is doing the exact same upgrade, for more in-depth analysis (including which non-1-2 transaction versions have historically been mined already), you can read their CHIP.

Inspired by bchn#1599.

Test Plan

Run the entire CI pipeline

Diff Detail

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

Event Timeline

Fabien requested changes to this revision.Feb 22 2023, 16:49

This deserves a release note

src/primitives/transaction.h
199–200 ↗(On Diff #37999)
test/functional/feature_tx_version.py
27 ↗(On Diff #37999)

See ADDRESS_ECREG_P2SH_OP_TRUE and SCRIPTSIG_OP_TRUE in address.py

58 ↗(On Diff #37999)
73 ↗(On Diff #37999)

Move the comment to its own line (same applies below)

129–140 ↗(On Diff #37999)

You can simplify by removing the unused/always set parameters

142 ↗(On Diff #37999)

This already exists in blocktools.py, and even imported so you're shadowing the function here. Please rename

156 ↗(On Diff #37999)

Called here

174 ↗(On Diff #37999)

I think it's useless as we already run sync_with_ping() upon connect so we're sure the peer sent the getheaders before the pong

177 ↗(On Diff #37999)

request_block is unused, reconnect can be removed and you should leave the default timeout.

187 ↗(On Diff #37999)

dito for reconnect

This revision now requires changes to proceed.Feb 22 2023, 16:49
tobias_ruck marked an inline comment as done.

Apply review feedback

Fabien requested changes to this revision.Feb 23 2023, 09:15
Fabien added inline comments.
test/functional/feature_tx_version.py
45 ↗(On Diff #38002)

use node, and save peer to call peer.send_txs_and_test()

121 ↗(On Diff #38002)

This whole block is duplicated, make it a function

183 ↗(On Diff #38002)

that's not very useful, you could just call peer.send_txs_and_test() directly

This revision now requires changes to proceed.Feb 23 2023, 09:15

Factor parts of the test into functions, remove send_txs method

Fabien requested changes to this revision.Feb 23 2023, 14:04

It's still missing the release note

This revision now requires changes to proceed.Feb 23 2023, 14:04
sdulfari requested changes to this revision.Feb 23 2023, 16:57
sdulfari added a subscriber: sdulfari.
sdulfari added inline comments.
src/consensus/activation.cpp
104–105 ↗(On Diff #38009)

Add sanity checks to src/test/activation_tests.cpp so we know the result is the same as the block index variant.

src/policy/policy.cpp
72–73 ↗(On Diff #38009)

nit: seems unnecessary to have separate constants for the standard versions since the numbers are the same but I have no problem with you cleaning these up later.

@sdulfari I added these constants because BCHN did, but indeed they seem unnecessary. Should I remove them?

src/policy/policy.cpp
72–73 ↗(On Diff #38009)

Let's keep them separate variables as they are representing a different thing.

Add sanity checks in activation_tests.cpp, also add notes to release-notes.md

Tail of the build log:

[428/486] Running utility command for check-bitcoin-checkpoints_tests
[429/486] Running utility command for check-bitcoin-validation_block_tests
[430/486] bitcoin: testing coinstatsindex_tests
[431/486] Running utility command for check-bitcoin-coinstatsindex_tests
[432/486] bitcoin: testing blockfilter_tests
[433/486] bitcoin: testing scheduler_tests
[434/486] bitcoin: testing key_io_tests
[435/486] bitcoin: testing script_tests
[436/486] Running utility command for check-bitcoin-blockfilter_tests
[437/486] Running utility command for check-bitcoin-scheduler_tests
[438/486] Running utility command for check-bitcoin-script_tests
[439/486] Running utility command for check-bitcoin-key_io_tests
[440/486] bitcoin: testing txvalidationcache_tests
[441/486] bitcoin: testing sigencoding_tests
[442/486] Running utility command for check-bitcoin-txvalidationcache_tests
[443/486] Running utility command for check-bitcoin-sigencoding_tests
[444/486] bitcoin: testing scriptpubkeyman_tests
[445/486] Running utility command for check-bitcoin-scriptpubkeyman_tests
[446/486] bitcoin: testing walletdb_tests
[447/486] Running utility command for check-bitcoin-walletdb_tests
[448/486] bitcoin: testing psbt_wallet_tests
[449/486] bitcoin: testing mempool_tests
[450/486] Running utility command for check-bitcoin-psbt_wallet_tests
[451/486] Running utility command for check-bitcoin-mempool_tests
[452/486] bitcoin: testing cuckoocache_tests
[453/486] bitcoin: testing crypto_tests
[454/486] bitcoin: testing miner_tests
[455/486] Running utility command for check-bitcoin-cuckoocache_tests
[456/486] bitcoin: testing merkle_tests
[457/486] bitcoin: testing logging_tests
[458/486] Running utility command for check-bitcoin-miner_tests
[459/486] Running utility command for check-bitcoin-merkle_tests
[460/486] Running utility command for check-bitcoin-crypto_tests
[461/486] Running utility command for check-bitcoin-logging_tests
[462/486] bitcoin: testing uint256_tests
[463/486] Running utility command for check-bitcoin-uint256_tests
[464/486] bitcoin: testing rcu_tests
[465/486] Running utility command for check-bitcoin-rcu_tests
[466/486] Linking CXX executable src/qt/test/test_bitcoin-qt
[467/486] bitcoin: testing wallet_crypto_tests
[468/486] Running utility command for check-bitcoin-wallet_crypto_tests
[469/486] bitcoin: testing txrequest_tests
[470/486] Running utility command for check-bitcoin-txrequest_tests
[471/486] bitcoin: testing radix_tests
[472/486] bitcoin-qt: testing test_bitcoin-qt
[473/486] Running bitcoin-qt test suite
PASSED: bitcoin-qt test suite
[474/486] Running utility command for check-bitcoin-radix_tests
[475/486] bitcoin: testing blockcheck_tests
[476/486] Running utility command for check-bitcoin-blockcheck_tests
[477/486] bitcoin: testing wallet_tests
[478/486] Running utility command for check-bitcoin-wallet_tests
[479/486] bitcoin: testing coinselector_tests
[480/486] Running utility command for check-bitcoin-coinselector_tests
[481/486] bitcoin: testing transaction_tests
[482/486] Running utility command for check-bitcoin-transaction_tests
[483/486] bitcoin: testing coins_tests
[484/486] Running utility command for check-bitcoin-coins_tests
ninja: build stopped: cannot make progress due to previous errors.
Build build-clang failed with exit code 1

Tail of the build log:

[421/479] bitcoin: testing coinstatsindex_tests
[422/479] bitcoin: testing fs_tests
[423/479] Running utility command for check-bitcoin-coinstatsindex_tests
[424/479] bitcoin: testing inv_tests
[425/479] bitcoin: testing checkpoints_tests
[426/479] bitcoin: testing crypto_tests
[427/479] Running utility command for check-bitcoin-fs_tests
[428/479] bitcoin: testing txvalidationcache_tests
[429/479] Running utility command for check-bitcoin-inv_tests
[430/479] Running utility command for check-bitcoin-checkpoints_tests
[431/479] Running utility command for check-bitcoin-crypto_tests
[432/479] Running utility command for check-bitcoin-txvalidationcache_tests
[433/479] bitcoin: testing blockfilter_tests
[434/479] bitcoin: testing cuckoocache_tests
[435/479] bitcoin: testing key_io_tests
[436/479] Running utility command for check-bitcoin-blockfilter_tests
[437/479] bitcoin: testing scriptpubkeyman_tests
[438/479] Running utility command for check-bitcoin-cuckoocache_tests
[439/479] Running utility command for check-bitcoin-key_io_tests
[440/479] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/__/__/wallet/test/wallet_test_fixture.cpp.o
[441/479] Running utility command for check-bitcoin-scriptpubkeyman_tests
[442/479] bitcoin: testing scheduler_tests
[443/479] Running utility command for check-bitcoin-scheduler_tests
[444/479] bitcoin: testing walletdb_tests
[445/479] Running utility command for check-bitcoin-walletdb_tests
[446/479] bitcoin: testing psbt_wallet_tests
[447/479] Running utility command for check-bitcoin-psbt_wallet_tests
[448/479] bitcoin: testing logging_tests
[449/479] Running utility command for check-bitcoin-logging_tests
[450/479] bitcoin: testing merkle_tests
[451/479] bitcoin: testing miner_tests
[452/479] Running utility command for check-bitcoin-merkle_tests
[453/479] Running utility command for check-bitcoin-miner_tests
[454/479] bitcoin: testing init_tests
[455/479] Running utility command for check-bitcoin-init_tests
[456/479] bitcoin: testing rcu_tests
[457/479] Running utility command for check-bitcoin-rcu_tests
[458/479] bitcoin: testing wallet_crypto_tests
[459/479] Running utility command for check-bitcoin-wallet_crypto_tests
[460/479] bitcoin: testing txrequest_tests
[461/479] Running utility command for check-bitcoin-txrequest_tests
[462/479] bitcoin: testing blockcheck_tests
[463/479] Running utility command for check-bitcoin-blockcheck_tests
[464/479] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/paymentservertests.cpp.o
[465/479] bitcoin: testing coinselector_tests
[466/479] Running utility command for check-bitcoin-coinselector_tests
[467/479] bitcoin: testing wallet_tests
[468/479] Running utility command for check-bitcoin-wallet_tests
[469/479] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/addressbooktests.cpp.o
[470/479] bitcoin: testing transaction_tests
[471/479] Running utility command for check-bitcoin-transaction_tests
[472/479] bitcoin: testing coins_tests
[473/479] Running utility command for check-bitcoin-coins_tests
[474/479] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/wallettests.cpp.o
[475/479] Linking CXX executable src/qt/test/test_bitcoin-qt
[476/479] bitcoin-qt: testing test_bitcoin-qt
[477/479] Running bitcoin-qt test suite
PASSED: bitcoin-qt test suite
ninja: build stopped: cannot make progress due to previous errors.
Build build-clang-tidy failed with exit code 1

Tail of the build log:

wallet_labels.py --descriptors            | ○ Skipped | 0 s
wallet_listreceivedby.py                  | ○ Skipped | 0 s
wallet_listsinceblock.py                  | ○ Skipped | 0 s
wallet_listsinceblock.py --descriptors    | ○ Skipped | 0 s
wallet_listtransactions.py                | ○ Skipped | 0 s
wallet_listtransactions.py --descriptors  | ○ Skipped | 0 s
wallet_multiwallet.py                     | ○ Skipped | 0 s
wallet_multiwallet.py --usecli            | ○ Skipped | 0 s
wallet_reorgsrestore.py                   | ○ Skipped | 0 s
wallet_resendwallettransactions.py        | ○ Skipped | 0 s
wallet_send.py                            | ○ Skipped | 0 s
wallet_startup.py                         | ○ Skipped | 0 s
wallet_timelock.py                        | ○ Skipped | 0 s
wallet_txn_clone.py                       | ○ Skipped | 0 s
wallet_txn_clone.py --mineblock           | ○ Skipped | 0 s
wallet_txn_doublespend.py                 | ○ Skipped | 0 s
wallet_txn_doublespend.py --mineblock     | ○ Skipped | 0 s
wallet_watchonly.py                       | ○ Skipped | 0 s
wallet_watchonly.py --usecli              | ○ Skipped | 0 s

ALL                                       | ✓ Passed  | 691 s (accumulated) 
Runtime: 142 s

[171/447] Test Bitcoin RPC authentication...
...
----------------------------------------------------------------------
Ran 3 tests in 0.005s

OK
[172/447] cd /work/contrib/devtools/chainparams && /usr/bin/python3.9 ./test_make_chainparams.py
.....
----------------------------------------------------------------------
Ran 5 tests in 0.001s

OK
[194/447] Running seeder test suite
PASSED: seeder test suite
[200/447] Running avalanche test suite
PASSED: avalanche test suite
[205/447] Running pow test suite
PASSED: pow test suite
[208/447] Running bitcoin-qt test suite
PASSED: bitcoin-qt test suite
[209/447] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/script_tests.cpp.o
In file included from /usr/include/boost/test/unit_test.hpp:19,
                 from ../../src/test/script_tests.cpp:30:
../../src/test/script_tests.cpp: In member function ‘void script_tests::script_build::test_method()’:
../../src/test/script_tests.cpp:540:22: note: variable tracking size limit exceeded with ‘-fvar-tracking-assignments’, retrying without
  540 | BOOST_AUTO_TEST_CASE(script_build) {
      |                      ^~~~~~~~~~~~
[334/447] bitcoin: testing activation_tests
FAILED: src/test/CMakeFiles/check-bitcoin-activation_tests 
cd /work/abc-ci-builds/build-without-wallet/src/test && /usr/bin/cmake -E make_directory /work/abc-ci-builds/build-without-wallet/test/junit && /usr/bin/cmake -E make_directory /work/abc-ci-builds/build-without-wallet/test/log && /usr/bin/cmake -E env /work/cmake/utils/log-and-print-on-failure.sh /work/abc-ci-builds/build-without-wallet/test/log/bitcoin-activation_tests.log /work/abc-ci-builds/build-without-wallet/src/test/test_bitcoin --run_test=activation_tests --logger=HRF,message:JUNIT,message,bitcoin-activation_tests.xml --catch_system_errors=no
Running 2 test cases...
../../src/test/activation_tests.cpp(58): error: in "activation_tests/iswellingtonenabled": check !IsWellingtonEnabled(params, activation) has failed

*** 1 failure is detected in the test module "Bitcoin ABC unit tests"
[444/447] Running utility command for check-bitcoin-coins_tests
ninja: build stopped: cannot make progress due to previous errors.
Build build-without-wallet failed with exit code 1

Tail of the build log:

wallet_labels.py                          | ✓ Passed  | 2 s
wallet_labels.py --descriptors            | ✓ Passed  | 2 s
wallet_listreceivedby.py                  | ✓ Passed  | 10 s
wallet_listsinceblock.py                  | ✓ Passed  | 10 s
wallet_listsinceblock.py --descriptors    | ✓ Passed  | 12 s
wallet_listtransactions.py                | ✓ Passed  | 6 s
wallet_listtransactions.py --descriptors  | ✓ Passed  | 4 s
wallet_multiwallet.py                     | ✓ Passed  | 20 s
wallet_multiwallet.py --usecli            | ✓ Passed  | 16 s
wallet_reorgsrestore.py                   | ✓ Passed  | 4 s
wallet_resendwallettransactions.py        | ✓ Passed  | 2 s
wallet_send.py                            | ✓ Passed  | 10 s
wallet_startup.py                         | ✓ Passed  | 3 s
wallet_timelock.py                        | ✓ Passed  | 1 s
wallet_txn_clone.py                       | ✓ Passed  | 2 s
wallet_txn_clone.py --mineblock           | ✓ Passed  | 3 s
wallet_txn_doublespend.py                 | ✓ Passed  | 2 s
wallet_txn_doublespend.py --mineblock     | ✓ Passed  | 3 s
wallet_watchonly.py                       | ✓ Passed  | 1 s
wallet_watchonly.py --usecli              | ✓ Passed  | 1 s
chronik_serve.py                          | ○ Skipped | 0 s
interface_usdt_net.py                     | ○ Skipped | 0 s
interface_usdt_utxocache.py               | ○ Skipped | 0 s
interface_usdt_validation.py              | ○ Skipped | 0 s

ALL                                       | ✓ Passed  | 2045 s (accumulated) 
Runtime: 413 s

[163/487] Test Bitcoin RPC authentication...
...
----------------------------------------------------------------------
Ran 3 tests in 0.015s

OK
[164/487] cd /work/contrib/devtools/chainparams && /usr/bin/python3.9 ./test_make_chainparams.py
.....
----------------------------------------------------------------------
Ran 5 tests in 0.003s

OK
[212/487] Running seeder test suite
PASSED: seeder test suite
[218/487] Running pow test suite
PASSED: pow test suite
[308/487] bitcoin: testing activation_tests
FAILED: src/test/CMakeFiles/check-bitcoin-activation_tests 
cd /work/abc-ci-builds/build-debug/src/test && /usr/bin/cmake -E make_directory /work/abc-ci-builds/build-debug/test/junit && /usr/bin/cmake -E make_directory /work/abc-ci-builds/build-debug/test/log && /usr/bin/cmake -E env /work/cmake/utils/log-and-print-on-failure.sh /work/abc-ci-builds/build-debug/test/log/bitcoin-activation_tests.log /work/abc-ci-builds/build-debug/src/test/test_bitcoin --run_test=activation_tests --logger=HRF,message:JUNIT,message,bitcoin-activation_tests.xml --catch_system_errors=no
Running 2 test cases...
../../src/test/activation_tests.cpp(58): error: in "activation_tests/iswellingtonenabled": check !IsWellingtonEnabled(params, activation) has failed

*** 1 failure is detected in the test module "Bitcoin ABC unit tests"
[418/487] Running secp256k1 test suite
PASSED: secp256k1 test suite
[457/487] Running avalanche test suite
PASSED: avalanche test suite
[468/487] Running bitcoin-qt test suite
PASSED: bitcoin-qt test suite
[484/487] Running utility command for check-bitcoin-coins_tests
ninja: build stopped: cannot make progress due to previous errors.
Build build-debug failed with exit code 1

Tail of the build log:

wallet_address_types.py                   | ✓ Passed  | 12 s
wallet_address_types.py --descriptors     | ✓ Passed  | 8 s
wallet_avoidreuse.py                      | ✓ Passed  | 4 s
wallet_avoidreuse.py --descriptors        | ✓ Passed  | 5 s
wallet_backup.py                          | ✓ Passed  | 18 s
wallet_balance.py                         | ✓ Passed  | 5 s
wallet_balance.py --descriptors           | ✓ Passed  | 5 s
wallet_basic.py                           | ✓ Passed  | 14 s
wallet_coinbase_category.py               | ✓ Passed  | 1 s
wallet_create_tx.py                       | ✓ Passed  | 5 s
wallet_createwallet.py                    | ✓ Passed  | 2 s
wallet_createwallet.py --descriptors      | ✓ Passed  | 2 s
wallet_createwallet.py --usecli           | ✓ Passed  | 2 s
wallet_descriptor.py                      | ✓ Passed  | 6 s
wallet_disable.py                         | ✓ Passed  | 1 s
wallet_dump.py                            | ✓ Passed  | 5 s
wallet_encryption.py                      | ✓ Passed  | 5 s
wallet_encryption.py --descriptors        | ✓ Passed  | 5 s
wallet_groups.py                          | ✓ Passed  | 13 s
wallet_hd.py                              | ✓ Passed  | 6 s
wallet_hd.py --descriptors                | ✓ Passed  | 6 s
wallet_import_rescan.py                   | ✓ Passed  | 7 s
wallet_import_with_label.py               | ✓ Passed  | 1 s
wallet_importdescriptors.py               | ✓ Passed  | 6 s
wallet_importmulti.py                     | ✓ Passed  | 2 s
wallet_importprunedfunds.py               | ✓ Passed  | 2 s
wallet_importprunedfunds.py --descriptors | ✓ Passed  | 2 s
wallet_keypool.py                         | ✓ Passed  | 3 s
wallet_keypool_topup.py                   | ✓ Passed  | 4 s
wallet_keypool_topup.py --descriptors     | ✓ Passed  | 5 s
wallet_labels.py                          | ✓ Passed  | 1 s
wallet_labels.py --descriptors            | ✓ Passed  | 1 s
wallet_listreceivedby.py                  | ✓ Passed  | 6 s
wallet_listsinceblock.py                  | ✓ Passed  | 6 s
wallet_listsinceblock.py --descriptors    | ✓ Passed  | 6 s
wallet_listtransactions.py                | ✓ Passed  | 4 s
wallet_listtransactions.py --descriptors  | ✓ Passed  | 4 s
wallet_multiwallet.py                     | ✓ Passed  | 36 s
wallet_multiwallet.py --usecli            | ✓ Passed  | 9 s
wallet_reorgsrestore.py                   | ✓ Passed  | 3 s
wallet_resendwallettransactions.py        | ✓ Passed  | 2 s
wallet_send.py                            | ✓ Passed  | 6 s
wallet_startup.py                         | ✓ Passed  | 2 s
wallet_timelock.py                        | ✓ Passed  | 1 s
wallet_txn_clone.py                       | ✓ Passed  | 2 s
wallet_txn_clone.py --mineblock           | ✓ Passed  | 3 s
wallet_txn_doublespend.py                 | ✓ Passed  | 1 s
wallet_txn_doublespend.py --mineblock     | ✓ Passed  | 3 s
wallet_watchonly.py                       | ✓ Passed  | 1 s
wallet_watchonly.py --usecli              | ✓ Passed  | 1 s
chronik_serve.py                          | ○ Skipped | 0 s
interface_usdt_net.py                     | ○ Skipped | 0 s
interface_usdt_utxocache.py               | ○ Skipped | 0 s
interface_usdt_validation.py              | ○ Skipped | 0 s

ALL                                       | ✓ Passed  | 1236 s (accumulated) 
Runtime: 251 s

ninja: build stopped: cannot make progress due to previous errors.
Build build-diff failed with exit code 1

Update release notes to be in line with D13171, also fix activation test (oopsie)

Fabien added inline comments.
test/functional/feature_tx_version.py
173 ↗(On Diff #38049)

Nit: this could be removed too and send_blocks_and_test called directly

Remove send_blocks method from feature_tx_version.py

This revision is now accepted and ready to land.Feb 27 2023, 16:30
tobias_ruck edited the summary of this revision. (Show Details)

Rebase from master

Indent points in Network upgrade using 2 spaces