Page MenuHomePhabricator

OP_REVERSEBYTES activation logic
ClosedPublic

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

Details

Reviewers
deadalnix
markblundeberg
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Project
Maniphest Tasks
T707: OP_REVERSEBYTES
Commits
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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

tobias_ruck created this revision.Sun, Feb 2, 18:19
Owners added a reviewer: Restricted Owners Package.Sun, Feb 2, 18:19
Herald added a reviewer: Restricted Project. · View Herald TranscriptSun, Feb 2, 18:19
deadalnix added inline comments.Sun, Feb 2, 23:42
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.

markblundeberg added inline comments.Sun, Feb 2, 23:56
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.

tobias_ruck updated this revision to Diff 15961.Mon, Feb 3, 14:33

Resolved lint issues

deadalnix added inline comments.Fri, Feb 7, 16:41
src/validation.cpp
492 ↗(On Diff #15961)

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).

markblundeberg added inline comments.Sat, Feb 8, 07:24
src/validation.cpp
492 ↗(On Diff #15961)
tobias_ruck updated this revision to Diff 16191.Sun, Feb 9, 12:19

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

teamcity added a comment.Sun, Feb 9, 12:24

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

tobias_ruck updated this revision to Diff 16192.Sun, Feb 9, 13:10

Fixed rebasing

deadalnix accepted this revision.Wed, Feb 12, 03:10
This revision is now accepted and ready to land.Wed, Feb 12, 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.