diff options
37 files changed, 361 insertions, 183 deletions
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 1bdaf8523d..0ef705250d 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -23,6 +23,28 @@ env: MAKEJOBS: '-j10' jobs: + test-each-commit: + name: 'test each commit' + runs-on: ubuntu-22.04 + if: github.event_name == 'pull_request' && github.event.pull_request.commits != 1 + timeout-minutes: 360 # Use maximum time, see https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#jobsjob_idtimeout-minutes. Assuming a worst case time of 1 hour per commit, this leads to a --max-count=6 below. + env: + MAX_COUNT: 6 + steps: + - run: echo "FETCH_DEPTH=$((${{ github.event.pull_request.commits }} + 2))" >> "$GITHUB_ENV" + - uses: actions/checkout@v4 + with: + ref: ${{ github.event.pull_request.head.sha }} + fetch-depth: ${{ env.FETCH_DEPTH }} + - run: | + git checkout HEAD~ + echo "COMMIT_AFTER_LAST_MERGE=$(git log $(git log --merges -1 --format=%H)..HEAD --format=%H --max-count=${{ env.MAX_COUNT }} | tail -1)" >> "$GITHUB_ENV" + - run: sudo apt install clang ccache build-essential libtool autotools-dev automake pkg-config bsdmainutils python3-zmq libevent-dev libboost-dev libsqlite3-dev libdb++-dev systemtap-sdt-dev libminiupnpc-dev libnatpmp-dev libqt5gui5 libqt5core5a libqt5dbus5 qttools5-dev qttools5-dev-tools qtwayland5 libqrencode-dev -y + - name: Compile and run tests + run: | + # Use clang++, because it is a bit faster and uses less memory than g++ + git rebase --exec "echo Running test-one-commit on \$( git log -1 ) && ./autogen.sh && CC=clang CXX=clang++ ./configure && make clean && make -j $(nproc) check && ./test/functional/test_runner.py -j $(( $(nproc) * 2 ))" ${{ env.COMMIT_AFTER_LAST_MERGE }}~1 + macos-native-x86_64: name: 'macOS 13 native, x86_64, no depends, sqlite only, gui' # Use latest image, but hardcode version to avoid silent upgrades (and breaks). diff --git a/ci/test/01_base_install.sh b/ci/test/01_base_install.sh index d8faae8340..b05d7547c8 100755 --- a/ci/test/01_base_install.sh +++ b/ci/test/01_base_install.sh @@ -42,7 +42,7 @@ if [ -n "$PIP_PACKAGES" ]; then fi if [[ ${USE_MEMORY_SANITIZER} == "true" ]]; then - git clone --depth=1 https://github.com/llvm/llvm-project -b llvmorg-16.0.6 /msan/llvm-project + git clone --depth=1 https://github.com/llvm/llvm-project -b "llvmorg-17.0.0-rc4" /msan/llvm-project cmake -G Ninja -B /msan/clang_build/ \ -DLLVM_ENABLE_PROJECTS="clang" \ @@ -66,8 +66,7 @@ if [[ ${USE_MEMORY_SANITIZER} == "true" ]]; then -DCMAKE_CXX_COMPILER=clang++ \ -DLLVM_TARGETS_TO_BUILD=Native \ -DLLVM_ENABLE_PER_TARGET_RUNTIME_DIR=OFF \ - -DLIBCXX_ENABLE_DEBUG_MODE=ON \ - -DLIBCXX_ENABLE_ASSERTIONS=ON \ + -DLIBCXX_HARDENING_MODE=debug \ -S /msan/llvm-project/runtimes ninja -C /msan/cxx_build/ "$MAKEJOBS" diff --git a/depends/hosts/linux.mk b/depends/hosts/linux.mk index 0e2496174e..1f33640c66 100644 --- a/depends/hosts/linux.mk +++ b/depends/hosts/linux.mk @@ -17,7 +17,7 @@ linux_release_CXXFLAGS=$(linux_release_CFLAGS) linux_debug_CFLAGS=-O1 linux_debug_CXXFLAGS=$(linux_debug_CFLAGS) -linux_debug_CPPFLAGS=-D_GLIBCXX_DEBUG -D_GLIBCXX_DEBUG_PEDANTIC -D_LIBCPP_ENABLE_ASSERTIONS=1 +linux_debug_CPPFLAGS=-D_GLIBCXX_DEBUG -D_GLIBCXX_DEBUG_PEDANTIC -D_LIBCPP_ENABLE_DEBUG_MODE=1 ifeq (86,$(findstring 86,$(build_arch))) i686_linux_CC=gcc -m32 diff --git a/src/addrdb.cpp b/src/addrdb.cpp index 50f576624c..8b85b77e2b 100644 --- a/src/addrdb.cpp +++ b/src/addrdb.cpp @@ -216,14 +216,14 @@ util::Result<std::unique_ptr<AddrMan>> LoadAddrman(const NetGroupManager& netgro void DumpAnchors(const fs::path& anchors_db_path, const std::vector<CAddress>& anchors) { LOG_TIME_SECONDS(strprintf("Flush %d outbound block-relay-only peer addresses to anchors.dat", anchors.size())); - SerializeFileDB("anchors", anchors_db_path, WithParams(CAddress::V2_DISK, anchors)); + SerializeFileDB("anchors", anchors_db_path, CAddress::V2_DISK(anchors)); } std::vector<CAddress> ReadAnchors(const fs::path& anchors_db_path) { std::vector<CAddress> anchors; try { - DeserializeFileDB(anchors_db_path, WithParams(CAddress::V2_DISK, anchors)); + DeserializeFileDB(anchors_db_path, CAddress::V2_DISK(anchors)); LogPrintf("Loaded %i addresses from %s\n", anchors.size(), fs::quoted(fs::PathToString(anchors_db_path.filename()))); } catch (const std::exception&) { anchors.clear(); diff --git a/src/addresstype.cpp b/src/addresstype.cpp index 2454cfb5d9..f199d1b479 100644 --- a/src/addresstype.cpp +++ b/src/addresstype.cpp @@ -54,11 +54,12 @@ bool ExtractDestination(const CScript& scriptPubKey, CTxDestination& addressRet) switch (whichType) { case TxoutType::PUBKEY: { CPubKey pubKey(vSolutions[0]); - if (!pubKey.IsValid()) - return false; - - addressRet = PKHash(pubKey); - return true; + if (!pubKey.IsValid()) { + addressRet = CNoDestination(scriptPubKey); + } else { + addressRet = PubKeyDestination(pubKey); + } + return false; } case TxoutType::PUBKEYHASH: { addressRet = PKHash(uint160(vSolutions[0])); @@ -87,16 +88,13 @@ bool ExtractDestination(const CScript& scriptPubKey, CTxDestination& addressRet) return true; } case TxoutType::WITNESS_UNKNOWN: { - WitnessUnknown unk; - unk.version = vSolutions[0][0]; - std::copy(vSolutions[1].begin(), vSolutions[1].end(), unk.program); - unk.length = vSolutions[1].size(); - addressRet = unk; + addressRet = WitnessUnknown{vSolutions[0][0], vSolutions[1]}; return true; } case TxoutType::MULTISIG: case TxoutType::NULL_DATA: case TxoutType::NONSTANDARD: + addressRet = CNoDestination(scriptPubKey); return false; } // no default case, so the compiler can warn about missing cases assert(false); @@ -108,7 +106,12 @@ class CScriptVisitor public: CScript operator()(const CNoDestination& dest) const { - return CScript(); + return dest.GetScript(); + } + + CScript operator()(const PubKeyDestination& dest) const + { + return CScript() << ToByteVector(dest.GetPubKey()) << OP_CHECKSIG; } CScript operator()(const PKHash& keyID) const @@ -138,9 +141,22 @@ public: CScript operator()(const WitnessUnknown& id) const { - return CScript() << CScript::EncodeOP_N(id.version) << std::vector<unsigned char>(id.program, id.program + id.length); + return CScript() << CScript::EncodeOP_N(id.GetWitnessVersion()) << id.GetWitnessProgram(); } }; + +class ValidDestinationVisitor +{ +public: + bool operator()(const CNoDestination& dest) const { return false; } + bool operator()(const PubKeyDestination& dest) const { return false; } + bool operator()(const PKHash& dest) const { return true; } + bool operator()(const ScriptHash& dest) const { return true; } + bool operator()(const WitnessV0KeyHash& dest) const { return true; } + bool operator()(const WitnessV0ScriptHash& dest) const { return true; } + bool operator()(const WitnessV1Taproot& dest) const { return true; } + bool operator()(const WitnessUnknown& dest) const { return true; } +}; } // namespace CScript GetScriptForDestination(const CTxDestination& dest) @@ -149,5 +165,5 @@ CScript GetScriptForDestination(const CTxDestination& dest) } bool IsValidDestination(const CTxDestination& dest) { - return dest.index() != 0; + return std::visit(ValidDestinationVisitor(), dest); } diff --git a/src/addresstype.h b/src/addresstype.h index 6b651e9014..d3422c6813 100644 --- a/src/addresstype.h +++ b/src/addresstype.h @@ -14,9 +14,30 @@ #include <algorithm> class CNoDestination { +private: + CScript m_script; + +public: + CNoDestination() = default; + CNoDestination(const CScript& script) : m_script(script) {} + + const CScript& GetScript() const { return m_script; } + + friend bool operator==(const CNoDestination& a, const CNoDestination& b) { return a.GetScript() == b.GetScript(); } + friend bool operator<(const CNoDestination& a, const CNoDestination& b) { return a.GetScript() < b.GetScript(); } +}; + +struct PubKeyDestination { +private: + CPubKey m_pubkey; + public: - friend bool operator==(const CNoDestination &a, const CNoDestination &b) { return true; } - friend bool operator<(const CNoDestination &a, const CNoDestination &b) { return true; } + PubKeyDestination(const CPubKey& pubkey) : m_pubkey(pubkey) {} + + const CPubKey& GetPubKey() const LIFETIMEBOUND { return m_pubkey; } + + friend bool operator==(const PubKeyDestination& a, const PubKeyDestination& b) { return a.GetPubKey() == b.GetPubKey(); } + friend bool operator<(const PubKeyDestination& a, const PubKeyDestination& b) { return a.GetPubKey() < b.GetPubKey(); } }; struct PKHash : public BaseHash<uint160> @@ -69,45 +90,55 @@ struct WitnessV1Taproot : public XOnlyPubKey //! CTxDestination subtype to encode any future Witness version struct WitnessUnknown { - unsigned int version; - unsigned int length; - unsigned char program[40]; +private: + unsigned int m_version; + std::vector<unsigned char> m_program; + +public: + WitnessUnknown(unsigned int version, const std::vector<unsigned char>& program) : m_version(version), m_program(program) {} + WitnessUnknown(int version, const std::vector<unsigned char>& program) : m_version(static_cast<unsigned int>(version)), m_program(program) {} + + unsigned int GetWitnessVersion() const { return m_version; } + const std::vector<unsigned char>& GetWitnessProgram() const LIFETIMEBOUND { return m_program; } friend bool operator==(const WitnessUnknown& w1, const WitnessUnknown& w2) { - if (w1.version != w2.version) return false; - if (w1.length != w2.length) return false; - return std::equal(w1.program, w1.program + w1.length, w2.program); + if (w1.GetWitnessVersion() != w2.GetWitnessVersion()) return false; + return w1.GetWitnessProgram() == w2.GetWitnessProgram(); } friend bool operator<(const WitnessUnknown& w1, const WitnessUnknown& w2) { - if (w1.version < w2.version) return true; - if (w1.version > w2.version) return false; - if (w1.length < w2.length) return true; - if (w1.length > w2.length) return false; - return std::lexicographical_compare(w1.program, w1.program + w1.length, w2.program, w2.program + w2.length); + if (w1.GetWitnessVersion() < w2.GetWitnessVersion()) return true; + if (w1.GetWitnessVersion() > w2.GetWitnessVersion()) return false; + return w1.GetWitnessProgram() < w2.GetWitnessProgram(); } }; /** - * A txout script template with a specific destination. It is either: - * * CNoDestination: no destination set - * * PKHash: TxoutType::PUBKEYHASH destination (P2PKH) - * * ScriptHash: TxoutType::SCRIPTHASH destination (P2SH) - * * WitnessV0ScriptHash: TxoutType::WITNESS_V0_SCRIPTHASH destination (P2WSH) - * * WitnessV0KeyHash: TxoutType::WITNESS_V0_KEYHASH destination (P2WPKH) - * * WitnessV1Taproot: TxoutType::WITNESS_V1_TAPROOT destination (P2TR) - * * WitnessUnknown: TxoutType::WITNESS_UNKNOWN destination (P2W???) + * A txout script categorized into standard templates. + * * CNoDestination: Optionally a script, no corresponding address. + * * PubKeyDestination: TxoutType::PUBKEY (P2PK), no corresponding address + * * PKHash: TxoutType::PUBKEYHASH destination (P2PKH address) + * * ScriptHash: TxoutType::SCRIPTHASH destination (P2SH address) + * * WitnessV0ScriptHash: TxoutType::WITNESS_V0_SCRIPTHASH destination (P2WSH address) + * * WitnessV0KeyHash: TxoutType::WITNESS_V0_KEYHASH destination (P2WPKH address) + * * WitnessV1Taproot: TxoutType::WITNESS_V1_TAPROOT destination (P2TR address) + * * WitnessUnknown: TxoutType::WITNESS_UNKNOWN destination (P2W??? address) * A CTxDestination is the internal data type encoded in a bitcoin address */ -using CTxDestination = std::variant<CNoDestination, PKHash, ScriptHash, WitnessV0ScriptHash, WitnessV0KeyHash, WitnessV1Taproot, WitnessUnknown>; +using CTxDestination = std::variant<CNoDestination, PubKeyDestination, PKHash, ScriptHash, WitnessV0ScriptHash, WitnessV0KeyHash, WitnessV1Taproot, WitnessUnknown>; -/** Check whether a CTxDestination is a CNoDestination. */ +/** Check whether a CTxDestination corresponds to one with an address. */ bool IsValidDestination(const CTxDestination& dest); /** - * Parse a standard scriptPubKey for the destination address. Assigns result to - * the addressRet parameter and returns true if successful. Currently only works for P2PK, - * P2PKH, P2SH, P2WPKH, and P2WSH scripts. + * Parse a scriptPubKey for the destination. + * + * For standard scripts that have addresses (and P2PK as an exception), a corresponding CTxDestination + * is assigned to addressRet. + * For all other scripts. addressRet is assigned as a CNoDestination containing the scriptPubKey. + * + * Returns true for standard destinations with addresses - P2PKH, P2SH, P2WPKH, P2WSH, P2TR and P2W??? scripts. + * Returns false for non-standard destinations and those without addresses - P2PK, bare multisig, null data, and nonstandard scripts. */ bool ExtractDestination(const CScript& scriptPubKey, CTxDestination& addressRet); diff --git a/src/key_io.cpp b/src/key_io.cpp index 1a0b51a28b..5bcbb8a069 100644 --- a/src/key_io.cpp +++ b/src/key_io.cpp @@ -67,16 +67,18 @@ public: std::string operator()(const WitnessUnknown& id) const { - if (id.version < 1 || id.version > 16 || id.length < 2 || id.length > 40) { + const std::vector<unsigned char>& program = id.GetWitnessProgram(); + if (id.GetWitnessVersion() < 1 || id.GetWitnessVersion() > 16 || program.size() < 2 || program.size() > 40) { return {}; } - std::vector<unsigned char> data = {(unsigned char)id.version}; - data.reserve(1 + (id.length * 8 + 4) / 5); - ConvertBits<8, 5, true>([&](unsigned char c) { data.push_back(c); }, id.program, id.program + id.length); + std::vector<unsigned char> data = {(unsigned char)id.GetWitnessVersion()}; + data.reserve(1 + (program.size() * 8 + 4) / 5); + ConvertBits<8, 5, true>([&](unsigned char c) { data.push_back(c); }, program.begin(), program.end()); return bech32::Encode(bech32::Encoding::BECH32M, m_params.Bech32HRP(), data); } std::string operator()(const CNoDestination& no) const { return {}; } + std::string operator()(const PubKeyDestination& pk) const { return {}; } }; CTxDestination DecodeDestination(const std::string& str, const CChainParams& params, std::string& error_str, std::vector<int>* error_locations) @@ -189,11 +191,7 @@ CTxDestination DecodeDestination(const std::string& str, const CChainParams& par return CNoDestination(); } - WitnessUnknown unk; - unk.version = version; - std::copy(data.begin(), data.end(), unk.program); - unk.length = data.size(); - return unk; + return WitnessUnknown{version, data}; } else { error_str = strprintf("Invalid padding in Bech32 data section"); return CNoDestination(); diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 6b415b3a1e..b046b3ac16 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -1415,8 +1415,8 @@ void PeerManagerImpl::PushNodeVersion(CNode& pnode, const Peer& peer) const bool tx_relay{!RejectIncomingTxs(pnode)}; m_connman.PushMessage(&pnode, CNetMsgMaker(INIT_PROTO_VERSION).Make(NetMsgType::VERSION, PROTOCOL_VERSION, my_services, nTime, - your_services, WithParams(CNetAddr::V1, addr_you), // Together the pre-version-31402 serialization of CAddress "addrYou" (without nTime) - my_services, WithParams(CNetAddr::V1, CService{}), // Together the pre-version-31402 serialization of CAddress "addrMe" (without nTime) + your_services, CNetAddr::V1(addr_you), // Together the pre-version-31402 serialization of CAddress "addrYou" (without nTime) + my_services, CNetAddr::V1(CService{}), // Together the pre-version-31402 serialization of CAddress "addrMe" (without nTime) nonce, strSubVersion, nNodeStartingHeight, tx_relay)); if (fLogIPs) { @@ -3293,7 +3293,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, nTime = 0; } vRecv.ignore(8); // Ignore the addrMe service bits sent by the peer - vRecv >> WithParams(CNetAddr::V1, addrMe); + vRecv >> CNetAddr::V1(addrMe); if (!pfrom.IsInboundConn()) { m_addrman.SetServices(pfrom.addr, nServices); diff --git a/src/netaddress.h b/src/netaddress.h index a0944c886f..7d3659a11a 100644 --- a/src/netaddress.h +++ b/src/netaddress.h @@ -218,6 +218,7 @@ public: }; struct SerParams { const Encoding enc; + SER_PARAMS_OPFUNC }; static constexpr SerParams V1{Encoding::V1}; static constexpr SerParams V2{Encoding::V2}; diff --git a/src/protocol.h b/src/protocol.h index 22e2108afb..56668898e4 100644 --- a/src/protocol.h +++ b/src/protocol.h @@ -396,6 +396,7 @@ public: }; struct SerParams : CNetAddr::SerParams { const Format fmt; + SER_PARAMS_OPFUNC }; static constexpr SerParams V1_NETWORK{{CNetAddr::Encoding::V1}, Format::Network}; static constexpr SerParams V2_NETWORK{{CNetAddr::Encoding::V2}, Format::Network}; diff --git a/src/rpc/output_script.cpp b/src/rpc/output_script.cpp index 4dd424fa14..f9343f48a8 100644 --- a/src/rpc/output_script.cpp +++ b/src/rpc/output_script.cpp @@ -280,6 +280,11 @@ static RPCHelpMan deriveaddresses() for (const CScript& script : scripts) { CTxDestination dest; if (!ExtractDestination(script, dest)) { + // ExtractDestination no longer returns true for P2PK since it doesn't have a corresponding address + // However combo will output P2PK and should just ignore that script + if (scripts.size() > 1 && std::get_if<PubKeyDestination>(&dest)) { + continue; + } throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Descriptor does not have a corresponding address"); } diff --git a/src/rpc/util.cpp b/src/rpc/util.cpp index 74ef04033e..9a941be181 100644 --- a/src/rpc/util.cpp +++ b/src/rpc/util.cpp @@ -253,6 +253,11 @@ public: return UniValue(UniValue::VOBJ); } + UniValue operator()(const PubKeyDestination& dest) const + { + return UniValue(UniValue::VOBJ); + } + UniValue operator()(const PKHash& keyID) const { UniValue obj(UniValue::VOBJ); @@ -303,8 +308,8 @@ public: { UniValue obj(UniValue::VOBJ); obj.pushKV("iswitness", true); - obj.pushKV("witness_version", (int)id.version); - obj.pushKV("witness_program", HexStr({id.program, id.length})); + obj.pushKV("witness_version", id.GetWitnessVersion()); + obj.pushKV("witness_program", HexStr(id.GetWitnessProgram())); return obj; } }; diff --git a/src/serialize.h b/src/serialize.h index f1595077e9..1ad8ac4373 100644 --- a/src/serialize.h +++ b/src/serialize.h @@ -167,9 +167,9 @@ const Out& AsBase(const In& x) return x; } -#define READWRITE(...) (::SerReadWriteMany(s, ser_action, __VA_ARGS__)) -#define SER_READ(obj, code) ::SerRead(s, ser_action, obj, [&](Stream& s, typename std::remove_const<Type>::type& obj) { code; }) -#define SER_WRITE(obj, code) ::SerWrite(s, ser_action, obj, [&](Stream& s, const Type& obj) { code; }) +#define READWRITE(...) (ser_action.SerReadWriteMany(s, __VA_ARGS__)) +#define SER_READ(obj, code) ser_action.SerRead(s, obj, [&](Stream& s, typename std::remove_const<Type>::type& obj) { code; }) +#define SER_WRITE(obj, code) ser_action.SerWrite(s, obj, [&](Stream& s, const Type& obj) { code; }) /** * Implement the Ser and Unser methods needed for implementing a formatter (see Using below). @@ -1008,17 +1008,65 @@ void Unserialize(Stream& is, std::shared_ptr<const T>& p) p = std::make_shared<const T>(deserialize, is); } +/** + * Support for (un)serializing many things at once + */ + +template <typename Stream, typename... Args> +void SerializeMany(Stream& s, const Args&... args) +{ + (::Serialize(s, args), ...); +} + +template <typename Stream, typename... Args> +inline void UnserializeMany(Stream& s, Args&&... args) +{ + (::Unserialize(s, args), ...); +} /** * Support for all macros providing or using the ser_action parameter of the SerializationOps method. */ struct ActionSerialize { - constexpr bool ForRead() const { return false; } + static constexpr bool ForRead() { return false; } + + template<typename Stream, typename... Args> + static void SerReadWriteMany(Stream& s, const Args&... args) + { + ::SerializeMany(s, args...); + } + + template<typename Stream, typename Type, typename Fn> + static void SerRead(Stream& s, Type&&, Fn&&) + { + } + + template<typename Stream, typename Type, typename Fn> + static void SerWrite(Stream& s, Type&& obj, Fn&& fn) + { + fn(s, std::forward<Type>(obj)); + } }; struct ActionUnserialize { - constexpr bool ForRead() const { return true; } -}; + static constexpr bool ForRead() { return true; } + template<typename Stream, typename... Args> + static void SerReadWriteMany(Stream& s, Args&&... args) + { + ::UnserializeMany(s, args...); + } + + template<typename Stream, typename Type, typename Fn> + static void SerRead(Stream& s, Type&& obj, Fn&& fn) + { + fn(s, std::forward<Type>(obj)); + } + + template<typename Stream, typename Type, typename Fn> + static void SerWrite(Stream& s, Type&&, Fn&&) + { + } +}; /* ::GetSerializeSize implementations * @@ -1065,52 +1113,6 @@ public: int GetVersion() const { return nVersion; } }; -template <typename Stream, typename... Args> -void SerializeMany(Stream& s, const Args&... args) -{ - (::Serialize(s, args), ...); -} - -template <typename Stream, typename... Args> -inline void UnserializeMany(Stream& s, Args&&... args) -{ - (::Unserialize(s, args), ...); -} - -template<typename Stream, typename... Args> -inline void SerReadWriteMany(Stream& s, ActionSerialize ser_action, const Args&... args) -{ - ::SerializeMany(s, args...); -} - -template<typename Stream, typename... Args> -inline void SerReadWriteMany(Stream& s, ActionUnserialize ser_action, Args&&... args) -{ - ::UnserializeMany(s, args...); -} - -template<typename Stream, typename Type, typename Fn> -inline void SerRead(Stream& s, ActionSerialize ser_action, Type&&, Fn&&) -{ -} - -template<typename Stream, typename Type, typename Fn> -inline void SerRead(Stream& s, ActionUnserialize ser_action, Type&& obj, Fn&& fn) -{ - fn(s, std::forward<Type>(obj)); -} - -template<typename Stream, typename Type, typename Fn> -inline void SerWrite(Stream& s, ActionSerialize ser_action, Type&& obj, Fn&& fn) -{ - fn(s, std::forward<Type>(obj)); -} - -template<typename Stream, typename Type, typename Fn> -inline void SerWrite(Stream& s, ActionUnserialize ser_action, Type&&, Fn&&) -{ -} - template<typename I> inline void WriteVarInt(CSizeComputer &s, I n) { @@ -1161,12 +1163,11 @@ public: template <typename Params, typename T> class ParamsWrapper { - static_assert(std::is_lvalue_reference<T>::value, "ParamsWrapper needs an lvalue reference type T"); const Params& m_params; - T m_object; + T& m_object; public: - explicit ParamsWrapper(const Params& params, T obj) : m_params{params}, m_object{obj} {} + explicit ParamsWrapper(const Params& params, T& obj) : m_params{params}, m_object{obj} {} template <typename Stream> void Serialize(Stream& s) const @@ -1190,7 +1191,20 @@ public: template <typename Params, typename T> static auto WithParams(const Params& params, T&& t) { - return ParamsWrapper<Params, T&>{params, t}; + return ParamsWrapper<Params, T>{params, t}; } +/** + * Helper macro for SerParams structs + * + * Allows you define SerParams instances and then apply them directly + * to an object via function call syntax, eg: + * + * constexpr SerParams FOO{....}; + * ss << FOO(obj); + */ +#define SER_PARAMS_OPFUNC \ + template <typename T> \ + auto operator()(T&& t) const { return WithParams(*this, t); } + #endif // BITCOIN_SERIALIZE_H diff --git a/src/test/addrman_tests.cpp b/src/test/addrman_tests.cpp index 941018a820..b01ba81c5f 100644 --- a/src/test/addrman_tests.cpp +++ b/src/test/addrman_tests.cpp @@ -1019,7 +1019,7 @@ static auto MakeCorruptPeersDat() std::optional<CNetAddr> resolved{LookupHost("252.2.2.2", false)}; BOOST_REQUIRE(resolved.has_value()); AddrInfo info = AddrInfo(addr, resolved.value()); - s << WithParams(CAddress::V1_DISK, info); + s << CAddress::V1_DISK(info); return s; } diff --git a/src/test/fuzz/addrman.cpp b/src/test/fuzz/addrman.cpp index 9611a872ec..1b11ff6fdf 100644 --- a/src/test/fuzz/addrman.cpp +++ b/src/test/fuzz/addrman.cpp @@ -83,7 +83,7 @@ CNetAddr RandAddr(FuzzedDataProvider& fuzzed_data_provider, FastRandomContext& f s << net; s << fast_random_context.randbytes(net_len_map.at(net)); - s >> WithParams(CAddress::V2_NETWORK, addr); + s >> CAddress::V2_NETWORK(addr); } // Return a dummy IPv4 5.5.5.5 if we generated an invalid address. diff --git a/src/test/fuzz/deserialize.cpp b/src/test/fuzz/deserialize.cpp index 100a6b4ee4..510ee7fb5b 100644 --- a/src/test/fuzz/deserialize.cpp +++ b/src/test/fuzz/deserialize.cpp @@ -91,9 +91,9 @@ void DeserializeFromFuzzingInput(FuzzBufferType buffer, T&& obj, const P& params } template <typename T> -CDataStream Serialize(const T& obj, const int version = INIT_PROTO_VERSION, const int ser_type = SER_NETWORK) +CDataStream Serialize(const T& obj) { - CDataStream ds(ser_type, version); + CDataStream ds{SER_NETWORK, INIT_PROTO_VERSION}; ds << obj; return ds; } @@ -107,12 +107,10 @@ T Deserialize(CDataStream ds) } template <typename T> -void DeserializeFromFuzzingInput(FuzzBufferType buffer, T&& obj, const std::optional<int> protocol_version = std::nullopt, const int ser_type = SER_NETWORK) +void DeserializeFromFuzzingInput(FuzzBufferType buffer, T&& obj) { - CDataStream ds(buffer, ser_type, INIT_PROTO_VERSION); - if (protocol_version) { - ds.SetVersion(*protocol_version); - } else { + CDataStream ds{buffer, SER_NETWORK, INIT_PROTO_VERSION}; + { try { int version; ds >> version; @@ -135,9 +133,9 @@ void AssertEqualAfterSerializeDeserialize(const T& obj, const P& params) assert(Deserialize<T>(Serialize(obj, params), params) == obj); } template <typename T> -void AssertEqualAfterSerializeDeserialize(const T& obj, const int version = INIT_PROTO_VERSION, const int ser_type = SER_NETWORK) +void AssertEqualAfterSerializeDeserialize(const T& obj) { - assert(Deserialize<T>(Serialize(obj, version, ser_type)) == obj); + assert(Deserialize<T>(Serialize(obj)) == obj); } } // namespace @@ -254,7 +252,7 @@ FUZZ_TARGET(netaddr_deserialize, .init = initialize_deserialize) if (!maybe_na) return; const CNetAddr& na{*maybe_na}; if (na.IsAddrV1Compatible()) { - AssertEqualAfterSerializeDeserialize(na, ConsumeDeserializationParams<CNetAddr::SerParams>(fdp)); + AssertEqualAfterSerializeDeserialize(na, CNetAddr::V1); } AssertEqualAfterSerializeDeserialize(na, CNetAddr::V2); } @@ -266,7 +264,7 @@ FUZZ_TARGET(service_deserialize, .init = initialize_deserialize) if (!maybe_s) return; const CService& s{*maybe_s}; if (s.IsAddrV1Compatible()) { - AssertEqualAfterSerializeDeserialize(s, ConsumeDeserializationParams<CNetAddr::SerParams>(fdp)); + AssertEqualAfterSerializeDeserialize(s, CNetAddr::V1); } AssertEqualAfterSerializeDeserialize(s, CNetAddr::V2); if (ser_params.enc == CNetAddr::Encoding::V1) { @@ -281,8 +279,8 @@ FUZZ_TARGET_DESERIALIZE(messageheader_deserialize, { FUZZ_TARGET(address_deserialize, .init = initialize_deserialize) { FuzzedDataProvider fdp{buffer.data(), buffer.size()}; - const auto ser_enc{ConsumeDeserializationParams<CNetAddr::SerParams>(fdp)}; - const auto maybe_a{ConsumeDeserializable<CAddress>(fdp, CAddress::SerParams{{ser_enc}, CAddress::Format::Network})}; + const auto ser_enc{ConsumeDeserializationParams<CAddress::SerParams>(fdp)}; + const auto maybe_a{ConsumeDeserializable<CAddress>(fdp, ser_enc)}; if (!maybe_a) return; const CAddress& a{*maybe_a}; // A CAddress in V1 mode will roundtrip diff --git a/src/test/fuzz/fuzz.cpp b/src/test/fuzz/fuzz.cpp index 32bd00ec03..5245b4607b 100644 --- a/src/test/fuzz/fuzz.cpp +++ b/src/test/fuzz/fuzz.cpp @@ -29,7 +29,7 @@ #include <utility> #include <vector> -#ifdef __AFL_FUZZ_INIT +#if defined(PROVIDE_FUZZ_MAIN_FUNCTION) && defined(__AFL_FUZZ_INIT) __AFL_FUZZ_INIT(); #endif diff --git a/src/test/fuzz/key.cpp b/src/test/fuzz/key.cpp index 60f4081432..be45443172 100644 --- a/src/test/fuzz/key.cpp +++ b/src/test/fuzz/key.cpp @@ -186,7 +186,7 @@ FUZZ_TARGET(key, .init = initialize_key) const CTxDestination tx_destination = GetDestinationForKey(pubkey, output_type); assert(output_type == OutputType::LEGACY); assert(IsValidDestination(tx_destination)); - assert(CTxDestination{PKHash{pubkey}} == tx_destination); + assert(PKHash{pubkey} == *std::get_if<PKHash>(&tx_destination)); const CScript script_for_destination = GetScriptForDestination(tx_destination); assert(script_for_destination.size() == 25); diff --git a/src/test/fuzz/script.cpp b/src/test/fuzz/script.cpp index acc82f55f6..fe41a8c6ae 100644 --- a/src/test/fuzz/script.cpp +++ b/src/test/fuzz/script.cpp @@ -149,13 +149,16 @@ FUZZ_TARGET(script, .init = initialize_script) const CTxDestination tx_destination_2{ConsumeTxDestination(fuzzed_data_provider)}; const std::string encoded_dest{EncodeDestination(tx_destination_1)}; const UniValue json_dest{DescribeAddress(tx_destination_1)}; - Assert(tx_destination_1 == DecodeDestination(encoded_dest)); (void)GetKeyForDestination(/*store=*/{}, tx_destination_1); const CScript dest{GetScriptForDestination(tx_destination_1)}; const bool valid{IsValidDestination(tx_destination_1)}; - Assert(dest.empty() != valid); - Assert(valid == IsValidDestinationString(encoded_dest)); + if (!std::get_if<PubKeyDestination>(&tx_destination_1)) { + // Only try to round trip non-pubkey destinations since PubKeyDestination has no encoding + Assert(dest.empty() != valid); + Assert(tx_destination_1 == DecodeDestination(encoded_dest)); + Assert(valid == IsValidDestinationString(encoded_dest)); + } (void)(tx_destination_1 < tx_destination_2); if (tx_destination_1 == tx_destination_2) { diff --git a/src/test/fuzz/util.cpp b/src/test/fuzz/util.cpp index ca2218e94c..87ca2f6aed 100644 --- a/src/test/fuzz/util.cpp +++ b/src/test/fuzz/util.cpp @@ -173,6 +173,15 @@ CTxDestination ConsumeTxDestination(FuzzedDataProvider& fuzzed_data_provider) no tx_destination = CNoDestination{}; }, [&] { + bool compressed = fuzzed_data_provider.ConsumeBool(); + CPubKey pk{ConstructPubKeyBytes( + fuzzed_data_provider, + ConsumeFixedLengthByteVector(fuzzed_data_provider, (compressed ? CPubKey::COMPRESSED_SIZE : CPubKey::SIZE)), + compressed + )}; + tx_destination = PubKeyDestination{pk}; + }, + [&] { tx_destination = PKHash{ConsumeUInt160(fuzzed_data_provider)}; }, [&] { @@ -188,15 +197,11 @@ CTxDestination ConsumeTxDestination(FuzzedDataProvider& fuzzed_data_provider) no tx_destination = WitnessV1Taproot{XOnlyPubKey{ConsumeUInt256(fuzzed_data_provider)}}; }, [&] { - WitnessUnknown witness_unknown{}; - witness_unknown.version = fuzzed_data_provider.ConsumeIntegralInRange(2, 16); - std::vector<uint8_t> witness_unknown_program_1{fuzzed_data_provider.ConsumeBytes<uint8_t>(40)}; - if (witness_unknown_program_1.size() < 2) { - witness_unknown_program_1 = {0, 0}; + std::vector<unsigned char> program{ConsumeRandomLengthByteVector(fuzzed_data_provider, /*max_length=*/40)}; + if (program.size() < 2) { + program = {0, 0}; } - witness_unknown.length = witness_unknown_program_1.size(); - std::copy(witness_unknown_program_1.begin(), witness_unknown_program_1.end(), witness_unknown.program); - tx_destination = witness_unknown; + tx_destination = WitnessUnknown{fuzzed_data_provider.ConsumeIntegralInRange<unsigned int>(2, 16), program}; })}; Assert(call_size == std::variant_size_v<CTxDestination>); return tx_destination; diff --git a/src/test/net_tests.cpp b/src/test/net_tests.cpp index 34d7867079..1e9c9d923b 100644 --- a/src/test/net_tests.cpp +++ b/src/test/net_tests.cpp @@ -850,7 +850,7 @@ BOOST_AUTO_TEST_CASE(initial_advertise_from_version_message) std::chrono::microseconds time_received_dummy{0}; const auto msg_version = - msg_maker.Make(NetMsgType::VERSION, PROTOCOL_VERSION, services, time, services, WithParams(CAddress::V1_NETWORK, peer_us)); + msg_maker.Make(NetMsgType::VERSION, PROTOCOL_VERSION, services, time, services, CAddress::V1_NETWORK(peer_us)); CDataStream msg_version_stream{msg_version.data, SER_NETWORK, PROTOCOL_VERSION}; m_node.peerman->ProcessMessage( @@ -876,7 +876,7 @@ BOOST_AUTO_TEST_CASE(initial_advertise_from_version_message) DataStream s{data}; std::vector<CAddress> addresses; - s >> WithParams(CAddress::V1_NETWORK, addresses); + s >> CAddress::V1_NETWORK(addresses); for (const auto& addr : addresses) { if (addr == expected) { @@ -1357,11 +1357,19 @@ BOOST_AUTO_TEST_CASE(v2transport_test) BOOST_CHECK(!(*ret)[1]); BOOST_CHECK((*ret)[2] && (*ret)[2]->m_type == "tx" && Span{(*ret)[2]->m_recv} == MakeByteSpan(msg_data_2)); - // Then send a message with a bit error, expecting failure. + // Then send a message with a bit error, expecting failure. It's possible this failure does + // not occur immediately (when the length descriptor was modified), but it should come + // eventually, and no messages can be delivered anymore. tester.SendMessage("bad", msg_data_1); tester.Damage(); - ret = tester.Interact(); - BOOST_CHECK(!ret); + while (true) { + ret = tester.Interact(); + if (!ret) break; // failure + BOOST_CHECK(ret->size() == 0); // no message can be delivered + // Send another message. + auto msg_data_3 = g_insecure_rand_ctx.randbytes<uint8_t>(InsecureRandRange(10000)); + tester.SendMessage(uint8_t(12), msg_data_3); // getheaders short id + } } // Normal scenario, with a transport in responder node. diff --git a/src/test/netbase_tests.cpp b/src/test/netbase_tests.cpp index e22bf7e7c0..74ff531cd9 100644 --- a/src/test/netbase_tests.cpp +++ b/src/test/netbase_tests.cpp @@ -561,7 +561,7 @@ BOOST_AUTO_TEST_CASE(caddress_serialize_v1) { DataStream s{}; - s << WithParams(CAddress::V1_NETWORK, fixture_addresses); + s << CAddress::V1_NETWORK(fixture_addresses); BOOST_CHECK_EQUAL(HexStr(s), stream_addrv1_hex); } @@ -570,7 +570,7 @@ BOOST_AUTO_TEST_CASE(caddress_unserialize_v1) DataStream s{ParseHex(stream_addrv1_hex)}; std::vector<CAddress> addresses_unserialized; - s >> WithParams(CAddress::V1_NETWORK, addresses_unserialized); + s >> CAddress::V1_NETWORK(addresses_unserialized); BOOST_CHECK(fixture_addresses == addresses_unserialized); } @@ -578,7 +578,7 @@ BOOST_AUTO_TEST_CASE(caddress_serialize_v2) { DataStream s{}; - s << WithParams(CAddress::V2_NETWORK, fixture_addresses); + s << CAddress::V2_NETWORK(fixture_addresses); BOOST_CHECK_EQUAL(HexStr(s), stream_addrv2_hex); } @@ -587,7 +587,7 @@ BOOST_AUTO_TEST_CASE(caddress_unserialize_v2) DataStream s{ParseHex(stream_addrv2_hex)}; std::vector<CAddress> addresses_unserialized; - s >> WithParams(CAddress::V2_NETWORK, addresses_unserialized); + s >> CAddress::V2_NETWORK(addresses_unserialized); BOOST_CHECK(fixture_addresses == addresses_unserialized); } diff --git a/src/test/script_standard_tests.cpp b/src/test/script_standard_tests.cpp index 1a205728d6..58bdb37b7c 100644 --- a/src/test/script_standard_tests.cpp +++ b/src/test/script_standard_tests.cpp @@ -203,8 +203,8 @@ BOOST_AUTO_TEST_CASE(script_standard_ExtractDestination) // TxoutType::PUBKEY s.clear(); s << ToByteVector(pubkey) << OP_CHECKSIG; - BOOST_CHECK(ExtractDestination(s, address)); - BOOST_CHECK(std::get<PKHash>(address) == PKHash(pubkey)); + BOOST_CHECK(!ExtractDestination(s, address)); + BOOST_CHECK(std::get<PubKeyDestination>(address) == PubKeyDestination(pubkey)); // TxoutType::PUBKEYHASH s.clear(); @@ -249,10 +249,7 @@ BOOST_AUTO_TEST_CASE(script_standard_ExtractDestination) s.clear(); s << OP_1 << ToByteVector(pubkey); BOOST_CHECK(ExtractDestination(s, address)); - WitnessUnknown unk; - unk.length = 33; - unk.version = 1; - std::copy(pubkey.begin(), pubkey.end(), unk.program); + WitnessUnknown unk{1, ToByteVector(pubkey)}; BOOST_CHECK(std::get<WitnessUnknown>(address) == unk); } diff --git a/src/test/util/net.cpp b/src/test/util/net.cpp index dc64c0b4c1..bf5a653090 100644 --- a/src/test/util/net.cpp +++ b/src/test/util/net.cpp @@ -33,9 +33,9 @@ void ConnmanTestMsg::Handshake(CNode& node, Using<CustomUintFormatter<8>>(remote_services), // int64_t{}, // dummy time int64_t{}, // ignored service bits - WithParams(CNetAddr::V1, CService{}), // dummy + CNetAddr::V1(CService{}), // dummy int64_t{}, // ignored service bits - WithParams(CNetAddr::V1, CService{}), // ignored + CNetAddr::V1(CService{}), // ignored uint64_t{1}, // dummy nonce std::string{}, // dummy subver int32_t{}, // dummy starting_height diff --git a/src/util/message.cpp b/src/util/message.cpp index ec845aeffb..1afb28cc10 100644 --- a/src/util/message.cpp +++ b/src/util/message.cpp @@ -47,7 +47,7 @@ MessageVerificationResult MessageVerify( return MessageVerificationResult::ERR_PUBKEY_NOT_RECOVERED; } - if (!(CTxDestination(PKHash(pubkey)) == destination)) { + if (!(PKHash(pubkey) == *std::get_if<PKHash>(&destination))) { return MessageVerificationResult::ERR_NOT_SIGNED; } diff --git a/src/wallet/feebumper.cpp b/src/wallet/feebumper.cpp index f99da926bc..512a011dfc 100644 --- a/src/wallet/feebumper.cpp +++ b/src/wallet/feebumper.cpp @@ -257,12 +257,12 @@ Result CreateRateBumpTransaction(CWallet& wallet, const uint256& txid, const CCo const auto& txouts = outputs.empty() ? wtx.tx->vout : outputs; for (size_t i = 0; i < txouts.size(); ++i) { const CTxOut& output = txouts.at(i); + CTxDestination dest; + ExtractDestination(output.scriptPubKey, dest); if (reduce_output.has_value() ? reduce_output.value() == i : OutputIsChange(wallet, output)) { - CTxDestination change_dest; - ExtractDestination(output.scriptPubKey, change_dest); - new_coin_control.destChange = change_dest; + new_coin_control.destChange = dest; } else { - CRecipient recipient = {output.scriptPubKey, output.nValue, false}; + CRecipient recipient = {dest, output.nValue, false}; recipients.push_back(recipient); } new_outputs_value += output.nValue; @@ -278,7 +278,7 @@ Result CreateRateBumpTransaction(CWallet& wallet, const uint256& txid, const CCo // Add change as recipient with SFFO flag enabled, so fees are deduced from it. // If the output differs from the original tx output (because the user customized it) a new change output will be created. - recipients.emplace_back(CRecipient{GetScriptForDestination(new_coin_control.destChange), new_outputs_value, /*fSubtractFeeFromAmount=*/true}); + recipients.emplace_back(CRecipient{new_coin_control.destChange, new_outputs_value, /*fSubtractFeeFromAmount=*/true}); new_coin_control.destChange = CNoDestination(); } diff --git a/src/wallet/rpc/addresses.cpp b/src/wallet/rpc/addresses.cpp index c1b99a4f97..e9b93afc30 100644 --- a/src/wallet/rpc/addresses.cpp +++ b/src/wallet/rpc/addresses.cpp @@ -427,6 +427,7 @@ public: explicit DescribeWalletAddressVisitor(const SigningProvider* _provider) : provider(_provider) {} UniValue operator()(const CNoDestination& dest) const { return UniValue(UniValue::VOBJ); } + UniValue operator()(const PubKeyDestination& dest) const { return UniValue(UniValue::VOBJ); } UniValue operator()(const PKHash& pkhash) const { diff --git a/src/wallet/rpc/spend.cpp b/src/wallet/rpc/spend.cpp index c4206e9897..c2f4321a60 100644 --- a/src/wallet/rpc/spend.cpp +++ b/src/wallet/rpc/spend.cpp @@ -39,7 +39,6 @@ static void ParseRecipients(const UniValue& address_amounts, const UniValue& sub } destinations.insert(dest); - CScript script_pub_key = GetScriptForDestination(dest); CAmount amount = AmountFromValue(address_amounts[i++]); bool subtract_fee = false; @@ -50,7 +49,7 @@ static void ParseRecipients(const UniValue& address_amounts, const UniValue& sub } } - CRecipient recipient = {script_pub_key, amount, subtract_fee}; + CRecipient recipient = {dest, amount, subtract_fee}; recipients.push_back(recipient); } } diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index 1f510d1c58..20f735da12 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -1716,8 +1716,23 @@ std::unordered_set<CScript, SaltedSipHasher> LegacyScriptPubKeyMan::GetScriptPub } // All watchonly scripts are raw - spks.insert(setWatchOnly.begin(), setWatchOnly.end()); + for (const CScript& script : setWatchOnly) { + // As the legacy wallet allowed to import any script, we need to verify the validity here. + // LegacyScriptPubKeyMan::IsMine() return 'ISMINE_NO' for invalid or not watched scripts (IsMineResult::INVALID or IsMineResult::NO). + // e.g. a "sh(sh(pkh()))" which legacy wallets allowed to import!. + if (IsMine(script) != ISMINE_NO) spks.insert(script); + } + + return spks; +} +std::unordered_set<CScript, SaltedSipHasher> LegacyScriptPubKeyMan::GetNotMineScriptPubKeys() const +{ + LOCK(cs_KeyStore); + std::unordered_set<CScript, SaltedSipHasher> spks; + for (const CScript& script : setWatchOnly) { + if (IsMine(script) == ISMINE_NO) spks.insert(script); + } return spks; } diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index ec7b017720..9d5ce6e125 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -525,6 +525,12 @@ public: std::set<CKeyID> GetKeys() const override; std::unordered_set<CScript, SaltedSipHasher> GetScriptPubKeys() const override; + /** + * Retrieves scripts that were imported by bugs into the legacy spkm and are + * simply invalid, such as a sh(sh(pkh())) script, or not watched. + */ + std::unordered_set<CScript, SaltedSipHasher> GetNotMineScriptPubKeys() const; + /** Get the DescriptorScriptPubKeyMans (with private keys) that have the same scriptPubKeys as this LegacyScriptPubKeyMan. * Does not modify this ScriptPubKeyMan. */ std::optional<MigrationData> MigrateToDescriptor(); diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index 96c9a3dc16..7e6fba33aa 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -505,8 +505,15 @@ std::map<CTxDestination, std::vector<COutput>> ListCoins(const CWallet& wallet) coins_params.skip_locked = false; for (const COutput& coin : AvailableCoins(wallet, &coin_control, /*feerate=*/std::nullopt, coins_params).All()) { CTxDestination address; - if ((coin.spendable || (wallet.IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS) && coin.solvable)) && - ExtractDestination(FindNonChangeParentOutput(wallet, coin.outpoint).scriptPubKey, address)) { + if ((coin.spendable || (wallet.IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS) && coin.solvable))) { + if (!ExtractDestination(FindNonChangeParentOutput(wallet, coin.outpoint).scriptPubKey, address)) { + // For backwards compatibility, we convert P2PK output scripts into PKHash destinations + if (auto pk_dest = std::get_if<PubKeyDestination>(&address)) { + address = PKHash(pk_dest->GetPubKey()); + } else { + continue; + } + } result[address].emplace_back(coin); } } @@ -1071,7 +1078,7 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal( // vouts to the payees for (const auto& recipient : vecSend) { - CTxOut txout(recipient.nAmount, recipient.scriptPubKey); + CTxOut txout(recipient.nAmount, GetScriptForDestination(recipient.dest)); // Include the fee cost for outputs. coin_selection_params.tx_noinputs_size += ::GetSerializeSize(txout, PROTOCOL_VERSION); @@ -1319,7 +1326,9 @@ bool FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& nFeeRet, // Turn the txout set into a CRecipient vector. for (size_t idx = 0; idx < tx.vout.size(); idx++) { const CTxOut& txOut = tx.vout[idx]; - CRecipient recipient = {txOut.scriptPubKey, txOut.nValue, setSubtractFeeFromOutputs.count(idx) == 1}; + CTxDestination dest; + ExtractDestination(txOut.scriptPubKey, dest); + CRecipient recipient = {dest, txOut.nValue, setSubtractFeeFromOutputs.count(idx) == 1}; vecSend.push_back(recipient); } diff --git a/src/wallet/test/spend_tests.cpp b/src/wallet/test/spend_tests.cpp index eca1d74cf6..68c98ae6b9 100644 --- a/src/wallet/test/spend_tests.cpp +++ b/src/wallet/test/spend_tests.cpp @@ -27,7 +27,7 @@ BOOST_FIXTURE_TEST_CASE(SubtractFee, TestChain100Setup) // leftover input amount which would have been change to the recipient // instead of the miner. auto check_tx = [&wallet](CAmount leftover_input_amount) { - CRecipient recipient{GetScriptForRawPubKey({}), 50 * COIN - leftover_input_amount, /*subtract_fee=*/true}; + CRecipient recipient{PubKeyDestination({}), 50 * COIN - leftover_input_amount, /*subtract_fee=*/true}; constexpr int RANDOM_CHANGE_POSITION = -1; CCoinControl coin_control; coin_control.m_feerate.emplace(10000); diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index 5c297d76e4..dac6e87983 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -645,7 +645,7 @@ void TestCoinsResult(ListCoinsTest& context, OutputType out_type, CAmount amount { LOCK(context.wallet->cs_wallet); util::Result<CTxDestination> dest = Assert(context.wallet->GetNewDestination(out_type, "")); - CWalletTx& wtx = context.AddTx(CRecipient{{GetScriptForDestination(*dest)}, amount, /*fSubtractFeeFromAmount=*/true}); + CWalletTx& wtx = context.AddTx(CRecipient{*dest, amount, /*fSubtractFeeFromAmount=*/true}); CoinFilterParams filter; filter.skip_locked = false; CoinsResult available_coins = AvailableCoins(*context.wallet, nullptr, std::nullopt, filter); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 6f5248efaf..3622b00c77 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2213,15 +2213,13 @@ OutputType CWallet::TransactionChangeType(const std::optional<OutputType>& chang bool any_pkh{false}; for (const auto& recipient : vecSend) { - std::vector<std::vector<uint8_t>> dummy; - const TxoutType type{Solver(recipient.scriptPubKey, dummy)}; - if (type == TxoutType::WITNESS_V1_TAPROOT) { + if (std::get_if<WitnessV1Taproot>(&recipient.dest)) { any_tr = true; - } else if (type == TxoutType::WITNESS_V0_KEYHASH) { + } else if (std::get_if<WitnessV0KeyHash>(&recipient.dest)) { any_wpkh = true; - } else if (type == TxoutType::SCRIPTHASH) { + } else if (std::get_if<ScriptHash>(&recipient.dest)) { any_sh = true; - } else if (type == TxoutType::PUBKEYHASH) { + } else if (std::get_if<PKHash>(&recipient.dest)) { any_pkh = true; } } @@ -3862,6 +3860,13 @@ bool CWallet::ApplyMigrationData(MigrationData& data, bilingual_str& error) return false; } + // Get all invalid or non-watched scripts that will not be migrated + std::set<CTxDestination> not_migrated_dests; + for (const auto& script : legacy_spkm->GetNotMineScriptPubKeys()) { + CTxDestination dest; + if (ExtractDestination(script, dest)) not_migrated_dests.emplace(dest); + } + for (auto& desc_spkm : data.desc_spkms) { if (m_spk_managers.count(desc_spkm->GetID()) > 0) { error = _("Error: Duplicate descriptors created during migration. Your wallet may be corrupted."); @@ -3968,6 +3973,13 @@ bool CWallet::ApplyMigrationData(MigrationData& data, bilingual_str& error) continue; } } + + // Skip invalid/non-watched scripts that will not be migrated + if (not_migrated_dests.count(addr_pair.first) > 0) { + dests_to_delete.push_back(addr_pair.first); + continue; + } + // Not ours, not in watchonly wallet, and not in solvable error = _("Error: Address book data in wallet cannot be identified to belong to migrated wallets"); return false; diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 091a573151..97d06641a7 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -288,7 +288,7 @@ inline std::optional<AddressPurpose> PurposeFromString(std::string_view s) struct CRecipient { - CScript scriptPubKey; + CTxDestination dest; CAmount nAmount; bool fSubtractFeeFromAmount; }; diff --git a/test/functional/rpc_deriveaddresses.py b/test/functional/rpc_deriveaddresses.py index e96b6bda90..64994d6bb3 100755 --- a/test/functional/rpc_deriveaddresses.py +++ b/test/functional/rpc_deriveaddresses.py @@ -42,7 +42,10 @@ class DeriveaddressesTest(BitcoinTestFramework): assert_raises_rpc_error(-8, "Range should be greater or equal than 0", self.nodes[0].deriveaddresses, descsum_create("wpkh(tprv8ZgxMBicQKsPd7Uf69XL1XwhmjHopUGep8GuEiJDZmbQz6o58LninorQAfcKZWARbtRtfnLcJ5MQ2AtHcQJCCRUcMRvmDUjyEmNUWwx8UbK/1/1/*)"), [-1, 0]) combo_descriptor = descsum_create("combo(tprv8ZgxMBicQKsPd7Uf69XL1XwhmjHopUGep8GuEiJDZmbQz6o58LninorQAfcKZWARbtRtfnLcJ5MQ2AtHcQJCCRUcMRvmDUjyEmNUWwx8UbK/1/1/0)") - assert_equal(self.nodes[0].deriveaddresses(combo_descriptor), ["mtfUoUax9L4tzXARpw1oTGxWyoogp52KhJ", "mtfUoUax9L4tzXARpw1oTGxWyoogp52KhJ", address, "2NDvEwGfpEqJWfybzpKPHF2XH3jwoQV3D7x"]) + assert_equal(self.nodes[0].deriveaddresses(combo_descriptor), ["mtfUoUax9L4tzXARpw1oTGxWyoogp52KhJ", address, "2NDvEwGfpEqJWfybzpKPHF2XH3jwoQV3D7x"]) + + # P2PK does not have a valid address + assert_raises_rpc_error(-5, "Descriptor does not have a corresponding address", self.nodes[0].deriveaddresses, descsum_create("pk(tprv8ZgxMBicQKsPd7Uf69XL1XwhmjHopUGep8GuEiJDZmbQz6o58LninorQAfcKZWARbtRtfnLcJ5MQ2AtHcQJCCRUcMRvmDUjyEmNUWwx8UbK)")) # Before #26275, bitcoind would crash when deriveaddresses was # called with derivation index 2147483647, which is the maximum diff --git a/test/functional/wallet_migration.py b/test/functional/wallet_migration.py index c565c879fb..395044c8b2 100755 --- a/test/functional/wallet_migration.py +++ b/test/functional/wallet_migration.py @@ -6,6 +6,7 @@ import random import shutil +from test_framework.address import script_to_p2sh from test_framework.descriptors import descsum_create from test_framework.test_framework import BitcoinTestFramework from test_framework.messages import COIN, CTransaction, CTxOut @@ -683,6 +684,21 @@ class WalletMigrationTest(BitcoinTestFramework): wallet.rpc.importaddress(address=script_wsh_pkh.hex(), label="raw_spk2", rescan=True, p2sh=False) assert_equal(wallet.getbalances()['watchonly']['trusted'], 5) + # Import sh(pkh()) script, by using importaddress(), with the p2sh flag enabled. + # This will wrap the script under another sh level, which is invalid!, and store it inside the wallet. + # The migration process must skip the invalid scripts and the addressbook records linked to them. + # They are not being watched by the current wallet, nor should be watched by the migrated one. + label_sh_pkh = "raw_sh_pkh" + script_pkh = key_to_p2pkh_script(df_wallet.getaddressinfo(df_wallet.getnewaddress())["pubkey"]) + script_sh_pkh = script_to_p2sh_script(script_pkh) + addy_script_sh_pkh = script_to_p2sh(script_pkh) # valid script address + addy_script_double_sh_pkh = script_to_p2sh(script_sh_pkh) # invalid script address + + # Note: 'importaddress()' will add two scripts, a valid one sh(pkh()) and an invalid one 'sh(sh(pkh()))'. + # Both of them will be stored with the same addressbook label. And only the latter one should + # be discarded during migration. The first one must be migrated. + wallet.rpc.importaddress(address=script_sh_pkh.hex(), label=label_sh_pkh, rescan=False, p2sh=True) + # Migrate wallet and re-check balance info_migration = wallet.migratewallet() wallet_wo = self.nodes[0].get_wallet_rpc(info_migration["watchonly_name"]) @@ -692,6 +708,20 @@ class WalletMigrationTest(BitcoinTestFramework): # The watch-only scripts are no longer part of the main wallet assert_equal(wallet.getbalances()['mine']['trusted'], 0) + # The invalid sh(sh(pk())) script label must not be part of the main wallet anymore + assert label_sh_pkh not in wallet.listlabels() + # But, the standard sh(pkh()) script should be part of the watch-only wallet. + addrs_by_label = wallet_wo.getaddressesbylabel(label_sh_pkh) + assert addy_script_sh_pkh in addrs_by_label + assert addy_script_double_sh_pkh not in addrs_by_label + + # Also, the watch-only wallet should have the descriptor for the standard sh(pkh()) + desc = descsum_create(f"addr({addy_script_sh_pkh})") + assert next(it['desc'] for it in wallet_wo.listdescriptors()['descriptors'] if it['desc'] == desc) + # And doesn't have a descriptor for the invalid one + desc_invalid = descsum_create(f"addr({addy_script_double_sh_pkh})") + assert_equal(next((it['desc'] for it in wallet_wo.listdescriptors()['descriptors'] if it['desc'] == desc_invalid), None), None) + # Just in case, also verify wallet restart self.nodes[0].unloadwallet(info_migration["watchonly_name"]) self.nodes[0].loadwallet(info_migration["watchonly_name"]) |