diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f29e31576a649..43f017aad9791 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 d8faae8340b0f..b05d7547c8dfd 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 0e2496174e302..1f33640c66bee 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 50f576624c600..8b85b77e2bfc3 100644 --- a/src/addrdb.cpp +++ b/src/addrdb.cpp @@ -216,14 +216,14 @@ util::Result> LoadAddrman(const NetGroupManager& netgro void DumpAnchors(const fs::path& anchors_db_path, const std::vector& 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 ReadAnchors(const fs::path& anchors_db_path) { std::vector 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 2454cfb5d9519..f199d1b479446 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 @@ class CScriptVisitor CScript operator()(const WitnessUnknown& id) const { - return CScript() << CScript::EncodeOP_N(id.version) << std::vector(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 6b651e9014019..d3422c68130f3 100644 --- a/src/addresstype.h +++ b/src/addresstype.h @@ -14,9 +14,30 @@ #include 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 @@ -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 m_program; + +public: + WitnessUnknown(unsigned int version, const std::vector& program) : m_version(version), m_program(program) {} + WitnessUnknown(int version, const std::vector& program) : m_version(static_cast(version)), m_program(program) {} + + unsigned int GetWitnessVersion() const { return m_version; } + const std::vector& 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; +using CTxDestination = std::variant; -/** 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/headerssync.cpp b/src/headerssync.cpp index a3adfb4f701d9..f891063cd23b1 100644 --- a/src/headerssync.cpp +++ b/src/headerssync.cpp @@ -7,6 +7,7 @@ #include #include #include +#include // The two constants below are computed using the simulation script on // https://gist.github.com/sipa/016ae445c132cdf65a2791534dfb7ae1 @@ -51,9 +52,9 @@ HeadersSyncState::HeadersSyncState(NodeId id, const Consensus::Params& consensus void HeadersSyncState::Finalize() { Assume(m_download_state != State::FINAL); - m_header_commitments = {}; + ClearShrink(m_header_commitments); m_last_header_received.SetNull(); - m_redownloaded_headers = {}; + ClearShrink(m_redownloaded_headers); m_redownload_buffer_last_hash.SetNull(); m_redownload_buffer_first_prev_hash.SetNull(); m_process_all_remaining_headers = false; diff --git a/src/key_io.cpp b/src/key_io.cpp index 1a0b51a28bc4b..5bcbb8a069426 100644 --- a/src/key_io.cpp +++ b/src/key_io.cpp @@ -67,16 +67,18 @@ class DestinationEncoder std::string operator()(const WitnessUnknown& id) const { - if (id.version < 1 || id.version > 16 || id.length < 2 || id.length > 40) { + const std::vector& program = id.GetWitnessProgram(); + if (id.GetWitnessVersion() < 1 || id.GetWitnessVersion() > 16 || program.size() < 2 || program.size() > 40) { return {}; } - std::vector 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 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* 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.cpp b/src/net.cpp index f6df5e3aab8fc..5711f1582a64b 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -35,6 +35,7 @@ #include #include #include +#include #include #ifdef WIN32 @@ -900,8 +901,7 @@ void V1Transport::MarkBytesSent(size_t bytes_sent) noexcept m_bytes_sent = 0; } else if (!m_sending_header && m_bytes_sent == m_message_to_send.data.size()) { // We're done sending a message's data. Wipe the data vector to reduce memory consumption. - m_message_to_send.data.clear(); - m_message_to_send.data.shrink_to_fit(); + ClearShrink(m_message_to_send.data); m_bytes_sent = 0; } } @@ -1124,8 +1124,8 @@ void V2Transport::ProcessReceivedMaybeV1Bytes() noexcept SetReceiveState(RecvState::V1); SetSendState(SendState::V1); // Reset v2 transport buffers to save memory. - m_recv_buffer = {}; - m_send_buffer = {}; + ClearShrink(m_recv_buffer); + ClearShrink(m_send_buffer); } else { // We have not received enough to distinguish v1 from v2 yet. Wait until more bytes come. } @@ -1185,8 +1185,7 @@ bool V2Transport::ProcessReceivedKeyBytes() noexcept /*ignore=*/false, /*output=*/MakeWritableByteSpan(m_send_buffer).last(BIP324Cipher::EXPANSION)); // We no longer need the garbage. - m_send_garbage.clear(); - m_send_garbage.shrink_to_fit(); + ClearShrink(m_send_garbage); // Construct version packet in the send buffer. m_send_buffer.resize(m_send_buffer.size() + BIP324Cipher::EXPANSION + VERSION_CONTENTS.size()); @@ -1276,7 +1275,7 @@ bool V2Transport::ProcessReceivedPacketBytes() noexcept // Ignore flag does not matter for garbage authentication. Any valid packet functions // as authentication. Receive and process the version packet next. SetReceiveState(RecvState::VERSION); - m_recv_garbage = {}; + ClearShrink(m_recv_garbage); break; case RecvState::VERSION: if (!ignore) { @@ -1296,9 +1295,9 @@ bool V2Transport::ProcessReceivedPacketBytes() noexcept Assume(false); } // Wipe the receive buffer where the next packet will be received into. - m_recv_buffer = {}; + ClearShrink(m_recv_buffer); // In all but APP_READY state, we can wipe the decoded contents. - if (m_recv_state != RecvState::APP_READY) m_recv_decode_buffer = {}; + if (m_recv_state != RecvState::APP_READY) ClearShrink(m_recv_decode_buffer); } else { // We either have less than 3 bytes, so we don't know the packet's length yet, or more // than 3 bytes but less than the packet's full ciphertext. Wait until those arrive. @@ -1512,7 +1511,7 @@ CNetMessage V2Transport::GetReceivedMessage(std::chrono::microseconds time, bool LogPrint(BCLog::NET, "V2 transport error: invalid message type (%u bytes contents), peer=%d\n", m_recv_decode_buffer.size(), m_nodeid); reject_message = true; } - m_recv_decode_buffer = {}; + ClearShrink(m_recv_decode_buffer); SetReceiveState(RecvState::APP); return msg; @@ -1546,7 +1545,7 @@ bool V2Transport::SetMessageToSend(CSerializedNetMsg& msg) noexcept m_cipher.Encrypt(MakeByteSpan(contents), {}, false, MakeWritableByteSpan(m_send_buffer)); m_send_type = msg.m_type; // Release memory - msg.data = {}; + ClearShrink(msg.data); return true; } @@ -1578,7 +1577,7 @@ void V2Transport::MarkBytesSent(size_t bytes_sent) noexcept // Wipe the buffer when everything is sent. if (m_send_pos == m_send_buffer.size()) { m_send_pos = 0; - m_send_buffer = {}; + ClearShrink(m_send_buffer); } } diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 6b415b3a1ea63..b046b3ac168c7 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 a0944c886fc68..7d3659a11af55 100644 --- a/src/netaddress.h +++ b/src/netaddress.h @@ -218,6 +218,7 @@ class CNetAddr }; 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 22e2108afb184..56668898e4943 100644 --- a/src/protocol.h +++ b/src/protocol.h @@ -396,6 +396,7 @@ class CAddress : public CService }; 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 befdf985165b6..3566ad18b6b73 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(&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 c5e83aa50814b..54e5cf85f0a98 100644 --- a/src/rpc/util.cpp +++ b/src/rpc/util.cpp @@ -253,6 +253,11 @@ class DescribeAddressVisitor 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 @@ class DescribeAddressVisitor { 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 f1595077e914a..1ad8ac4373500 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& 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& 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& p) p = std::make_shared(deserialize, is); } +/** + * Support for (un)serializing many things at once + */ + +template +void SerializeMany(Stream& s, const Args&... args) +{ + (::Serialize(s, args), ...); +} + +template +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 + static void SerReadWriteMany(Stream& s, const Args&... args) + { + ::SerializeMany(s, args...); + } + + template + static void SerRead(Stream& s, Type&&, Fn&&) + { + } + + template + static void SerWrite(Stream& s, Type&& obj, Fn&& fn) + { + fn(s, std::forward(obj)); + } }; struct ActionUnserialize { - constexpr bool ForRead() const { return true; } -}; + static constexpr bool ForRead() { return true; } + template + static void SerReadWriteMany(Stream& s, Args&&... args) + { + ::UnserializeMany(s, args...); + } + + template + static void SerRead(Stream& s, Type&& obj, Fn&& fn) + { + fn(s, std::forward(obj)); + } + + template + static void SerWrite(Stream& s, Type&&, Fn&&) + { + } +}; /* ::GetSerializeSize implementations * @@ -1065,52 +1113,6 @@ class CSizeComputer int GetVersion() const { return nVersion; } }; -template -void SerializeMany(Stream& s, const Args&... args) -{ - (::Serialize(s, args), ...); -} - -template -inline void UnserializeMany(Stream& s, Args&&... args) -{ - (::Unserialize(s, args), ...); -} - -template -inline void SerReadWriteMany(Stream& s, ActionSerialize ser_action, const Args&... args) -{ - ::SerializeMany(s, args...); -} - -template -inline void SerReadWriteMany(Stream& s, ActionUnserialize ser_action, Args&&... args) -{ - ::UnserializeMany(s, args...); -} - -template -inline void SerRead(Stream& s, ActionSerialize ser_action, Type&&, Fn&&) -{ -} - -template -inline void SerRead(Stream& s, ActionUnserialize ser_action, Type&& obj, Fn&& fn) -{ - fn(s, std::forward(obj)); -} - -template -inline void SerWrite(Stream& s, ActionSerialize ser_action, Type&& obj, Fn&& fn) -{ - fn(s, std::forward(obj)); -} - -template -inline void SerWrite(Stream& s, ActionUnserialize ser_action, Type&&, Fn&&) -{ -} - template inline void WriteVarInt(CSizeComputer &s, I n) { @@ -1161,12 +1163,11 @@ class ParamsStream template class ParamsWrapper { - static_assert(std::is_lvalue_reference::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 void Serialize(Stream& s) const @@ -1190,7 +1191,20 @@ class ParamsWrapper template static auto WithParams(const Params& params, T&& t) { - return ParamsWrapper{params, t}; + return ParamsWrapper{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 \ + 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 941018a8206cb..b01ba81c5f119 100644 --- a/src/test/addrman_tests.cpp +++ b/src/test/addrman_tests.cpp @@ -1019,7 +1019,7 @@ static auto MakeCorruptPeersDat() std::optional 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 9611a872ec4f9..1b11ff6fdf3f0 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/fuzz.cpp b/src/test/fuzz/fuzz.cpp index f5697f14b101a..5245b4607bf50 100644 --- a/src/test/fuzz/fuzz.cpp +++ b/src/test/fuzz/fuzz.cpp @@ -29,7 +29,7 @@ #include #include -#ifdef __AFL_FUZZ_INIT +#if defined(PROVIDE_FUZZ_MAIN_FUNCTION) && defined(__AFL_FUZZ_INIT) __AFL_FUZZ_INIT(); #endif @@ -192,17 +192,11 @@ int main(int argc, char** argv) { initialize(); static const auto& test_one_input = *Assert(g_test_one_input); -#ifdef __AFL_HAVE_MANUAL_CONTROL - // Enable AFL deferred forkserver mode. Requires compilation using - // afl-clang-fast++. See fuzzing.md for details. - __AFL_INIT(); -#endif - #ifdef __AFL_LOOP // Enable AFL persistent mode. Requires compilation using afl-clang-fast++. // See fuzzing.md for details. const uint8_t* buffer = __AFL_FUZZ_TESTCASE_BUF; - while (__AFL_LOOP(1000)) { + while (__AFL_LOOP(100000)) { size_t buffer_len = __AFL_FUZZ_TESTCASE_LEN; test_one_input({buffer, buffer_len}); } diff --git a/src/test/fuzz/key.cpp b/src/test/fuzz/key.cpp index 60f4081432a90..be45443172f46 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(&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 acc82f55f6c13..fe41a8c6ae4c6 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(&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 ca2218e94cd4d..87ca2f6aede71 100644 --- a/src/test/fuzz/util.cpp +++ b/src/test/fuzz/util.cpp @@ -172,6 +172,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 witness_unknown_program_1{fuzzed_data_provider.ConsumeBytes(40)}; - if (witness_unknown_program_1.size() < 2) { - witness_unknown_program_1 = {0, 0}; + std::vector 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(2, 16), program}; })}; Assert(call_size == std::variant_size_v); return tx_destination; diff --git a/src/test/net_tests.cpp b/src/test/net_tests.cpp index 34d7867079329..1e9c9d923b8c4 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 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(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 e22bf7e7c0231..74ff531cd9476 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 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 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 1a205728d6e08..58bdb37b7c8c7 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(address) == PKHash(pubkey)); + BOOST_CHECK(!ExtractDestination(s, address)); + BOOST_CHECK(std::get(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(address) == unk); } diff --git a/src/test/util/net.cpp b/src/test/util/net.cpp index dc64c0b4c1bbc..bf5a653090b3d 100644 --- a/src/test/util/net.cpp +++ b/src/test/util/net.cpp @@ -33,9 +33,9 @@ void ConnmanTestMsg::Handshake(CNode& node, Using>(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/test/util_tests.cpp b/src/test/util_tests.cpp index 26677bfa55ee6..67f71bd266dd0 100644 --- a/src/test/util_tests.cpp +++ b/src/test/util_tests.cpp @@ -1791,4 +1791,29 @@ BOOST_AUTO_TEST_CASE(util_WriteBinaryFile) BOOST_CHECK(valid); BOOST_CHECK_EQUAL(actual_text, expected_text); } + +BOOST_AUTO_TEST_CASE(clearshrink_test) +{ + { + std::vector v = {1, 2, 3}; + ClearShrink(v); + BOOST_CHECK_EQUAL(v.size(), 0); + BOOST_CHECK_EQUAL(v.capacity(), 0); + } + + { + std::vector v = {false, true, false, false, true, true}; + ClearShrink(v); + BOOST_CHECK_EQUAL(v.size(), 0); + BOOST_CHECK_EQUAL(v.capacity(), 0); + } + + { + std::deque v = {1, 3, 3, 7}; + ClearShrink(v); + BOOST_CHECK_EQUAL(v.size(), 0); + // std::deque has no capacity() we can observe. + } +} + BOOST_AUTO_TEST_SUITE_END() diff --git a/src/util/message.cpp b/src/util/message.cpp index dd73e95a806a9..95defcc157040 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(&destination))) { return MessageVerificationResult::ERR_NOT_SIGNED; } diff --git a/src/util/vector.h b/src/util/vector.h index 9b9218e54ff7e..40ff73c293c17 100644 --- a/src/util/vector.h +++ b/src/util/vector.h @@ -49,4 +49,22 @@ inline V Cat(V v1, const V& v2) return v1; } +/** Clear a vector (or std::deque) and release its allocated memory. */ +template +inline void ClearShrink(V& v) noexcept +{ + // There are various ways to clear a vector and release its memory: + // + // 1. V{}.swap(v) + // 2. v = V{} + // 3. v = {}; v.shrink_to_fit(); + // 4. v.clear(); v.shrink_to_fit(); + // + // (2) does not appear to release memory in glibc debug mode, even if v.shrink_to_fit() + // follows. (3) and (4) rely on std::vector::shrink_to_fit, which is only a non-binding + // request. Therefore, we use method (1). + + V{}.swap(v); +} + #endif // BITCOIN_UTIL_VECTOR_H diff --git a/src/wallet/feebumper.cpp b/src/wallet/feebumper.cpp index f99da926bc5a3..512a011dfc038 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 5f9a5d0a6be5f..e32bddf00e5de 100644 --- a/src/wallet/rpc/addresses.cpp +++ b/src/wallet/rpc/addresses.cpp @@ -427,6 +427,7 @@ class DescribeWalletAddressVisitor 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 bc080061b52a0..20275a10f8394 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 fcaaa66aa44fe..a177877d85e50 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -1716,8 +1716,23 @@ std::unordered_set 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 LegacyScriptPubKeyMan::GetNotMineScriptPubKeys() const +{ + LOCK(cs_KeyStore); + std::unordered_set 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 ec7b017720f81..9d5ce6e12539a 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -525,6 +525,12 @@ class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProv std::set GetKeys() const override; std::unordered_set 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 GetNotMineScriptPubKeys() const; + /** Get the DescriptorScriptPubKeyMans (with private keys) that have the same scriptPubKeys as this LegacyScriptPubKeyMan. * Does not modify this ScriptPubKeyMan. */ std::optional MigrateToDescriptor(); diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index 96c9a3dc167d1..7e6fba33aad9f 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -505,8 +505,15 @@ std::map> 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(&address)) { + address = PKHash(pk_dest->GetPubKey()); + } else { + continue; + } + } result[address].emplace_back(coin); } } @@ -1071,7 +1078,7 @@ static util::Result 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 eca1d74cf637b..68c98ae6b9ebe 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 467e145954160..fb3ed95146aee 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 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 81645dd189b01..6c9714418db7a 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2213,15 +2213,13 @@ OutputType CWallet::TransactionChangeType(const std::optional& chang bool any_pkh{false}; for (const auto& recipient : vecSend) { - std::vector> dummy; - const TxoutType type{Solver(recipient.scriptPubKey, dummy)}; - if (type == TxoutType::WITNESS_V1_TAPROOT) { + if (std::get_if(&recipient.dest)) { any_tr = true; - } else if (type == TxoutType::WITNESS_V0_KEYHASH) { + } else if (std::get_if(&recipient.dest)) { any_wpkh = true; - } else if (type == TxoutType::SCRIPTHASH) { + } else if (std::get_if(&recipient.dest)) { any_sh = true; - } else if (type == TxoutType::PUBKEYHASH) { + } else if (std::get_if(&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 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 64586039f262b..6a788a609cc76 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -288,7 +288,7 @@ inline std::optional 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 e96b6bda90613..64994d6bb3086 100644 --- a/test/functional/rpc_deriveaddresses.py +++ b/test/functional/rpc_deriveaddresses.py @@ -42,7 +42,10 @@ def run_test(self): 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 c565c879fb6c1..395044c8b2e17 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 @@ def send_to_script(script, amount): 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 @@ def send_to_script(script, amount): # 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"])