Page MenuHomePhabricator

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

Authored by tobias_ruck on Fri, Sep 6, 17:39.

Details

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
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

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

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

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

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

rebase onto D16766, refactor slightly

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

let's add some tests

exposing CastToBool is not needed

@bot build-bench build-fuzzer

Fabien requested changes to this revision.Tue, Sep 17, 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.Tue, Sep 17, 08:24
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

Fabien requested changes to this revision.Tue, Sep 17, 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.Tue, Sep 17, 09:42
This revision is now accepted and ready to land.Tue, Sep 17, 10:06