Page MenuHomePhabricator
Differential D5130

OP_REVERSEBYTES activation logic
ClosedPublic
Actions

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

tobias_ruck created this revision.Feb 2 2020, 18:19
Owners added a reviewer: Restricted Owners Package.Feb 2 2020, 18:19
Herald added a reviewer: Restricted Project. · View Herald TranscriptFeb 2 2020, 18:19
teamcity added a comment.Feb 2 2020, 18:20

Build Bitcoin-ABC / Diffs / Diff Testing started.

teamcity added a comment.Feb 2 2020, 18:26

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B9229: Diff 15944.Feb 2 2020, 18:26
deadalnix added inline comments.Feb 2 2020, 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.Feb 2 2020, 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.Feb 3 2020, 14:33

Resolved lint issues

teamcity added a comment.Feb 3 2020, 14:34

Build Bitcoin-ABC / Diffs / Diff Testing started.

teamcity added a comment.Feb 3 2020, 14:40

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B9242: Diff 15961.Feb 3 2020, 14:40
Mengerian added a task: T707: OP_REVERSEBYTES.Feb 6 2020, 18:06
deadalnix added inline comments.Feb 7 2020, 16:41
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.

markblundeberg added a comment.Feb 8 2020, 05:18

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.Feb 8 2020, 07:24
src/validation.cpp
492

https://reviews.bitcoinabc.org/D5220

tobias_ruck updated this revision to Diff 16191.Feb 9 2020, 12:19

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

teamcity added a comment.Feb 9 2020, 12:19

Build Bitcoin-ABC / Diffs / Diff Testing started.

teamcity added a comment.Feb 9 2020, 12:24
Build Bitcoin-ABC / Diffs / Diff Testing failed.

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

Harbormaster failed remote builds in B9370: Diff 16191!Feb 9 2020, 12:24
tobias_ruck updated this revision to Diff 16192.Feb 9 2020, 13:10

Fixed rebasing

teamcity added a comment.Feb 9 2020, 13:10

Build Bitcoin-ABC / Diffs / Diff Testing started.

teamcity added a comment.Feb 9 2020, 13:19

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B9371: Diff 16192.Feb 9 2020, 13:19
deadalnix accepted this revision.Feb 12 2020, 03:10
This revision is now accepted and ready to land.Feb 12 2020, 03:10
markblundeberg added a comment.Feb 12 2020, 03:28

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.

Closed by commit rABCfcf22e8fcc94: OP_REVERSEBYTES activation logic (authored by tobias_ruck). · Explain WhyFeb 12 2020, 07:11
This revision was automatically updated to reflect the committed changes.
tobias_ruck added a commit: rABCfcf22e8fcc94: OP_REVERSEBYTES activation logic.
markblundeberg mentioned this in D5224: [standardness] activate SCRIPT_VERIFY_INPUT_SIGCHECKS in next upgrade.Feb 12 2020, 15:14
deadalnix added a reverting change: D5278: Revert "OP_REVERSEBYTES activation logic".Feb 12 2020, 22:24
deadalnix added a reverting change: rABCadaa9fe22fbc: Revert "OP_REVERSEBYTES activation logic".Feb 12 2020, 22:52
tobias_ruck mentioned this in D5283: OP_REVERSEBYTES activation logic.Feb 13 2020, 03:05
tobias_ruck mentioned this in rABC9bd868e48eb0: OP_REVERSEBYTES activation logic.Feb 13 2020, 05:34
markblundeberg mentioned this in D5244: Enable new ancestor/descendants chains limit at fork.Feb 14 2020, 01:48
markblundeberg mentioned this in D5245: [qa] Test that mempool reverts to pre-phonon policies.Feb 14 2020, 02:22
tobias_ruck added a commit: rSTAGINGfcf22e8fcc94: OP_REVERSEBYTES activation logic.Apr 7 2020, 22:58
tobias_ruck mentioned this in rSTAGING9bd868e48eb0: OP_REVERSEBYTES activation logic.
deadalnix added a reverting change: rSTAGINGadaa9fe22fbc: Revert "OP_REVERSEBYTES activation logic".Apr 7 2020, 22:59

Revision Contents
Changeset List

PathSize
src/
24 lines
test/
functional/
289 lines

Diff 15961

View Options

src/validation.cpp

Loading...
View Options

test/functional/abc-op-reversebytes-activation.py

Loading...