From 9ef68d1905ed961b0a3cfac142f3de8b40382749 Mon Sep 17 00:00:00 2001 From: fanquake Date: Wed, 26 May 2021 11:25:51 +0800 Subject: [PATCH 1/9] Merge bitcoin/bitcoin#22061: ci: Bump multiprocess memory fa0bfc5239824e26f07ddef04aad5be947cde2a0 ci: Bump multiprocess memory (MarcoFalke) Pull request description: Fixes #22059 ACKs for top commit: ryanofsky: Code review ACK fa0bfc5239824e26f07ddef04aad5be947cde2a0. Thanks for the update, and interesting to know about #21869. It looks like relevant build https://cirrus-ci.com/task/4807455453478912 is succeeding too fanquake: ACK fa0bfc5239824e26f07ddef04aad5be947cde2a0 Tree-SHA512: f6e49aadf33199ffa7960c8da0b81bdc5ffea61f373e1b0367d000cdbd214614374b9f1a8b3ce9b8270e6d13a24a2029ab07bddb48e44c86dcb687d645e5ef34 --- .cirrus.yml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.cirrus.yml b/.cirrus.yml index 3af0f0228e05a..3837ea1558a97 100644 --- a/.cirrus.yml +++ b/.cirrus.yml @@ -139,7 +139,10 @@ task: << : *GLOBAL_TASK_TEMPLATE container: image: ubuntu:focal + cpu: 4 + memory: 16G # The default memory is sometimes just a bit too small, so double everything env: + MAKEJOBS: "-j8" FILE_ENV: "./ci/test/00_setup_env_native_multiprocess.sh" task: From b60951414220d3b654d6582a51088d0deb0e4f93 Mon Sep 17 00:00:00 2001 From: Carl Dong Date: Tue, 2 Feb 2021 13:35:45 -0500 Subject: [PATCH 2/9] Merge #22381: guix: Test security-check sanity before performing them BACKPORT NOTICE: it includes missing commit: d6ef3543ae16847d5a91fa9271acee9bd2164b32 lint: Run mypy with --show-error-codes When using mypy ignore directives, the error code needs to be specified. Somehow mypy doesn't print it by default... --- test/lint/lint-python.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/lint/lint-python.sh b/test/lint/lint-python.sh index a889c9ff33765..9b4cd2820f9ad 100755 --- a/test/lint/lint-python.sh +++ b/test/lint/lint-python.sh @@ -111,7 +111,7 @@ if ! PYTHONWARNINGS="ignore" $FLAKECMD --ignore=B,C,E,F,I,N,W --select=$(IFS="," EXIT_CODE=1 fi -if ! mypy --ignore-missing-imports $(git ls-files "test/functional/*.py" "contrib/devtools/*.py" | grep -v contrib/devtools/github-merge.py) ; then +if ! mypy --ignore-missing-imports --show-error-codes $(git ls-files "test/functional/*.py" "contrib/devtools/*.py" | grep -v contrib/devtools/github-merge.py) ; then EXIT_CODE=1 fi From f0c62d50a5da4e7afe5d5777bbe4e33d8b6a009b Mon Sep 17 00:00:00 2001 From: fanquake Date: Thu, 15 Jul 2021 13:37:36 +0800 Subject: [PATCH 3/9] Merge bitcoin/bitcoin#22447: test: whitelist rpc_rawtransaction peers to speed up tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit a3d6ec5bb567481a634638cea7ae37c355119a7b test: move rpc_rawtransaction tests to < 30s group (Jon Atack) 5a1ed96077852c739034c21d399da65db09e7714 test: whitelist rpc_rawtransaction peers to speed up tests (Jon Atack) Pull request description: Speed up the somewhat slow `rpc_rawtransaction.py` test by more than 3x (from 45-55 seconds to 15 seconds on a laptop running 2 x 2.5GHz). ACKs for top commit: mjdietzx: ACK a3d6ec5bb567481a634638cea7ae37c355119a7b kristapsk: ACK a3d6ec5bb567481a634638cea7ae37c355119a7b theStack: ACK a3d6ec5bb567481a634638cea7ae37c355119a7b 🐎 brunoerg: tACK a3d6ec5bb567481a634638cea7ae37c355119a7b Tree-SHA512: f1d105594c9b5b257a7096b631a6fa5aeb50e330a351f75c2d6ffa7dd73abdb6e1f596a78c16d204a9bac3fe506e0519f9ad96bb8477ab6424c8e18125ccb659 --- test/functional/rpc_rawtransaction.py | 4 ++++ test/functional/test_runner.py | 4 ++-- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/test/functional/rpc_rawtransaction.py b/test/functional/rpc_rawtransaction.py index d5cea22f78d2f..a696a25101425 100755 --- a/test/functional/rpc_rawtransaction.py +++ b/test/functional/rpc_rawtransaction.py @@ -56,6 +56,10 @@ def set_test_params(self): ["-txindex"], ["-txindex"], ] + # whitelist all peers to speed up tx relay / mempool sync + for args in self.extra_args: + args.append("-whitelist=noban@127.0.0.1") + self.supports_cli = False def skip_test_if_missing_module(self): diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py index f91be1fb41039..46f43c0909a9e 100755 --- a/test/functional/test_runner.py +++ b/test/functional/test_runner.py @@ -144,8 +144,6 @@ 'wallet_abandonconflict.py --legacy-wallet', 'wallet_abandonconflict.py --descriptors', 'feature_csv_activation.py', - 'rpc_rawtransaction.py --legacy-wallet', - 'rpc_rawtransaction.py --descriptors', 'feature_reindex.py', 'feature_abortnode.py', # vv Tests less than 30s vv @@ -196,6 +194,8 @@ 'feature_proxy.py', 'rpc_signrawtransaction.py --legacy-wallet', 'rpc_signrawtransaction.py --descriptors', + 'rpc_rawtransaction.py --legacy-wallet', + 'rpc_rawtransaction.py --descriptors', 'p2p_addrv2_relay.py', 'wallet_groups.py --legacy-wallet', 'wallet_groups.py --descriptors', From 1430897fc4aa15b6e95d3ae92919caf267845a06 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Sun, 18 Jul 2021 09:45:58 +0200 Subject: [PATCH 4/9] Merge bitcoin/bitcoin#22445: fuzz: Move implementations of non-template fuzz helpers from util.h to util.cpp MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit a2aca207b1ad00ec05d7533dbd75bbff830e1d75 Move implementations of non-template fuzz helpers (Sriram) Pull request description: There are 78 cpp files that include `util.h` (`grep -iIr "#include " src/test/fuzz | wc -l`). Modifying the implementation of a fuzz helper in `src/test/fuzz/util.h` will cause all fuzz tests to be recompiled. Keeping the declarations of these non-template fuzz helpers in `util.h` and moving their implementations to `util.cpp` will skip the redundant recompilation of all the fuzz tests, and builds these helpers only once in `util.cpp`. Functions moved from `util.h` to `util.cpp`: - `ConsumeTxMemPoolEntry` - `ContainsSpentInput` - `ConsumeNetAddr` - Methods of `FuzzedFileProvider::(open, read, write, seek, close)` ACKs for top commit: MarcoFalke: review ACK a2aca207b1ad00ec05d7533dbd75bbff830e1d75 🍂 Tree-SHA512: e7037ebb86d0fc56048e4f3d8733eefc21da11683b09d2b22926bda410719628d89c52ddd9b4c18aa243607a66fdb4d13a63e62ca010e66b3ec9174fd18107f0 --- src/test/fuzz/util.cpp | 156 +++++++++++++++++++++++++++++++++++++++++ src/test/fuzz/util.h | 156 +++-------------------------------------- 2 files changed, 164 insertions(+), 148 deletions(-) diff --git a/src/test/fuzz/util.cpp b/src/test/fuzz/util.cpp index c30ff2d3e92bb..3cfa9467fb9f1 100644 --- a/src/test/fuzz/util.cpp +++ b/src/test/fuzz/util.cpp @@ -361,3 +361,159 @@ CKey ConsumePrivateKey(FuzzedDataProvider& fuzzed_data_provider, std::optional(ConsumeMoney(fuzzed_data_provider), std::numeric_limits::max() / static_cast(100000)); + assert(MoneyRange(fee)); + const int64_t time = fuzzed_data_provider.ConsumeIntegral(); + const unsigned int entry_height = fuzzed_data_provider.ConsumeIntegral(); + const bool spends_coinbase = fuzzed_data_provider.ConsumeBool(); + const bool dip1_status = fuzzed_data_provider.ConsumeBool(); + const unsigned int sig_op_cost = fuzzed_data_provider.ConsumeIntegralInRange(0, MaxBlockSigOps(dip1_status)); + return CTxMemPoolEntry{MakeTransactionRef(tx), fee, time, entry_height, spends_coinbase, sig_op_cost, {}}; +} + +bool ContainsSpentInput(const CTransaction& tx, const CCoinsViewCache& inputs) noexcept +{ + for (const CTxIn& tx_in : tx.vin) { + const Coin& coin = inputs.AccessCoin(tx_in.prevout); + if (coin.IsSpent()) { + return true; + } + } + return false; +} + +CNetAddr ConsumeNetAddr(FuzzedDataProvider& fuzzed_data_provider) noexcept +{ + const Network network = fuzzed_data_provider.PickValueInArray({Network::NET_IPV4, Network::NET_IPV6, Network::NET_INTERNAL, Network::NET_ONION}); + CNetAddr net_addr; + if (network == Network::NET_IPV4) { + in_addr v4_addr = {}; + v4_addr.s_addr = fuzzed_data_provider.ConsumeIntegral(); + net_addr = CNetAddr{v4_addr}; + } else if (network == Network::NET_IPV6) { + if (fuzzed_data_provider.remaining_bytes() >= 16) { + in6_addr v6_addr = {}; + memcpy(v6_addr.s6_addr, fuzzed_data_provider.ConsumeBytes(16).data(), 16); + net_addr = CNetAddr{v6_addr, fuzzed_data_provider.ConsumeIntegral()}; + } + } else if (network == Network::NET_INTERNAL) { + net_addr.SetInternal(fuzzed_data_provider.ConsumeBytesAsString(32)); + } else if (network == Network::NET_ONION) { + net_addr.SetSpecial(fuzzed_data_provider.ConsumeBytesAsString(32)); + } + return net_addr; +} + +FILE* FuzzedFileProvider::open() +{ + SetFuzzedErrNo(m_fuzzed_data_provider); + if (m_fuzzed_data_provider.ConsumeBool()) { + return nullptr; + } + std::string mode; + CallOneOf( + m_fuzzed_data_provider, + [&] { + mode = "r"; + }, + [&] { + mode = "r+"; + }, + [&] { + mode = "w"; + }, + [&] { + mode = "w+"; + }, + [&] { + mode = "a"; + }, + [&] { + mode = "a+"; + }); +#if defined _GNU_SOURCE && !defined __ANDROID__ + const cookie_io_functions_t io_hooks = { + FuzzedFileProvider::read, + FuzzedFileProvider::write, + FuzzedFileProvider::seek, + FuzzedFileProvider::close, + }; + return fopencookie(this, mode.c_str(), io_hooks); +#else + (void)mode; + return nullptr; +#endif +} + +ssize_t FuzzedFileProvider::read(void* cookie, char* buf, size_t size) +{ + FuzzedFileProvider* fuzzed_file = (FuzzedFileProvider*)cookie; + SetFuzzedErrNo(fuzzed_file->m_fuzzed_data_provider); + if (buf == nullptr || size == 0 || fuzzed_file->m_fuzzed_data_provider.ConsumeBool()) { + return fuzzed_file->m_fuzzed_data_provider.ConsumeBool() ? 0 : -1; + } + const std::vector random_bytes = fuzzed_file->m_fuzzed_data_provider.ConsumeBytes(size); + if (random_bytes.empty()) { + return 0; + } + std::memcpy(buf, random_bytes.data(), random_bytes.size()); + if (AdditionOverflow(fuzzed_file->m_offset, (int64_t)random_bytes.size())) { + return fuzzed_file->m_fuzzed_data_provider.ConsumeBool() ? 0 : -1; + } + fuzzed_file->m_offset += random_bytes.size(); + return random_bytes.size(); +} + +ssize_t FuzzedFileProvider::write(void* cookie, const char* buf, size_t size) +{ + FuzzedFileProvider* fuzzed_file = (FuzzedFileProvider*)cookie; + SetFuzzedErrNo(fuzzed_file->m_fuzzed_data_provider); + const ssize_t n = fuzzed_file->m_fuzzed_data_provider.ConsumeIntegralInRange(0, size); + if (AdditionOverflow(fuzzed_file->m_offset, (int64_t)n)) { + return fuzzed_file->m_fuzzed_data_provider.ConsumeBool() ? 0 : -1; + } + fuzzed_file->m_offset += n; + return n; +} + +int FuzzedFileProvider::seek(void* cookie, int64_t* offset, int whence) +{ + assert(whence == SEEK_SET || whence == SEEK_CUR || whence == SEEK_END); + FuzzedFileProvider* fuzzed_file = (FuzzedFileProvider*)cookie; + SetFuzzedErrNo(fuzzed_file->m_fuzzed_data_provider); + int64_t new_offset = 0; + if (whence == SEEK_SET) { + new_offset = *offset; + } else if (whence == SEEK_CUR) { + if (AdditionOverflow(fuzzed_file->m_offset, *offset)) { + return -1; + } + new_offset = fuzzed_file->m_offset + *offset; + } else if (whence == SEEK_END) { + const int64_t n = fuzzed_file->m_fuzzed_data_provider.ConsumeIntegralInRange(0, 4096); + if (AdditionOverflow(n, *offset)) { + return -1; + } + new_offset = n + *offset; + } + if (new_offset < 0) { + return -1; + } + fuzzed_file->m_offset = new_offset; + *offset = new_offset; + return fuzzed_file->m_fuzzed_data_provider.ConsumeIntegralInRange(-1, 0); +} + +int FuzzedFileProvider::close(void* cookie) +{ + FuzzedFileProvider* fuzzed_file = (FuzzedFileProvider*)cookie; + SetFuzzedErrNo(fuzzed_file->m_fuzzed_data_provider); + return fuzzed_file->m_fuzzed_data_provider.ConsumeIntegralInRange(-1, 0); +} diff --git a/src/test/fuzz/util.h b/src/test/fuzz/util.h index e49c8130b895d..521164bad49e6 100644 --- a/src/test/fuzz/util.h +++ b/src/test/fuzz/util.h @@ -218,21 +218,7 @@ template return UintToArith256(ConsumeUInt256(fuzzed_data_provider)); } -[[nodiscard]] inline CTxMemPoolEntry ConsumeTxMemPoolEntry(FuzzedDataProvider& fuzzed_data_provider, const CTransaction& tx) noexcept -{ - // Avoid: - // policy/feerate.cpp:28:34: runtime error: signed integer overflow: 34873208148477500 * 1000 cannot be represented in type 'long' - // - // Reproduce using CFeeRate(348732081484775, 10).GetFeePerK() - const CAmount fee = std::min(ConsumeMoney(fuzzed_data_provider), std::numeric_limits::max() / static_cast(100000)); - assert(MoneyRange(fee)); - const int64_t time = fuzzed_data_provider.ConsumeIntegral(); - const unsigned int entry_height = fuzzed_data_provider.ConsumeIntegral(); - const bool spends_coinbase = fuzzed_data_provider.ConsumeBool(); - const bool dip1_status = fuzzed_data_provider.ConsumeBool(); - const unsigned int sig_op_cost = fuzzed_data_provider.ConsumeIntegralInRange(0, MaxBlockSigOps(dip1_status)); - return CTxMemPoolEntry{MakeTransactionRef(tx), fee, time, entry_height, spends_coinbase, sig_op_cost, {}}; -} +[[nodiscard]] CTxMemPoolEntry ConsumeTxMemPoolEntry(FuzzedDataProvider& fuzzed_data_provider, const CTransaction& tx) noexcept; [[nodiscard]] inline CTxDestination ConsumeTxDestination(FuzzedDataProvider& fuzzed_data_provider) noexcept { @@ -276,16 +262,7 @@ template } } -[[nodiscard]] inline bool ContainsSpentInput(const CTransaction& tx, const CCoinsViewCache& inputs) noexcept -{ - for (const CTxIn& tx_in : tx.vin) { - const Coin& coin = inputs.AccessCoin(tx_in.prevout); - if (coin.IsSpent()) { - return true; - } - } - return false; -} +[[nodiscard]] bool ContainsSpentInput(const CTransaction& tx, const CCoinsViewCache& inputs) noexcept; /** * Sets errno to a value selected from the given std::array `errnos`. @@ -319,27 +296,7 @@ template return random_bytes; } -inline CNetAddr ConsumeNetAddr(FuzzedDataProvider& fuzzed_data_provider) noexcept -{ - const Network network = fuzzed_data_provider.PickValueInArray({Network::NET_IPV4, Network::NET_IPV6, Network::NET_INTERNAL, Network::NET_ONION}); - CNetAddr net_addr; - if (network == Network::NET_IPV4) { - in_addr v4_addr = {}; - v4_addr.s_addr = fuzzed_data_provider.ConsumeIntegral(); - net_addr = CNetAddr{v4_addr}; - } else if (network == Network::NET_IPV6) { - if (fuzzed_data_provider.remaining_bytes() >= 16) { - in6_addr v6_addr = {}; - memcpy(v6_addr.s6_addr, fuzzed_data_provider.ConsumeBytes(16).data(), 16); - net_addr = CNetAddr{v6_addr, fuzzed_data_provider.ConsumeIntegral()}; - } - } else if (network == Network::NET_INTERNAL) { - net_addr.SetInternal(fuzzed_data_provider.ConsumeBytesAsString(32)); - } else if (network == Network::NET_ONION) { - net_addr.SetSpecial(fuzzed_data_provider.ConsumeBytesAsString(32)); - } - return net_addr; -} +CNetAddr ConsumeNetAddr(FuzzedDataProvider& fuzzed_data_provider) noexcept; inline CSubNet ConsumeSubNet(FuzzedDataProvider& fuzzed_data_provider) noexcept { @@ -408,112 +365,15 @@ class FuzzedFileProvider { } - FILE* open() - { - SetFuzzedErrNo(m_fuzzed_data_provider); - if (m_fuzzed_data_provider.ConsumeBool()) { - return nullptr; - } - std::string mode; - CallOneOf( - m_fuzzed_data_provider, - [&] { - mode = "r"; - }, - [&] { - mode = "r+"; - }, - [&] { - mode = "w"; - }, - [&] { - mode = "w+"; - }, - [&] { - mode = "a"; - }, - [&] { - mode = "a+"; - }); -#if defined _GNU_SOURCE && !defined __ANDROID__ - const cookie_io_functions_t io_hooks = { - FuzzedFileProvider::read, - FuzzedFileProvider::write, - FuzzedFileProvider::seek, - FuzzedFileProvider::close, - }; - return fopencookie(this, mode.c_str(), io_hooks); -#else - (void)mode; - return nullptr; -#endif - } + FILE* open(); - static ssize_t read(void* cookie, char* buf, size_t size) - { - FuzzedFileProvider* fuzzed_file = (FuzzedFileProvider*)cookie; - SetFuzzedErrNo(fuzzed_file->m_fuzzed_data_provider); - if (buf == nullptr || size == 0 || fuzzed_file->m_fuzzed_data_provider.ConsumeBool()) { - return fuzzed_file->m_fuzzed_data_provider.ConsumeBool() ? 0 : -1; - } - const std::vector random_bytes = fuzzed_file->m_fuzzed_data_provider.ConsumeBytes(size); - if (random_bytes.empty()) { - return 0; - } - std::memcpy(buf, random_bytes.data(), random_bytes.size()); - if (AdditionOverflow(fuzzed_file->m_offset, (int64_t)random_bytes.size())) { - return fuzzed_file->m_fuzzed_data_provider.ConsumeBool() ? 0 : -1; - } - fuzzed_file->m_offset += random_bytes.size(); - return random_bytes.size(); - } + static ssize_t read(void* cookie, char* buf, size_t size); - static ssize_t write(void* cookie, const char* buf, size_t size) - { - FuzzedFileProvider* fuzzed_file = (FuzzedFileProvider*)cookie; - SetFuzzedErrNo(fuzzed_file->m_fuzzed_data_provider); - const ssize_t n = fuzzed_file->m_fuzzed_data_provider.ConsumeIntegralInRange(0, size); - if (AdditionOverflow(fuzzed_file->m_offset, (int64_t)n)) { - return fuzzed_file->m_fuzzed_data_provider.ConsumeBool() ? 0 : -1; - } - fuzzed_file->m_offset += n; - return n; - } + static ssize_t write(void* cookie, const char* buf, size_t size); - static int seek(void* cookie, int64_t* offset, int whence) - { - assert(whence == SEEK_SET || whence == SEEK_CUR || whence == SEEK_END); - FuzzedFileProvider* fuzzed_file = (FuzzedFileProvider*)cookie; - SetFuzzedErrNo(fuzzed_file->m_fuzzed_data_provider); - int64_t new_offset = 0; - if (whence == SEEK_SET) { - new_offset = *offset; - } else if (whence == SEEK_CUR) { - if (AdditionOverflow(fuzzed_file->m_offset, *offset)) { - return -1; - } - new_offset = fuzzed_file->m_offset + *offset; - } else if (whence == SEEK_END) { - const int64_t n = fuzzed_file->m_fuzzed_data_provider.ConsumeIntegralInRange(0, 4096); - if (AdditionOverflow(n, *offset)) { - return -1; - } - new_offset = n + *offset; - } - if (new_offset < 0) { - return -1; - } - fuzzed_file->m_offset = new_offset; - *offset = new_offset; - return fuzzed_file->m_fuzzed_data_provider.ConsumeIntegralInRange(-1, 0); - } + static int seek(void* cookie, int64_t* offset, int whence); - static int close(void* cookie) - { - FuzzedFileProvider* fuzzed_file = (FuzzedFileProvider*)cookie; - SetFuzzedErrNo(fuzzed_file->m_fuzzed_data_provider); - return fuzzed_file->m_fuzzed_data_provider.ConsumeIntegralInRange(-1, 0); - } + static int close(void* cookie); }; [[nodiscard]] inline FuzzedFileProvider ConsumeFile(FuzzedDataProvider& fuzzed_data_provider) noexcept From facf685285671576eac9f84639049330a207ef75 Mon Sep 17 00:00:00 2001 From: fanquake Date: Tue, 20 Jul 2021 20:39:57 +0800 Subject: [PATCH 5/9] Merge bitcoin/bitcoin#22261: [p2p/mempool] Two small fixes to node broadcast logic 5a77abd4e657458852875a07692898982f4b1db5 [style] Clean up BroadcastTransaction() (John Newbery) 7282d4c0363ab5152baa34af626cb49afbfddc32 [test] Allow rebroadcast for same-txid-different-wtxid transactions (glozow) cd48372b67d961fe661990a2c6d3cc3d91478924 [mempool] Allow rebroadcast for same-txid-different-wtxid transactions (John Newbery) 847b6ed48d7bacec9024618922e9b339d2d97676 [test] Test transactions are not re-added to unbroadcast set (Duncan Dean) 2837a9f1eaa2c6bf402d1d9891d9aa84c4a56033 [mempool] Only add a transaction to the unbroadcast set when it's added to the mempool (John Newbery) Pull request description: 1. Only add a transaction to the unbroadcast set when it's added to the mempool Currently, if BroadcastTransaction() is called to rebroadcast a transaction (e.g. by ResendWalletTransactions()), then we add the transaction to the unbroadcast set. That transaction has already been broadcast in the past, so peers are unlikely to request it again, meaning RemoveUnbroadcastTx() won't be called and it won't be removed from m_unbroadcast_txids. Net processing will therefore continue to attempt rebroadcast for the transaction every 10-15 minutes. This will most likely continue until the node connects to a new peer which hasn't yet seen the transaction (or perhaps indefinitely). Fix by only adding the transaction to the broadcast set when it's added to the mempool. 2. Allow rebroadcast for same-txid-different-wtxid transactions There is some slightly unexpected behaviour when: - there is already transaction in the mempool (the "mempool tx") - BroadcastTransaction() is called for a transaction with the same txid as the mempool transaction but a different witness (the "new tx") Prior to this commit, if BroadcastTransaction() is called with relay=true, then it'll call RelayTransaction() using the txid/wtxid of the new tx, not the txid/wtxid of the mempool tx. For wtxid relay peers, in SendMessages(), the wtxid of the new tx will be taken from setInventoryTxToSend, but will then be filtered out from the vector of wtxids to announce, since m_mempool.info() won't find the transaction (the mempool contains the mempool tx, which has a different wtxid from the new tx). Fix this by calling RelayTransaction() with the wtxid of the mempool transaction in this case. The third commit is a comment/whitespace only change to tidy up the BroadcastTransaction() function. ACKs for top commit: duncandean: reACK 5a77abd naumenkogs: ACK 5a77abd4e657458852875a07692898982f4b1db5 theStack: re-ACK 5a77abd4e657458852875a07692898982f4b1db5 lsilva01: re-ACK https://github.com/bitcoin/bitcoin/pull/22261/commits/5a77abd4e657458852875a07692898982f4b1db5 Tree-SHA512: d1a46d32a9f975220e5b432ff6633fac9be01ea41925b4958395b8d641680500dc44476b12d18852e5b674d2d87e4d0160b4483e45d3d149176bdff9f4dc8516 --- src/node/transaction.cpp | 105 ++++++++++++++----------- test/functional/mempool_unbroadcast.py | 6 ++ 2 files changed, 63 insertions(+), 48 deletions(-) diff --git a/src/node/transaction.cpp b/src/node/transaction.cpp index 9c039c1a6e0ba..b36af2c1b587f 100644 --- a/src/node/transaction.cpp +++ b/src/node/transaction.cpp @@ -31,34 +31,42 @@ static TransactionError HandleATMPError(const TxValidationState& state, std::str TransactionError BroadcastTransaction(NodeContext& node, const CTransactionRef tx, bilingual_str& err_string, const CAmount& max_tx_fee, bool relay, bool wait_callback, bool bypass_limits) { - // BroadcastTransaction can be called by either sendrawtransaction RPC or wallet RPCs. - // node.peerman is assigned both before chain clients and before RPC server is accepting calls, - // and reset after chain clients and RPC sever are stopped. node.peerman should never be null here. - assert(node.peerman); + // BroadcastTransaction can be called by either sendrawtransaction RPC or the wallet. + // chainman, mempool and peerman are initialized before the RPC server and wallet are started + // and reset after the RPC sever and wallet are stopped. + assert(node.chainman); assert(node.mempool); + assert(node.peerman); + std::promise promise; - uint256 hashTx = tx->GetHash(); + uint256 txid = tx->GetHash(); bool callback_set = false; - { // cs_main scope - assert(node.chainman); - LOCK(cs_main); - // If the transaction is already confirmed in the chain, don't do anything - // and return early. - CCoinsViewCache &view = node.chainman->ActiveChainstate().CoinsTip(); - for (size_t o = 0; o < tx->vout.size(); o++) { - const Coin& existingCoin = view.AccessCoin(COutPoint(hashTx, o)); - // IsSpent does not mean the coin is spent, it means the output does not exist. - // So if the output does exist, then this transaction exists in the chain. - if (!existingCoin.IsSpent()) return TransactionError::ALREADY_IN_CHAIN; - } - if (!node.mempool->exists(hashTx)) { - // Transaction is not already in the mempool. - if (max_tx_fee > 0) { - // First, call ATMP with test_accept and check the fee. If ATMP - // fails here, return error immediately. - const MempoolAcceptResult result = AcceptToMemoryPool(node.chainman->ActiveChainstate(), *node.mempool, tx, - bypass_limits, true /* test_accept */); + { + LOCK(cs_main); + + // If the transaction is already confirmed in the chain, don't do anything + // and return early. + CCoinsViewCache &view = node.chainman->ActiveChainstate().CoinsTip(); + for (size_t o = 0; o < tx->vout.size(); o++) { + const Coin& existingCoin = view.AccessCoin(COutPoint(txid, o)); + // IsSpent does not mean the coin is spent, it means the output does not exist. + // So if the output does exist, then this transaction exists in the chain. + if (!existingCoin.IsSpent()) return TransactionError::ALREADY_IN_CHAIN; + } + if (auto mempool_tx = node.mempool->get(txid); mempool_tx) { + // There's already a transaction in the mempool with this txid. Don't + // try to submit this transaction to the mempool (since it'll be + // rejected as a TX_CONFLICT), but do attempt to reannounce the mempool + // transaction if relay=true. + // + } else { + // Transaction is not already in the mempool. + if (max_tx_fee > 0) { + // First, call ATMP with test_accept and check the fee. If ATMP + // fails here, return error immediately. + const MempoolAcceptResult result = AcceptToMemoryPool(node.chainman->ActiveChainstate(), *node.mempool, tx, + bypass_limits, true /* test_accept */); if (result.m_result_type != MempoolAcceptResult::ResultType::VALID) { return HandleATMPError(result.m_state, err_string.original); } else if (result.m_base_fees.value() > max_tx_fee) { @@ -68,28 +76,33 @@ TransactionError BroadcastTransaction(NodeContext& node, const CTransactionRef t // Try to submit the transaction to the mempool. const MempoolAcceptResult result = AcceptToMemoryPool(node.chainman->ActiveChainstate(), *node.mempool, tx, bypass_limits, false /* test_accept */); - if (result.m_result_type != MempoolAcceptResult::ResultType::VALID) { - return HandleATMPError(result.m_state, err_string.original); - } + if (result.m_result_type != MempoolAcceptResult::ResultType::VALID) { + return HandleATMPError(result.m_state, err_string.original); + } - // Transaction was accepted to the mempool. + // Transaction was accepted to the mempool. - if (wait_callback) { - // For transactions broadcast from outside the wallet, make sure - // that the wallet has been notified of the transaction before - // continuing. - // - // This prevents a race where a user might call sendrawtransaction - // with a transaction to/from their wallet, immediately call some - // wallet RPC, and get a stale result because callbacks have not - // yet been processed. - CallFunctionInValidationInterfaceQueue([&promise] { - promise.set_value(); - }); - callback_set = true; - } - } + if (relay) { + // the mempool tracks locally submitted transactions to make a + // best-effort of initial broadcast + node.mempool->AddUnbroadcastTx(txid); + } + if (wait_callback) { + // For transactions broadcast from outside the wallet, make sure + // that the wallet has been notified of the transaction before + // continuing. + // + // This prevents a race where a user might call sendrawtransaction + // with a transaction to/from their wallet, immediately call some + // wallet RPC, and get a stale result because callbacks have not + // yet been processed. + CallFunctionInValidationInterfaceQueue([&promise] { + promise.set_value(); + }); + callback_set = true; + } + } } // cs_main if (callback_set) { @@ -99,12 +112,8 @@ TransactionError BroadcastTransaction(NodeContext& node, const CTransactionRef t } if (relay) { - // the mempool tracks locally submitted transactions to make a - // best-effort of initial broadcast - node.mempool->AddUnbroadcastTx(hashTx); - LOCK(cs_main); - node.peerman->RelayTransaction(hashTx); + node.peerman->RelayTransaction(txid); } return TransactionError::OK; diff --git a/test/functional/mempool_unbroadcast.py b/test/functional/mempool_unbroadcast.py index b475b65e6857d..7d9e6c306d8ac 100755 --- a/test/functional/mempool_unbroadcast.py +++ b/test/functional/mempool_unbroadcast.py @@ -92,6 +92,12 @@ def test_broadcast(self): self.disconnect_nodes(0, 1) node.disconnect_p2ps() + self.log.info("Rebroadcast transaction and ensure it is not added to unbroadcast set when already in mempool") + rpc_tx_hsh = node.sendrawtransaction(txFS["hex"]) + mempool = node.getrawmempool(True) + assert rpc_tx_hsh in mempool + assert not mempool[rpc_tx_hsh]['unbroadcast'] + def test_txn_removal(self): self.log.info("Test that transactions removed from mempool are removed from unbroadcast set") node = self.nodes[0] From c8725560c9d523e42736b42ea3702f105e2144f6 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Wed, 5 May 2021 16:33:56 +0200 Subject: [PATCH 6/9] Merge bitcoin/bitcoin#21856: doc: add OSS-Fuzz section to fuzzing.md doc 47c3ea021e867206172cdb6546a76d23baa958bb doc: add OSS-Fuzz section to fuzzing.md doc (Adam Jonas) Pull request description: This adds documentation about [Bitcoin Core's participation](https://github.com/google/oss-fuzz/pull/5699/files) in Google's OSS-Fuzz program and adds the caveat that the project may not disclose vulnerabilities within the 90-day window described in the [program's disclosure guidelines](https://google.github.io/oss-fuzz/getting-started/bug-disclosure-guidelines/). ACKs for top commit: jonatack: ACK 47c3ea021e867206172cdb6546a76d23baa958bb Tree-SHA512: 87bf0146fb74d1e4b3b8839e6c8f3d53046008a6d5b926ffe5b95be3c396a5e47e47967533422f60b04c4446482f49d210ada410b742f69781a7afde623d704d --- doc/fuzzing.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/doc/fuzzing.md b/doc/fuzzing.md index 31d5de130c57b..614f475d41abd 100644 --- a/doc/fuzzing.md +++ b/doc/fuzzing.md @@ -259,3 +259,9 @@ $ honggfuzz/honggfuzz --exit_upon_crash --quiet --timeout 4 -n 1 -Q \ -nodebuglogfile -bind=127.0.0.1:18444 -logthreadnames \ -debug ``` + +# OSS-Fuzz + +Bitcoin Core participates in Google's [OSS-Fuzz](https://github.com/google/oss-fuzz/tree/master/projects/bitcoin-core) +program, which includes a dashboard of [publicly disclosed vulnerabilities](https://bugs.chromium.org/p/oss-fuzz/issues/list). +For more details, see [Bitcoin's OSS-fuzz](https://github.com/bitcoin/bitcoin/tree/master/doc/fuzzing.md) From 3b05a99b505ac80518776c0ffeaa35f62e93ca11 Mon Sep 17 00:00:00 2001 From: fanquake Date: Wed, 2 Jun 2021 10:44:16 +0800 Subject: [PATCH 7/9] Merge bitcoin/bitcoin#22106: refactor: address ProcessNewBlock comments from #21713 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit e12f287498e5836bb5e32de5abaef02f3d20d868 net: cleanup newly added PeerManagerImpl::ProcessNewBlock (fanquake) 610151f5b076d4b1ab90c0dd2717e5410aba6b19 validation: change ProcessNewBlock() to take a CBlock reference (fanquake) Pull request description: Addresses some [post-merge comments](https://github.com/bitcoin/bitcoin/pull/21713#pullrequestreview-638777410) from #21713. Also makes `ChainstateManager::ProcessNewBlock` take a const reference argument, as it [was asked](https://github.com/bitcoin/bitcoin/pull/21713#discussion_r615229548) why it was not the case in that PR. ACKs for top commit: jnewbery: Code review ACK e12f287498e5836bb5e32de5abaef02f3d20d868 MarcoFalke: review ACK e12f287498e5836bb5e32de5abaef02f3d20d868 🚚 Tree-SHA512: 9c3e7353240c862d50bce2a0f58741c109dd628040b56ed46250103f8ebe9009238b131da710486791e28e3a83c985057b7be0a32aed1a929269b43097c7425b --- src/net_processing.cpp | 21 +++++++++++---------- src/validation.cpp | 12 ++++++------ src/validation.h | 15 +++++++-------- 3 files changed, 24 insertions(+), 24 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 4ea95ac0cafb4..4faf4c0d812d1 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -935,7 +935,8 @@ class PeerManagerImpl final : public PeerManager void ProcessGetData(CNode& pfrom, Peer& peer, const std::atomic& interruptMsgProc) LOCKS_EXCLUDED(cs_main) EXCLUSIVE_LOCKS_REQUIRED(peer.m_getdata_requests_mutex); - void ProcessBlock(CNode& pfrom, const std::shared_ptr& pblock, bool fForceProcessing); + /** Process a new block. Perform any post-processing housekeeping */ + void ProcessBlock(CNode& from, const std::shared_ptr& pblock, bool force_processing); /** Relay map (txid -> CTransactionRef) */ typedef std::map MapRelay; @@ -3290,15 +3291,15 @@ std::pair static ValidateDSTX(CDeterministicMN return {true, false}; } -void PeerManagerImpl::ProcessBlock(CNode& pfrom, const std::shared_ptr& pblock, bool fForceProcessing) +void PeerManagerImpl::ProcessBlock(CNode& node, const std::shared_ptr& block, bool force_processing) { - bool fNewBlock = false; - m_chainman.ProcessNewBlock(m_chainparams, pblock, fForceProcessing, &fNewBlock); - if (fNewBlock) { - pfrom.m_last_block_time = GetTime(); + bool new_block{false}; + m_chainman.ProcessNewBlock(m_chainparams, block, force_processing, &new_block); + if (new_block) { + node.m_last_block_time = GetTime(); } else { LOCK(cs_main); - mapBlockSource.erase(pblock->GetHash()); + mapBlockSource.erase(block->GetHash()); } } @@ -4490,7 +4491,7 @@ void PeerManagerImpl::ProcessMessage( LOCK(cs_main); mapBlockSource.emplace(pblock->GetHash(), std::make_pair(pfrom.GetId(), false)); } - // Setting fForceProcessing to true means that we bypass some of + // Setting force_processing to true means that we bypass some of // our anti-DoS protections in AcceptBlock, which filters // unrequested blocks that might be trying to waste our resources // (eg disk space). Because we only try to reconstruct blocks when @@ -4499,7 +4500,7 @@ void PeerManagerImpl::ProcessMessage( // we have a chain with at least nMinimumChainWork), and we ignore // compact blocks with less work than our tip, it is safe to treat // reconstructed compact blocks as having been requested. - ProcessBlock(pfrom, pblock, /*fForceProcessing=*/true); + ProcessBlock(pfrom, pblock, /*force_processing=*/true); LOCK(cs_main); // hold cs_main for CBlockIndex::IsValid() if (pindex->IsValid(BLOCK_VALID_TRANSACTIONS)) { // Clear download state for this block, which is in @@ -4582,7 +4583,7 @@ void PeerManagerImpl::ProcessMessage( // disk-space attacks), but this should be safe due to the // protections in the compact block handler -- see related comment // in compact block optimistic reconstruction handling. - ProcessBlock(pfrom, pblock, /*fForceProcessing=*/true); + ProcessBlock(pfrom, pblock, /*force_processing=*/true); } return; } diff --git a/src/validation.cpp b/src/validation.cpp index 5f422c122e1a3..5dd943fe1a7b7 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -3948,13 +3948,13 @@ bool CChainState::AcceptBlock(const std::shared_ptr& pblock, Block return true; } -bool ChainstateManager::ProcessNewBlock(const CChainParams& chainparams, const std::shared_ptr pblock, bool fForceProcessing, bool* fNewBlock) +bool ChainstateManager::ProcessNewBlock(const CChainParams& chainparams, const std::shared_ptr& block, bool force_processing, bool* new_block) { AssertLockNotHeld(cs_main); { CBlockIndex *pindex = nullptr; - if (fNewBlock) *fNewBlock = false; + if (new_block) *new_block = false; BlockValidationState state; // CheckBlock() does not support multi-threaded block validation because CBlock::fChecked can cause data race. @@ -3966,13 +3966,13 @@ bool ChainstateManager::ProcessNewBlock(const CChainParams& chainparams, const s // malleability that cause CheckBlock() to fail; see e.g. CVE-2012-2459 and // https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2019-February/016697.html. Because CheckBlock() is // not very expensive, the anti-DoS benefits of caching failure (of a definitely-invalid block) are not substantial. - bool ret = CheckBlock(*pblock, state, chainparams.GetConsensus()); + bool ret = CheckBlock(*block, state, chainparams.GetConsensus()); if (ret) { // Store to disk - ret = ActiveChainstate().AcceptBlock(pblock, state, &pindex, fForceProcessing, nullptr, fNewBlock); + ret = ActiveChainstate().AcceptBlock(block, state, &pindex, force_processing, nullptr, new_block); } if (!ret) { - GetMainSignals().BlockChecked(*pblock, state); + GetMainSignals().BlockChecked(*block, state); return error("%s: AcceptBlock FAILED: %s", __func__, state.ToString()); } } @@ -3980,7 +3980,7 @@ bool ChainstateManager::ProcessNewBlock(const CChainParams& chainparams, const s NotifyHeaderTip(ActiveChainstate()); BlockValidationState state; // Only used to report errors, not invalidity - ignore it - if (!ActiveChainstate().ActivateBestChain(state, pblock)) + if (!ActiveChainstate().ActivateBestChain(state, block)) return error("%s: ActivateBestChain failed: %s", __func__, state.ToString()); LogPrintf("%s : ACCEPTED\n", __func__); diff --git a/src/validation.h b/src/validation.h index 2365aead49176..1f11e3fab43cb 100644 --- a/src/validation.h +++ b/src/validation.h @@ -995,22 +995,21 @@ class ChainstateManager * block is made active. Note that it does not, however, guarantee that the * specific block passed to it has been checked for validity! * - * If you want to *possibly* get feedback on whether pblock is valid, you must + * If you want to *possibly* get feedback on whether block is valid, you must * install a CValidationInterface (see validationinterface.h) - this will have * its BlockChecked method called whenever *any* block completes validation. * - * Note that we guarantee that either the proof-of-work is valid on pblock, or + * Note that we guarantee that either the proof-of-work is valid on block, or * (and possibly also) BlockChecked will have been called. * - * May not be called in a - * validationinterface callback. + * May not be called in a validationinterface callback. * - * @param[in] pblock The block we want to process. - * @param[in] fForceProcessing Process this block even if unrequested; used for non-network block sources. - * @param[out] fNewBlock A boolean which is set to indicate if the block was first received via this call + * @param[in] block The block we want to process. + * @param[in] force_processing Process this block even if unrequested; used for non-network block sources. + * @param[out] new_block A boolean which is set to indicate if the block was first received via this call * @returns If the block was processed, independently of block validity */ - bool ProcessNewBlock(const CChainParams& chainparams, const std::shared_ptr pblock, bool fForceProcessing, bool* fNewBlock) LOCKS_EXCLUDED(cs_main); + bool ProcessNewBlock(const CChainParams& chainparams, const std::shared_ptr& block, bool force_processing, bool* new_block) LOCKS_EXCLUDED(cs_main); /** * Process incoming block headers. From 9b22501a4d491254fb0a5f86bc8bf2eff1a72d3a Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Thu, 3 Jun 2021 07:46:08 +0200 Subject: [PATCH 8/9] Merge bitcoin/bitcoin#22122: ci: Bump macOS image to big-sur-xcode-12.5 faa8dfd6a1fcd4df3624aabb3ff08c1f2be198e7 ci: Bump macOS image to big-sur-xcode-12.5 (MarcoFalke) Pull request description: Closes #22068 ACKs for top commit: hebasto: ACK faa8dfd6a1fcd4df3624aabb3ff08c1f2be198e7, I have reviewed the code and it looks OK, I agree it can be merged, and the Cirrus CI is green. Tree-SHA512: e29f6290163f3727f3603a3d6b4cf47677f6b02fff370e8d9073962a42bd7ab1ae8d247306e4c41bcadf0a208784344a6229627fe1a883b1e5112df30ea88635 --- .cirrus.yml | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/.cirrus.yml b/.cirrus.yml index 3837ea1558a97..b107d7b94eeaf 100644 --- a/.cirrus.yml +++ b/.cirrus.yml @@ -164,12 +164,11 @@ task: task: name: 'macOS 11 native [gui] [no depends]' brew_install_script: - - brew update - brew install boost libevent berkeley-db4 qt@5 miniupnpc ccache zeromq qrencode sqlite libtool automake pkg-config gnu-getopt << : *GLOBAL_TASK_TEMPLATE macos_instance: # Use latest image, but hardcode version to avoid silent upgrades (and breaks) - image: big-sur-xcode-12.4 # https://cirrus-ci.org/guide/macOS + image: big-sur-xcode-12.5 # https://cirrus-ci.org/guide/macOS env: DANGER_RUN_CI_ON_HOST: "true" CI_USE_APT_INSTALL: "no" From 8f06ac9dfac3fcc04b541e7ca6cf8794fb9017d3 Mon Sep 17 00:00:00 2001 From: "W. J. van der Laan" Date: Sat, 12 Jun 2021 12:22:49 +0200 Subject: [PATCH 9/9] Merge bitcoin/bitcoin#22172: doc: update tor.md, release notes with removal of tor v2 support 2ad034a89095bc1b148bdeeacfa5b75ec23b8a21 doc: update release notes with removal of tor v2 support (Jon Atack) 49938eee9c338cd96e357a0dd51dcce934ceb212 doc: update tor.md with removal of tor v2 support (Jon Atack) Pull request description: Follow-up documentation to #22050 that removed support for Tor version 2 hidden services from Bitcoin Core. ACKs for top commit: laanwj: ACK 2ad034a89095bc1b148bdeeacfa5b75ec23b8a21 Tree-SHA512: 0f13f9d1db7e11f1e3d9967b6d17b8dc3144b3ab3a258c706464c5e6ac5cbcf2ce2db4ea54be9939f05a82ebd1e7f325f50b435f9822c08b4f21ed4ac58de0af --- doc/tor.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/doc/tor.md b/doc/tor.md index b9e44eb9ec514..20c63a0158116 100644 --- a/doc/tor.md +++ b/doc/tor.md @@ -8,6 +8,14 @@ may not. In particular, the Tor Browser Bundle defaults to listening on port 915 See [Tor Project FAQ:TBBSocksPort](https://www.torproject.org/docs/faq.html.en#TBBSocksPort) for how to properly configure Tor. +## Compatibility + +- Starting with version 20.0, Dash Core only supports Tor version 3 hidden + services (Tor v3). Tor v2 addresses are ignored by Dash Core and neither + relayed nor stored. + +- Tor removed v2 support beginning with version 0.4.6. + ## How to see information about your Tor configuration via Dash Core There are several ways to see your local onion address in Dash Core: