Details

Reviewers

Group Reviewers

Restricted Project

Commits

rABCf2fd05ff267c: [Iguana] Add `ScriptInterpreter` class, make `EvalScript` use it

Summary

In order to get a nice transaction debugger that allows users to step through individual opcodes, we need to split out the script execution code into a RunNextOp function, which then allows users to inspect the stack.

For that, introduce ScriptInterpreter, which simply contains all the state that would normally be in EvalScript, and add a few functions to interact with it, most importantly RunNextOp, which now contains one single execution of the next opcode.

EvalScript is then refactored to use ScriptInterpreter and calls RunNextOp in a loop.

There's no change in behavior.

NB: Curiously, removing two indents has the nice side effect that we also remove around 25 newlines when reformatting the interpreter code.

Depends on D16766.

Test Plan

ninja check && ninja check-functional

Diff Detail

Repository

rABC Bitcoin ABC

Branch

iguana-add-script-interpreter

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 30317
Build 60161: Build Diff	build-debug · build-without-wallet · lint-circular-dependencies · build-diff · build-clang · build-clang-tidy
Build 60160: arc lint + arc unit

Event Timeline

tobias_ruck created this revision.Sep 6 2024, 17:39

Herald added a reviewer: Restricted Project. · View Herald TranscriptSep 6 2024, 17:39

Harbormaster completed remote builds in B30211: Diff 49536.Sep 6 2024, 17:42

Build Bitcoin ABC Diffs / Diff Testing (build-without-wallet) failed.

Tail of the build log:

/usr/bin/ccache /usr/bin/c++ -DBOOST_ALL_NO_LIB -DBUILD_BITCOIN_INTERNAL -DENABLE_AVX2 -DENABLE_SHANI -DENABLE_SSE41 -DHAVE_BUILD_INFO -DHAVE_CONFIG_H -DHAVE_CONSENSUS_LIB -DLEVELDB_ATOMIC_PRESENT -DLEVELDB_PLATFORM_POSIX -DOS_LINUX -I../../src/leveldb/helpers/memenv -I../../src/. -Isrc -I../../src/univalue/include -Isrc/crypto/.. -I../../src/secp256k1/include -I../../src/leveldb/include -isystem /usr/include/jemalloc -isystem /usr/include/miniupnpc -Werror -g -O2 -fPIC -fvisibility=hidden -fstack-reuse=none -fstack-protector-all -Wstack-protector -fcf-protection=full -fstack-clash-protection -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -Wall -Wextra -Wformat -Wvla -Wcast-align -Wunused-parameter -Wmissing-braces -Wredundant-decls -Wsign-compare -Wduplicated-branches -Wduplicated-cond -Wlogical-op -Wformat-security -Wredundant-move -Woverloaded-virtual -Wno-unused-parameter -Wno-implicit-fallthrough -Wno-psabi -pthread -std=gnu++17 -MD -MT src/CMakeFiles/server.dir/rpc/rawtransaction.cpp.o -MF src/CMakeFiles/server.dir/rpc/rawtransaction.cpp.o.d -o src/CMakeFiles/server.dir/rpc/rawtransaction.cpp.o -c ../../src/rpc/rawtransaction.cpp
In file included from ../../src/./script/sign.h:12,
                 from ../../src/./psbt.h:11,
                 from ../../src/./node/psbt.h:8,
                 from ../../src/rpc/rawtransaction.cpp:18:
