From 4aaee239211a5287fbc361c0eb158b105ae8c8db Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Mon, 20 Nov 2023 14:54:37 -0500 Subject: [PATCH 01/33] test: add ipc test to test multiprocess type conversion code Add unit test to test IPC method calls and type conversion between bitcoin c++ types and capnproto messages. Right now there are custom type hooks in bitcoin IPC code, so the test is simple, but in upcoming commits, code will be added to convert bitcoin types to capnproto messages, and the test will be expanded. --- build_msvc/test_bitcoin/test_bitcoin.vcxproj | 2 +- src/Makefile.test.include | 36 +++++++++++++ src/test/.gitignore | 2 + src/test/ipc_test.capnp | 15 ++++++ src/test/ipc_test.cpp | 57 ++++++++++++++++++++ src/test/ipc_test.h | 18 +++++++ src/test/ipc_tests.cpp | 13 +++++ 7 files changed, 142 insertions(+), 1 deletion(-) create mode 100644 src/test/.gitignore create mode 100644 src/test/ipc_test.capnp create mode 100644 src/test/ipc_test.cpp create mode 100644 src/test/ipc_test.h create mode 100644 src/test/ipc_tests.cpp diff --git a/build_msvc/test_bitcoin/test_bitcoin.vcxproj b/build_msvc/test_bitcoin/test_bitcoin.vcxproj index de836bc01d551..b5aa58057fd5f 100644 --- a/build_msvc/test_bitcoin/test_bitcoin.vcxproj +++ b/build_msvc/test_bitcoin/test_bitcoin.vcxproj @@ -10,7 +10,7 @@ - + diff --git a/src/Makefile.test.include b/src/Makefile.test.include index 416a11b0c0112..b8afc47bd4e51 100644 --- a/src/Makefile.test.include +++ b/src/Makefile.test.include @@ -216,6 +216,39 @@ BITCOIN_TEST_SUITE += \ wallet/test/init_test_fixture.h endif # ENABLE_WALLET +if BUILD_MULTIPROCESS +# Add boost ipc_tests definition to BITCOIN_TESTS +BITCOIN_TESTS += test/ipc_tests.cpp + +# Build ipc_test code in a separate library so it can be compiled with custom +# LIBMULTIPROCESS_CFLAGS without those flags affecting other tests +LIBBITCOIN_IPC_TEST=libbitcoin_ipc_test.a +EXTRA_LIBRARIES += $(LIBBITCOIN_IPC_TEST) +libbitcoin_ipc_test_a_SOURCES = \ + test/ipc_test.cpp \ + test/ipc_test.h +libbitcoin_ipc_test_a_CPPFLAGS = $(AM_CPPFLAGS) $(BITCOIN_INCLUDES) +libbitcoin_ipc_test_a_CXXFLAGS = $(AM_CXXFLAGS) $(PIE_FLAGS) $(LIBMULTIPROCESS_CFLAGS) + +# Generate various .c++/.h files from the ipc_test.capnp file +include $(MPGEN_PREFIX)/include/mpgen.mk +EXTRA_DIST += test/ipc_test.capnp +libbitcoin_ipc_test_mpgen_output = \ + test/ipc_test.capnp.c++ \ + test/ipc_test.capnp.h \ + test/ipc_test.capnp.proxy-client.c++ \ + test/ipc_test.capnp.proxy-server.c++ \ + test/ipc_test.capnp.proxy-types.c++ \ + test/ipc_test.capnp.proxy-types.h \ + test/ipc_test.capnp.proxy.h +nodist_libbitcoin_ipc_test_a_SOURCES = $(libbitcoin_ipc_test_mpgen_output) +CLEANFILES += $(libbitcoin_ipc_test_mpgen_output) +endif + +# Explicitly list dependencies on generated headers as described in +# https://www.gnu.org/software/automake/manual/html_node/Built-Sources-Example.html#Recording-Dependencies-manually +test/libbitcoin_ipc_test_a-ipc_test.$(OBJEXT): test/ipc_test.capnp.h + test_test_bitcoin_SOURCES = $(BITCOIN_TEST_SUITE) $(BITCOIN_TESTS) $(JSON_TEST_FILES) $(RAW_TEST_FILES) test_test_bitcoin_CPPFLAGS = $(AM_CPPFLAGS) $(BITCOIN_INCLUDES) $(TESTDEFS) $(BOOST_CPPFLAGS) $(EVENT_CFLAGS) test_test_bitcoin_LDADD = $(LIBTEST_UTIL) @@ -223,6 +256,9 @@ if ENABLE_WALLET test_test_bitcoin_LDADD += $(LIBBITCOIN_WALLET) test_test_bitcoin_CPPFLAGS += $(BDB_CPPFLAGS) endif +if BUILD_MULTIPROCESS +test_test_bitcoin_LDADD += $(LIBBITCOIN_IPC_TEST) $(LIBMULTIPROCESS_LIBS) +endif test_test_bitcoin_LDADD += $(LIBBITCOIN_NODE) $(LIBBITCOIN_CLI) $(LIBBITCOIN_COMMON) $(LIBBITCOIN_UTIL) $(LIBBITCOIN_CONSENSUS) $(LIBBITCOIN_CRYPTO) $(LIBUNIVALUE) \ $(LIBLEVELDB) $(LIBMEMENV) $(LIBSECP256K1) $(EVENT_LIBS) $(EVENT_PTHREADS_LIBS) $(MINISKETCH_LIBS) diff --git a/src/test/.gitignore b/src/test/.gitignore new file mode 100644 index 0000000000000..036df1430c561 --- /dev/null +++ b/src/test/.gitignore @@ -0,0 +1,2 @@ +# capnp generated files +*.capnp.* diff --git a/src/test/ipc_test.capnp b/src/test/ipc_test.capnp new file mode 100644 index 0000000000000..f8473a4dec349 --- /dev/null +++ b/src/test/ipc_test.capnp @@ -0,0 +1,15 @@ +# Copyright (c) 2023 The Bitcoin Core developers +# Distributed under the MIT software license, see the accompanying +# file COPYING or http://www.opensource.org/licenses/mit-license.php. + +@0xd71b0fc8727fdf83; + +using Cxx = import "/capnp/c++.capnp"; +$Cxx.namespace("gen"); + +using Proxy = import "/mp/proxy.capnp"; +$Proxy.include("test/ipc_test.h"); + +interface FooInterface $Proxy.wrap("FooImplementation") { + add @0 (a :Int32, b :Int32) -> (result :Int32); +} diff --git a/src/test/ipc_test.cpp b/src/test/ipc_test.cpp new file mode 100644 index 0000000000000..b84255f68b6fe --- /dev/null +++ b/src/test/ipc_test.cpp @@ -0,0 +1,57 @@ +// Copyright (c) 2023 The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#include +#include +#include +#include +#include + +#include +#include +#include +#include + +#include + +//! Unit test that tests execution of IPC calls without actually creating a +//! separate process. This test is primarily intended to verify behavior of type +//! conversion code that converts C++ objects to Cap'n Proto messages and vice +//! versa. +//! +//! The test creates a thread which creates a FooImplementation object (defined +//! in ipc_test.h) and a two-way pipe accepting IPC requests which call methods +//! on the object through FooInterface (defined in ipc_test.capnp). +void IpcTest() +{ + // Setup: create FooImplemention object and listen for FooInterface requests + std::promise>> foo_promise; + std::function disconnect_client; + std::thread thread([&]() { + mp::EventLoop loop("IpcTest", [](bool raise, const std::string& log) { LogPrintf("LOG%i: %s\n", raise, log); }); + auto pipe = loop.m_io_context.provider->newTwoWayPipe(); + + auto connection_client = std::make_unique(loop, kj::mv(pipe.ends[0])); + auto foo_client = std::make_unique>( + connection_client->m_rpc_system.bootstrap(mp::ServerVatId().vat_id).castAs(), + connection_client.get(), /* destroy_connection= */ false); + foo_promise.set_value(std::move(foo_client)); + disconnect_client = [&] { loop.sync([&] { connection_client.reset(); }); }; + + auto connection_server = std::make_unique(loop, kj::mv(pipe.ends[1]), [&](mp::Connection& connection) { + auto foo_server = kj::heap>(std::make_shared(), connection); + return capnp::Capability::Client(kj::mv(foo_server)); + }); + connection_server->onDisconnect([&] { connection_server.reset(); }); + loop.loop(); + }); + std::unique_ptr> foo{foo_promise.get_future().get()}; + + // Test: make sure arguments were sent and return value is received + BOOST_CHECK_EQUAL(foo->add(1, 2), 3); + + // Test cleanup: disconnect pipe and join thread + disconnect_client(); + thread.join(); +} diff --git a/src/test/ipc_test.h b/src/test/ipc_test.h new file mode 100644 index 0000000000000..61c85b5a47a9d --- /dev/null +++ b/src/test/ipc_test.h @@ -0,0 +1,18 @@ +// Copyright (c) 2023 The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#ifndef BITCOIN_TEST_IPC_TEST_H +#define BITCOIN_TEST_IPC_TEST_H + +#include + +class FooImplementation +{ +public: + int add(int a, int b) { return a + b; } +}; + +void IpcTest(); + +#endif // BITCOIN_TEST_IPC_TEST_H diff --git a/src/test/ipc_tests.cpp b/src/test/ipc_tests.cpp new file mode 100644 index 0000000000000..6e144b0f418b6 --- /dev/null +++ b/src/test/ipc_tests.cpp @@ -0,0 +1,13 @@ +// Copyright (c) 2023 The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#include +#include + +BOOST_AUTO_TEST_SUITE(ipc_tests) +BOOST_AUTO_TEST_CASE(ipc_tests) +{ + IpcTest(); +} +BOOST_AUTO_TEST_SUITE_END() From 0cc74fce72e0c79849109ee5d7afe707991b3512 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Mon, 20 Nov 2023 15:49:55 -0500 Subject: [PATCH 02/33] multiprocess: Add type conversion code for serializable types Allow any C++ object that has Serialize and Unserialize methods and can be serialized to a bitcoin CDataStream to be converted to a capnproto Data field and passed as arguments or return values to capnproto methods using the Data type. Extend IPC unit test to cover this and verify the serialization happens correctly. --- src/Makefile.am | 1 + src/ipc/capnp/common-types.h | 89 ++++++++++++++++++++++++++++++++++++ src/test/ipc_test.capnp | 2 + src/test/ipc_test.cpp | 6 ++- src/test/ipc_test.h | 1 + 5 files changed, 98 insertions(+), 1 deletion(-) create mode 100644 src/ipc/capnp/common-types.h diff --git a/src/Makefile.am b/src/Makefile.am index 99b2184cf25df..746c4df677ead 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -1096,6 +1096,7 @@ ipc/capnp/libbitcoin_ipc_a-protocol.$(OBJEXT): $(libbitcoin_ipc_mpgen_input:=.h) if BUILD_MULTIPROCESS LIBBITCOIN_IPC=libbitcoin_ipc.a libbitcoin_ipc_a_SOURCES = \ + ipc/capnp/common-types.h \ ipc/capnp/context.h \ ipc/capnp/init-types.h \ ipc/capnp/protocol.cpp \ diff --git a/src/ipc/capnp/common-types.h b/src/ipc/capnp/common-types.h new file mode 100644 index 0000000000000..d1343c40dd1a0 --- /dev/null +++ b/src/ipc/capnp/common-types.h @@ -0,0 +1,89 @@ +// Copyright (c) 2023 The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#ifndef BITCOIN_IPC_CAPNP_COMMON_TYPES_H +#define BITCOIN_IPC_CAPNP_COMMON_TYPES_H + +#include +#include + +#include +#include +#include +#include + +namespace ipc { +namespace capnp { +//! Use SFINAE to define Serializeable trait which is true if type T has a +//! Serialize(stream) method, false otherwise. +template +struct Serializable { +private: + template + static std::true_type test(decltype(std::declval().Serialize(std::declval()))*); + template + static std::false_type test(...); + +public: + static constexpr bool value = decltype(test(nullptr))::value; +}; + +//! Use SFINAE to define Unserializeable trait which is true if type T has +//! an Unserialize(stream) method, false otherwise. +template +struct Unserializable { +private: + template + static std::true_type test(decltype(std::declval().Unserialize(std::declval()))*); + template + static std::false_type test(...); + +public: + static constexpr bool value = decltype(test(nullptr))::value; +}; +} // namespace capnp +} // namespace ipc + +//! Functions to serialize / deserialize common bitcoin types. +namespace mp { +//! Overload multiprocess library's CustomBuildField hook to allow any +//! serializable object to be stored in a capnproto Data field or passed to a +//! canproto interface. Use Priority<1> so this hook has medium priority, and +//! higher priority hooks could take precedence over this one. +template +void CustomBuildField( + TypeList, Priority<1>, InvokeContext& invoke_context, Value&& value, Output&& output, + // Enable if serializeable and if LocalType is not cv or reference + // qualified. If LocalType is cv or reference qualified, it is important to + // fall back to lower-priority Priority<0> implementation of this function + // that strips cv references, to prevent this CustomBuildField overload from + // taking precedence over more narrow overloads for specific LocalTypes. + std::enable_if_t::value && + std::is_same_v>>>* enable = nullptr) +{ + DataStream stream; + value.Serialize(stream); + auto result = output.init(stream.size()); + memcpy(result.begin(), stream.data(), stream.size()); +} + +//! Overload multiprocess library's CustomReadField hook to allow any object +//! with an Unserialize method to be read from a capnproto Data field or +//! returned from canproto interface. Use Priority<1> so this hook has medium +//! priority, and higher priority hooks could take precedence over this one. +template +decltype(auto) +CustomReadField(TypeList, Priority<1>, InvokeContext& invoke_context, Input&& input, ReadDest&& read_dest, + std::enable_if_t::value>* enable = nullptr) +{ + return read_dest.update([&](auto& value) { + if (!input.has()) return; + auto data = input.get(); + SpanReader stream({data.begin(), data.end()}); + value.Unserialize(stream); + }); +} +} // namespace mp + +#endif // BITCOIN_IPC_CAPNP_COMMON_TYPES_H diff --git a/src/test/ipc_test.capnp b/src/test/ipc_test.capnp index f8473a4dec349..7b970e2affe2e 100644 --- a/src/test/ipc_test.capnp +++ b/src/test/ipc_test.capnp @@ -9,7 +9,9 @@ $Cxx.namespace("gen"); using Proxy = import "/mp/proxy.capnp"; $Proxy.include("test/ipc_test.h"); +$Proxy.includeTypes("ipc/capnp/common-types.h"); interface FooInterface $Proxy.wrap("FooImplementation") { add @0 (a :Int32, b :Int32) -> (result :Int32); + passOutPoint @1 (arg :Data) -> (result :Data); } diff --git a/src/test/ipc_test.cpp b/src/test/ipc_test.cpp index b84255f68b6fe..f835859705d49 100644 --- a/src/test/ipc_test.cpp +++ b/src/test/ipc_test.cpp @@ -2,8 +2,8 @@ // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. -#include #include +#include #include #include #include @@ -51,6 +51,10 @@ void IpcTest() // Test: make sure arguments were sent and return value is received BOOST_CHECK_EQUAL(foo->add(1, 2), 3); + COutPoint txout1{Txid::FromUint256(uint256{100}), 200}; + COutPoint txout2{foo->passOutPoint(txout1)}; + BOOST_CHECK(txout1 == txout2); + // Test cleanup: disconnect pipe and join thread disconnect_client(); thread.join(); diff --git a/src/test/ipc_test.h b/src/test/ipc_test.h index 61c85b5a47a9d..f100ae8c5d2a8 100644 --- a/src/test/ipc_test.h +++ b/src/test/ipc_test.h @@ -11,6 +11,7 @@ class FooImplementation { public: int add(int a, int b) { return a + b; } + COutPoint passOutPoint(COutPoint o) { return o; } }; void IpcTest(); From 6acec6b9ff02b91de132bb1575d75908a8a2d27b Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Mon, 20 Nov 2023 15:49:55 -0500 Subject: [PATCH 03/33] multiprocess: Add type conversion code for UniValue types Extend IPC unit test to cover this and verify the serialization happens correctly. --- src/ipc/capnp/common-types.h | 19 +++++++++++++++++++ src/test/ipc_test.capnp | 1 + src/test/ipc_test.cpp | 6 ++++++ src/test/ipc_test.h | 2 ++ 4 files changed, 28 insertions(+) diff --git a/src/ipc/capnp/common-types.h b/src/ipc/capnp/common-types.h index d1343c40dd1a0..39e368491b4d0 100644 --- a/src/ipc/capnp/common-types.h +++ b/src/ipc/capnp/common-types.h @@ -7,6 +7,7 @@ #include #include +#include #include #include @@ -84,6 +85,24 @@ CustomReadField(TypeList, Priority<1>, InvokeContext& invoke_context, value.Unserialize(stream); }); } + +template +void CustomBuildField(TypeList, Priority<1>, InvokeContext& invoke_context, Value&& value, Output&& output) +{ + std::string str = value.write(); + auto result = output.init(str.size()); + memcpy(result.begin(), str.data(), str.size()); +} + +template +decltype(auto) CustomReadField(TypeList, Priority<1>, InvokeContext& invoke_context, Input&& input, + ReadDest&& read_dest) +{ + return read_dest.update([&](auto& value) { + auto data = input.get(); + value.read(std::string_view{data.begin(), data.size()}); + }); +} } // namespace mp #endif // BITCOIN_IPC_CAPNP_COMMON_TYPES_H diff --git a/src/test/ipc_test.capnp b/src/test/ipc_test.capnp index 7b970e2affe2e..55a3dc2683933 100644 --- a/src/test/ipc_test.capnp +++ b/src/test/ipc_test.capnp @@ -14,4 +14,5 @@ $Proxy.includeTypes("ipc/capnp/common-types.h"); interface FooInterface $Proxy.wrap("FooImplementation") { add @0 (a :Int32, b :Int32) -> (result :Int32); passOutPoint @1 (arg :Data) -> (result :Data); + passUniValue @2 (arg :Text) -> (result :Text); } diff --git a/src/test/ipc_test.cpp b/src/test/ipc_test.cpp index f835859705d49..ce4edaceb0821 100644 --- a/src/test/ipc_test.cpp +++ b/src/test/ipc_test.cpp @@ -55,6 +55,12 @@ void IpcTest() COutPoint txout2{foo->passOutPoint(txout1)}; BOOST_CHECK(txout1 == txout2); + UniValue uni1{UniValue::VOBJ}; + uni1.pushKV("i", 1); + uni1.pushKV("s", "two"); + UniValue uni2{foo->passUniValue(uni1)}; + BOOST_CHECK_EQUAL(uni1.write(), uni2.write()); + // Test cleanup: disconnect pipe and join thread disconnect_client(); thread.join(); diff --git a/src/test/ipc_test.h b/src/test/ipc_test.h index f100ae8c5d2a8..bcfcc2125c68b 100644 --- a/src/test/ipc_test.h +++ b/src/test/ipc_test.h @@ -6,12 +6,14 @@ #define BITCOIN_TEST_IPC_TEST_H #include +#include class FooImplementation { public: int add(int a, int b) { return a + b; } COutPoint passOutPoint(COutPoint o) { return o; } + UniValue passUniValue(UniValue v) { return v; } }; void IpcTest(); From d298ff8b62b2624ed390c8a2f905c888ffc956ff Mon Sep 17 00:00:00 2001 From: Luke Dashjr Date: Sat, 2 Jan 2021 01:46:00 +0000 Subject: [PATCH 04/33] During IBD, prune as much as possible until we get close to where we will eventually keep blocks --- src/node/blockstorage.cpp | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp index 53f616de23b68..d5cbd9311aba6 100644 --- a/src/node/blockstorage.cpp +++ b/src/node/blockstorage.cpp @@ -298,6 +298,7 @@ void BlockManager::FindFilesToPrune( // Distribute our -prune budget over all chainstates. const auto target = std::max( MIN_DISK_SPACE_FOR_BLOCK_FILES, GetPruneTarget() / chainman.GetAll().size()); + const uint64_t target_sync_height = chainman.m_best_header->nHeight; if (chain.m_chain.Height() < 0 || target == 0) { return; @@ -320,10 +321,13 @@ void BlockManager::FindFilesToPrune( // On a prune event, the chainstate DB is flushed. // To avoid excessive prune events negating the benefit of high dbcache // values, we should not prune too rapidly. - // So when pruning in IBD, increase the buffer a bit to avoid a re-prune too soon. - if (chainman.IsInitialBlockDownload()) { - // Since this is only relevant during IBD, we use a fixed 10% - nBuffer += target / 10; + // So when pruning in IBD, increase the buffer to avoid a re-prune too soon. + const auto chain_tip_height = chain.m_chain.Height(); + if (chainman.IsInitialBlockDownload() && target_sync_height > (uint64_t)chain_tip_height) { + // Since this is only relevant during IBD, we assume blocks are at least 1 MB on average + static constexpr uint64_t average_block_size = 1000000; /* 1 MB */ + const uint64_t remaining_blocks = target_sync_height - chain_tip_height; + nBuffer += average_block_size * remaining_blocks; } for (int fileNumber = 0; fileNumber < this->MaxBlockfileNum(); fileNumber++) { From 4dbd0475d8c16ed10c309bf6badc17f2d2eaaa69 Mon Sep 17 00:00:00 2001 From: Cory Fields Date: Thu, 4 Jan 2024 16:23:01 +0000 Subject: [PATCH 05/33] crypto: remove use of BUILD_BITCOIN_INTERNAL macro in sha256 Replace it with a more explicit DISABLE_OPTIMIZED_SHA256 and clean up some. The macro was originally used by libbitcoinconsensus which opts out of optimized sha256 for the sake of simplicity. Also remove the BUILD_BITCOIN_INTERNAL define from libbitcoinkernel for now as it does not export an api. When it does we can pick a less confusing define to control its exports. Removing the define should have the effect of enabling sha256 optimizations for the kernel. --- src/Makefile.am | 4 ++-- src/crypto/sha256.cpp | 18 ++++++++++++------ 2 files changed, 14 insertions(+), 8 deletions(-) diff --git a/src/Makefile.am b/src/Makefile.am index b6f0daaabadbd..9e7fad4a83ea5 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -906,7 +906,7 @@ lib_LTLIBRARIES += $(LIBBITCOINKERNEL) libbitcoinkernel_la_LDFLAGS = $(AM_LDFLAGS) -no-undefined $(RELDFLAGS) $(PTHREAD_FLAGS) libbitcoinkernel_la_LIBADD = $(LIBBITCOIN_CRYPTO) $(LIBLEVELDB) $(LIBMEMENV) $(LIBSECP256K1) -libbitcoinkernel_la_CPPFLAGS = $(AM_CPPFLAGS) -I$(builddir)/obj -I$(srcdir)/secp256k1/include -DBUILD_BITCOIN_INTERNAL $(BOOST_CPPFLAGS) $(LEVELDB_CPPFLAGS) +libbitcoinkernel_la_CPPFLAGS = $(AM_CPPFLAGS) -I$(builddir)/obj -I$(srcdir)/secp256k1/include $(BOOST_CPPFLAGS) $(LEVELDB_CPPFLAGS) # libbitcoinkernel requires default symbol visibility, explicitly specify that # here so that things still work even when user configures with @@ -1012,7 +1012,7 @@ libbitcoinconsensus_la_SOURCES = support/cleanse.cpp $(crypto_libbitcoin_crypto_ libbitcoinconsensus_la_LDFLAGS = $(AM_LDFLAGS) -no-undefined $(RELDFLAGS) libbitcoinconsensus_la_LIBADD = $(LIBSECP256K1) -libbitcoinconsensus_la_CPPFLAGS = $(AM_CPPFLAGS) -I$(builddir)/obj -I$(srcdir)/secp256k1/include -DBUILD_BITCOIN_INTERNAL +libbitcoinconsensus_la_CPPFLAGS = $(AM_CPPFLAGS) -I$(builddir)/obj -I$(srcdir)/secp256k1/include -DBUILD_BITCOIN_INTERNAL -DDISABLE_OPTIMIZED_SHA256 libbitcoinconsensus_la_CXXFLAGS = $(AM_CXXFLAGS) $(PIE_FLAGS) endif diff --git a/src/crypto/sha256.cpp b/src/crypto/sha256.cpp index 5eacaa44e18aa..11aabeb1da9ee 100644 --- a/src/crypto/sha256.cpp +++ b/src/crypto/sha256.cpp @@ -8,14 +8,15 @@ #include #include +#if !defined(DISABLE_OPTIMIZED_SHA256) #include -#if defined(__linux__) && defined(ENABLE_ARM_SHANI) && !defined(BUILD_BITCOIN_INTERNAL) +#if defined(__linux__) && defined(ENABLE_ARM_SHANI) #include #include #endif -#if defined(MAC_OSX) && defined(ENABLE_ARM_SHANI) && !defined(BUILD_BITCOIN_INTERNAL) +#if defined(MAC_OSX) && defined(ENABLE_ARM_SHANI) #include #include #endif @@ -58,6 +59,7 @@ namespace sha256d64_arm_shani { void Transform_2way(unsigned char* out, const unsigned char* in); } +#endif // DISABLE_OPTIMIZED_SHA256 // Internal implementation code. namespace @@ -567,6 +569,7 @@ bool SelfTest() { return true; } +#if !defined(DISABLE_OPTIMIZED_SHA256) #if defined(USE_ASM) && (defined(__x86_64__) || defined(__amd64__) || defined(__i386__)) /** Check whether the OS has enabled AVX registers. */ bool AVXEnabled() @@ -576,6 +579,7 @@ bool AVXEnabled() return (a & 6) == 6; } #endif +#endif // DISABLE_OPTIMIZED_SHA256 } // namespace @@ -588,6 +592,7 @@ std::string SHA256AutoDetect(sha256_implementation::UseImplementation use_implem TransformD64_4way = nullptr; TransformD64_8way = nullptr; +#if !defined(DISABLE_OPTIMIZED_SHA256) #if defined(USE_ASM) && defined(HAVE_GETCPUID) bool have_sse4 = false; bool have_xsave = false; @@ -616,7 +621,7 @@ std::string SHA256AutoDetect(sha256_implementation::UseImplementation use_implem } } -#if defined(ENABLE_X86_SHANI) && !defined(BUILD_BITCOIN_INTERNAL) +#if defined(ENABLE_X86_SHANI) if (have_x86_shani) { Transform = sha256_x86_shani::Transform; TransformD64 = TransformD64Wrapper; @@ -633,13 +638,13 @@ std::string SHA256AutoDetect(sha256_implementation::UseImplementation use_implem TransformD64 = TransformD64Wrapper; ret = "sse4(1way)"; #endif -#if defined(ENABLE_SSE41) && !defined(BUILD_BITCOIN_INTERNAL) +#if defined(ENABLE_SSE41) TransformD64_4way = sha256d64_sse41::Transform_4way; ret += ",sse41(4way)"; #endif } -#if defined(ENABLE_AVX2) && !defined(BUILD_BITCOIN_INTERNAL) +#if defined(ENABLE_AVX2) if (have_avx2 && have_avx && enabled_avx) { TransformD64_8way = sha256d64_avx2::Transform_8way; ret += ",avx2(8way)"; @@ -647,7 +652,7 @@ std::string SHA256AutoDetect(sha256_implementation::UseImplementation use_implem #endif #endif // defined(USE_ASM) && defined(HAVE_GETCPUID) -#if defined(ENABLE_ARM_SHANI) && !defined(BUILD_BITCOIN_INTERNAL) +#if defined(ENABLE_ARM_SHANI) bool have_arm_shani = false; if (use_implementation & sha256_implementation::USE_SHANI) { #if defined(__linux__) @@ -679,6 +684,7 @@ std::string SHA256AutoDetect(sha256_implementation::UseImplementation use_implem ret = "arm_shani(1way,2way)"; } #endif +#endif // DISABLE_OPTIMIZED_SHA256 assert(SelfTest()); return ret; From bbf218d06164b7247f5e9df5ba143383022fbf74 Mon Sep 17 00:00:00 2001 From: Cory Fields Date: Fri, 5 Jan 2024 11:24:13 +0000 Subject: [PATCH 06/33] crypto: remove sha256_sse4 from the base crypto helper lib It was unused there and a confusing outlier. --- src/Makefile.am | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/src/Makefile.am b/src/Makefile.am index 9e7fad4a83ea5..6e48ac07b756a 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -50,6 +50,10 @@ LIBBITCOIN_WALLET_TOOL=libbitcoin_wallet_tool.a endif LIBBITCOIN_CRYPTO = $(LIBBITCOIN_CRYPTO_BASE) +if USE_ASM +LIBBITCOIN_CRYPTO_SSE4 = crypto/libbitcoin_crypto_sse4.la +LIBBITCOIN_CRYPTO += $(LIBBITCOIN_CRYPTO_SSE4) +endif if ENABLE_SSE41 LIBBITCOIN_CRYPTO_SSE41 = crypto/libbitcoin_crypto_sse41.la LIBBITCOIN_CRYPTO += $(LIBBITCOIN_CRYPTO_SSE41) @@ -541,6 +545,10 @@ libbitcoin_wallet_tool_a_SOURCES = \ # # crypto # + +# crypto_base contains the unspecialized (unoptimized) versions of our +# crypto functions. Functions that require custom compiler flags and/or +# runtime opt-in are omitted. crypto_libbitcoin_crypto_base_la_CPPFLAGS = $(AM_CPPFLAGS) # Specify -static in both CXXFLAGS and LDFLAGS so libtool will only build a @@ -581,9 +589,12 @@ crypto_libbitcoin_crypto_base_la_SOURCES = \ crypto/siphash.cpp \ crypto/siphash.h -if USE_ASM -crypto_libbitcoin_crypto_base_la_SOURCES += crypto/sha256_sse4.cpp -endif +# See explanation for -static in crypto_libbitcoin_crypto_base_la's LDFLAGS and +# CXXFLAGS above +crypto_libbitcoin_crypto_sse4_la_LDFLAGS = $(AM_LDFLAGS) -static +crypto_libbitcoin_crypto_sse4_la_CXXFLAGS = $(AM_CXXFLAGS) $(PIE_FLAGS) -static +crypto_libbitcoin_crypto_sse4_la_CPPFLAGS = $(AM_CPPFLAGS) +crypto_libbitcoin_crypto_sse4_la_SOURCES = crypto/sha256_sse4.cpp # See explanation for -static in crypto_libbitcoin_crypto_base_la's LDFLAGS and # CXXFLAGS above From 97181decf5726aab6c5cd01b3e1964072f2531ff Mon Sep 17 00:00:00 2001 From: Chris Stewart Date: Thu, 11 Jan 2024 14:38:39 -0600 Subject: [PATCH 07/33] Add test for negative transaction version w/ CSV to tx_valid.json --- src/test/data/tx_valid.json | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/test/data/tx_valid.json b/src/test/data/tx_valid.json index b874f6f26ca77..70df0d0f697d0 100644 --- a/src/test/data/tx_valid.json +++ b/src/test/data/tx_valid.json @@ -319,6 +319,10 @@ [[["0000000000000000000000000000000000000000000000000000000000000100", 0, "HASH160 0x14 0x7c17aff532f22beb54069942f9bf567a66133eaf EQUAL"]], "0200000001000100000000000000000000000000000000000000000000000000000000000000000000030251b2010000000100000000000000000000000000", "NONE"], +["Valid CHECKSEQUENCEVERIFY even with negative tx version number"], +[[["0000000000000000000000000000000000000000000000000000000000000100", 0, "HASH160 0x14 0x7c17aff532f22beb54069942f9bf567a66133eaf EQUAL"]], +"ffffffff01000100000000000000000000000000000000000000000000000000000000000000000000030251b2010000000100000000000000000000000000", "NONE"], + ["Valid P2WPKH (Private key of segwit tests is L5AQtV2HDm4xGsseLokK2VAT2EtYKcTm3c7HwqnJBFt9LdaQULsM)"], [[["0000000000000000000000000000000000000000000000000000000000000100", 0, "0x00 0x14 0x4c9c3dfac4207d5d8cb89df5722cb3d712385e3f", 1000]], "0100000000010100010000000000000000000000000000000000000000000000000000000000000000000000ffffffff01e8030000000000001976a9144c9c3dfac4207d5d8cb89df5722cb3d712385e3f88ac02483045022100cfb07164b36ba64c1b1e8c7720a56ad64d96f6ef332d3d37f9cb3c96477dc44502200a464cd7a9cf94cd70f66ce4f4f0625ef650052c7afcfe29d7d7e01830ff91ed012103596d3451025c19dbbdeb932d6bf8bfb4ad499b95b6f88db8899efac102e5fc7100000000", "NONE"], From 08cd5aca18f0774258c7c459773b9e8b386d48ef Mon Sep 17 00:00:00 2001 From: fanquake Date: Mon, 8 Jan 2024 13:59:18 +0000 Subject: [PATCH 08/33] build: always set -g -O2 in CORE_CXXFLAGS This avoids cases of missing -O2, when *FLAGS has been overriden. Removes the need for duplicate code to clear autoconf defaults. Also, move CORE_CXXFLAGS before DEBUG_CXXFLAGS, so that -O2 is always overriden if debugging etc. --- configure.ac | 23 ++++++++++------------- src/Makefile.am | 2 +- 2 files changed, 11 insertions(+), 14 deletions(-) diff --git a/configure.ac b/configure.ac index adc862d251459..472c47740a229 100644 --- a/configure.ac +++ b/configure.ac @@ -323,6 +323,15 @@ AC_ARG_ENABLE([external-signer], AC_LANG_PUSH([C++]) +dnl Always set -g -O2 in our CXXFLAGS. Autoconf will try and set CXXFLAGS to "-g -O2" by default, +dnl so we suppress that (if CXXFLAGS hasn't been overridden by the user), given we are adding it +dnl ourselves. +CORE_CXXFLAGS="$CORE_CXXFLAGS -g -O2" + +if test "$CXXFLAGS_overridden" = "no"; then + CXXFLAGS="" +fi + dnl Check for a flag to turn compiler warnings into errors. This is helpful for checks which may dnl appear to succeed because by default they merely emit warnings when they fail. dnl @@ -347,12 +356,6 @@ case $host in esac if test "$enable_debug" = "yes"; then - dnl If debugging is enabled, and the user hasn't overridden CXXFLAGS, clear - dnl them, to prevent autoconfs "-g -O2" being added. Otherwise we'd end up - dnl with "-O0 -g3 -g -O2". - if test "$CXXFLAGS_overridden" = "no"; then - CXXFLAGS="" - fi dnl Disable all optimizations AX_CHECK_COMPILE_FLAG([-O0], [DEBUG_CXXFLAGS="$DEBUG_CXXFLAGS -O0"], [], [$CXXFLAG_WERROR]) @@ -859,12 +862,6 @@ if test "$use_lcov" = "yes"; then [AC_MSG_ERROR([lcov testing requested but --coverage linker flag does not work])]) AX_CHECK_COMPILE_FLAG([--coverage],[CORE_CXXFLAGS="$CORE_CXXFLAGS --coverage"], [AC_MSG_ERROR([lcov testing requested but --coverage flag does not work])]) - dnl If coverage is enabled, and the user hasn't overridden CXXFLAGS, clear - dnl them, to prevent autoconfs "-g -O2" being added. Otherwise we'd end up - dnl with "--coverage -Og -O0 -g -O2". - if test "$CXXFLAGS_overridden" = "no"; then - CXXFLAGS="" - fi CORE_CXXFLAGS="$CORE_CXXFLAGS -Og -O0" fi @@ -1996,7 +1993,7 @@ echo " CC = $CC" echo " CFLAGS = $PTHREAD_CFLAGS $CFLAGS" echo " CPPFLAGS = $DEBUG_CPPFLAGS $HARDENED_CPPFLAGS $CORE_CPPFLAGS $CPPFLAGS" echo " CXX = $CXX" -echo " CXXFLAGS = $DEBUG_CXXFLAGS $HARDENED_CXXFLAGS $WARN_CXXFLAGS $NOWARN_CXXFLAGS $ERROR_CXXFLAGS $GPROF_CXXFLAGS $CORE_CXXFLAGS $CXXFLAGS" +echo " CXXFLAGS = $CORE_CXXFLAGS $DEBUG_CXXFLAGS $HARDENED_CXXFLAGS $WARN_CXXFLAGS $NOWARN_CXXFLAGS $ERROR_CXXFLAGS $GPROF_CXXFLAGS $CXXFLAGS" echo " LDFLAGS = $PTHREAD_LIBS $HARDENED_LDFLAGS $GPROF_LDFLAGS $CORE_LDFLAGS $LDFLAGS" echo " AR = $AR" echo " ARFLAGS = $ARFLAGS" diff --git a/src/Makefile.am b/src/Makefile.am index 780f547b4b838..b70c072d3623b 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -9,7 +9,7 @@ print-%: FORCE DIST_SUBDIRS = secp256k1 AM_LDFLAGS = $(LIBTOOL_LDFLAGS) $(HARDENED_LDFLAGS) $(GPROF_LDFLAGS) $(SANITIZER_LDFLAGS) $(CORE_LDFLAGS) -AM_CXXFLAGS = $(DEBUG_CXXFLAGS) $(HARDENED_CXXFLAGS) $(WARN_CXXFLAGS) $(NOWARN_CXXFLAGS) $(ERROR_CXXFLAGS) $(GPROF_CXXFLAGS) $(SANITIZER_CXXFLAGS) $(CORE_CXXFLAGS) +AM_CXXFLAGS = $(CORE_CXXFLAGS) $(DEBUG_CXXFLAGS) $(HARDENED_CXXFLAGS) $(WARN_CXXFLAGS) $(NOWARN_CXXFLAGS) $(ERROR_CXXFLAGS) $(GPROF_CXXFLAGS) $(SANITIZER_CXXFLAGS) AM_CPPFLAGS = $(DEBUG_CPPFLAGS) $(HARDENED_CPPFLAGS) $(CORE_CPPFLAGS) AM_LIBTOOLFLAGS = --preserve-dup-deps PTHREAD_FLAGS = $(PTHREAD_CFLAGS) $(PTHREAD_LIBS) From 6cc2a38c1388b696e9c28a08c6bd9c93da4fa6b8 Mon Sep 17 00:00:00 2001 From: fanquake Date: Mon, 8 Jan 2024 15:48:35 +0000 Subject: [PATCH 09/33] build: add sanitizer flags to configure output --- configure.ac | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/configure.ac b/configure.ac index 472c47740a229..2173f78bc3fa2 100644 --- a/configure.ac +++ b/configure.ac @@ -1993,8 +1993,8 @@ echo " CC = $CC" echo " CFLAGS = $PTHREAD_CFLAGS $CFLAGS" echo " CPPFLAGS = $DEBUG_CPPFLAGS $HARDENED_CPPFLAGS $CORE_CPPFLAGS $CPPFLAGS" echo " CXX = $CXX" -echo " CXXFLAGS = $CORE_CXXFLAGS $DEBUG_CXXFLAGS $HARDENED_CXXFLAGS $WARN_CXXFLAGS $NOWARN_CXXFLAGS $ERROR_CXXFLAGS $GPROF_CXXFLAGS $CXXFLAGS" -echo " LDFLAGS = $PTHREAD_LIBS $HARDENED_LDFLAGS $GPROF_LDFLAGS $CORE_LDFLAGS $LDFLAGS" +echo " CXXFLAGS = $CORE_CXXFLAGS $DEBUG_CXXFLAGS $HARDENED_CXXFLAGS $WARN_CXXFLAGS $NOWARN_CXXFLAGS $ERROR_CXXFLAGS $GPROF_CXXFLAGS $SANITIZER_CXXFLAGS $CXXFLAGS" +echo " LDFLAGS = $PTHREAD_LIBS $HARDENED_LDFLAGS $GPROF_LDFLAGS $SANITIZER_LDFLAGS $CORE_LDFLAGS $LDFLAGS" echo " AR = $AR" echo " ARFLAGS = $ARFLAGS" echo From 1dc2c9b385f8345c588449848149b8e470653afc Mon Sep 17 00:00:00 2001 From: fanquake Date: Mon, 8 Jan 2024 14:37:17 +0000 Subject: [PATCH 10/33] ci: cleanup C*FLAG usage in Valgrind jobs This was being used to avoid a missing -O2. After the previous commits, this is no-longer an issue. --- ci/test/00_setup_env_native_fuzz_with_valgrind.sh | 2 +- ci/test/00_setup_env_native_valgrind.sh | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/ci/test/00_setup_env_native_fuzz_with_valgrind.sh b/ci/test/00_setup_env_native_fuzz_with_valgrind.sh index 1f60c4680368c..4f80d7ed428c3 100755 --- a/ci/test/00_setup_env_native_fuzz_with_valgrind.sh +++ b/ci/test/00_setup_env_native_fuzz_with_valgrind.sh @@ -16,5 +16,5 @@ export RUN_FUZZ_TESTS=true export FUZZ_TESTS_CONFIG="--valgrind" export GOAL="install" # Temporarily pin dwarf 4, until using Valgrind 3.20 or later -export BITCOIN_CONFIG="--enable-fuzz --with-sanitizers=fuzzer CC='clang -gdwarf-4' CXX='clang++ -gdwarf-4'" +export BITCOIN_CONFIG="--enable-fuzz --with-sanitizers=fuzzer CC=clang CXX=clang++ CFLAGS=-gdwarf-4 CXXFLAGS=-gdwarf-4" export CCACHE_MAXSIZE=200M diff --git a/ci/test/00_setup_env_native_valgrind.sh b/ci/test/00_setup_env_native_valgrind.sh index daa1a0cdb311d..9bdb2b7860c02 100755 --- a/ci/test/00_setup_env_native_valgrind.sh +++ b/ci/test/00_setup_env_native_valgrind.sh @@ -14,4 +14,4 @@ export NO_DEPENDS=1 export TEST_RUNNER_EXTRA="--exclude feature_init,rpc_bind,feature_bind_extra" # Excluded for now, see https://github.com/bitcoin/bitcoin/issues/17765#issuecomment-602068547 export GOAL="install" # Temporarily pin dwarf 4, until using Valgrind 3.20 or later -export BITCOIN_CONFIG="--enable-zmq --with-incompatible-bdb --with-gui=no CC='clang -gdwarf-4' CXX='clang++ -gdwarf-4'" # TODO enable GUI +export BITCOIN_CONFIG="--enable-zmq --with-incompatible-bdb --with-gui=no CC=clang CXX=clang++ CFLAGS=-gdwarf-4 CXXFLAGS=-gdwarf-4" # TODO enable GUI From 00c1e2aa4496b5f038ae5199dbd16d8313766533 Mon Sep 17 00:00:00 2001 From: fanquake Date: Wed, 10 Jan 2024 13:54:44 +0000 Subject: [PATCH 11/33] build: fix optimisation flags used for --coverage -O0 is just overriding -Og. --- configure.ac | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/configure.ac b/configure.ac index 2173f78bc3fa2..951dbdbf83603 100644 --- a/configure.ac +++ b/configure.ac @@ -862,7 +862,7 @@ if test "$use_lcov" = "yes"; then [AC_MSG_ERROR([lcov testing requested but --coverage linker flag does not work])]) AX_CHECK_COMPILE_FLAG([--coverage],[CORE_CXXFLAGS="$CORE_CXXFLAGS --coverage"], [AC_MSG_ERROR([lcov testing requested but --coverage flag does not work])]) - CORE_CXXFLAGS="$CORE_CXXFLAGS -Og -O0" + CORE_CXXFLAGS="$CORE_CXXFLAGS -Og" fi if test "$use_lcov_branch" != "no"; then From 32a9f13cb805ecf6aebb5cf4e5d92b8a47c4548b Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Thu, 2 Nov 2023 13:26:57 +0100 Subject: [PATCH 12/33] wallet: avoid returning a reference to vMasterKey after releasing the mutex that guards it `CWallet::GetEncryptionKey()` would return a reference to the internal `CWallet::vMasterKey`, guarded by `CWallet::cs_wallet`, which is unsafe. Returning a copy would be a shorter solution, but could have security implications of the master key remaining somewhere in the memory even after `CWallet::Lock()` (the current calls to `CWallet::GetEncryptionKey()` are safe, but that is not future proof). So, instead of `EncryptSecret(m_storage.GetEncryptionKey(), ...)` change the `GetEncryptionKey()` method to provide the encryption key to a given callback: `m_storage.WithEncryptionKey([](const CKeyingMaterial& k) { EncryptSecret(k, ...); })` This silences the following (clang 18): ``` wallet/wallet.cpp:3520:12: error: returning variable 'vMasterKey' by reference requires holding mutex 'cs_wallet' [-Werror,-Wthread-safety-reference-return] 3520 | return vMasterKey; | ^ ``` --- src/wallet/scriptpubkeyman.cpp | 16 ++++++++++++---- src/wallet/scriptpubkeyman.h | 4 +++- src/wallet/wallet.cpp | 5 +++-- src/wallet/wallet.h | 3 ++- 4 files changed, 20 insertions(+), 8 deletions(-) diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index 6bbe6a1647f72..dc5c442b95df9 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -811,7 +811,9 @@ bool LegacyScriptPubKeyMan::AddKeyPubKeyInner(const CKey& key, const CPubKey &pu std::vector vchCryptedSecret; CKeyingMaterial vchSecret{UCharCast(key.begin()), UCharCast(key.end())}; - if (!EncryptSecret(m_storage.GetEncryptionKey(), vchSecret, pubkey.GetHash(), vchCryptedSecret)) { + if (!m_storage.WithEncryptionKey([&](const CKeyingMaterial& encryption_key) { + return EncryptSecret(encryption_key, vchSecret, pubkey.GetHash(), vchCryptedSecret); + })) { return false; } @@ -997,7 +999,9 @@ bool LegacyScriptPubKeyMan::GetKey(const CKeyID &address, CKey& keyOut) const { const CPubKey &vchPubKey = (*mi).second.first; const std::vector &vchCryptedSecret = (*mi).second.second; - return DecryptKey(m_storage.GetEncryptionKey(), vchCryptedSecret, vchPubKey, keyOut); + return m_storage.WithEncryptionKey([&](const CKeyingMaterial& encryption_key) { + return DecryptKey(encryption_key, vchCryptedSecret, vchPubKey, keyOut); + }); } return false; } @@ -2128,7 +2132,9 @@ std::map DescriptorScriptPubKeyMan::GetKeys() const const CPubKey& pubkey = key_pair.second.first; const std::vector& crypted_secret = key_pair.second.second; CKey key; - DecryptKey(m_storage.GetEncryptionKey(), crypted_secret, pubkey, key); + m_storage.WithEncryptionKey([&](const CKeyingMaterial& encryption_key) { + return DecryptKey(encryption_key, crypted_secret, pubkey, key); + }); keys[pubkey.GetID()] = key; } return keys; @@ -2262,7 +2268,9 @@ bool DescriptorScriptPubKeyMan::AddDescriptorKeyWithDB(WalletBatch& batch, const std::vector crypted_secret; CKeyingMaterial secret{UCharCast(key.begin()), UCharCast(key.end())}; - if (!EncryptSecret(m_storage.GetEncryptionKey(), secret, pubkey.GetHash(), crypted_secret)) { + if (!m_storage.WithEncryptionKey([&](const CKeyingMaterial& encryption_key) { + return EncryptSecret(encryption_key, secret, pubkey.GetHash(), crypted_secret); + })) { return false; } diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index 449a75eb6b668..b63ba5bda04f4 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -22,6 +22,7 @@ #include +#include #include #include @@ -46,7 +47,8 @@ class WalletStorage virtual void UnsetBlankWalletFlag(WalletBatch&) = 0; virtual bool CanSupportFeature(enum WalletFeature) const = 0; virtual void SetMinVersion(enum WalletFeature, WalletBatch* = nullptr) = 0; - virtual const CKeyingMaterial& GetEncryptionKey() const = 0; + //! Pass the encryption key to cb(). + virtual bool WithEncryptionKey(std::function cb) const = 0; virtual bool HasEncryptionKeys() const = 0; virtual bool IsLocked() const = 0; }; diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index e03f5532fcf0b..e93cd4b4b9b8a 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3513,9 +3513,10 @@ void CWallet::SetupLegacyScriptPubKeyMan() AddScriptPubKeyMan(id, std::move(spk_manager)); } -const CKeyingMaterial& CWallet::GetEncryptionKey() const +bool CWallet::WithEncryptionKey(std::function cb) const { - return vMasterKey; + LOCK(cs_wallet); + return cb(vMasterKey); } bool CWallet::HasEncryptionKeys() const diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 487921443fdc4..cc961068a54a1 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -962,7 +962,8 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati //! Make a LegacyScriptPubKeyMan and set it for all types, internal, and external. void SetupLegacyScriptPubKeyMan(); - const CKeyingMaterial& GetEncryptionKey() const override; + bool WithEncryptionKey(std::function cb) const override; + bool HasEncryptionKeys() const override; /** Get last block processed height */ From cbea49c0d32badb975fbf22d44f8e25cc7972af7 Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Fri, 19 Jan 2024 10:08:41 +0000 Subject: [PATCH 13/33] build: Pass sanitize flags to instrument `libsecp256k1` code Also a new UBSan suppression has been added. --- configure.ac | 8 ++++++-- test/sanitizer_suppressions/ubsan | 1 + 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/configure.ac b/configure.ac index 68edc84dedac3..38c8a94ae8f4c 100644 --- a/configure.ac +++ b/configure.ac @@ -388,7 +388,8 @@ if test "$use_sanitizers" != ""; then dnl fail if a bad argument is passed, e.g. -fsanitize=undfeined AX_CHECK_COMPILE_FLAG( [-fsanitize=$use_sanitizers], - [SANITIZER_CXXFLAGS="-fsanitize=$use_sanitizers"], + [SANITIZER_CXXFLAGS="-fsanitize=$use_sanitizers" + SANITIZER_CFLAGS="-fsanitize=$use_sanitizers"], [AC_MSG_ERROR([compiler did not accept requested flags])]) dnl Some compilers (e.g. GCC) require additional libraries like libasan, @@ -1946,6 +1947,9 @@ CPPFLAGS_TEMP="$CPPFLAGS" unset CPPFLAGS CPPFLAGS="$CPPFLAGS_TEMP" +if test -n "$use_sanitizers"; then + export SECP_CFLAGS="$SECP_CFLAGS $SANITIZER_CFLAGS" +fi ac_configure_args="${ac_configure_args} --disable-shared --with-pic --enable-benchmark=no --enable-module-recovery --disable-module-ecdh" AC_CONFIG_SUBDIRS([src/secp256k1]) @@ -2006,7 +2010,7 @@ echo " target os = $host_os" echo " build os = $build_os" echo echo " CC = $CC" -echo " CFLAGS = $PTHREAD_CFLAGS $CFLAGS" +echo " CFLAGS = $PTHREAD_CFLAGS $SANITIZER_CFLAGS $CFLAGS" echo " CPPFLAGS = $DEBUG_CPPFLAGS $HARDENED_CPPFLAGS $CORE_CPPFLAGS $CPPFLAGS" echo " CXX = $CXX" echo " CXXFLAGS = $LTO_CXXFLAGS $DEBUG_CXXFLAGS $HARDENED_CXXFLAGS $WARN_CXXFLAGS $NOWARN_CXXFLAGS $ERROR_CXXFLAGS $GPROF_CXXFLAGS $CORE_CXXFLAGS $CXXFLAGS" diff --git a/test/sanitizer_suppressions/ubsan b/test/sanitizer_suppressions/ubsan index f0ee698909f46..57155a32102b6 100644 --- a/test/sanitizer_suppressions/ubsan +++ b/test/sanitizer_suppressions/ubsan @@ -23,6 +23,7 @@ implicit-integer-sign-change:secp256k1/ implicit-signed-integer-truncation:*/include/c++/ implicit-signed-integer-truncation:leveldb/ implicit-signed-integer-truncation:secp256k1/ +implicit-signed-integer-truncation,implicit-integer-sign-change:secp256k1_modinv64_posdivsteps_62_var implicit-unsigned-integer-truncation:*/include/c++/ implicit-unsigned-integer-truncation:leveldb/ implicit-unsigned-integer-truncation:secp256k1/ From 435fe5cd96599c518e26efe444c9d94d1277996b Mon Sep 17 00:00:00 2001 From: josibake Date: Fri, 22 Dec 2023 17:47:03 +0100 Subject: [PATCH 14/33] test: add tests for fundrawtx and sendmany rpcs If the serialized transaction passed to `fundrawtransaction` contains duplicates, they will be deserialized and added to the transaction. Add a test to ensure this behavior is not changed during the refactor. A user can pass any number of duplicated and unrelated addresses as an SFFO argument to `sendmany` and the RPC will not throw an error (note, all the rest of the RPCs which take SFFO as an argument will error if the user passes duplicates or specifies outputs not present in the transaction). Add a test to ensure this behavior is not changed during the refactor. --- test/functional/test_runner.py | 2 + test/functional/wallet_fundrawtransaction.py | 31 ++++++++++++++ test/functional/wallet_sendmany.py | 43 ++++++++++++++++++++ 3 files changed, 76 insertions(+) create mode 100755 test/functional/wallet_sendmany.py diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py index 9c8f0eca26211..fcec2ecdbefa9 100755 --- a/test/functional/test_runner.py +++ b/test/functional/test_runner.py @@ -333,6 +333,8 @@ 'wallet_send.py --descriptors', 'wallet_sendall.py --legacy-wallet', 'wallet_sendall.py --descriptors', + 'wallet_sendmany.py --descriptors', + 'wallet_sendmany.py --legacy-wallet', 'wallet_create_tx.py --descriptors', 'wallet_inactive_hdchains.py --legacy-wallet', 'wallet_spend_unconfirmed.py', diff --git a/test/functional/wallet_fundrawtransaction.py b/test/functional/wallet_fundrawtransaction.py index a331ba997e9fc..d886a59ac15bb 100755 --- a/test/functional/wallet_fundrawtransaction.py +++ b/test/functional/wallet_fundrawtransaction.py @@ -8,10 +8,13 @@ from decimal import Decimal from itertools import product from math import ceil +from test_framework.address import address_to_scriptpubkey from test_framework.descriptors import descsum_create from test_framework.messages import ( COIN, + CTransaction, + CTxOut, ) from test_framework.test_framework import BitcoinTestFramework from test_framework.util import ( @@ -147,6 +150,34 @@ def run_test(self): self.test_22670() self.test_feerate_rounding() self.test_input_confs_control() + self.test_duplicate_outputs() + + def test_duplicate_outputs(self): + self.log.info("Test deserializing and funding a transaction with duplicate outputs") + self.nodes[1].createwallet("fundtx_duplicate_outputs") + w = self.nodes[1].get_wallet_rpc("fundtx_duplicate_outputs") + + addr = w.getnewaddress(address_type="bech32") + self.nodes[0].sendtoaddress(addr, 5) + self.generate(self.nodes[0], 1) + + address = self.nodes[0].getnewaddress("bech32") + tx = CTransaction() + tx.vin = [] + tx.vout = [CTxOut(1 * COIN, bytearray(address_to_scriptpubkey(address)))] * 2 + tx.nLockTime = 0 + tx_hex = tx.serialize().hex() + res = w.fundrawtransaction(tx_hex, add_inputs=True) + signed_res = w.signrawtransactionwithwallet(res["hex"]) + txid = w.sendrawtransaction(signed_res["hex"]) + assert self.nodes[1].getrawtransaction(txid) + + self.log.info("Test SFFO with duplicate outputs") + + res_sffo = w.fundrawtransaction(tx_hex, add_inputs=True, subtractFeeFromOutputs=[0,1]) + signed_res_sffo = w.signrawtransactionwithwallet(res_sffo["hex"]) + txid_sffo = w.sendrawtransaction(signed_res_sffo["hex"]) + assert self.nodes[1].getrawtransaction(txid_sffo) def test_change_position(self): """Ensure setting changePosition in fundraw with an exact match is handled properly.""" diff --git a/test/functional/wallet_sendmany.py b/test/functional/wallet_sendmany.py new file mode 100755 index 0000000000000..5751993143341 --- /dev/null +++ b/test/functional/wallet_sendmany.py @@ -0,0 +1,43 @@ +#!/usr/bin/env python3 +# Copyright (c) 2022 The Bitcoin Core developers +# Distributed under the MIT software license, see the accompanying +# file COPYING or http://www.opensource.org/licenses/mit-license.php. +"""Test the sendmany RPC command.""" + +from test_framework.test_framework import BitcoinTestFramework + +class SendmanyTest(BitcoinTestFramework): + # Setup and helpers + def add_options(self, parser): + self.add_wallet_options(parser) + + + def skip_test_if_missing_module(self): + self.skip_if_no_wallet() + + + def set_test_params(self): + self.num_nodes = 1 + self.setup_clean_chain = True + + def test_sffo_repeated_address(self): + addr_1 = self.wallet.getnewaddress() + addr_2 = self.wallet.getnewaddress() + addr_3 = self.wallet.getnewaddress() + + self.log.info("Test using duplicate address in SFFO argument") + self.def_wallet.sendmany(dummy='', amounts={addr_1: 1, addr_2: 1}, subtractfeefrom=[addr_1, addr_1, addr_1]) + self.log.info("Test using address not present in tx.vout in SFFO argument") + self.def_wallet.sendmany(dummy='', amounts={addr_1: 1, addr_2: 1}, subtractfeefrom=[addr_3]) + + def run_test(self): + self.nodes[0].createwallet("activewallet") + self.wallet = self.nodes[0].get_wallet_rpc("activewallet") + self.def_wallet = self.nodes[0].get_wallet_rpc(self.default_wallet_name) + self.generate(self.nodes[0], 101) + + self.test_sffo_repeated_address() + + +if __name__ == '__main__': + SendmanyTest().main() From 6f569ac903e5ddaac275996a5d0c31b2220b7b81 Mon Sep 17 00:00:00 2001 From: josibake Date: Fri, 22 Dec 2023 11:10:39 +0100 Subject: [PATCH 15/33] refactor: move normalization to new function Move the univalue formatting logic out of AddOutputs and into its own function, `NormalizeOutputs`. This allows us to re-use this logic in later commits. --- src/rpc/rawtransaction_util.cpp | 9 ++++++++- src/rpc/rawtransaction_util.h | 5 ++++- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/src/rpc/rawtransaction_util.cpp b/src/rpc/rawtransaction_util.cpp index c471986a44810..eb8067cc23577 100644 --- a/src/rpc/rawtransaction_util.cpp +++ b/src/rpc/rawtransaction_util.cpp @@ -70,7 +70,7 @@ void AddInputs(CMutableTransaction& rawTx, const UniValue& inputs_in, std::optio } } -void AddOutputs(CMutableTransaction& rawTx, const UniValue& outputs_in) +UniValue NormalizeOutputs(const UniValue& outputs_in) { if (outputs_in.isNull()) { throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter, output argument must be non-null"); @@ -94,6 +94,13 @@ void AddOutputs(CMutableTransaction& rawTx, const UniValue& outputs_in) } outputs = std::move(outputs_dict); } + return outputs; +} + +void AddOutputs(CMutableTransaction& rawTx, const UniValue& outputs_in) +{ + UniValue outputs(UniValue::VOBJ); + outputs = NormalizeOutputs(outputs_in); // Duplicate checking std::set destinations; diff --git a/src/rpc/rawtransaction_util.h b/src/rpc/rawtransaction_util.h index a863432b7a8bb..f3d5c3616ef56 100644 --- a/src/rpc/rawtransaction_util.h +++ b/src/rpc/rawtransaction_util.h @@ -42,7 +42,10 @@ void ParsePrevouts(const UniValue& prevTxsUnival, FillableSigningProvider* keyst /** Normalize univalue-represented inputs and add them to the transaction */ void AddInputs(CMutableTransaction& rawTx, const UniValue& inputs_in, bool rbf); -/** Normalize univalue-represented outputs and add them to the transaction */ +/** Normalize univalue-represented outputs */ +UniValue NormalizeOutputs(const UniValue& outputs_in); + +/** Normalize, parse, and add outputs to the transaction */ void AddOutputs(CMutableTransaction& rawTx, const UniValue& outputs_in); /** Create a transaction from univalue parameters */ From f7384b921c3460c7a3cc7827a68b2c613bd98f8e Mon Sep 17 00:00:00 2001 From: josibake Date: Fri, 22 Dec 2023 11:27:53 +0100 Subject: [PATCH 16/33] refactor: move parsing to new function Move the parsing and validation out of `AddOutputs` into its own function, `ParseOutputs`. This allows us to re-use this logic in `ParseRecipients` in a later commit, where the code is currently duplicated. The new `ParseOutputs` function returns a CTxDestination,CAmount tuples. This allows the caller to then translate the validated outputs into either CRecipients or CTxOuts. --- src/rpc/rawtransaction_util.cpp | 36 ++++++++++++++++++++------------- src/rpc/rawtransaction_util.h | 6 +++++- 2 files changed, 27 insertions(+), 15 deletions(-) diff --git a/src/rpc/rawtransaction_util.cpp b/src/rpc/rawtransaction_util.cpp index eb8067cc23577..a9e11622a7aea 100644 --- a/src/rpc/rawtransaction_util.cpp +++ b/src/rpc/rawtransaction_util.cpp @@ -97,15 +97,12 @@ UniValue NormalizeOutputs(const UniValue& outputs_in) return outputs; } -void AddOutputs(CMutableTransaction& rawTx, const UniValue& outputs_in) +std::vector> ParseOutputs(const UniValue& outputs) { - UniValue outputs(UniValue::VOBJ); - outputs = NormalizeOutputs(outputs_in); - // Duplicate checking std::set destinations; + std::vector> parsed_outputs; bool has_data{false}; - for (const std::string& name_ : outputs.getKeys()) { if (name_ == "data") { if (has_data) { @@ -113,11 +110,12 @@ void AddOutputs(CMutableTransaction& rawTx, const UniValue& outputs_in) } has_data = true; std::vector data = ParseHexV(outputs[name_].getValStr(), "Data"); - - CTxOut out(0, CScript() << OP_RETURN << data); - rawTx.vout.push_back(out); + CTxDestination destination{CNoDestination{CScript() << OP_RETURN << data}}; + CAmount amount{0}; + parsed_outputs.emplace_back(destination, amount); } else { - CTxDestination destination = DecodeDestination(name_); + CTxDestination destination{DecodeDestination(name_)}; + CAmount amount{AmountFromValue(outputs[name_])}; if (!IsValidDestination(destination)) { throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, std::string("Invalid Bitcoin address: ") + name_); } @@ -125,13 +123,23 @@ void AddOutputs(CMutableTransaction& rawTx, const UniValue& outputs_in) if (!destinations.insert(destination).second) { throw JSONRPCError(RPC_INVALID_PARAMETER, std::string("Invalid parameter, duplicated address: ") + name_); } + parsed_outputs.emplace_back(destination, amount); + } + } + return parsed_outputs; +} - CScript scriptPubKey = GetScriptForDestination(destination); - CAmount nAmount = AmountFromValue(outputs[name_]); +void AddOutputs(CMutableTransaction& rawTx, const UniValue& outputs_in) +{ + UniValue outputs(UniValue::VOBJ); + outputs = NormalizeOutputs(outputs_in); - CTxOut out(nAmount, scriptPubKey); - rawTx.vout.push_back(out); - } + std::vector> parsed_outputs = ParseOutputs(outputs); + for (const auto& [destination, nAmount] : parsed_outputs) { + CScript scriptPubKey = GetScriptForDestination(destination); + + CTxOut out(nAmount, scriptPubKey); + rawTx.vout.push_back(out); } } diff --git a/src/rpc/rawtransaction_util.h b/src/rpc/rawtransaction_util.h index f3d5c3616ef56..964d0b095b1c3 100644 --- a/src/rpc/rawtransaction_util.h +++ b/src/rpc/rawtransaction_util.h @@ -5,6 +5,8 @@ #ifndef BITCOIN_RPC_RAWTRANSACTION_UTIL_H #define BITCOIN_RPC_RAWTRANSACTION_UTIL_H +#include +#include #include #include #include @@ -38,13 +40,15 @@ void SignTransactionResultToJSON(CMutableTransaction& mtx, bool complete, const */ void ParsePrevouts(const UniValue& prevTxsUnival, FillableSigningProvider* keystore, std::map& coins); - /** Normalize univalue-represented inputs and add them to the transaction */ void AddInputs(CMutableTransaction& rawTx, const UniValue& inputs_in, bool rbf); /** Normalize univalue-represented outputs */ UniValue NormalizeOutputs(const UniValue& outputs_in); +/** Parse normalized outputs into destination, amount tuples */ +std::vector> ParseOutputs(const UniValue& outputs); + /** Normalize, parse, and add outputs to the transaction */ void AddOutputs(CMutableTransaction& rawTx, const UniValue& outputs_in); From 47353a608dc6e20e5fd2ca53850d6f9aa3240d4a Mon Sep 17 00:00:00 2001 From: josibake Date: Fri, 22 Dec 2023 13:58:56 +0100 Subject: [PATCH 17/33] refactor: remove out param from `ParseRecipients` Have `ParseRecipients` return a vector of `CRecipients` and rename to `CreateRecipients`. --- src/wallet/rpc/spend.cpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/wallet/rpc/spend.cpp b/src/wallet/rpc/spend.cpp index 5a13b5ac8ecfa..eb3e607320182 100644 --- a/src/wallet/rpc/spend.cpp +++ b/src/wallet/rpc/spend.cpp @@ -24,8 +24,9 @@ namespace wallet { -static void ParseRecipients(const UniValue& address_amounts, const UniValue& subtract_fee_outputs, std::vector& recipients) +std::vector CreateRecipients(const UniValue& address_amounts, const UniValue& subtract_fee_outputs) { + std::vector recipients; std::set destinations; int i = 0; for (const std::string& address: address_amounts.getKeys()) { @@ -52,6 +53,7 @@ static void ParseRecipients(const UniValue& address_amounts, const UniValue& sub CRecipient recipient = {dest, amount, subtract_fee}; recipients.push_back(recipient); } + return recipients; } static void InterpretFeeEstimationInstructions(const UniValue& conf_target, const UniValue& estimate_mode, const UniValue& fee_rate, UniValue& options) @@ -301,8 +303,7 @@ RPCHelpMan sendtoaddress() subtractFeeFromAmount.push_back(address); } - std::vector recipients; - ParseRecipients(address_amounts, subtractFeeFromAmount, recipients); + std::vector recipients = CreateRecipients(address_amounts, subtractFeeFromAmount); const bool verbose{request.params[10].isNull() ? false : request.params[10].get_bool()}; return SendMoney(*pwallet, coin_control, recipients, mapValue, verbose); @@ -397,8 +398,7 @@ RPCHelpMan sendmany() SetFeeEstimateMode(*pwallet, coin_control, /*conf_target=*/request.params[6], /*estimate_mode=*/request.params[7], /*fee_rate=*/request.params[8], /*override_min_fee=*/false); - std::vector recipients; - ParseRecipients(sendTo, subtractFeeFromAmount, recipients); + std::vector recipients = CreateRecipients(sendTo, subtractFeeFromAmount); const bool verbose{request.params[9].isNull() ? false : request.params[9].get_bool()}; return SendMoney(*pwallet, coin_control, recipients, std::move(mapValue), verbose); From 5ad19668dbcc47486d1c18f711cea3d8a9d2e7e2 Mon Sep 17 00:00:00 2001 From: josibake Date: Mon, 2 Oct 2023 14:36:53 +0200 Subject: [PATCH 18/33] refactor: simplify `CreateRecipients` Move validation logic out of `CreateRecipients` and instead take the already validated outputs from `ParseOutputs` as an input. Move SFFO parsing out of `CreateRecipients` into a new function, `InterpretSubtractFeeFromOutputsInstructions`. This takes the SFFO instructions from `sendmany` and `sendtoaddress` and turns them into a set of integers. In a later commit, we will also move the SFFO parsing logic from `FundTransaction` into this function. Worth noting: a user can pass duplicate addresses and addresses that dont exist in the transaction outputs as SFFO args to `sendmany` and `sendtoaddress` without triggering a warning. This behavior is preserved in to keep this commit strictly a refactor. --- src/wallet/rpc/spend.cpp | 74 ++++++++++++++++++---------------------- 1 file changed, 33 insertions(+), 41 deletions(-) diff --git a/src/wallet/rpc/spend.cpp b/src/wallet/rpc/spend.cpp index eb3e607320182..6e39e4d5a6fcb 100644 --- a/src/wallet/rpc/spend.cpp +++ b/src/wallet/rpc/spend.cpp @@ -24,33 +24,12 @@ namespace wallet { -std::vector CreateRecipients(const UniValue& address_amounts, const UniValue& subtract_fee_outputs) +std::vector CreateRecipients(const std::vector>& outputs, const std::set& subtract_fee_outputs) { std::vector recipients; - std::set destinations; - int i = 0; - for (const std::string& address: address_amounts.getKeys()) { - CTxDestination dest = DecodeDestination(address); - if (!IsValidDestination(dest)) { - throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, std::string("Invalid Bitcoin address: ") + address); - } - - if (destinations.count(dest)) { - throw JSONRPCError(RPC_INVALID_PARAMETER, std::string("Invalid parameter, duplicated address: ") + address); - } - destinations.insert(dest); - - CAmount amount = AmountFromValue(address_amounts[i++]); - - bool subtract_fee = false; - for (unsigned int idx = 0; idx < subtract_fee_outputs.size(); idx++) { - const UniValue& addr = subtract_fee_outputs[idx]; - if (addr.get_str() == address) { - subtract_fee = true; - } - } - - CRecipient recipient = {dest, amount, subtract_fee}; + for (size_t i = 0; i < outputs.size(); ++i) { + const auto& [destination, amount] = outputs.at(i); + CRecipient recipient{destination, amount, subtract_fee_outputs.contains(i)}; recipients.push_back(recipient); } return recipients; @@ -78,6 +57,27 @@ static void InterpretFeeEstimationInstructions(const UniValue& conf_target, cons } } +std::set InterpretSubtractFeeFromOutputInstructions(const UniValue& sffo_instructions, const std::vector& destinations) +{ + std::set sffo_set; + if (sffo_instructions.isNull()) return sffo_set; + if (sffo_instructions.isBool()) { + if (sffo_instructions.get_bool()) sffo_set.insert(0); + return sffo_set; + } + for (const auto& sffo : sffo_instructions.getValues()) { + if (sffo.isStr()) { + for (size_t i = 0; i < destinations.size(); ++i) { + if (sffo.get_str() == destinations.at(i)) { + sffo_set.insert(i); + break; + } + } + } + } + return sffo_set; +} + static UniValue FinishTransaction(const std::shared_ptr pwallet, const UniValue& options, const CMutableTransaction& rawTx) { // Make a blank psbt @@ -277,11 +277,6 @@ RPCHelpMan sendtoaddress() if (!request.params[3].isNull() && !request.params[3].get_str().empty()) mapValue["to"] = request.params[3].get_str(); - bool fSubtractFeeFromAmount = false; - if (!request.params[4].isNull()) { - fSubtractFeeFromAmount = request.params[4].get_bool(); - } - CCoinControl coin_control; if (!request.params[5].isNull()) { coin_control.m_signal_bip125_rbf = request.params[5].get_bool(); @@ -298,12 +293,10 @@ RPCHelpMan sendtoaddress() UniValue address_amounts(UniValue::VOBJ); const std::string address = request.params[0].get_str(); address_amounts.pushKV(address, request.params[1]); - UniValue subtractFeeFromAmount(UniValue::VARR); - if (fSubtractFeeFromAmount) { - subtractFeeFromAmount.push_back(address); - } - - std::vector recipients = CreateRecipients(address_amounts, subtractFeeFromAmount); + std::vector recipients = CreateRecipients( + ParseOutputs(address_amounts), + InterpretSubtractFeeFromOutputInstructions(request.params[4], address_amounts.getKeys()) + ); const bool verbose{request.params[10].isNull() ? false : request.params[10].get_bool()}; return SendMoney(*pwallet, coin_control, recipients, mapValue, verbose); @@ -387,10 +380,6 @@ RPCHelpMan sendmany() if (!request.params[3].isNull() && !request.params[3].get_str().empty()) mapValue["comment"] = request.params[3].get_str(); - UniValue subtractFeeFromAmount(UniValue::VARR); - if (!request.params[4].isNull()) - subtractFeeFromAmount = request.params[4].get_array(); - CCoinControl coin_control; if (!request.params[5].isNull()) { coin_control.m_signal_bip125_rbf = request.params[5].get_bool(); @@ -398,7 +387,10 @@ RPCHelpMan sendmany() SetFeeEstimateMode(*pwallet, coin_control, /*conf_target=*/request.params[6], /*estimate_mode=*/request.params[7], /*fee_rate=*/request.params[8], /*override_min_fee=*/false); - std::vector recipients = CreateRecipients(sendTo, subtractFeeFromAmount); + std::vector recipients = CreateRecipients( + ParseOutputs(sendTo), + InterpretSubtractFeeFromOutputInstructions(request.params[4], sendTo.getKeys()) + ); const bool verbose{request.params[9].isNull() ? false : request.params[9].get_bool()}; return SendMoney(*pwallet, coin_control, recipients, std::move(mapValue), verbose); From 18ad1b9142e91cef2f5c6a693eeb2d0fbb8c517d Mon Sep 17 00:00:00 2001 From: josibake Date: Mon, 2 Oct 2023 16:57:44 +0200 Subject: [PATCH 19/33] refactor: pass CRecipient to FundTransaction Instead turning tx.vout into a vector of `CRecipient`, make `FundTransaction` take a `CRecipient` vector directly. This allows us to remove SFFO logic from the wrapper RPC `FundTransaction` since the `CRecipient` objects have already been created with the correct SFFO values. This also allows us to remove SFFO from both `FundTransaction` function signatures. This sets us up in a future PR to be able to use these RPCs with BIP352 static payment codes. --- src/wallet/rpc/spend.cpp | 78 +++++++++++++++++--------- src/wallet/spend.cpp | 15 ++--- src/wallet/spend.h | 2 +- src/wallet/test/fuzz/notifications.cpp | 13 ++++- 4 files changed, 70 insertions(+), 38 deletions(-) diff --git a/src/wallet/rpc/spend.cpp b/src/wallet/rpc/spend.cpp index 6e39e4d5a6fcb..6060f017ce994 100644 --- a/src/wallet/rpc/spend.cpp +++ b/src/wallet/rpc/spend.cpp @@ -74,6 +74,16 @@ std::set InterpretSubtractFeeFromOutputInstructions(const UniValue& sffo_in } } } + if (sffo.isNum()) { + int pos = sffo.getInt(); + if (sffo_set.contains(pos)) + throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Invalid parameter, duplicated position: %d", pos)); + if (pos < 0) + throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Invalid parameter, negative position: %d", pos)); + if (pos >= int(destinations.size())) + throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Invalid parameter, position too large: %d", pos)); + sffo_set.insert(pos); + } } return sffo_set; } @@ -480,17 +490,17 @@ static std::vector FundTxDoc(bool solving_data = true) return args; } -CreatedTransactionResult FundTransaction(CWallet& wallet, const CMutableTransaction& tx, const UniValue& options, CCoinControl& coinControl, bool override_min_fee) +CreatedTransactionResult FundTransaction(CWallet& wallet, const CMutableTransaction& tx, const std::vector& recipients, const UniValue& options, CCoinControl& coinControl, bool override_min_fee) { + // We want to make sure tx.vout is not used now that we are passing outputs as a vector of recipients. + // This sets us up to remove tx completely in a future PR in favor of passing the inputs directly. + CHECK_NONFATAL(tx.vout.empty()); // Make sure the results are valid at least up to the most recent block // the user could have gotten from another RPC command prior to now wallet.BlockUntilSyncedToCurrentChain(); std::optional change_position; bool lockUnspents = false; - UniValue subtractFeeFromOutputs; - std::set setSubtractFeeFromOutputs; - if (!options.isNull()) { if (options.type() == UniValue::VBOOL) { // backward compatibility bool only fallback @@ -545,7 +555,7 @@ CreatedTransactionResult FundTransaction(CWallet& wallet, const CMutableTransact if (options.exists("changePosition") || options.exists("change_position")) { int pos = (options.exists("change_position") ? options["change_position"] : options["changePosition"]).getInt(); - if (pos < 0 || (unsigned int)pos > tx.vout.size()) { + if (pos < 0 || (unsigned int)pos > recipients.size()) { throw JSONRPCError(RPC_INVALID_PARAMETER, "changePosition out of bounds"); } change_position = (unsigned int)pos; @@ -587,9 +597,6 @@ CreatedTransactionResult FundTransaction(CWallet& wallet, const CMutableTransact coinControl.fOverrideFeeRate = true; } - if (options.exists("subtractFeeFromOutputs") || options.exists("subtract_fee_from_outputs") ) - subtractFeeFromOutputs = (options.exists("subtract_fee_from_outputs") ? options["subtract_fee_from_outputs"] : options["subtractFeeFromOutputs"]).get_array(); - if (options.exists("replaceable")) { coinControl.m_signal_bip125_rbf = options["replaceable"].get_bool(); } @@ -695,21 +702,10 @@ CreatedTransactionResult FundTransaction(CWallet& wallet, const CMutableTransact } } - if (tx.vout.size() == 0) + if (recipients.empty()) throw JSONRPCError(RPC_INVALID_PARAMETER, "TX must have at least one output"); - for (unsigned int idx = 0; idx < subtractFeeFromOutputs.size(); idx++) { - int pos = subtractFeeFromOutputs[idx].getInt(); - if (setSubtractFeeFromOutputs.count(pos)) - throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Invalid parameter, duplicated position: %d", pos)); - if (pos < 0) - throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Invalid parameter, negative position: %d", pos)); - if (pos >= int(tx.vout.size())) - throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Invalid parameter, position too large: %d", pos)); - setSubtractFeeFromOutputs.insert(pos); - } - - auto txr = FundTransaction(wallet, tx, change_position, lockUnspents, setSubtractFeeFromOutputs, coinControl); + auto txr = FundTransaction(wallet, tx, recipients, change_position, lockUnspents, coinControl); if (!txr) { throw JSONRPCError(RPC_WALLET_ERROR, ErrorString(txr).original); } @@ -835,11 +831,25 @@ RPCHelpMan fundrawtransaction() if (!DecodeHexTx(tx, request.params[0].get_str(), try_no_witness, try_witness)) { throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "TX decode failed"); } - + UniValue options = request.params[1]; + std::vector> destinations; + for (const auto& tx_out : tx.vout) { + CTxDestination dest; + ExtractDestination(tx_out.scriptPubKey, dest); + destinations.emplace_back(dest, tx_out.nValue); + } + std::vector dummy(destinations.size(), "dummy"); + std::vector recipients = CreateRecipients( + destinations, + InterpretSubtractFeeFromOutputInstructions(options["subtractFeeFromOutputs"], dummy) + ); CCoinControl coin_control; // Automatically select (additional) coins. Can be overridden by options.add_inputs. coin_control.m_allow_other_inputs = true; - auto txr = FundTransaction(*pwallet, tx, request.params[1], coin_control, /*override_min_fee=*/true); + // Clear tx.vout since it is not meant to be used now that we are passing outputs directly. + // This sets us up for a future PR to completely remove tx from the function signature in favor of passing inputs directly + tx.vout.clear(); + auto txr = FundTransaction(*pwallet, tx, recipients, options, coin_control, /*override_min_fee=*/true); UniValue result(UniValue::VOBJ); result.pushKV("hex", EncodeHexTx(*txr.tx)); @@ -1267,13 +1277,22 @@ RPCHelpMan send() bool rbf{options.exists("replaceable") ? options["replaceable"].get_bool() : pwallet->m_signal_rbf}; + UniValue outputs(UniValue::VOBJ); + outputs = NormalizeOutputs(request.params[0]); + std::vector recipients = CreateRecipients( + ParseOutputs(outputs), + InterpretSubtractFeeFromOutputInstructions(options["subtract_fee_from_outputs"], outputs.getKeys()) + ); CMutableTransaction rawTx = ConstructTransaction(options["inputs"], request.params[0], options["locktime"], rbf); CCoinControl coin_control; // Automatically select coins, unless at least one is manually selected. Can // be overridden by options.add_inputs. coin_control.m_allow_other_inputs = rawTx.vin.size() == 0; SetOptionsInputWeights(options["inputs"], options); - auto txr = FundTransaction(*pwallet, rawTx, options, coin_control, /*override_min_fee=*/false); + // Clear tx.vout since it is not meant to be used now that we are passing outputs directly. + // This sets us up for a future PR to completely remove tx from the function signature in favor of passing inputs directly + rawTx.vout.clear(); + auto txr = FundTransaction(*pwallet, rawTx, recipients, options, coin_control, /*override_min_fee=*/false); return FinishTransaction(pwallet, options, CMutableTransaction(*txr.tx)); } @@ -1703,12 +1722,21 @@ RPCHelpMan walletcreatefundedpsbt() const UniValue &replaceable_arg = options["replaceable"]; const bool rbf{replaceable_arg.isNull() ? wallet.m_signal_rbf : replaceable_arg.get_bool()}; CMutableTransaction rawTx = ConstructTransaction(request.params[0], request.params[1], request.params[2], rbf); + UniValue outputs(UniValue::VOBJ); + outputs = NormalizeOutputs(request.params[1]); + std::vector recipients = CreateRecipients( + ParseOutputs(outputs), + InterpretSubtractFeeFromOutputInstructions(options["subtractFeeFromOutputs"], outputs.getKeys()) + ); CCoinControl coin_control; // Automatically select coins, unless at least one is manually selected. Can // be overridden by options.add_inputs. coin_control.m_allow_other_inputs = rawTx.vin.size() == 0; SetOptionsInputWeights(request.params[0], options); - auto txr = FundTransaction(wallet, rawTx, options, coin_control, /*override_min_fee=*/true); + // Clear tx.vout since it is not meant to be used now that we are passing outputs directly. + // This sets us up for a future PR to completely remove tx from the function signature in favor of passing inputs directly + rawTx.vout.clear(); + auto txr = FundTransaction(wallet, rawTx, recipients, options, coin_control, /*override_min_fee=*/true); // Make a blank psbt PartiallySignedTransaction psbtx(CMutableTransaction(*txr.tx)); diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index b51cd6332fb6d..a2bf4976dc8da 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -1365,18 +1365,11 @@ util::Result CreateTransaction( return res; } -util::Result FundTransaction(CWallet& wallet, const CMutableTransaction& tx, std::optional change_pos, bool lockUnspents, const std::set& setSubtractFeeFromOutputs, CCoinControl coinControl) +util::Result FundTransaction(CWallet& wallet, const CMutableTransaction& tx, const std::vector& vecSend, std::optional change_pos, bool lockUnspents, CCoinControl coinControl) { - std::vector vecSend; - - // Turn the txout set into a CRecipient vector. - for (size_t idx = 0; idx < tx.vout.size(); idx++) { - const CTxOut& txOut = tx.vout[idx]; - CTxDestination dest; - ExtractDestination(txOut.scriptPubKey, dest); - CRecipient recipient = {dest, txOut.nValue, setSubtractFeeFromOutputs.count(idx) == 1}; - vecSend.push_back(recipient); - } + // We want to make sure tx.vout is not used now that we are passing outputs as a vector of recipients. + // This sets us up to remove tx completely in a future PR in favor of passing the inputs directly. + assert(tx.vout.empty()); // Set the user desired locktime coinControl.m_locktime = tx.nLockTime; diff --git a/src/wallet/spend.h b/src/wallet/spend.h index 3bd37cfd0eca4..62a7b4e4c8926 100644 --- a/src/wallet/spend.h +++ b/src/wallet/spend.h @@ -224,7 +224,7 @@ util::Result CreateTransaction(CWallet& wallet, const * Insert additional inputs into the transaction by * calling CreateTransaction(); */ -util::Result FundTransaction(CWallet& wallet, const CMutableTransaction& tx, std::optional change_pos, bool lockUnspents, const std::set& setSubtractFeeFromOutputs, CCoinControl); +util::Result FundTransaction(CWallet& wallet, const CMutableTransaction& tx, const std::vector& recipients, std::optional change_pos, bool lockUnspents, CCoinControl); } // namespace wallet #endif // BITCOIN_WALLET_SPEND_H diff --git a/src/wallet/test/fuzz/notifications.cpp b/src/wallet/test/fuzz/notifications.cpp index 203ab5f606a79..376060421c137 100644 --- a/src/wallet/test/fuzz/notifications.cpp +++ b/src/wallet/test/fuzz/notifications.cpp @@ -132,6 +132,14 @@ struct FuzzedWallet { } } } + std::vector recipients; + for (size_t idx = 0; idx < tx.vout.size(); idx++) { + const CTxOut& tx_out = tx.vout[idx]; + CTxDestination dest; + ExtractDestination(tx_out.scriptPubKey, dest); + CRecipient recipient = {dest, tx_out.nValue, subtract_fee_from_outputs.count(idx) == 1}; + recipients.push_back(recipient); + } CCoinControl coin_control; coin_control.m_allow_other_inputs = fuzzed_data_provider.ConsumeBool(); CallOneOf( @@ -158,7 +166,10 @@ struct FuzzedWallet { int change_position{fuzzed_data_provider.ConsumeIntegralInRange(-1, tx.vout.size() - 1)}; bilingual_str error; - (void)FundTransaction(*wallet, tx, change_position, /*lockUnspents=*/false, subtract_fee_from_outputs, coin_control); + // Clear tx.vout since it is not meant to be used now that we are passing outputs directly. + // This sets us up for a future PR to completely remove tx from the function signature in favor of passing inputs directly + tx.vout.clear(); + (void)FundTransaction(*wallet, tx, recipients, change_position, /*lockUnspents=*/false, coin_control); } }; From 3bfc5bd36e38ca07306a41a3f56ea102ffe02b67 Mon Sep 17 00:00:00 2001 From: stickies-v Date: Fri, 19 Jan 2024 14:12:40 +0000 Subject: [PATCH 20/33] test: ensure output is large enough to pay for its fees Fixes a (rare) intermittency issue in wallet_import_rescan. Since we use `subtract_fee_from_outputs=[0]` in the `send` command, the output amount must at least be as large as the fee we're paying. --- test/functional/wallet_import_rescan.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/test/functional/wallet_import_rescan.py b/test/functional/wallet_import_rescan.py index 7f01d23941c6b..56cb71cd62395 100755 --- a/test/functional/wallet_import_rescan.py +++ b/test/functional/wallet_import_rescan.py @@ -24,6 +24,7 @@ AddressType, ADDRESS_BCRT1_UNSPENDABLE, ) +from test_framework.messages import COIN from test_framework.util import ( assert_equal, set_node_times, @@ -270,7 +271,9 @@ def run_test(self): address_type=variant.address_type.value, )) variant.key = self.nodes[1].dumpprivkey(variant.address["address"]) - variant.initial_amount = get_rand_amount() * 2 + # Ensure output is large enough to pay for fees: conservatively assuming txsize of + # 500 vbytes and feerate of 20 sats/vbytes + variant.initial_amount = max(get_rand_amount(), (500 * 20 / COIN) + AMOUNT_DUST) variant.initial_txid = self.nodes[0].sendtoaddress(variant.address["address"], variant.initial_amount) variant.confirmation_height = 0 variant.timestamp = timestamp From 2d58629ee63eebc760e2a9226afcd0c46d3ec2bd Mon Sep 17 00:00:00 2001 From: Richard Myers Date: Wed, 17 Jan 2024 15:57:02 +0100 Subject: [PATCH 21/33] wallet: fix coin selection tracing to return -1 when no change pos --- src/wallet/spend.cpp | 4 ++-- .../interface_usdt_coinselection.py | 23 +++++++++++++++++++ 2 files changed, 25 insertions(+), 2 deletions(-) diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index b51cd6332fb6d..5b464ae3f5468 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -1337,7 +1337,7 @@ util::Result CreateTransaction( auto res = CreateTransactionInternal(wallet, vecSend, change_pos, coin_control, sign); TRACE4(coin_selection, normal_create_tx_internal, wallet.GetName().c_str(), bool(res), - res ? res->fee : 0, res && res->change_pos.has_value() ? *res->change_pos : 0); + res ? res->fee : 0, res && res->change_pos.has_value() ? int32_t(*res->change_pos) : -1); if (!res) return res; const auto& txr_ungrouped = *res; // try with avoidpartialspends unless it's enabled already @@ -1355,7 +1355,7 @@ util::Result CreateTransaction( // if fee of this alternative one is within the range of the max fee, we use this one const bool use_aps{txr_grouped.has_value() ? (txr_grouped->fee <= txr_ungrouped.fee + wallet.m_max_aps_fee) : false}; TRACE5(coin_selection, aps_create_tx_internal, wallet.GetName().c_str(), use_aps, txr_grouped.has_value(), - txr_grouped.has_value() ? txr_grouped->fee : 0, txr_grouped.has_value() && txr_grouped->change_pos.has_value() ? *txr_grouped->change_pos : 0); + txr_grouped.has_value() ? txr_grouped->fee : 0, txr_grouped.has_value() && txr_grouped->change_pos.has_value() ? int32_t(*txr_grouped->change_pos) : -1); if (txr_grouped) { wallet.WalletLogPrintf("Fee non-grouped = %lld, grouped = %lld, using %s\n", txr_ungrouped.fee, txr_grouped->fee, use_aps ? "grouped" : "non-grouped"); diff --git a/test/functional/interface_usdt_coinselection.py b/test/functional/interface_usdt_coinselection.py index aff90ea5fc74d..30931a41cd252 100755 --- a/test/functional/interface_usdt_coinselection.py +++ b/test/functional/interface_usdt_coinselection.py @@ -204,6 +204,29 @@ def run_test(self): assert_equal(success, True) assert_equal(use_aps, None) + self.log.info("Change position is -1 if no change is created with APS when APS was initially not used") + # We should have 2 tracepoints in the order: + # 1. selected_coins (type 1) + # 2. normal_create_tx_internal (type 2) + # 3. attempting_aps_create_tx (type 3) + # 4. selected_coins (type 1) + # 5. aps_create_tx_internal (type 4) + wallet.sendtoaddress(address=wallet.getnewaddress(), amount=wallet.getbalance(), subtractfeefromamount=True, avoid_reuse=False) + events = self.get_tracepoints([1, 2, 3, 1, 4]) + success, use_aps, algo, waste, change_pos = self.determine_selection_from_usdt(events) + assert_equal(success, True) + assert_equal(change_pos, -1) + + self.log.info("Change position is -1 if no change is created normally and APS is not used") + # We should have 2 tracepoints in the order: + # 1. selected_coins (type 1) + # 2. normal_create_tx_internal (type 2) + wallet.sendtoaddress(address=wallet.getnewaddress(), amount=wallet.getbalance(), subtractfeefromamount=True) + events = self.get_tracepoints([1, 2]) + success, use_aps, algo, waste, change_pos = self.determine_selection_from_usdt(events) + assert_equal(success, True) + assert_equal(change_pos, -1) + self.bpf.cleanup() From d55fdb1a495190e213b1b5127f5d91e4a409765e Mon Sep 17 00:00:00 2001 From: Richard Myers Date: Sat, 20 Jan 2024 12:25:03 +0100 Subject: [PATCH 22/33] Move TRACEx parameters to seperate lines --- src/wallet/spend.cpp | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index 5b464ae3f5468..e9dd96d22c6d3 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -1127,7 +1127,12 @@ static util::Result CreateTransactionInternal( return util::Error{err.empty() ?_("Insufficient funds") : err}; } const SelectionResult& result = *select_coins_res; - TRACE5(coin_selection, selected_coins, wallet.GetName().c_str(), GetAlgorithmName(result.GetAlgo()).c_str(), result.GetTarget(), result.GetWaste(), result.GetSelectedValue()); + TRACE5(coin_selection, selected_coins, + wallet.GetName().c_str(), + GetAlgorithmName(result.GetAlgo()).c_str(), + result.GetTarget(), + result.GetWaste(), + result.GetSelectedValue()); const CAmount change_amount = result.GetChange(coin_selection_params.min_viable_change, coin_selection_params.m_change_fee); if (change_amount > 0) { @@ -1336,8 +1341,11 @@ util::Result CreateTransaction( LOCK(wallet.cs_wallet); auto res = CreateTransactionInternal(wallet, vecSend, change_pos, coin_control, sign); - TRACE4(coin_selection, normal_create_tx_internal, wallet.GetName().c_str(), bool(res), - res ? res->fee : 0, res && res->change_pos.has_value() ? int32_t(*res->change_pos) : -1); + TRACE4(coin_selection, normal_create_tx_internal, + wallet.GetName().c_str(), + bool(res), + res ? res->fee : 0, + res && res->change_pos.has_value() ? int32_t(*res->change_pos) : -1); if (!res) return res; const auto& txr_ungrouped = *res; // try with avoidpartialspends unless it's enabled already @@ -1354,8 +1362,12 @@ util::Result CreateTransaction( auto txr_grouped = CreateTransactionInternal(wallet, vecSend, change_pos, tmp_cc, sign); // if fee of this alternative one is within the range of the max fee, we use this one const bool use_aps{txr_grouped.has_value() ? (txr_grouped->fee <= txr_ungrouped.fee + wallet.m_max_aps_fee) : false}; - TRACE5(coin_selection, aps_create_tx_internal, wallet.GetName().c_str(), use_aps, txr_grouped.has_value(), - txr_grouped.has_value() ? txr_grouped->fee : 0, txr_grouped.has_value() && txr_grouped->change_pos.has_value() ? int32_t(*txr_grouped->change_pos) : -1); + TRACE5(coin_selection, aps_create_tx_internal, + wallet.GetName().c_str(), + use_aps, + txr_grouped.has_value(), + txr_grouped.has_value() ? txr_grouped->fee : 0, + txr_grouped.has_value() && txr_grouped->change_pos.has_value() ? int32_t(*txr_grouped->change_pos) : -1); if (txr_grouped) { wallet.WalletLogPrintf("Fee non-grouped = %lld, grouped = %lld, using %s\n", txr_ungrouped.fee, txr_grouped->fee, use_aps ? "grouped" : "non-grouped"); From 966f5de99a9f5da05c91378ad1e8ea8ed37ac3b3 Mon Sep 17 00:00:00 2001 From: furszy Date: Tue, 9 Jan 2024 09:54:49 -0300 Subject: [PATCH 23/33] init: improve corrupted/empty settings file error msg The preceding "Unable to parse settings file" message lacked the necessary detail and guidance for users on what steps to take next in order to resolve the startup error. Co-authored-by: Ryan Ofsky --- src/common/settings.cpp | 4 +++- src/test/settings_tests.cpp | 4 +++- test/functional/feature_settings.py | 2 +- 3 files changed, 7 insertions(+), 3 deletions(-) diff --git a/src/common/settings.cpp b/src/common/settings.cpp index 5761e8b321e37..cbf794a7c6207 100644 --- a/src/common/settings.cpp +++ b/src/common/settings.cpp @@ -81,7 +81,9 @@ bool ReadSettings(const fs::path& path, std::map& va SettingsValue in; if (!in.read(std::string{std::istreambuf_iterator(file), std::istreambuf_iterator()})) { - errors.emplace_back(strprintf("Unable to parse settings file %s", fs::PathToString(path))); + errors.emplace_back(strprintf("Settings file %s does not contain valid JSON. This is probably caused by disk corruption or a crash, " + "and can be fixed by removing the file, which will reset settings to default values.", + fs::PathToString(path))); return false; } diff --git a/src/test/settings_tests.cpp b/src/test/settings_tests.cpp index fa8ceb8dd6b09..41190b357985f 100644 --- a/src/test/settings_tests.cpp +++ b/src/test/settings_tests.cpp @@ -99,7 +99,9 @@ BOOST_AUTO_TEST_CASE(ReadWrite) // Check invalid json not allowed WriteText(path, R"(invalid json)"); BOOST_CHECK(!common::ReadSettings(path, values, errors)); - std::vector fail_parse = {strprintf("Unable to parse settings file %s", fs::PathToString(path))}; + std::vector fail_parse = {strprintf("Settings file %s does not contain valid JSON. This is probably caused by disk corruption or a crash, " + "and can be fixed by removing the file, which will reset settings to default values.", + fs::PathToString(path))}; BOOST_CHECK_EQUAL_COLLECTIONS(errors.begin(), errors.end(), fail_parse.begin(), fail_parse.end()); } diff --git a/test/functional/feature_settings.py b/test/functional/feature_settings.py index 1bacdd447a2a5..4175699152e00 100755 --- a/test/functional/feature_settings.py +++ b/test/functional/feature_settings.py @@ -53,7 +53,7 @@ def run_test(self): # Test invalid json with settings.open("w") as fp: fp.write("invalid json") - node.assert_start_raises_init_error(expected_msg='Unable to parse settings file', match=ErrorMatch.PARTIAL_REGEX) + node.assert_start_raises_init_error(expected_msg='does not contain valid JSON. This is probably caused by disk corruption or a crash', match=ErrorMatch.PARTIAL_REGEX) # Test invalid json object with settings.open("w") as fp: From e9014042a6bed8c16cc9a31fc35cb709d4b3c766 Mon Sep 17 00:00:00 2001 From: furszy Date: Tue, 9 Jan 2024 10:25:08 -0300 Subject: [PATCH 24/33] settings: add auto-generated warning msg for editing the file manually Hopefully, refraining users from modifying the file unless they are certain about the potential consequences. Co-authored-by: Ryan Ofsky --- src/common/settings.cpp | 11 +++++++++++ src/qt/test/optiontests.cpp | 4 ++++ test/functional/feature_settings.py | 7 ++++--- 3 files changed, 19 insertions(+), 3 deletions(-) diff --git a/src/common/settings.cpp b/src/common/settings.cpp index cbf794a7c6207..913ad6d76a98e 100644 --- a/src/common/settings.cpp +++ b/src/common/settings.cpp @@ -4,6 +4,10 @@ #include +#if defined(HAVE_CONFIG_H) +#include +#endif + #include #include #include @@ -116,6 +120,13 @@ bool WriteSettings(const fs::path& path, std::vector& errors) { SettingsValue out(SettingsValue::VOBJ); + // Add auto-generated warning comment only if it does not exist + if (!values.contains("_warning_")) { + out.pushKV("_warning_", strprintf("This file is automatically generated and updated by %s. Please do not edit this file while the node " + "is running, as any changes might be ignored or overwritten.", + PACKAGE_NAME)); + } + // Push settings values for (const auto& value : values) { out.pushKVEnd(value.first, value.second); } diff --git a/src/qt/test/optiontests.cpp b/src/qt/test/optiontests.cpp index e5a5179d87951..7100603616cee 100644 --- a/src/qt/test/optiontests.cpp +++ b/src/qt/test/optiontests.cpp @@ -61,7 +61,11 @@ void OptionTests::migrateSettings() QVERIFY(!settings.contains("addrSeparateProxyTor")); std::ifstream file(gArgs.GetDataDirNet() / "settings.json"); + std::string default_warning = strprintf("This file is automatically generated and updated by %s. Please do not edit this file while the node " + "is running, as any changes might be ignored or overwritten.", + PACKAGE_NAME); QCOMPARE(std::string(std::istreambuf_iterator(file), std::istreambuf_iterator()).c_str(), "{\n" + " \"_warning_\": \""+ default_warning+"\",\n" " \"dbcache\": \"600\",\n" " \"listen\": false,\n" " \"onion\": \"onion:234\",\n" diff --git a/test/functional/feature_settings.py b/test/functional/feature_settings.py index 4175699152e00..0214e781de0c4 100755 --- a/test/functional/feature_settings.py +++ b/test/functional/feature_settings.py @@ -23,10 +23,11 @@ def run_test(self): settings = node.chain_path / "settings.json" conf = node.datadir_path / "bitcoin.conf" - # Assert empty settings file was created + # Assert default settings file was created self.stop_node(0) + default_settings = {"_warning_": "This file is automatically generated and updated by Bitcoin Core. Please do not edit this file while the node is running, as any changes might be ignored or overwritten."} with settings.open() as fp: - assert_equal(json.load(fp), {}) + assert_equal(json.load(fp), default_settings) # Assert settings are parsed and logged with settings.open("w") as fp: @@ -48,7 +49,7 @@ def run_test(self): # Assert settings are unchanged after shutdown with settings.open() as fp: - assert_equal(json.load(fp), {"string": "string", "num": 5, "bool": True, "null": None, "list": [6, 7]}) + assert_equal(json.load(fp), {**default_settings, **{"string": "string", "num": 5, "bool": True, "null": None, "list": [6, 7]}}) # Test invalid json with settings.open("w") as fp: From b8105b3ed7c97cd6545dba99d0e13c33f183e450 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Thu, 18 Jan 2024 15:10:28 -0500 Subject: [PATCH 25/33] depends: Update libmultiprocess library to fix C++20 macos build error Fixes #29248 The std::result_of type was removed in c++20, but was being referenced in some old, unused code in the library. The issue was fixed in: https://github.com/chaincodelabs/libmultiprocess/pull/91 util: Drop Bind, BindTuple, ComposeFn, GetFn, and ThrowFn helpers This update also includes other recent libmultiprocess changes to improve C++20 support and fix build issues: https://github.com/chaincodelabs/libmultiprocess/pull/89 pkgconfig: Drop -std=c++17 compile flag https://github.com/chaincodelabs/libmultiprocess/pull/90 pkgconfig: Use @CMAKE_INSTALL_LIBDIR@ variable https://github.com/chaincodelabs/libmultiprocess/pull/93 Fix support for vector serialization with libc++ --- depends/packages/native_libmultiprocess.mk | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/depends/packages/native_libmultiprocess.mk b/depends/packages/native_libmultiprocess.mk index 946e885354aac..bcdb1f9e7c42b 100644 --- a/depends/packages/native_libmultiprocess.mk +++ b/depends/packages/native_libmultiprocess.mk @@ -1,8 +1,8 @@ package=native_libmultiprocess -$(package)_version=414542f81e0997354b45b8ade13ca144a3e35ff1 +$(package)_version=8da797c5f1644df1bffd84d10c1ae9836dc70d60 $(package)_download_path=https://github.com/chaincodelabs/libmultiprocess/archive $(package)_file_name=$($(package)_version).tar.gz -$(package)_sha256_hash=8542dbaf8c4fce8fd7af6929f5dc9b34dffa51c43e9ee360e93ee0f34b180bc2 +$(package)_sha256_hash=030f4d393d2ac9deba98d2e1973e22fc439ffc009d5f8ae3225c90639f86beb0 $(package)_dependencies=native_capnp define $(package)_config_cmds From ff54314d4abed3bf9a78daf785a01c63af15c69d Mon Sep 17 00:00:00 2001 From: marco Date: Tue, 23 Jan 2024 17:34:16 -0700 Subject: [PATCH 26/33] wallet: clarify replaced_by_txid and replaces_txid in help output --- src/wallet/rpc/transactions.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/wallet/rpc/transactions.cpp b/src/wallet/rpc/transactions.cpp index 0e30d3e0fe82f..e6c021d4261aa 100644 --- a/src/wallet/rpc/transactions.cpp +++ b/src/wallet/rpc/transactions.cpp @@ -415,8 +415,8 @@ static std::vector TransactionDescriptionString() { {RPCResult::Type::STR_HEX, "txid", "The transaction id."}, }}, - {RPCResult::Type::STR_HEX, "replaced_by_txid", /*optional=*/true, "The txid if this tx was replaced."}, - {RPCResult::Type::STR_HEX, "replaces_txid", /*optional=*/true, "The txid if the tx replaces one."}, + {RPCResult::Type::STR_HEX, "replaced_by_txid", /*optional=*/true, "Only if 'category' is 'send'. The txid if this tx was replaced."}, + {RPCResult::Type::STR_HEX, "replaces_txid", /*optional=*/true, "Only if 'category' is 'send'. The txid if this tx replaces another."}, {RPCResult::Type::STR, "to", /*optional=*/true, "If a comment to is associated with the transaction."}, {RPCResult::Type::NUM_TIME, "time", "The transaction time expressed in " + UNIX_EPOCH_TIME + "."}, {RPCResult::Type::NUM_TIME, "timereceived", "The time received expressed in " + UNIX_EPOCH_TIME + "."}, From 9d09c873a50d344e2a9cb35fe246a52688b9fa34 Mon Sep 17 00:00:00 2001 From: dergoegge Date: Wed, 24 Jan 2024 10:50:53 +0000 Subject: [PATCH 27/33] fuzz: Exit and log stderr for parse_test_list errors --- test/fuzz/test_runner.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/fuzz/test_runner.py b/test/fuzz/test_runner.py index e72977fac06d4..bf8d4929c12ed 100755 --- a/test/fuzz/test_runner.py +++ b/test/fuzz/test_runner.py @@ -372,8 +372,8 @@ def parse_test_list(*, fuzz_bin): 'PRINT_ALL_FUZZ_TARGETS_AND_ABORT': '' }, stdout=subprocess.PIPE, - stderr=subprocess.DEVNULL, text=True, + check=True, ).stdout.splitlines() return test_list_all From 8023640a71a10ec54a6a8e6b95a29d07f7be218d Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Thu, 25 Jan 2024 10:26:26 +0000 Subject: [PATCH 28/33] qt: Avoid non-self-contained Windows header Using the `windows.h` header guarantees correctness regardless of the content of other headers. For more details, please refer to https://stackoverflow.com/questions/4845198/fatal-error-no-target-architecture-in-visual-studio Fixes the MSVC build when using the upcoming CMake-based build system and Qt packages installed via the vcpkg package manager. --- src/qt/winshutdownmonitor.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qt/winshutdownmonitor.h b/src/qt/winshutdownmonitor.h index 78f287637f324..060d8546e3921 100644 --- a/src/qt/winshutdownmonitor.h +++ b/src/qt/winshutdownmonitor.h @@ -10,7 +10,7 @@ #include #include -#include // for HWND +#include #include From ec25e745420fce5fd3e14b0c39e6f475d918d5ad Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Thu, 25 Jan 2024 11:55:57 +0000 Subject: [PATCH 29/33] ci: Update cache action This change fixes deprecation warnings for Node.js 16 actions in the GHA CI. See: - https://github.com/marketplace/actions/cache - https://github.com/actions/cache/releases/tag/v4.0.0 --- .github/workflows/ci.yml | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index d8262b9f90f65..5857753e14e1d 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -103,7 +103,7 @@ jobs: - name: Restore Ccache cache id: ccache-cache - uses: actions/cache/restore@v3 + uses: actions/cache/restore@v4 with: path: ${{ env.CCACHE_DIR }} key: ${{ github.job }}-ccache-${{ github.run_id }} @@ -113,7 +113,7 @@ jobs: run: ./ci/test_run_all.sh - name: Save Ccache cache - uses: actions/cache/save@v3 + uses: actions/cache/save@v4 if: github.event_name != 'pull_request' && steps.ccache-cache.outputs.cache-hit != 'true' with: path: ${{ env.CCACHE_DIR }} @@ -159,7 +159,7 @@ jobs: - name: Restore static Qt cache id: static-qt-cache - uses: actions/cache/restore@v3 + uses: actions/cache/restore@v4 with: path: C:\Qt_static key: ${{ github.job }}-static-qt-${{ hashFiles('msbuild_version', 'qt_url', 'qt_conf') }} @@ -202,14 +202,14 @@ jobs: - name: Save static Qt cache if: steps.static-qt-cache.outputs.cache-hit != 'true' - uses: actions/cache/save@v3 + uses: actions/cache/save@v4 with: path: C:\Qt_static key: ${{ github.job }}-static-qt-${{ hashFiles('msbuild_version', 'qt_url', 'qt_conf') }} - name: Ccache installation cache id: ccache-installation-cache - uses: actions/cache@v3 + uses: actions/cache@v4 with: path: | C:\ProgramData\chocolatey\lib\ccache @@ -226,7 +226,7 @@ jobs: - name: Restore Ccache cache id: ccache-cache - uses: actions/cache/restore@v3 + uses: actions/cache/restore@v4 with: path: ~/AppData/Local/ccache key: ${{ github.job }}-ccache-${{ github.run_id }} @@ -242,13 +242,13 @@ jobs: Get-Content -Path "$env:GITHUB_WORKSPACE\vcpkg_commit" - name: vcpkg tools cache - uses: actions/cache@v3 + uses: actions/cache@v4 with: path: C:/vcpkg/downloads/tools key: ${{ github.job }}-vcpkg-tools - name: vcpkg binary cache - uses: actions/cache@v3 + uses: actions/cache@v4 with: path: ~/AppData/Local/vcpkg/archives key: ${{ github.job }}-vcpkg-binary-${{ hashFiles('vcpkg_commit', 'msbuild_version', 'toolset_version', 'build_msvc/vcpkg.json') }} @@ -266,7 +266,7 @@ jobs: run: ccache --show-stats - name: Save Ccache cache - uses: actions/cache/save@v3 + uses: actions/cache/save@v4 if: github.event_name != 'pull_request' && steps.ccache-cache.outputs.cache-hit != 'true' with: path: ~/AppData/Local/ccache From 2b0dd88f1ce9084324dc54db578fade9c926fd71 Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Thu, 25 Jan 2024 12:23:49 +0000 Subject: [PATCH 30/33] depends: Ensure definitions are passed when building SQLite with DEBUG=1 The SQLite build system overrides the `CFLAGS` when is configured with the `--enable-debug` option. --- depends/packages/sqlite.mk | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/depends/packages/sqlite.mk b/depends/packages/sqlite.mk index a8ec89c6c6479..d7ee1416cca91 100644 --- a/depends/packages/sqlite.mk +++ b/depends/packages/sqlite.mk @@ -12,9 +12,9 @@ $(package)_config_opts_freebsd=--with-pic $(package)_config_opts_netbsd=--with-pic $(package)_config_opts_openbsd=--with-pic $(package)_config_opts_debug=--enable-debug -$(package)_cflags+=-DSQLITE_DQS=0 -DSQLITE_DEFAULT_MEMSTATUS=0 -DSQLITE_OMIT_DEPRECATED -$(package)_cflags+=-DSQLITE_OMIT_SHARED_CACHE -DSQLITE_OMIT_JSON -DSQLITE_LIKE_DOESNT_MATCH_BLOBS -$(package)_cflags+=-DSQLITE_OMIT_DECLTYPE -DSQLITE_OMIT_PROGRESS_CALLBACK -DSQLITE_OMIT_AUTOINIT +$(package)_cppflags+=-DSQLITE_DQS=0 -DSQLITE_DEFAULT_MEMSTATUS=0 -DSQLITE_OMIT_DEPRECATED +$(package)_cppflags+=-DSQLITE_OMIT_SHARED_CACHE -DSQLITE_OMIT_JSON -DSQLITE_LIKE_DOESNT_MATCH_BLOBS +$(package)_cppflags+=-DSQLITE_OMIT_DECLTYPE -DSQLITE_OMIT_PROGRESS_CALLBACK -DSQLITE_OMIT_AUTOINIT endef define $(package)_preprocess_cmds From 5fb8f0f80fc41cc636da56864195244d8fd9116e Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Thu, 25 Jan 2024 12:25:27 +0000 Subject: [PATCH 31/33] depends: Do not override CFLAGS when building SQLite with DEBUG=1 The `--enable-debug` configure option for the SQLite package does two things. It adds three preprocessor definitions and overrides CFLAGS with "-g -O0". The latter breaks the user's ability to provide sanitizer and LTO flags. --- depends/packages/sqlite.mk | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/depends/packages/sqlite.mk b/depends/packages/sqlite.mk index d7ee1416cca91..6809b391139fb 100644 --- a/depends/packages/sqlite.mk +++ b/depends/packages/sqlite.mk @@ -11,7 +11,9 @@ $(package)_config_opts_linux=--with-pic $(package)_config_opts_freebsd=--with-pic $(package)_config_opts_netbsd=--with-pic $(package)_config_opts_openbsd=--with-pic -$(package)_config_opts_debug=--enable-debug +# We avoid using `--enable-debug` because it overrides CFLAGS, a behavior we want to prevent. +$(package)_cflags_debug += -g +$(package)_cppflags_debug += -DSQLITE_DEBUG $(package)_cppflags+=-DSQLITE_DQS=0 -DSQLITE_DEFAULT_MEMSTATUS=0 -DSQLITE_OMIT_DEPRECATED $(package)_cppflags+=-DSQLITE_OMIT_SHARED_CACHE -DSQLITE_OMIT_JSON -DSQLITE_LIKE_DOESNT_MATCH_BLOBS $(package)_cppflags+=-DSQLITE_OMIT_DECLTYPE -DSQLITE_OMIT_PROGRESS_CALLBACK -DSQLITE_OMIT_AUTOINIT From fa3373d3adbace7e4665cf391363319a55a09a96 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Thu, 25 Jan 2024 13:46:58 +0100 Subject: [PATCH 32/33] refactor: Compile unreachable code When unreachable code isn't compiled, compile failures are not detected. Fix this by leaving it unreachable, but compiling it. Fixes https://github.com/bitcoin/bitcoin/pull/28999#discussion_r1465010916 Can be reviewed with --ignore-all-space --- src/wallet/walletdb.cpp | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp index 9820c7c0ee5e7..f3dd5b328e791 100644 --- a/src/wallet/walletdb.cpp +++ b/src/wallet/walletdb.cpp @@ -1498,20 +1498,26 @@ std::unique_ptr MakeDatabase(const fs::path& path, const Databas if (format == DatabaseFormat::SQLITE) { #ifdef USE_SQLITE - return MakeSQLiteDatabase(path, options, status, error); -#else - error = Untranslated(strprintf("Failed to open database path '%s'. Build does not support SQLite database format.", fs::PathToString(path))); - status = DatabaseStatus::FAILED_BAD_FORMAT; - return nullptr; + if constexpr (true) { + return MakeSQLiteDatabase(path, options, status, error); + } else #endif + { + error = Untranslated(strprintf("Failed to open database path '%s'. Build does not support SQLite database format.", fs::PathToString(path))); + status = DatabaseStatus::FAILED_BAD_FORMAT; + return nullptr; + } } #ifdef USE_BDB - return MakeBerkeleyDatabase(path, options, status, error); -#else - error = Untranslated(strprintf("Failed to open database path '%s'. Build does not support Berkeley DB database format.", fs::PathToString(path))); - status = DatabaseStatus::FAILED_BAD_FORMAT; - return nullptr; + if constexpr (true) { + return MakeBerkeleyDatabase(path, options, status, error); + } else #endif + { + error = Untranslated(strprintf("Failed to open database path '%s'. Build does not support Berkeley DB database format.", fs::PathToString(path))); + status = DatabaseStatus::FAILED_BAD_FORMAT; + return nullptr; + } } } // namespace wallet From cf937b2068dba167b6c7ea6f8ee9ba366803c3bb Mon Sep 17 00:00:00 2001 From: fanquake Date: Fri, 26 Jan 2024 13:56:04 +0000 Subject: [PATCH 33/33] fuzz: also set MSAN_SYMBOLIZER_PATH --- test/fuzz/test_runner.py | 1 + 1 file changed, 1 insertion(+) diff --git a/test/fuzz/test_runner.py b/test/fuzz/test_runner.py index bf8d4929c12ed..8a5853b1628b2 100755 --- a/test/fuzz/test_runner.py +++ b/test/fuzz/test_runner.py @@ -24,6 +24,7 @@ def get_fuzz_env(*, target, source_dir): 'UBSAN_SYMBOLIZER_PATH':symbolizer, "ASAN_OPTIONS": "detect_stack_use_after_return=1:check_initialization_order=1:strict_init_order=1", 'ASAN_SYMBOLIZER_PATH':symbolizer, + 'MSAN_SYMBOLIZER_PATH':symbolizer, }