Page MenuHomePhabricator

OP_REVERSEBYTES activation logic
ClosedPublic

Authored by tobias_ruck on Feb 2 2020, 18:19.

Details

Reviewers
deadalnix
markblundeberg
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Project
Maniphest Tasks
T707: OP_REVERSEBYTES
Commits
rSTAGINGfcf22e8fcc94: OP_REVERSEBYTES activation logic
rABCfcf22e8fcc94: OP_REVERSEBYTES activation logic
Summary

Depends on D4871. Based on D3736.

Test Plan

ninja check
test/functional/test_runner.py
test/functional/test_runner.py --with-phononactivation

Diff Detail

Repository
rABC Bitcoin ABC
Branch
OP_REVERSEBYTES
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 9242
Build 16429: Default Diff Build & Tests
Build 16428: arc lint + arc unit

Event Timeline

Owners added a reviewer: Restricted Owners Package.Feb 2 2020, 18:19
src/validation.cpp
1 ↗(On Diff #15944)

It seems that the linter has a message for you.

492 ↗(On Diff #15944)

That is somewhat scary this needs to be duplicated.

src/validation.cpp
492 ↗(On Diff #15944)

Yeah when I made this function I looked a bit into copying from GetNextBlockScriptFlags.

i.e.,

flags = (GetNextBlockScriptFlags & ~CONSENSUS_ONLY_SCRIPT_VERIFY_FLAGS) | STANDARD_SCRIPT_VERIFY_FLAGS, something like that.

It's a possibility, and would remove the need for duplication of most activations.

src/validation.cpp
492

There is clearly a need o rationalize this guy and GetNextBlockScriptFlags . Duplicating more and more shit between the two is not going to fly.

Please rebase on master and run arc lint again, it may pick some things up in the functional test (more aggressive rules in place now).

Resolved scary activation flag duplication; activation logic is now solely in GetNextBlockScriptFlags, as per @markblundeberg‘s refactoring (D5220).

Snippet of first build failure:

[12:23:54] :	 [Step 1/1]  [0m [0;34mwallet_createwallet.py --usecli         | ✓ Passed  | 2 s
[12:23:54] :	 [Step 1/1]  [0m [0;34mwallet_disable.py                       | ✓ Passed  | 0 s
[12:23:54] :	 [Step 1/1]  [0m [0;34mwallet_dump.py                          | ✓ Passed  | 3 s
[12:23:54] :	 [Step 1/1]  [0m [0;34mwallet_encryption.py                    | ✓ Passed  | 4 s
[12:23:54] :	 [Step 1/1]  [0m [0;34mwallet_groups.py                        | ✓ Passed  | 11 s
[12:23:54] :	 [Step 1/1]  [0m [0;34mwallet_hd.py                            | ✓ Passed  | 5 s
[12:23:54] :	 [Step 1/1]  [0m [0;34mwallet_import_rescan.py                 | ✓ Passed  | 6 s
[12:23:54] :	 [Step 1/1]  [0m [0;34mwallet_importmulti.py                   | ✓ Passed  | 2 s
[12:23:54] :	 [Step 1/1]  [0m [0;34mwallet_importprunedfunds.py             | ✓ Passed  | 1 s
[12:23:54] :	 [Step 1/1]  [0m [0;34mwallet_keypool.py                       | ✓ Passed  | 3 s
[12:23:54] :	 [Step 1/1]  [0m [0;34mwallet_keypool_topup.py                 | ✓ Passed  | 3 s
[12:23:54] :	 [Step 1/1]  [0m [0;34mwallet_labels.py                        | ✓ Passed  | 5 s
[12:23:54] :	 [Step 1/1]  [0m [0;34mwallet_listreceivedby.py                | ✓ Passed  | 7 s
[12:23:54] :	 [Step 1/1]  [0m [0;34mwallet_listsinceblock.py                | ✓ Passed  | 2 s
[12:23:54] :	 [Step 1/1]  [0m [0;34mwallet_listtransactions.py              | ✓ Passed  | 13 s
[12:23:54] :	 [Step 1/1]  [0m [0;34mwallet_multiwallet.py                   | ✓ Passed  | 9 s
[12:23:54] :	 [Step 1/1]  [0m [0;34mwallet_multiwallet.py --usecli          | ✓ Passed  | 10 s
[12:23:54] :	 [Step 1/1]  [0m [0;34mwallet_resendwallettransactions.py      | ✓ Passed  | 1 s
[12:23:54] :	 [Step 1/1]  [0m [0;34mwallet_txn_clone.py                     | ✓ Passed  | 3 s
[12:23:54] :	 [Step 1/1]  [0m [0;34mwallet_txn_clone.py --mineblock         | ✓ Passed  | 3 s
[12:23:54] :	 [Step 1/1]  [0m [0;34mwallet_txn_doublespend.py               | ✓ Passed  | 3 s
[12:23:54] :	 [Step 1/1]  [0m [0;34mwallet_txn_doublespend.py --mineblock   | ✓ Passed  | 3 s
[12:23:54] :	 [Step 1/1]  [0m [0;34mwallet_zapwallettxes.py                 | ✓ Passed  | 2 s
[12:23:54] :	 [Step 1/1]  [0m [0;31mabc-op-reversebytes-activation.py       | ✖ Failed  | 1 s
[12:23:54] :	 [Step 1/1]  [0m [0;31m [1m
[12:23:54] :	 [Step 1/1] ALL                                     | ✖ Failed  | 521 s (accumulated) 
[12:23:54] :	 [Step 1/1]  [0m [0mRuntime: 117 s
[12:23:54] :	 [Step 1/1] 
[12:23:54] :	 [Step 1/1] [219/359] cd /home/teamcity/buildAgent/work/c4a5708f2bae7929/contrib/devtools/chainparams && /usr/bin/python3 ./test_make_chainparams.py
[12:23:54] :	 [Step 1/1] .....
[12:23:54] :	 [Step 1/1] ----------------------------------------------------------------------
[12:23:54] :	 [Step 1/1] Ran 5 tests in 0.001s
[12:23:54] :	 [Step 1/1] 
[12:23:54] :	 [Step 1/1] OK
[12:23:54] :	 [Step 1/1] [242/359] Running secp256k1 test suite
[12:23:54] :	 [Step 1/1] PASSED: secp256k1 test suite
[12:23:54] :	 [Step 1/1] [246/359] Running bitcoin-qt test suite
[12:23:54] :	 [Step 1/1] PASSED: bitcoin-qt test suite
[12:23:54] :	 [Step 1/1] [248/359] Running leveldb test suite
[12:23:54] :	 [Step 1/1] PASSED: leveldb test suite
[12:23:54] :	 [Step 1/1] [249/359] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/script_tests.cpp.o
[12:23:54] :	 [Step 1/1] In file included from /usr/include/boost/test/unit_test.hpp:19,
[12:23:54] :	 [Step 1/1]                  from ../src/test/script_tests.cpp:29:
[12:23:54] :	 [Step 1/1] ../src/test/script_tests.cpp: In member function ‘void script_tests::script_build::test_method()’:
[12:23:54] :	 [Step 1/1] ../src/test/script_tests.cpp:575:22: note: variable tracking size limit exceeded with -fvar-tracking-assignments, retrying without
[12:23:54] :	 [Step 1/1]  BOOST_AUTO_TEST_CASE(script_build) {
[12:23:54] :	 [Step 1/1]                       ^~~~~~~~~~~~
[12:23:54] :	 [Step 1/1] [358/359] Running bitcoin test suite
[12:23:54] :	 [Step 1/1] PASSED: bitcoin test suite
[12:23:54] :	 [Step 1/1] FAILED: test/CMakeFiles/check-functional 
[12:23:54] :	 [Step 1/1] cd /home/teamcity/buildAgent/work/c4a5708f2bae7929/build/test && /usr/bin/python3 ./functional/test_runner.py
[12:23:54] :	 [Step 1/1] ninja: build stopped: subcommand failed.
[12:23:54] :	 [Step 1/1] *** Output of /tmp/sanitizer_logs/*.log.* ***
[12:23:54]W:	 [Step 1/1] ++ print_sanitizers_log
[12:23:54]W:	 [Step 1/1] ++ for log in "${SAN_LOG_DIR}"/*.log.*
[12:23:54]W:	 [Step 1/1] ++ echo '*** Output of /tmp/sanitizer_logs/*.log.* ***'
[12:23:54]W:	 [Step 1/1] ++ cat '/tmp/sanitizer_logs/*.log.*'
[12:23:54]W:	 [Step 1/1] cat: '/tmp/sanitizer_logs/*.log.*': No such file or directory
[12:23:54]W:	 [Step 1/1] Process exited with code 1
[12:23:55]E:	 [Step 1/1] Process exited with code 1 (Step: Command Line)

Each failure log is accessible here:
Bitcoin ABC functional tests: abc-op-reversebytes-activation.py

This revision is now accepted and ready to land.Feb 12 2020, 03:10

Good!

src/validation.cpp
1317 ↗(On Diff #16192)

This can be simplified for this upgrade (avoiding an extra script check) since it's purely additive feature ... but I'll leave that optimization for a later diff.

This revision was automatically updated to reflect the committed changes.