../../src/./script/interpreter.h:150:7: error: ‘ScriptInterpreter’ has a field ‘ScriptInterpreter::vfExec’ whose type uses the anonymous namespace [-Werror=subobject-linkage]
  150 | class ScriptInterpreter {
      |       ^~~~~~~~~~~~~~~~~
cc1plus: all warnings being treated as errors
[532/640] Building CXX object src/CMakeFiles/server.dir/init.cpp.o
FAILED: src/CMakeFiles/server.dir/init.cpp.o 
/usr/bin/ccache /usr/bin/c++ -DBOOST_ALL_NO_LIB -DBUILD_BITCOIN_INTERNAL -DENABLE_AVX2 -DENABLE_SHANI -DENABLE_SSE41 -DHAVE_BUILD_INFO -DHAVE_CONFIG_H -DHAVE_CONSENSUS_LIB -DLEVELDB_ATOMIC_PRESENT -DLEVELDB_PLATFORM_POSIX -DOS_LINUX -I../../src/leveldb/helpers/memenv -I../../src/. -Isrc -I../../src/univalue/include -Isrc/crypto/.. -I../../src/secp256k1/include -I../../src/leveldb/include -isystem /usr/include/jemalloc -isystem /usr/include/miniupnpc -Werror -g -O2 -fPIC -fvisibility=hidden -fstack-reuse=none -fstack-protector-all -Wstack-protector -fcf-protection=full -fstack-clash-protection -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -Wall -Wextra -Wformat -Wvla -Wcast-align -Wunused-parameter -Wmissing-braces -Wredundant-decls -Wsign-compare -Wduplicated-branches -Wduplicated-cond -Wlogical-op -Wformat-security -Wredundant-move -Woverloaded-virtual -Wno-unused-parameter -Wno-implicit-fallthrough -Wno-psabi -pthread -std=gnu++17 -MD -MT src/CMakeFiles/server.dir/init.cpp.o -MF src/CMakeFiles/server.dir/init.cpp.o.d -o src/CMakeFiles/server.dir/init.cpp.o -c ../../src/init.cpp
In file included from ../../src/./script/sigcache.h:9,
                 from ../../src/./kernel/validation_cache_sizes.h:9,
                 from ../../src/init.cpp:13:
../../src/./script/interpreter.h:150:7: error: ‘ScriptInterpreter’ has a field ‘ScriptInterpreter::vfExec’ whose type uses the anonymous namespace [-Werror=subobject-linkage]
  150 | class ScriptInterpreter {
      |       ^~~~~~~~~~~~~~~~~
cc1plus: all warnings being treated as errors
[533/640] Building CXX object src/CMakeFiles/server.dir/rpc/blockchain.cpp.o
FAILED: src/CMakeFiles/server.dir/rpc/blockchain.cpp.o 
/usr/bin/ccache /usr/bin/c++ -DBOOST_ALL_NO_LIB -DBUILD_BITCOIN_INTERNAL -DENABLE_AVX2 -DENABLE_SHANI -DENABLE_SSE41 -DHAVE_BUILD_INFO -DHAVE_CONFIG_H -DHAVE_CONSENSUS_LIB -DLEVELDB_ATOMIC_PRESENT -DLEVELDB_PLATFORM_POSIX -DOS_LINUX -I../../src/leveldb/helpers/memenv -I../../src/. -Isrc -I../../src/univalue/include -Isrc/crypto/.. -I../../src/secp256k1/include -I../../src/leveldb/include -isystem /usr/include/jemalloc -isystem /usr/include/miniupnpc -Werror -g -O2 -fPIC -fvisibility=hidden -fstack-reuse=none -fstack-protector-all -Wstack-protector -fcf-protection=full -fstack-clash-protection -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -Wall -Wextra -Wformat -Wvla -Wcast-align -Wunused-parameter -Wmissing-braces -Wredundant-decls -Wsign-compare -Wduplicated-branches -Wduplicated-cond -Wlogical-op -Wformat-security -Wredundant-move -Woverloaded-virtual -Wno-unused-parameter -Wno-implicit-fallthrough -Wno-psabi -pthread -std=gnu++17 -MD -MT src/CMakeFiles/server.dir/rpc/blockchain.cpp.o -MF src/CMakeFiles/server.dir/rpc/blockchain.cpp.o.d -o src/CMakeFiles/server.dir/rpc/blockchain.cpp.o -c ../../src/rpc/blockchain.cpp
In file included from ../../src/./script/sign.h:12,
                 from ../../src/./rpc/util.h:14,
                 from ../../src/./rpc/server.h:13,
                 from ../../src/rpc/blockchain.cpp:29:
../../src/./script/interpreter.h:150:7: error: ‘ScriptInterpreter’ has a field ‘ScriptInterpreter::vfExec’ whose type uses the anonymous namespace [-Werror=subobject-linkage]
  150 | class ScriptInterpreter {
      |       ^~~~~~~~~~~~~~~~~
cc1plus: all warnings being treated as errors
[534/640] Building CXX object src/test/CMakeFiles/testutil.dir/util/setup_common.cpp.o
FAILED: src/test/CMakeFiles/testutil.dir/util/setup_common.cpp.o 
/usr/bin/ccache /usr/bin/c++ -DBOOST_ALL_NO_LIB -DBUILD_BITCOIN_INTERNAL -DENABLE_AVX2 -DENABLE_SHANI -DENABLE_SSE41 -DHAVE_BUILD_INFO -DHAVE_CONFIG_H -DHAVE_CONSENSUS_LIB -DLEVELDB_ATOMIC_PRESENT -DLEVELDB_PLATFORM_POSIX -DOS_LINUX -I../../src/. -Isrc -I../../src/univalue/include -Isrc/crypto/.. -I../../src/secp256k1/include -I../../src/leveldb/include -isystem /usr/include/jemalloc -isystem /usr/include/miniupnpc -Werror -g -O2 -fPIC -fvisibility=hidden -fstack-reuse=none -fstack-protector-all -Wstack-protector -fcf-protection=full -fstack-clash-protection -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -Wall -Wextra -Wformat -Wvla -Wcast-align -Wunused-parameter -Wmissing-braces -Wredundant-decls -Wsign-compare -Wduplicated-branches -Wduplicated-cond -Wlogical-op -Wformat-security -Wredundant-move -Woverloaded-virtual -Wno-unused-parameter -Wno-implicit-fallthrough -Wno-psabi -pthread -std=gnu++17 -MD -MT src/test/CMakeFiles/testutil.dir/util/setup_common.cpp.o -MF src/test/CMakeFiles/testutil.dir/util/setup_common.cpp.o.d -o src/test/CMakeFiles/testutil.dir/util/setup_common.cpp.o -c ../../src/test/util/setup_common.cpp
In file included from ../../src/./script/sigcache.h:9,
                 from ../../src/./kernel/validation_cache_sizes.h:9,
                 from ../../src/test/util/setup_common.cpp:8:
../../src/./script/interpreter.h:150:7: error: ‘ScriptInterpreter’ has a field ‘ScriptInterpreter::vfExec’ whose type uses the anonymous namespace [-Werror=subobject-linkage]
  150 | class ScriptInterpreter {
      |       ^~~~~~~~~~~~~~~~~
cc1plus: all warnings being treated as errors
[535/640] Building CXX object src/CMakeFiles/server.dir/validation.cpp.o
FAILED: src/CMakeFiles/server.dir/validation.cpp.o 
/usr/bin/ccache /usr/bin/c++ -DBOOST_ALL_NO_LIB -DBUILD_BITCOIN_INTERNAL -DENABLE_AVX2 -DENABLE_SHANI -DENABLE_SSE41 -DHAVE_BUILD_INFO -DHAVE_CONFIG_H -DHAVE_CONSENSUS_LIB -DLEVELDB_ATOMIC_PRESENT -DLEVELDB_PLATFORM_POSIX -DOS_LINUX -I../../src/leveldb/helpers/memenv -I../../src/. -Isrc -I../../src/univalue/include -Isrc/crypto/.. -I../../src/secp256k1/include -I../../src/leveldb/include -isystem /usr/include/jemalloc -isystem /usr/include/miniupnpc -Werror -g -O2 -fPIC -fvisibility=hidden -fstack-reuse=none -fstack-protector-all -Wstack-protector -fcf-protection=full -fstack-clash-protection -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -Wall -Wextra -Wformat -Wvla -Wcast-align -Wunused-parameter -Wmissing-braces -Wredundant-decls -Wsign-compare -Wduplicated-branches -Wduplicated-cond -Wlogical-op -Wformat-security -Wredundant-move -Woverloaded-virtual -Wno-unused-parameter -Wno-implicit-fallthrough -Wno-psabi -pthread -std=gnu++17 -MD -MT src/CMakeFiles/server.dir/validation.cpp.o -MF src/CMakeFiles/server.dir/validation.cpp.o.d -o src/CMakeFiles/server.dir/validation.cpp.o -c ../../src/validation.cpp
In file included from ../../src/./script/sigcache.h:9,
                 from ../../src/validation.cpp:49:
../../src/./script/interpreter.h:150:7: error: ‘ScriptInterpreter’ has a field ‘ScriptInterpreter::vfExec’ whose type uses the anonymous namespace [-Werror=subobject-linkage]
  150 | class ScriptInterpreter {
      |       ^~~~~~~~~~~~~~~~~
cc1plus: all warnings being treated as errors
[536/640] Building CXX object src/CMakeFiles/bitcoinkernel.dir/validation.cpp.o
FAILED: src/CMakeFiles/bitcoinkernel.dir/validation.cpp.o 
/usr/bin/ccache /usr/bin/c++ -DBOOST_ALL_NO_LIB -DENABLE_AVX2 -DENABLE_SHANI -DENABLE_SSE41 -DHAVE_CONFIG_H -DLEVELDB_ATOMIC_PRESENT -DLEVELDB_PLATFORM_POSIX -DOS_LINUX -I../../src/. -I../../src/leveldb/helpers/memenv -Isrc -Isrc/crypto/.. -I../../src/univalue/include -I../../src/secp256k1/include -I../../src/leveldb/include -isystem /usr/include/jemalloc -Werror -g -O2 -fPIC -fvisibility=hidden -fstack-reuse=none -fstack-protector-all -Wstack-protector -fcf-protection=full -fstack-clash-protection -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -Wall -Wextra -Wformat -Wvla -Wcast-align -Wunused-parameter -Wmissing-braces -Wredundant-decls -Wsign-compare -Wduplicated-branches -Wduplicated-cond -Wlogical-op -Wformat-security -Wredundant-move -Woverloaded-virtual -Wno-unused-parameter -Wno-implicit-fallthrough -Wno-psabi -pthread -std=gnu++17 -MD -MT src/CMakeFiles/bitcoinkernel.dir/validation.cpp.o -MF src/CMakeFiles/bitcoinkernel.dir/validation.cpp.o.d -o src/CMakeFiles/bitcoinkernel.dir/validation.cpp.o -c ../../src/validation.cpp
In file included from ../../src/./script/sigcache.h:9,
                 from ../../src/validation.cpp:49:
../../src/./script/interpreter.h:150:7: error: ‘ScriptInterpreter’ has a field ‘ScriptInterpreter::vfExec’ whose type uses the anonymous namespace [-Werror=subobject-linkage]
  150 | class ScriptInterpreter {
      |       ^~~~~~~~~~~~~~~~~
cc1plus: all warnings being treated as errors
ninja: build stopped: cannot make progress due to previous errors.
Build build-without-wallet failed with exit code 1

Build Bitcoin ABC Diffs / Diff Testing (build-debug) failed.

Tail of the build log:

  150 | class ScriptInterpreter {
      |       ^~~~~~~~~~~~~~~~~
cc1plus: all warnings being treated as errors
[429/572] Building CXX object src/wallet/CMakeFiles/wallet.dir/rpc/backup.cpp.o
FAILED: src/wallet/CMakeFiles/wallet.dir/rpc/backup.cpp.o 
/usr/bin/ccache /usr/bin/c++ -DABORT_ON_FAILED_ASSUME -DBOOST_ALL_NO_LIB -DBUILD_BITCOIN_INTERNAL -DDEBUG -DDEBUG_LOCKORDER -DENABLE_AVX2 -DENABLE_SHANI -DENABLE_SSE41 -DHAVE_BUILD_INFO -DHAVE_CONFIG_H -DHAVE_CONSENSUS_LIB -DLEVELDB_ATOMIC_PRESENT -DLEVELDB_PLATFORM_POSIX -DOS_LINUX -I../../src/. -Isrc -I../../src/univalue/include -Isrc/crypto/.. -I../../src/secp256k1/include -I../../src/leveldb/include -isystem /usr/include/jemalloc -isystem /usr/include/miniupnpc -Werror -O0 -fPIC -fvisibility=hidden -g3 -ftrapv -fstack-reuse=none -fstack-protector-all -Wstack-protector -fcf-protection=full -fstack-clash-protection -U_FORTIFY_SOURCE -Wall -Wextra -Wformat -Wvla -Wcast-align -Wunused-parameter -Wmissing-braces -Wredundant-decls -Wsign-compare -Wduplicated-branches -Wduplicated-cond -Wlogical-op -Wformat-security -Wredundant-move -Woverloaded-virtual -Wno-unused-parameter -Wno-implicit-fallthrough -Wno-psabi -pthread -std=gnu++17 -MD -MT src/wallet/CMakeFiles/wallet.dir/rpc/backup.cpp.o -MF src/wallet/CMakeFiles/wallet.dir/rpc/backup.cpp.o.d -o src/wallet/CMakeFiles/wallet.dir/rpc/backup.cpp.o -c ../../src/wallet/rpc/backup.cpp
In file included from ../../src/./script/sign.h:12,
                 from ../../src/./rpc/util.h:14,
                 from ../../src/./rpc/server.h:13,
                 from ../../src/wallet/rpc/backup.cpp:14:
../../src/./script/interpreter.h:150:7: error: ‘ScriptInterpreter’ has a field ‘ScriptInterpreter::vfExec’ whose type uses the anonymous namespace [-Werror=subobject-linkage]
  150 | class ScriptInterpreter {
      |       ^~~~~~~~~~~~~~~~~
cc1plus: all warnings being treated as errors
[430/572] Building CXX object src/seeder/CMakeFiles/bitcoin-seeder.dir/main.cpp.o
[431/572] Building CXX object src/wallet/CMakeFiles/wallet.dir/scriptpubkeyman.cpp.o
FAILED: src/wallet/CMakeFiles/wallet.dir/scriptpubkeyman.cpp.o 
/usr/bin/ccache /usr/bin/c++ -DABORT_ON_FAILED_ASSUME -DBOOST_ALL_NO_LIB -DBUILD_BITCOIN_INTERNAL -DDEBUG -DDEBUG_LOCKORDER -DENABLE_AVX2 -DENABLE_SHANI -DENABLE_SSE41 -DHAVE_BUILD_INFO -DHAVE_CONFIG_H -DHAVE_CONSENSUS_LIB -DLEVELDB_ATOMIC_PRESENT -DLEVELDB_PLATFORM_POSIX -DOS_LINUX -I../../src/. -Isrc -I../../src/univalue/include -Isrc/crypto/.. -I../../src/secp256k1/include -I../../src/leveldb/include -isystem /usr/include/jemalloc -isystem /usr/include/miniupnpc -Werror -O0 -fPIC -fvisibility=hidden -g3 -ftrapv -fstack-reuse=none -fstack-protector-all -Wstack-protector -fcf-protection=full -fstack-clash-protection -U_FORTIFY_SOURCE -Wall -Wextra -Wformat -Wvla -Wcast-align -Wunused-parameter -Wmissing-braces -Wredundant-decls -Wsign-compare -Wduplicated-branches -Wduplicated-cond -Wlogical-op -Wformat-security -Wredundant-move -Woverloaded-virtual -Wno-unused-parameter -Wno-implicit-fallthrough -Wno-psabi -pthread -std=gnu++17 -MD -MT src/wallet/CMakeFiles/wallet.dir/scriptpubkeyman.cpp.o -MF src/wallet/CMakeFiles/wallet.dir/scriptpubkeyman.cpp.o.d -o src/wallet/CMakeFiles/wallet.dir/scriptpubkeyman.cpp.o -c ../../src/wallet/scriptpubkeyman.cpp
In file included from ../../src/./script/sign.h:12,
                 from ../../src/./script/descriptor.h:10,
                 from ../../src/wallet/scriptpubkeyman.cpp:11:
../../src/./script/interpreter.h:150:7: error: ‘ScriptInterpreter’ has a field ‘ScriptInterpreter::vfExec’ whose type uses the anonymous namespace [-Werror=subobject-linkage]
  150 | class ScriptInterpreter {
      |       ^~~~~~~~~~~~~~~~~
cc1plus: all warnings being treated as errors
[432/572] Building CXX object src/wallet/CMakeFiles/wallet.dir/rpcwallet.cpp.o
FAILED: src/wallet/CMakeFiles/wallet.dir/rpcwallet.cpp.o 
/usr/bin/ccache /usr/bin/c++ -DABORT_ON_FAILED_ASSUME -DBOOST_ALL_NO_LIB -DBUILD_BITCOIN_INTERNAL -DDEBUG -DDEBUG_LOCKORDER -DENABLE_AVX2 -DENABLE_SHANI -DENABLE_SSE41 -DHAVE_BUILD_INFO -DHAVE_CONFIG_H -DHAVE_CONSENSUS_LIB -DLEVELDB_ATOMIC_PRESENT -DLEVELDB_PLATFORM_POSIX -DOS_LINUX -I../../src/. -Isrc -I../../src/univalue/include -Isrc/crypto/.. -I../../src/secp256k1/include -I../../src/leveldb/include -isystem /usr/include/jemalloc -isystem /usr/include/miniupnpc -Werror -O0 -fPIC -fvisibility=hidden -g3 -ftrapv -fstack-reuse=none -fstack-protector-all -Wstack-protector -fcf-protection=full -fstack-clash-protection -U_FORTIFY_SOURCE -Wall -Wextra -Wformat -Wvla -Wcast-align -Wunused-parameter -Wmissing-braces -Wredundant-decls -Wsign-compare -Wduplicated-branches -Wduplicated-cond -Wlogical-op -Wformat-security -Wredundant-move -Woverloaded-virtual -Wno-unused-parameter -Wno-implicit-fallthrough -Wno-psabi -pthread -std=gnu++17 -MD -MT src/wallet/CMakeFiles/wallet.dir/rpcwallet.cpp.o -MF src/wallet/CMakeFiles/wallet.dir/rpcwallet.cpp.o.d -o src/wallet/CMakeFiles/wallet.dir/rpcwallet.cpp.o -c ../../src/wallet/rpcwallet.cpp
In file included from ../../src/./script/sign.h:12,
                 from ../../src/./rpc/util.h:14,
                 from ../../src/./rpc/server.h:13,
                 from ../../src/wallet/rpcwallet.cpp:20:
../../src/./script/interpreter.h:150:7: error: ‘ScriptInterpreter’ has a field ‘ScriptInterpreter::vfExec’ whose type uses the anonymous namespace [-Werror=subobject-linkage]
  150 | class ScriptInterpreter {
      |       ^~~~~~~~~~~~~~~~~
cc1plus: all warnings being treated as errors
[433/572] Building CXX object src/seeder/CMakeFiles/seeder-base.dir/bitcoin.cpp.o
[434/572] Building CXX object src/wallet/CMakeFiles/wallet.dir/walletdb.cpp.o
FAILED: src/wallet/CMakeFiles/wallet.dir/walletdb.cpp.o 
/usr/bin/ccache /usr/bin/c++ -DABORT_ON_FAILED_ASSUME -DBOOST_ALL_NO_LIB -DBUILD_BITCOIN_INTERNAL -DDEBUG -DDEBUG_LOCKORDER -DENABLE_AVX2 -DENABLE_SHANI -DENABLE_SSE41 -DHAVE_BUILD_INFO -DHAVE_CONFIG_H -DHAVE_CONSENSUS_LIB -DLEVELDB_ATOMIC_PRESENT -DLEVELDB_PLATFORM_POSIX -DOS_LINUX -I../../src/. -Isrc -I../../src/univalue/include -Isrc/crypto/.. -I../../src/secp256k1/include -I../../src/leveldb/include -isystem /usr/include/jemalloc -isystem /usr/include/miniupnpc -Werror -O0 -fPIC -fvisibility=hidden -g3 -ftrapv -fstack-reuse=none -fstack-protector-all -Wstack-protector -fcf-protection=full -fstack-clash-protection -U_FORTIFY_SOURCE -Wall -Wextra -Wformat -Wvla -Wcast-align -Wunused-parameter -Wmissing-braces -Wredundant-decls -Wsign-compare -Wduplicated-branches -Wduplicated-cond -Wlogical-op -Wformat-security -Wredundant-move -Woverloaded-virtual -Wno-unused-parameter -Wno-implicit-fallthrough -Wno-psabi -pthread -std=gnu++17 -MD -MT src/wallet/CMakeFiles/wallet.dir/walletdb.cpp.o -MF src/wallet/CMakeFiles/wallet.dir/walletdb.cpp.o.d -o src/wallet/CMakeFiles/wallet.dir/walletdb.cpp.o -c ../../src/wallet/walletdb.cpp
In file included from ../../src/./script/sign.h:12,
                 from ../../src/./wallet/walletdb.h:11,
                 from ../../src/wallet/walletdb.cpp:7:
../../src/./script/interpreter.h:150:7: error: ‘ScriptInterpreter’ has a field ‘ScriptInterpreter::vfExec’ whose type uses the anonymous namespace [-Werror=subobject-linkage]
  150 | class ScriptInterpreter {
      |       ^~~~~~~~~~~~~~~~~
cc1plus: all warnings being treated as errors
[435/572] Building CXX object src/wallet/CMakeFiles/wallet.dir/wallet.cpp.o
FAILED: src/wallet/CMakeFiles/wallet.dir/wallet.cpp.o 
/usr/bin/ccache /usr/bin/c++ -DABORT_ON_FAILED_ASSUME -DBOOST_ALL_NO_LIB -DBUILD_BITCOIN_INTERNAL -DDEBUG -DDEBUG_LOCKORDER -DENABLE_AVX2 -DENABLE_SHANI -DENABLE_SSE41 -DHAVE_BUILD_INFO -DHAVE_CONFIG_H -DHAVE_CONSENSUS_LIB -DLEVELDB_ATOMIC_PRESENT -DLEVELDB_PLATFORM_POSIX -DOS_LINUX -I../../src/. -Isrc -I../../src/univalue/include -Isrc/crypto/.. -I../../src/secp256k1/include -I../../src/leveldb/include -isystem /usr/include/jemalloc -isystem /usr/include/miniupnpc -Werror -O0 -fPIC -fvisibility=hidden -g3 -ftrapv -fstack-reuse=none -fstack-protector-all -Wstack-protector -fcf-protection=full -fstack-clash-protection -U_FORTIFY_SOURCE -Wall -Wextra -Wformat -Wvla -Wcast-align -Wunused-parameter -Wmissing-braces -Wredundant-decls -Wsign-compare -Wduplicated-branches -Wduplicated-cond -Wlogical-op -Wformat-security -Wredundant-move -Woverloaded-virtual -Wno-unused-parameter -Wno-implicit-fallthrough -Wno-psabi -pthread -std=gnu++17 -MD -MT src/wallet/CMakeFiles/wallet.dir/wallet.cpp.o -MF src/wallet/CMakeFiles/wallet.dir/wallet.cpp.o.d -o src/wallet/CMakeFiles/wallet.dir/wallet.cpp.o -c ../../src/wallet/wallet.cpp
In file included from ../../src/./script/sign.h:12,
                 from ../../src/./psbt.h:11,
                 from ../../src/./wallet/wallet.h:17,
                 from ../../src/wallet/wallet.cpp:6:
../../src/./script/interpreter.h:150:7: error: ‘ScriptInterpreter’ has a field ‘ScriptInterpreter::vfExec’ whose type uses the anonymous namespace [-Werror=subobject-linkage]
  150 | class ScriptInterpreter {
      |       ^~~~~~~~~~~~~~~~~
cc1plus: all warnings being treated as errors
ninja: build stopped: cannot make progress due to previous errors.
Build build-debug failed with exit code 1

Build Bitcoin ABC Diffs / Diff Testing (build-diff) failed.

Tail of the build log:

                 from ../../src/validation.cpp:49:
../../src/./script/interpreter.h:150:7: error: ‘ScriptInterpreter’ has a field ‘ScriptInterpreter::vfExec’ whose type uses the anonymous namespace [-Werror=subobject-linkage]
  150 | class ScriptInterpreter {
      |       ^~~~~~~~~~~~~~~~~
cc1plus: all warnings being treated as errors
[431/572] Building CXX object src/wallet/CMakeFiles/wallet.dir/scriptpubkeyman.cpp.o
FAILED: src/wallet/CMakeFiles/wallet.dir/scriptpubkeyman.cpp.o 
/usr/bin/ccache /usr/bin/c++ -DBOOST_ALL_NO_LIB -DBUILD_BITCOIN_INTERNAL -DENABLE_AVX2 -DENABLE_SHANI -DENABLE_SSE41 -DHAVE_BUILD_INFO -DHAVE_CONFIG_H -DHAVE_CONSENSUS_LIB -DLEVELDB_ATOMIC_PRESENT -DLEVELDB_PLATFORM_POSIX -DOS_LINUX -I../../src/. -Isrc -I../../src/univalue/include -Isrc/crypto/.. -I../../src/secp256k1/include -I../../src/leveldb/include -isystem /usr/include/jemalloc -isystem /usr/include/miniupnpc -Werror -g -O2 -fPIC -fvisibility=hidden -fstack-reuse=none -fstack-protector-all -Wstack-protector -fcf-protection=full -fstack-clash-protection -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -Wall -Wextra -Wformat -Wvla -Wcast-align -Wunused-parameter -Wmissing-braces -Wredundant-decls -Wsign-compare -Wduplicated-branches -Wduplicated-cond -Wlogical-op -Wformat-security -Wredundant-move -Woverloaded-virtual -Wno-unused-parameter -Wno-implicit-fallthrough -Wno-psabi -pthread -std=gnu++17 -MD -MT src/wallet/CMakeFiles/wallet.dir/scriptpubkeyman.cpp.o -MF src/wallet/CMakeFiles/wallet.dir/scriptpubkeyman.cpp.o.d -o src/wallet/CMakeFiles/wallet.dir/scriptpubkeyman.cpp.o -c ../../src/wallet/scriptpubkeyman.cpp
In file included from ../../src/./script/sign.h:12,
                 from ../../src/./script/descriptor.h:10,
                 from ../../src/wallet/scriptpubkeyman.cpp:11:
../../src/./script/interpreter.h:150:7: error: ‘ScriptInterpreter’ has a field ‘ScriptInterpreter::vfExec’ whose type uses the anonymous namespace [-Werror=subobject-linkage]
  150 | class ScriptInterpreter {
      |       ^~~~~~~~~~~~~~~~~
cc1plus: all warnings being treated as errors
[432/572] Building CXX object src/wallet/CMakeFiles/wallet.dir/walletdb.cpp.o
FAILED: src/wallet/CMakeFiles/wallet.dir/walletdb.cpp.o 
/usr/bin/ccache /usr/bin/c++ -DBOOST_ALL_NO_LIB -DBUILD_BITCOIN_INTERNAL -DENABLE_AVX2 -DENABLE_SHANI -DENABLE_SSE41 -DHAVE_BUILD_INFO -DHAVE_CONFIG_H -DHAVE_CONSENSUS_LIB -DLEVELDB_ATOMIC_PRESENT -DLEVELDB_PLATFORM_POSIX -DOS_LINUX -I../../src/. -Isrc -I../../src/univalue/include -Isrc/crypto/.. -I../../src/secp256k1/include -I../../src/leveldb/include -isystem /usr/include/jemalloc -isystem /usr/include/miniupnpc -Werror -g -O2 -fPIC -fvisibility=hidden -fstack-reuse=none -fstack-protector-all -Wstack-protector -fcf-protection=full -fstack-clash-protection -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -Wall -Wextra -Wformat -Wvla -Wcast-align -Wunused-parameter -Wmissing-braces -Wredundant-decls -Wsign-compare -Wduplicated-branches -Wduplicated-cond -Wlogical-op -Wformat-security -Wredundant-move -Woverloaded-virtual -Wno-unused-parameter -Wno-implicit-fallthrough -Wno-psabi -pthread -std=gnu++17 -MD -MT src/wallet/CMakeFiles/wallet.dir/walletdb.cpp.o -MF src/wallet/CMakeFiles/wallet.dir/walletdb.cpp.o.d -o src/wallet/CMakeFiles/wallet.dir/walletdb.cpp.o -c ../../src/wallet/walletdb.cpp
In file included from ../../src/./script/sign.h:12,
                 from ../../src/./wallet/walletdb.h:11,
                 from ../../src/wallet/walletdb.cpp:7:
../../src/./script/interpreter.h:150:7: error: ‘ScriptInterpreter’ has a field ‘ScriptInterpreter::vfExec’ whose type uses the anonymous namespace [-Werror=subobject-linkage]
  150 | class ScriptInterpreter {
      |       ^~~~~~~~~~~~~~~~~
cc1plus: all warnings being treated as errors
[433/572] Building CXX object src/wallet/CMakeFiles/wallet.dir/rpc/backup.cpp.o
FAILED: src/wallet/CMakeFiles/wallet.dir/rpc/backup.cpp.o 
/usr/bin/ccache /usr/bin/c++ -DBOOST_ALL_NO_LIB -DBUILD_BITCOIN_INTERNAL -DENABLE_AVX2 -DENABLE_SHANI -DENABLE_SSE41 -DHAVE_BUILD_INFO -DHAVE_CONFIG_H -DHAVE_CONSENSUS_LIB -DLEVELDB_ATOMIC_PRESENT -DLEVELDB_PLATFORM_POSIX -DOS_LINUX -I../../src/. -Isrc -I../../src/univalue/include -Isrc/crypto/.. -I../../src/secp256k1/include -I../../src/leveldb/include -isystem /usr/include/jemalloc -isystem /usr/include/miniupnpc -Werror -g -O2 -fPIC -fvisibility=hidden -fstack-reuse=none -fstack-protector-all -Wstack-protector -fcf-protection=full -fstack-clash-protection -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -Wall -Wextra -Wformat -Wvla -Wcast-align -Wunused-parameter -Wmissing-braces -Wredundant-decls -Wsign-compare -Wduplicated-branches -Wduplicated-cond -Wlogical-op -Wformat-security -Wredundant-move -Woverloaded-virtual -Wno-unused-parameter -Wno-implicit-fallthrough -Wno-psabi -pthread -std=gnu++17 -MD -MT src/wallet/CMakeFiles/wallet.dir/rpc/backup.cpp.o -MF src/wallet/CMakeFiles/wallet.dir/rpc/backup.cpp.o.d -o src/wallet/CMakeFiles/wallet.dir/rpc/backup.cpp.o -c ../../src/wallet/rpc/backup.cpp
In file included from ../../src/./script/sign.h:12,
                 from ../../src/./rpc/util.h:14,
                 from ../../src/./rpc/server.h:13,
                 from ../../src/wallet/rpc/backup.cpp:14:
../../src/./script/interpreter.h:150:7: error: ‘ScriptInterpreter’ has a field ‘ScriptInterpreter::vfExec’ whose type uses the anonymous namespace [-Werror=subobject-linkage]
  150 | class ScriptInterpreter {
      |       ^~~~~~~~~~~~~~~~~
cc1plus: all warnings being treated as errors
[434/572] Building CXX object src/wallet/CMakeFiles/wallet.dir/wallet.cpp.o
FAILED: src/wallet/CMakeFiles/wallet.dir/wallet.cpp.o 
/usr/bin/ccache /usr/bin/c++ -DBOOST_ALL_NO_LIB -DBUILD_BITCOIN_INTERNAL -DENABLE_AVX2 -DENABLE_SHANI -DENABLE_SSE41 -DHAVE_BUILD_INFO -DHAVE_CONFIG_H -DHAVE_CONSENSUS_LIB -DLEVELDB_ATOMIC_PRESENT -DLEVELDB_PLATFORM_POSIX -DOS_LINUX -I../../src/. -Isrc -I../../src/univalue/include -Isrc/crypto/.. -I../../src/secp256k1/include -I../../src/leveldb/include -isystem /usr/include/jemalloc -isystem /usr/include/miniupnpc -Werror -g -O2 -fPIC -fvisibility=hidden -fstack-reuse=none -fstack-protector-all -Wstack-protector -fcf-protection=full -fstack-clash-protection -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -Wall -Wextra -Wformat -Wvla -Wcast-align -Wunused-parameter -Wmissing-braces -Wredundant-decls -Wsign-compare -Wduplicated-branches -Wduplicated-cond -Wlogical-op -Wformat-security -Wredundant-move -Woverloaded-virtual -Wno-unused-parameter -Wno-implicit-fallthrough -Wno-psabi -pthread -std=gnu++17 -MD -MT src/wallet/CMakeFiles/wallet.dir/wallet.cpp.o -MF src/wallet/CMakeFiles/wallet.dir/wallet.cpp.o.d -o src/wallet/CMakeFiles/wallet.dir/wallet.cpp.o -c ../../src/wallet/wallet.cpp
In file included from ../../src/./script/sign.h:12,
                 from ../../src/./psbt.h:11,
                 from ../../src/./wallet/wallet.h:17,
                 from ../../src/wallet/wallet.cpp:6:
../../src/./script/interpreter.h:150:7: error: ‘ScriptInterpreter’ has a field ‘ScriptInterpreter::vfExec’ whose type uses the anonymous namespace [-Werror=subobject-linkage]
  150 | class ScriptInterpreter {
      |       ^~~~~~~~~~~~~~~~~
cc1plus: all warnings being treated as errors
[435/572] Building CXX object src/wallet/CMakeFiles/wallet.dir/rpcwallet.cpp.o
FAILED: src/wallet/CMakeFiles/wallet.dir/rpcwallet.cpp.o 
/usr/bin/ccache /usr/bin/c++ -DBOOST_ALL_NO_LIB -DBUILD_BITCOIN_INTERNAL -DENABLE_AVX2 -DENABLE_SHANI -DENABLE_SSE41 -DHAVE_BUILD_INFO -DHAVE_CONFIG_H -DHAVE_CONSENSUS_LIB -DLEVELDB_ATOMIC_PRESENT -DLEVELDB_PLATFORM_POSIX -DOS_LINUX -I../../src/. -Isrc -I../../src/univalue/include -Isrc/crypto/.. -I../../src/secp256k1/include -I../../src/leveldb/include -isystem /usr/include/jemalloc -isystem /usr/include/miniupnpc -Werror -g -O2 -fPIC -fvisibility=hidden -fstack-reuse=none -fstack-protector-all -Wstack-protector -fcf-protection=full -fstack-clash-protection -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -Wall -Wextra -Wformat -Wvla -Wcast-align -Wunused-parameter -Wmissing-braces -Wredundant-decls -Wsign-compare -Wduplicated-branches -Wduplicated-cond -Wlogical-op -Wformat-security -Wredundant-move -Woverloaded-virtual -Wno-unused-parameter -Wno-implicit-fallthrough -Wno-psabi -pthread -std=gnu++17 -MD -MT src/wallet/CMakeFiles/wallet.dir/rpcwallet.cpp.o -MF src/wallet/CMakeFiles/wallet.dir/rpcwallet.cpp.o.d -o src/wallet/CMakeFiles/wallet.dir/rpcwallet.cpp.o -c ../../src/wallet/rpcwallet.cpp
In file included from ../../src/./script/sign.h:12,
                 from ../../src/./rpc/util.h:14,
                 from ../../src/./rpc/server.h:13,
                 from ../../src/wallet/rpcwallet.cpp:20:
../../src/./script/interpreter.h:150:7: error: ‘ScriptInterpreter’ has a field ‘ScriptInterpreter::vfExec’ whose type uses the anonymous namespace [-Werror=subobject-linkage]
  150 | class ScriptInterpreter {
      |       ^~~~~~~~~~~~~~~~~
cc1plus: all warnings being treated as errors
ninja: build stopped: cannot make progress due to previous errors.
Build build-diff failed with exit code 1

remove anonymous namespace for ConditionStack

Harbormaster completed remote builds in B30212: Diff 49537.Sep 6 2024, 19:57

tobias_ruck added a child revision: Restricted Differential Revision.Sep 6 2024, 20:29

rebase onto D16766, refactor slightly

tobias_ruck edited the summary of this revision. (Show Details)Sep 16 2024, 09:59

Harbormaster completed remote builds in B30294: Diff 49643.Sep 16 2024, 10:13

tobias_ruck published this revision for review.Sep 16 2024, 10:49

tobias_ruck edited the summary of this revision. (Show Details)

tobias_ruck added a parent revision: D16766: [Iguana] Move ConditionStack to conditionstack.h.

let's add some tests

add tests, refactor a bit

exposing CastToBool is not needed

Harbormaster completed remote builds in B30298: Diff 49649.Sep 16 2024, 17:08

Harbormaster completed remote builds in B30299: Diff 49650.Sep 16 2024, 17:19

@bot build-bench build-fuzzer

teamcity awarded a token.Sep 17 2024, 07:42

Fabien requested changes to this revision.Sep 17 2024, 08:24

Fabien added inline comments.

src/script/interpreter.cpp
151 ↗	(On Diff #49650)	This is technically true but also leaks (and relies on ) the internal implementation. Not all iterators have such a comparison feature that does what you expect. You should add a static assert that pc and pend are of the prevector iterator type to be on the safe side, or use the always valid !=operator instead (but I prefer <). This can also be done in another diff as this code is not introduced by this diff.
154 ↗	(On Diff #49650)	This is never used, and would actually drop the ScriptError::BAD_OPCODE error if used
src/test/scriptinterpreter_tests.cpp
111 ↗	(On Diff #49650)	This looks weird... shouldn't IsRunning() return `pc < pend && script_error == ScriptError::UNKNOWN;` ?

This revision now requires changes to proceed.Sep 17 2024, 08:24

tobias_ruck added inline comments.Sep 17 2024, 08:38

src/script/interpreter.cpp
151 ↗	(On Diff #49650)	Well I just moved what the previous interpreter did here; maybe we can refactor it after this diff. I'm also not sure if there's a way to jump over pend with some oversized opcode. But the static_assert makes sense, no clue how to write that though
154 ↗	(On Diff #49650)	Oh true; I can remove it. It's needed later but out of scope here.
src/test/scriptinterpreter_tests.cpp
111 ↗	(On Diff #49650)	Well in case of an integer overflow (i.e. an exception), the error is also set to UNKNOWN, so that wouldn't catch that. I thought I'd keep the `IsRunning` simple, maybe we can add a internal flag in the future. Alternatively we can also call it `HasReachedEnd` or `IsAtEOF` or so, which is more exact

remove GetNextOp, rename some functions to map more closely to what they do

The goal is to simply be a reshuffling of existing code without introducing any new behavior

Harbormaster completed remote builds in B30316: Diff 49669.Sep 17 2024, 09:18

Fabien requested changes to this revision.Sep 17 2024, 09:42

Fabien added inline comments.

src/script/interpreter.cpp
151 ↗	(On Diff #49669)	pc >= pend
175 ↗	(On Diff #49669)

This revision now requires changes to proceed.Sep 17 2024, 09:42

fix !

Fabien accepted this revision.Sep 17 2024, 10:06

This revision is now accepted and ready to land.Sep 17 2024, 10:06

Harbormaster completed remote builds in B30317: Diff 49670.Sep 17 2024, 10:23

Closed by commit rABCf2fd05ff267c: [Iguana] Add `ScriptInterpreter` class, make `EvalScript` use it (authored by tobias_ruck). · Explain WhySep 17 2024, 12:17

This revision was automatically updated to reflect the committed changes.

tobias_ruck added a commit: rABCf2fd05ff267c: [Iguana] Add `ScriptInterpreter` class, make `EvalScript` use it.

[Iguana] Add `ScriptInterpreter` class, make `EvalScript` use it
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 49670

src/script/interpreter.h

src/script/interpreter.cpp

src/test/CMakeLists.txt

src/test/scriptinterpreter_tests.cpp

[Iguana] Add `ScriptInterpreter` class, make `EvalScript` use itClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 49670

src/script/interpreter.h

src/script/interpreter.cpp

src/test/CMakeLists.txt

src/test/scriptinterpreter_tests.cpp

[Iguana] Add `ScriptInterpreter` class, make `EvalScript` use it
ClosedPublic
Actions

Revision Contents
Changeset List