From c4415c33d881472c20d3d0584101ecf713f09129 Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Mon, 11 Mar 2024 09:34:19 -0400 Subject: [PATCH 1/6] Merge bitcoin/bitcoin#29458: refactor: Preallocate result in TryParseHex to avoid resizing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit a19235c14b3dc02de30b5d769de29d1752c23dbd Preallocate result in `TryParseHex` to avoid resizing (Lőrinc) b7489ecb52c1f99facb7c81c5e46963394d0620d Add benchmark for TryParseHex (Lőrinc) Pull request description: This pull request introduces optimizations to the `TryParseHex` function, focusing primarily on the ideal case (valid hexadecimal input without spaces). A new benchmark, `HexParse` was introduced in a separate commit. The main optimization preallocates the result vector based on the input string's length. This aims to completely avoid costly dynamic reallocations when no spaces are present. ------------ Before: ``` | ns/base16 | base16/s | err% | total | benchmark |--------------------:|--------------------:|--------:|----------:|:---------- | 1.60 | 623,238,893.11 | 0.3% | 0.01 | `HexParse` | 1.65 | 606,747,566.34 | 0.6% | 0.01 | `HexParse` | 1.60 | 626,149,544.07 | 0.3% | 0.01 | `HexParse` ``` After: ``` | ns/base16 | base16/s | err% | total | benchmark |--------------------:|--------------------:|--------:|----------:|:---------- | 0.68 | 1,465,555,976.27 | 0.8% | 0.01 | `HexParse` | 0.68 | 1,472,962,920.18 | 0.3% | 0.01 | `HexParse` | 0.68 | 1,476,159,423.00 | 0.3% | 0.01 | `HexParse` ``` ACKs for top commit: achow101: ACK a19235c14b3dc02de30b5d769de29d1752c23dbd hebasto: ACK a19235c14b3dc02de30b5d769de29d1752c23dbd. andrewtoth: Re-ACK a19235c14b3dc02de30b5d769de29d1752c23dbd Empact: Re-ACK https://github.com/bitcoin/bitcoin/pull/29458/commits/a19235c14b3dc02de30b5d769de29d1752c23dbd Tree-SHA512: e09a59791104be3fd1026862ce98de9efafa1f949626fa01e3b7d58e6a2ef02a11f0de55ddba5c43230a53effd24e6d368c1e12848b17e8ce91d7908a59333f0 --- src/Makefile.bench.include | 1 + src/bench/parse_hex.cpp | 36 ++++++++++++++++++++++++++++++++++++ src/util/strencodings.cpp | 2 ++ 3 files changed, 39 insertions(+) create mode 100644 src/bench/parse_hex.cpp diff --git a/src/Makefile.bench.include b/src/Makefile.bench.include index 7dec51f2b0c1..f42a270413fc 100644 --- a/src/Makefile.bench.include +++ b/src/Makefile.bench.include @@ -47,6 +47,7 @@ bench_bench_dash_SOURCES = \ bench/merkle_root.cpp \ bench/nanobench.cpp \ bench/nanobench.h \ + bench/parse_hex.cpp \ bench/peer_eviction.cpp \ bench/poly1305.cpp \ bench/pool.cpp \ diff --git a/src/bench/parse_hex.cpp b/src/bench/parse_hex.cpp new file mode 100644 index 000000000000..db3ead043c81 --- /dev/null +++ b/src/bench/parse_hex.cpp @@ -0,0 +1,36 @@ +// Copyright (c) 2024- 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 + +std::string generateHexString(size_t length) { + const auto hex_digits = "0123456789ABCDEF"; + FastRandomContext rng(/*fDeterministic=*/true); + + std::string data; + while (data.size() < length) { + auto digit = hex_digits[rng.randbits(4)]; + data.push_back(digit); + } + return data; +} + +static void HexParse(benchmark::Bench& bench) +{ + auto data = generateHexString(130); // Generates 678B0EDA0A1FD30904D5A65E3568DB82DB2D918B0AD8DEA18A63FECCB877D07CAD1495C7157584D877420EF38B8DA473A6348B4F51811AC13C786B962BEE5668F9 by default + + bench.batch(data.size()).unit("base16").run([&] { + auto result = TryParseHex(data); + assert(result != std::nullopt); // make sure we're measuring the successful case + ankerl::nanobench::doNotOptimizeAway(result); + }); +} + +BENCHMARK(HexParse, benchmark::PriorityLevel::HIGH); diff --git a/src/util/strencodings.cpp b/src/util/strencodings.cpp index cbbbb5c9692b..5f0aea5d5cd0 100644 --- a/src/util/strencodings.cpp +++ b/src/util/strencodings.cpp @@ -81,6 +81,8 @@ template std::optional> TryParseHex(std::string_view str) { std::vector vch; + vch.reserve(str.size() / 2); // two hex characters form a single byte + auto it = str.begin(); while (it != str.end()) { if (IsSpace(*it)) { From 1525ce52dcac833b2a9a3b8052a7951f60415bc3 Mon Sep 17 00:00:00 2001 From: fanquake Date: Tue, 12 Mar 2024 09:39:07 +0000 Subject: [PATCH 2/6] (partial) Merge bitcoin/bitcoin#29236: log: Nuke error(...) fa391513949b7a3b56321436e2015c7e9e6dac2b refactor: Remove unused error() (MarcoFalke) fad0335517096f435d76adce7833e213d3cc23d1 scripted-diff: Replace error() with LogError() (MarcoFalke) fa808fb74972637840675e310f6d4a0f06028d61 refactor: Make error() return type void (MarcoFalke) fa1d62434843866d242bff9f9c55cb838a4f0d83 scripted-diff: return error(...); ==> error(...); return false; (MarcoFalke) fa9a5e80ab86c997102a1c3d4ba017bbe86641d5 refactor: Add missing {} around error() calls (MarcoFalke) Pull request description: `error(...)` has many issues: * It is often used in the context of `return error(...)`, implying that it has a "fancy" type, creating confusion with `util::Result/Error` * `-logsourcelocations` does not work with it, because it will pretend the error happened inside of `logging.h` * The log line contains `ERROR: `, as opposed to `[error]`, like for other errors logged with `LogError`. Fix all issues by removing it. ACKs for top commit: fjahr: re-utACK fa391513949b7a3b56321436e2015c7e9e6dac2b stickies-v: re-ACK fa391513949b7a3b56321436e2015c7e9e6dac2b, no changes since 4a903741b0 ryanofsky: Code review ACK fa391513949b7a3b56321436e2015c7e9e6dac2b. Just rebase since last review Tree-SHA512: ec5bb502ab0d3733fdb14a8a00762638fce0417afd8dd6294ae0d485ce2b7ca5b1efeb50fc2cd7467f6c652e4ed3e99b0f283b08aeca04bbfb7ea4f2c95d283a Rebase and fixing blockstorage.cpp file src/validation.cpp - Fixing linting error --- src/addrdb.cpp | 14 ++-- src/flatfile.cpp | 9 ++- src/index/base.cpp | 3 +- src/index/blockfilterindex.cpp | 38 +++++++---- src/index/coinstatsindex.cpp | 24 ++++--- src/index/txindex.cpp | 12 ++-- src/kernel/coinstats.cpp | 3 +- src/net.cpp | 2 +- src/netbase.cpp | 54 ++++++++++----- src/node/blockstorage.cpp | 35 ++++++---- src/script/signingprovider.cpp | 6 +- src/validation.cpp | 121 ++++++++++++++++++++++----------- 12 files changed, 215 insertions(+), 106 deletions(-) diff --git a/src/addrdb.cpp b/src/addrdb.cpp index b8cff96fcbe9..27200eca43ac 100644 --- a/src/addrdb.cpp +++ b/src/addrdb.cpp @@ -40,7 +40,8 @@ bool SerializeDB(Stream& stream, const Data& data) hashwriter << Params().MessageStart() << data; stream << hashwriter.GetHash(); } catch (const std::exception& e) { - return error("%s: Serialize or I/O error - %s", __func__, e.what()); + LogError("%s: Serialize or I/O error - %s\n", __func__, e.what()); + return false; } return true; @@ -60,7 +61,8 @@ bool SerializeFileDB(const std::string& prefix, const fs::path& path, const Data if (fileout.IsNull()) { fileout.fclose(); remove(pathTmp); - return error("%s: Failed to open file %s", __func__, fs::PathToString(pathTmp)); + LogError("%s: Failed to open file %s\n", __func__, fs::PathToString(pathTmp)); + return false; } // Serialize @@ -72,14 +74,16 @@ bool SerializeFileDB(const std::string& prefix, const fs::path& path, const Data if (!FileCommit(fileout.Get())) { fileout.fclose(); remove(pathTmp); - return error("%s: Failed to flush file %s", __func__, fs::PathToString(pathTmp)); + LogError("%s: Failed to flush file %s\n", __func__, fs::PathToString(pathTmp)); + return false; } fileout.fclose(); // replace existing file, if any, with new file if (!RenameOver(pathTmp, path)) { remove(pathTmp); - return error("%s: Rename-into-place failed", __func__); + LogError("%s: Rename-into-place failed\n", __func__); + return false; } return true; @@ -137,7 +141,7 @@ bool CBanDB::Write(const banmap_t& banSet) } for (const auto& err : errors) { - error("%s", err); + LogError("%s\n", err); } return false; } diff --git a/src/flatfile.cpp b/src/flatfile.cpp index 93f077b17287..22137422f2cc 100644 --- a/src/flatfile.cpp +++ b/src/flatfile.cpp @@ -83,15 +83,18 @@ bool FlatFileSeq::Flush(const FlatFilePos& pos, bool finalize) { FILE* file = Open(FlatFilePos(pos.nFile, 0)); // Avoid fseek to nPos if (!file) { - return error("%s: failed to open file %d", __func__, pos.nFile); + LogError("%s: failed to open file %d\n", __func__, pos.nFile); + return false; } if (finalize && !TruncateFile(file, pos.nPos)) { fclose(file); - return error("%s: failed to truncate file %d", __func__, pos.nFile); + LogError("%s: failed to truncate file %d\n", __func__, pos.nFile); + return false; } if (!FileCommit(file)) { fclose(file); - return error("%s: failed to commit file %d", __func__, pos.nFile); + LogError("%s: failed to commit file %d\n", __func__, pos.nFile); + return false; } DirectoryCommit(m_dir); diff --git a/src/index/base.cpp b/src/index/base.cpp index b52ff2d11055..57fb9e42f362 100644 --- a/src/index/base.cpp +++ b/src/index/base.cpp @@ -230,7 +230,8 @@ bool BaseIndex::Commit() } } if (!ok) { - return error("%s: Failed to commit latest %s state", __func__, GetName()); + LogError("%s: Failed to commit latest %s state\n", __func__, GetName()); + return false; } return true; } diff --git a/src/index/blockfilterindex.cpp b/src/index/blockfilterindex.cpp index 47866b359153..7a5151a282a8 100644 --- a/src/index/blockfilterindex.cpp +++ b/src/index/blockfilterindex.cpp @@ -148,8 +148,9 @@ bool BlockFilterIndex::CustomInit(const std::optional& blo // indicate database corruption or a disk failure, and starting the index would cause // further corruption. if (m_db->Exists(DB_FILTER_POS)) { - return error("%s: Cannot read current %s state; index may be corrupted", + LogError("%s: Cannot read current %s state; index may be corrupted\n", __func__, GetName()); + return false; } // If the DB_FILTER_POS is not set, then initialize to the first location. @@ -171,10 +172,12 @@ bool BlockFilterIndex::CustomCommit(CDBBatch& batch) // Flush current filter file to disk. AutoFile file{m_filter_fileseq->Open(pos)}; if (file.IsNull()) { - return error("%s: Failed to open filter file %d", __func__, pos.nFile); + LogError("%s: Failed to open filter file %d\n", __func__, pos.nFile); + return false; } if (!FileCommit(file.Get())) { - return error("%s: Failed to commit filter file %d", __func__, pos.nFile); + LogError("%s: Failed to commit filter file %d\n", __func__, pos.nFile); + return false; } batch.Write(DB_FILTER_POS, pos); @@ -198,11 +201,15 @@ bool BlockFilterIndex::ReadFilterFromFile(AutoFile& file, const uint256& hash, B std::vector encoded_filter; try { file >> block_hash >> encoded_filter; - if (Hash(encoded_filter) != hash) return error("Checksum mismatch in filter decode."); + if (Hash(encoded_filter) != hash) { + LogError("Checksum mismatch in filter decode.\n"); + return false; + } filter = BlockFilter(GetFilterType(), block_hash, std::move(encoded_filter), /*skip_decode_check=*/true); } catch (const std::exception& e) { - return error("%s: Failed to deserialize block filter from disk: %s", __func__, e.what()); + LogError("%s: Failed to deserialize block filter from disk: %s\n", __func__, e.what()); + return false; } return true; @@ -274,8 +281,9 @@ bool BlockFilterIndex::CustomAppend(const interfaces::BlockInfo& block) uint256 expected_block_hash = *Assert(block.prev_hash); if (read_out.first != expected_block_hash) { - return error("%s: previous block header belongs to unexpected block %s; expected %s", + LogError("%s: previous block header belongs to unexpected block %s; expected %s\n", __func__, read_out.first.ToString(), expected_block_hash.ToString()); + return false; } prev_header = read_out.second.header; @@ -309,14 +317,16 @@ bool BlockFilterIndex::CustomAppend(const interfaces::BlockInfo& block) for (int height = start_height; height <= stop_height; ++height) { if (!db_it.GetKey(key) || key.height != height) { - return error("%s: unexpected key in %s: expected (%c, %d)", + LogError("%s: unexpected key in %s: expected (%c, %d)\n", __func__, index_name, DB_BLOCK_HEIGHT, height); + return false; } std::pair value; if (!db_it.GetValue(value)) { - return error("%s: unable to read value in %s at key (%c, %d)", + LogError("%s: unable to read value in %s at key (%c, %d)\n", __func__, index_name, DB_BLOCK_HEIGHT, height); + return false; } batch.Write(DBHashKey(value.first), std::move(value.second)); @@ -369,11 +379,13 @@ static bool LookupRange(CDBWrapper& db, const std::string& index_name, int start const CBlockIndex* stop_index, std::vector& results) { if (start_height < 0) { - return error("%s: start height (%d) is negative", __func__, start_height); + LogError("%s: start height (%d) is negative\n", __func__, start_height); + return false; } if (start_height > stop_index->nHeight) { - return error("%s: start height (%d) is greater than stop height (%d)", + LogError("%s: start height (%d) is greater than stop height (%d)\n", __func__, start_height, stop_index->nHeight); + return false; } size_t results_size = static_cast(stop_index->nHeight - start_height + 1); @@ -389,8 +401,9 @@ static bool LookupRange(CDBWrapper& db, const std::string& index_name, int start size_t i = static_cast(height - start_height); if (!db_it->GetValue(values[i])) { - return error("%s: unable to read value in %s at key (%c, %d)", + LogError("%s: unable to read value in %s at key (%c, %d)\n", __func__, index_name, DB_BLOCK_HEIGHT, height); + return false; } db_it->Next(); @@ -412,8 +425,9 @@ static bool LookupRange(CDBWrapper& db, const std::string& index_name, int start } if (!db.Read(DBHashKey(block_hash), results[i])) { - return error("%s: unable to read value in %s at key (%c, %s)", + LogError("%s: unable to read value in %s at key (%c, %s)\n", __func__, index_name, DB_BLOCK_HASH, block_hash.ToString()); + return false; } } diff --git a/src/index/coinstatsindex.cpp b/src/index/coinstatsindex.cpp index 59c9cf99a863..406d33ad6997 100644 --- a/src/index/coinstatsindex.cpp +++ b/src/index/coinstatsindex.cpp @@ -141,8 +141,9 @@ bool CoinStatsIndex::CustomAppend(const interfaces::BlockInfo& block) read_out.first.ToString(), expected_block_hash.ToString()); if (!m_db->Read(DBHashKey(expected_block_hash), read_out)) { - return error("%s: previous block header not found; expected %s", + LogError("%s: previous block header not found; expected %s\n", __func__, expected_block_hash.ToString()); + return false; } } @@ -241,14 +242,16 @@ bool CoinStatsIndex::CustomAppend(const interfaces::BlockInfo& block) for (int height = start_height; height <= stop_height; ++height) { if (!db_it.GetKey(key) || key.height != height) { - return error("%s: unexpected key in %s: expected (%c, %d)", + LogError("%s: unexpected key in %s: expected (%c, %d)\n", __func__, index_name, DB_BLOCK_HEIGHT, height); + return false; } std::pair value; if (!db_it.GetValue(value)) { - return error("%s: unable to read value in %s at key (%c, %d)", + LogError("%s: unable to read value in %s at key (%c, %d)\n", __func__, index_name, DB_BLOCK_HEIGHT, height); + return false; } batch.Write(DBHashKey(value.first), std::move(value.second)); @@ -282,8 +285,9 @@ bool CoinStatsIndex::CustomRewind(const interfaces::BlockKey& current_tip, const CBlock block; if (!ReadBlockFromDisk(block, iter_tip, consensus_params)) { - return error("%s: Failed to read block %s from disk", + LogError("%s: Failed to read block %s from disk\n", __func__, iter_tip->GetBlockHash().ToString()); + return false; } if (!ReverseBlock(block, iter_tip)) { @@ -350,22 +354,25 @@ bool CoinStatsIndex::CustomInit(const std::optional& block // exist. Any other errors indicate database corruption or a disk // failure, and starting the index would cause further corruption. if (m_db->Exists(DB_MUHASH)) { - return error("%s: Cannot read current %s state; index may be corrupted", + LogError("%s: Cannot read current %s state; index may be corrupted\n", __func__, GetName()); + return false; } } if (block) { DBVal entry; if (!LookUpOne(*m_db, *block, entry)) { - return error("%s: Cannot read current %s state; index may be corrupted", + LogError("%s: Cannot read current %s state; index may be corrupted\n", __func__, GetName()); + return false; } uint256 out; m_muhash.Finalize(out); if (entry.muhash != out) { - return error("%s: Cannot read current %s state; index may be corrupted", + LogError("%s: Cannot read current %s state; index may be corrupted\n", __func__, GetName()); + return false; } m_transaction_output_count = entry.transaction_output_count; m_bogo_size = entry.bogo_size; @@ -417,8 +424,9 @@ bool CoinStatsIndex::ReverseBlock(const CBlock& block, const CBlockIndex* pindex read_out.first.ToString(), expected_block_hash.ToString()); if (!m_db->Read(DBHashKey(expected_block_hash), read_out)) { - return error("%s: previous block header not found; expected %s", + LogError("%s: previous block header not found; expected %s\n", __func__, expected_block_hash.ToString()); + return false; } } } diff --git a/src/index/txindex.cpp b/src/index/txindex.cpp index 69a43f442346..f9894dc9ed6d 100644 --- a/src/index/txindex.cpp +++ b/src/index/txindex.cpp @@ -81,20 +81,24 @@ bool TxIndex::FindTx(const uint256& tx_hash, uint256& block_hash, CTransactionRe AutoFile file{OpenBlockFile(postx, true)}; if (file.IsNull()) { - return error("%s: OpenBlockFile failed", __func__); + LogError("%s: OpenBlockFile failed\n", __func__); + return false; } CBlockHeader header; try { file >> header; if (fseek(file.Get(), postx.nTxOffset, SEEK_CUR)) { - return error("%s: fseek(...) failed", __func__); + LogError("%s: fseek(...) failed\n", __func__); + return false; } file >> tx; } catch (const std::exception& e) { - return error("%s: Deserialize or I/O error - %s", __func__, e.what()); + LogError("%s: Deserialize or I/O error - %s\n", __func__, e.what()); + return false; } if (tx->GetHash() != tx_hash) { - return error("%s: txid mismatch", __func__); + LogError("%s: txid mismatch\n", __func__); + return false; } block_hash = header.GetHash(); return true; diff --git a/src/kernel/coinstats.cpp b/src/kernel/coinstats.cpp index 3da24b12186a..d0e7b4cc0f0a 100644 --- a/src/kernel/coinstats.cpp +++ b/src/kernel/coinstats.cpp @@ -135,7 +135,8 @@ static bool ComputeUTXOStats(CCoinsView* view, CCoinsStats& stats, T hash_obj, c outputs[key.n] = std::move(coin); stats.coins_count++; } else { - return error("%s: unable to read value", __func__); + LogError("%s: unable to read value\n", __func__); + return false; } pcursor->Next(); } diff --git a/src/net.cpp b/src/net.cpp index d96058c96db0..5e17f1a43115 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -570,7 +570,7 @@ void CNode::SetAddrLocal(const CService& addrLocalIn) { AssertLockNotHeld(m_addr_local_mutex); LOCK(m_addr_local_mutex); if (addrLocal.IsValid()) { - error("Addr local already set for node: %i. Refusing to change from %s to %s", id, addrLocal.ToStringAddrPort(), addrLocalIn.ToStringAddrPort()); + LogError("Addr local already set for node: %i. Refusing to change from %s to %s\n", id, addrLocal.ToStringAddrPort(), addrLocalIn.ToStringAddrPort()); } else { addrLocal = addrLocalIn; } diff --git a/src/netbase.cpp b/src/netbase.cpp index 9144f5bd20e0..b6e81795d08e 100644 --- a/src/netbase.cpp +++ b/src/netbase.cpp @@ -372,7 +372,8 @@ bool Socks5(const std::string& strDest, uint16_t port, const ProxyCredentials* a IntrRecvError recvr; LogPrint(BCLog::NET, "SOCKS5 connecting %s\n", strDest); if (strDest.size() > 255) { - return error("Hostname too long"); + LogError("Hostname too long\n"); + return false; } // Construct the version identifier/method selection message std::vector vSocks5Init; @@ -387,7 +388,8 @@ bool Socks5(const std::string& strDest, uint16_t port, const ProxyCredentials* a } ssize_t ret = sock.Send(vSocks5Init.data(), vSocks5Init.size(), MSG_NOSIGNAL); if (ret != (ssize_t)vSocks5Init.size()) { - return error("Error sending to proxy"); + LogError("Error sending to proxy\n"); + return false; } uint8_t pchRet1[2]; if (InterruptibleRecv(pchRet1, 2, g_socks5_recv_timeout, sock) != IntrRecvError::OK) { @@ -395,34 +397,42 @@ bool Socks5(const std::string& strDest, uint16_t port, const ProxyCredentials* a return false; } if (pchRet1[0] != SOCKSVersion::SOCKS5) { - return error("Proxy failed to initialize"); + LogError("Proxy failed to initialize\n"); + return false; } if (pchRet1[1] == SOCKS5Method::USER_PASS && auth) { // Perform username/password authentication (as described in RFC1929) std::vector vAuth; vAuth.push_back(0x01); // Current (and only) version of user/pass subnegotiation - if (auth->username.size() > 255 || auth->password.size() > 255) - return error("Proxy username or password too long"); + if (auth->username.size() > 255 || auth->password.size() > 255) { + LogError("Proxy username or password too long\n"); + return false; + } + vAuth.push_back(auth->username.size()); vAuth.insert(vAuth.end(), auth->username.begin(), auth->username.end()); vAuth.push_back(auth->password.size()); vAuth.insert(vAuth.end(), auth->password.begin(), auth->password.end()); ret = sock.Send(vAuth.data(), vAuth.size(), MSG_NOSIGNAL); if (ret != (ssize_t)vAuth.size()) { - return error("Error sending authentication to proxy"); + LogError("Error sending authentication to proxy\n"); + return false; } LogPrint(BCLog::PROXY, "SOCKS5 sending proxy authentication %s:%s\n", auth->username, auth->password); uint8_t pchRetA[2]; if (InterruptibleRecv(pchRetA, 2, g_socks5_recv_timeout, sock) != IntrRecvError::OK) { - return error("Error reading proxy authentication response"); + LogError("Error reading proxy authentication response\n"); + return false; } if (pchRetA[0] != 0x01 || pchRetA[1] != 0x00) { - return error("Proxy authentication unsuccessful"); + LogError("Proxy authentication unsuccessful\n"); + return false; } } else if (pchRet1[1] == SOCKS5Method::NOAUTH) { // Perform no authentication } else { - return error("Proxy requested wrong authentication method %02x", pchRet1[1]); + LogError("Proxy requested wrong authentication method %02x\n", pchRet1[1]); + return false; } std::vector vSocks5; vSocks5.push_back(SOCKSVersion::SOCKS5); // VER protocol version @@ -435,7 +445,8 @@ bool Socks5(const std::string& strDest, uint16_t port, const ProxyCredentials* a vSocks5.push_back((port >> 0) & 0xFF); ret = sock.Send(vSocks5.data(), vSocks5.size(), MSG_NOSIGNAL); if (ret != (ssize_t)vSocks5.size()) { - return error("Error sending to proxy"); + LogError("Error sending to proxy\n"); + return false; } uint8_t pchRet2[4]; if ((recvr = InterruptibleRecv(pchRet2, 4, g_socks5_recv_timeout, sock)) != IntrRecvError::OK) { @@ -445,11 +456,13 @@ bool Socks5(const std::string& strDest, uint16_t port, const ProxyCredentials* a * error message. */ return false; } else { - return error("Error while reading proxy response"); + LogError("Error while reading proxy response\n"); + return false; } } if (pchRet2[0] != SOCKSVersion::SOCKS5) { - return error("Proxy failed to accept request"); + LogError("Proxy failed to accept request\n"); + return false; } if (pchRet2[1] != SOCKS5Reply::SUCCEEDED) { // Failures to connect to a peer that are not proxy errors @@ -457,7 +470,8 @@ bool Socks5(const std::string& strDest, uint16_t port, const ProxyCredentials* a return false; } if (pchRet2[2] != 0x00) { // Reserved field must be 0 - return error("Error: malformed proxy response"); + LogError("Error: malformed proxy response\n"); + return false; } uint8_t pchRet3[256]; switch (pchRet2[3]) @@ -468,19 +482,25 @@ bool Socks5(const std::string& strDest, uint16_t port, const ProxyCredentials* a { recvr = InterruptibleRecv(pchRet3, 1, g_socks5_recv_timeout, sock); if (recvr != IntrRecvError::OK) { - return error("Error reading from proxy"); + LogError("Error reading from proxy\n"); + return false; } int nRecv = pchRet3[0]; recvr = InterruptibleRecv(pchRet3, nRecv, g_socks5_recv_timeout, sock); break; } - default: return error("Error: malformed proxy response"); + default: { + LogError("Error: malformed proxy response\n"); + return false; + } } if (recvr != IntrRecvError::OK) { - return error("Error reading from proxy"); + LogError("Error reading from proxy\n"); + return false; } if (InterruptibleRecv(pchRet3, 2, g_socks5_recv_timeout, sock) != IntrRecvError::OK) { - return error("Error reading from proxy"); + LogError("Error reading from proxy\n"); + return false; } LogPrint(BCLog::NET, "SOCKS5 connected %s\n", strDest); return true; diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp index cc847e32155d..4fb4965d7977 100644 --- a/src/node/blockstorage.cpp +++ b/src/node/blockstorage.cpp @@ -295,7 +295,8 @@ bool BlockManager::LoadBlockIndex() for (CBlockIndex* pindex : vSortedByHeight) { if (ShutdownRequested()) return false; if (previous_index && pindex->nHeight > previous_index->nHeight + 1) { - return error("%s: block index is non-contiguous, index of height %d missing", __func__, previous_index->nHeight + 1); + LogError("%s: block index is non-contiguous, index of height %d missing\n", __func__, previous_index->nHeight + 1); + return false; } previous_index = pindex; pindex->nChainWork = (pindex->pprev ? pindex->pprev->nChainWork : 0) + GetBlockProof(*pindex); @@ -510,7 +511,8 @@ static bool UndoWriteToDisk(const CBlockUndo& blockundo, FlatFilePos& pos, const // Open history file to append CAutoFile fileout(OpenUndoFile(pos), SER_DISK, CLIENT_VERSION); if (fileout.IsNull()) { - return error("%s: OpenUndoFile failed", __func__); + LogError("%s: OpenUndoFile failed\n", __func__); + return false; } // Write index header @@ -520,7 +522,8 @@ static bool UndoWriteToDisk(const CBlockUndo& blockundo, FlatFilePos& pos, const // Write undo data long fileOutPos = ftell(fileout.Get()); if (fileOutPos < 0) { - return error("%s: ftell failed", __func__); + LogError("%s: ftell failed\n", __func__); + return false; } pos.nPos = (unsigned int)fileOutPos; fileout << blockundo; @@ -539,13 +542,15 @@ bool UndoReadFromDisk(CBlockUndo& blockundo, const CBlockIndex* pindex) const FlatFilePos pos{WITH_LOCK(::cs_main, return pindex->GetUndoPos())}; if (pos.IsNull()) { - return error("%s: no undo data available", __func__); + LogError("%s: no undo data available\n", __func__); + return false; } // Open history file to read CAutoFile filein(OpenUndoFile(pos, true), SER_DISK, CLIENT_VERSION); if (filein.IsNull()) { - return error("%s: OpenUndoFile failed", __func__); + LogError("%s: OpenUndoFile failed\n", __func__); + return false; } // Read block @@ -556,12 +561,14 @@ bool UndoReadFromDisk(CBlockUndo& blockundo, const CBlockIndex* pindex) verifier >> blockundo; filein >> hashChecksum; } catch (const std::exception& e) { - return error("%s: Deserialize or I/O error - %s", __func__, e.what()); + LogError("%s: Deserialize or I/O error - %s\n", __func__, e.what()); + return false; } // Verify checksum if (hashChecksum != verifier.GetHash()) { - return error("%s: Checksum mismatch", __func__); + LogError("%s: Checksum mismatch\n", __func__); + return false; } return true; @@ -743,7 +750,8 @@ static bool WriteBlockToDisk(const CBlock& block, FlatFilePos& pos, const CMessa // Open history file to append CAutoFile fileout(OpenBlockFile(pos), SER_DISK, CLIENT_VERSION); if (fileout.IsNull()) { - return error("WriteBlockToDisk: OpenBlockFile failed"); + LogError("WriteBlockToDisk: OpenBlockFile failed\n"); + return false; } // Write index header @@ -753,7 +761,8 @@ static bool WriteBlockToDisk(const CBlock& block, FlatFilePos& pos, const CMessa // Write block long fileOutPos = ftell(fileout.Get()); if (fileOutPos < 0) { - return error("WriteBlockToDisk: ftell failed"); + LogError("WriteBlockToDisk: ftell failed\n"); + return false; } pos.nPos = (unsigned int)fileOutPos; fileout << block; @@ -768,7 +777,8 @@ bool BlockManager::WriteUndoDataForBlock(const CBlockUndo& blockundo, BlockValid if (block.GetUndoPos().IsNull()) { FlatFilePos _pos; if (!FindUndoPos(state, block.nFile, _pos, ::GetSerializeSize(blockundo, CLIENT_VERSION) + 40)) { - return error("ConnectBlock(): FindUndoPos failed"); + LogError("ConnectBlock(): FindUndoPos failed\n"); + return false; } if (!UndoWriteToDisk(blockundo, _pos, block.pprev->GetBlockHash(), GetParams().MessageStart())) { return AbortNode(state, "Failed to write undo data"); @@ -830,8 +840,9 @@ bool ReadBlockFromDisk(CBlock& block, const CBlockIndex* pindex, const Consensus return false; } if (*hash != pindex->GetBlockHash()) { - return error("ReadBlockFromDisk(CBlock&, CBlockIndex*): GetHash() doesn't match index for %s at %s", + LogError("ReadBlockFromDisk(CBlock&, CBlockIndex*): GetHash() doesn't match index for %s at %s\n", pindex->ToString(), block_pos.ToString()); + return false; } return true; } @@ -850,7 +861,7 @@ FlatFilePos BlockManager::SaveBlockToDisk(const CBlock& block, int nHeight, cons nBlockSize += static_cast(BLOCK_SERIALIZATION_HEADER_SIZE); } if (!FindBlockPos(blockPos, nBlockSize, nHeight, block.GetBlockTime(), position_known)) { - error("%s: FindBlockPos failed", __func__); + LogError("%s: FindBlockPos failed\n", __func__); return FlatFilePos(); } if (!position_known) { diff --git a/src/script/signingprovider.cpp b/src/script/signingprovider.cpp index 3f6c9c2ad7c8..2eee235126ad 100644 --- a/src/script/signingprovider.cpp +++ b/src/script/signingprovider.cpp @@ -110,8 +110,10 @@ bool FillableSigningProvider::GetKey(const CKeyID &address, CKey &keyOut) const bool FillableSigningProvider::AddCScript(const CScript& redeemScript) { - if (redeemScript.size() > MAX_SCRIPT_ELEMENT_SIZE) - return error("FillableSigningProvider::AddCScript(): redeemScripts > %i bytes are invalid", MAX_SCRIPT_ELEMENT_SIZE); + if (redeemScript.size() > MAX_SCRIPT_ELEMENT_SIZE) { + LogError("FillableSigningProvider::AddCScript(): redeemScripts > %i bytes are invalid\n", MAX_SCRIPT_ELEMENT_SIZE); + return false; + } LOCK(cs_KeyStore); mapScripts[CScriptID(redeemScript)] = redeemScript; diff --git a/src/validation.cpp b/src/validation.cpp index 54322912d356..baa658aabd04 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -796,8 +796,10 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) return false; // state filled in by CheckTransaction } - if (!ContextualCheckTransaction(tx, state, chainparams.GetConsensus(), m_active_chainstate.m_chain.Tip())) - return error("%s: ContextualCheckTransaction: %s, %s", __func__, hash.ToString(), state.ToString()); + if (!ContextualCheckTransaction(tx, state, chainparams.GetConsensus(), m_active_chainstate.m_chain.Tip())) { + LogError("%s: ContextualCheckTransaction: %s, %s\n", __func__, hash.ToString(), state.ToString()); + return false; + } if (tx.IsSpecialTxVersion() && tx.nType == TRANSACTION_QUORUM_COMMITMENT) { // quorum commitment is not allowed outside of blocks @@ -2023,12 +2025,12 @@ DisconnectResult Chainstate::DisconnectBlock(const CBlock& block, const CBlockIn CBlockUndo blockUndo; if (!UndoReadFromDisk(blockUndo, pindex)) { - error("DisconnectBlock(): failure reading undo data"); + LogError("DisconnectBlock(): failure reading undo data\n"); return DISCONNECT_FAILED; } if (blockUndo.vtxundo.size() + 1 != block.vtx.size()) { - error("DisconnectBlock(): block and undo data inconsistent"); + LogError("DisconnectBlock(): block and undo data inconsistent\n"); return DISCONNECT_FAILED; } @@ -2073,7 +2075,7 @@ DisconnectResult Chainstate::DisconnectBlock(const CBlock& block, const CBlockIn if (i > 0) { // not coinbases CTxUndo &txundo = blockUndo.vtxundo[i-1]; if (txundo.vprevout.size() != tx.vin.size()) { - error("DisconnectBlock(): transaction and undo data inconsistent"); + LogError("DisconnectBlock(): transaction and undo data inconsistent\n"); return DISCONNECT_FAILED; } for (unsigned int j = tx.vin.size(); j > 0;) { @@ -2236,7 +2238,8 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state, // problems. return AbortNode(state, "Corrupt block found indicating potential hardware failure; shutting down"); } - return error("%s: Consensus::CheckBlock: %s", __func__, state.ToString()); + LogError("%s: Consensus::CheckBlock: %s\n", __func__, state.ToString()); + return false; } if (pindex->pprev && pindex->phashBlock && m_chain_helper->HasConflictingChainLock(pindex->nHeight, pindex->GetBlockHash())) { @@ -2384,8 +2387,9 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state, // MUST process special txes before updating UTXO to ensure consistency between mempool and block processing std::optional mnlist_updates_opt{std::nullopt}; if (!m_chain_helper->special_tx->ProcessSpecialTxsInBlock(*this, block, pindex, view, fJustCheck, fScriptChecks, state, mnlist_updates_opt)) { - return error("ConnectBlock(DASH): ProcessSpecialTxsInBlock for block %s failed with %s", + LogError("ConnectBlock(DASH): ProcessSpecialTxsInBlock for block %s failed with %s", pindex->GetBlockHash().ToString(), state.ToString()); + return false; } const auto time_2_1{SteadyClock::now()}; @@ -2407,7 +2411,7 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state, TxValidationState tx_state; if (!Consensus::CheckTxInputs(tx, tx_state, view, pindex->nHeight, txfee)) { // Any transaction validation failure in ConnectBlock is a block consensus failure - LogPrintf("ERROR: %s: Consensus::CheckTxInputs: %s, %s\n", __func__, tx.GetHash().ToString(), state.ToString()); + LogError("%s: Consensus::CheckTxInputs: %s, %s\n", __func__, tx.GetHash().ToString(), state.ToString()); return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, tx_state.GetRejectReason(), tx_state.GetDebugMessage()); } @@ -2450,7 +2454,7 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state, // Any transaction validation failure in ConnectBlock is a block consensus failure state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, tx_state.GetRejectReason(), tx_state.GetDebugMessage()); - LogPrintf("ERROR: ConnectBlock(): CheckInputScripts on %s failed with %s\n", + LogError("ConnectBlock(): CheckInputScripts on %s failed with %s\n", tx.GetHash().ToString(), state.ToString()); return false; } @@ -2960,7 +2964,8 @@ bool Chainstate::DisconnectTip(BlockValidationState& state, DisconnectedBlockTra std::shared_ptr pblock = std::make_shared(); CBlock& block = *pblock; if (!ReadBlockFromDisk(block, pindexDelete, m_chainman.GetConsensus())) { - return error("DisconnectTip(): Failed to read block"); + LogError("DisconnectTip(): Failed to read block\n"); + return false; } // UndoSpecialTxsInBlock does its undo work under the scheme that will be in // force once this block is gone, but it only ever switches basic -> legacy. @@ -2977,8 +2982,10 @@ bool Chainstate::DisconnectTip(BlockValidationState& state, DisconnectedBlockTra CCoinsViewCache view(&CoinsTip()); assert(view.GetBestBlock() == pindexDelete->GetBlockHash()); - if (DisconnectBlock(block, pindexDelete, view) != DISCONNECT_OK) - return error("DisconnectTip(): DisconnectBlock %s failed", pindexDelete->GetBlockHash().ToString()); + if (DisconnectBlock(block, pindexDelete, view) != DISCONNECT_OK) { + LogError("DisconnectTip(): DisconnectBlock %s failed\n", pindexDelete->GetBlockHash().ToString()); + return false; + } bool flushed = view.Flush(); assert(flushed); dbTx->Commit(); @@ -3135,7 +3142,8 @@ bool Chainstate::ConnectTip(BlockValidationState& state, CBlockIndex* pindexNew, if (!rv) { if (state.IsInvalid()) InvalidBlockFound(pindexNew, state); - return error("%s: ConnectBlock %s failed, %s", __func__, pindexNew->GetBlockHash().ToString(), state.ToString()); + LogError("%s: ConnectBlock %s failed, %s\n", __func__, pindexNew->GetBlockHash().ToString(), state.ToString()); + return false; } time_3 = SteadyClock::now(); time_connect_total += time_3 - time_2; @@ -4508,7 +4516,8 @@ bool ChainstateManager::AcceptBlock(const std::shared_ptr& pblock, pindex->nStatus |= BLOCK_FAILED_VALID; m_blockman.m_dirty_blockindex.insert(pindex); } - return error("%s: %s", __func__, state.ToString()); + LogError("%s: %s\n", __func__, state.ToString()); + return false; } // Header is valid/has work, merkle tree is good...RELAY NOW @@ -4574,15 +4583,18 @@ bool ChainstateManager::ProcessNewBlock(const std::shared_ptr& blo } if (!ret) { GetMainSignals().BlockChecked(*block, state); - return error("%s: AcceptBlock FAILED: %s", __func__, state.ToString()); + LogError("%s: AcceptBlock FAILED (%s)\n", __func__, state.ToString()); + return false; } } NotifyHeaderTip(ActiveChainstate()); BlockValidationState state; // Only used to report errors, not invalidity - ignore it - if (!ActiveChainstate().ActivateBestChain(state, block)) - return error("%s: ActivateBestChain failed: %s", __func__, state.ToString()); + if (!ActiveChainstate().ActivateBestChain(state, block)) { + LogError("%s: ActivateBestChain failed (%s)\n", __func__, state.ToString()); + return false; + } LogPrintf("%s : ACCEPTED\n", __func__); return true; @@ -4633,11 +4645,20 @@ bool TestBlockValidity(BlockValidationState& state, // NOTE: CheckBlockHeader is called by CheckBlock if (!ContextualCheckBlockHeader(block, state, chainstate.m_blockman, chainstate.m_chainman, pindexPrev)) - return error("%s: Consensus::ContextualCheckBlockHeader: %s", __func__, state.ToString()); + { + LogError("%s: Consensus::ContextualCheckBlockHeader: %s\n", __func__, state.ToString()); + return false; + } if (!CheckBlock(block, state, chainparams.GetConsensus(), fCheckPOW, fCheckMerkleRoot)) - return error("%s: Consensus::CheckBlock: %s", __func__, state.ToString()); + { + LogError("%s: Consensus::CheckBlock: %s\n", __func__, state.ToString()); + return false; + } if (!ContextualCheckBlock(block, state, chainstate.m_chainman, pindexPrev)) - return error("%s: Consensus::ContextualCheckBlock: %s", __func__, state.ToString()); + { + LogError("%s: Consensus::ContextualCheckBlock: %s\n", __func__, state.ToString()); + return false; + } if (!chainstate.ConnectBlock(block, state, &indexDummy, viewNew, true)) return false; @@ -4756,19 +4777,22 @@ bool CVerifyDB::VerifyDB( CBlock block; // check level 0: read from disk if (!ReadBlockFromDisk(block, pindex, consensus_params)) { - return error("VerifyDB(): *** ReadBlockFromDisk failed at %d, hash=%s", pindex->nHeight, pindex->GetBlockHash().ToString()); + LogError("VerifyDB(): *** ReadBlockFromDisk failed at %d, hash=%s\n", pindex->nHeight, pindex->GetBlockHash().ToString()); + return false; } // check level 1: verify block validity if (nCheckLevel >= 1 && !CheckBlock(block, state, consensus_params)) { - return error("%s: *** found bad block at %d, hash=%s (%s)\n", __func__, - pindex->nHeight, pindex->GetBlockHash().ToString(), state.ToString()); + LogError("%s: *** found bad block at %d, hash=%s (%s)\n", __func__, + pindex->nHeight, pindex->GetBlockHash().ToString(), state.ToString()); + return false; } // check level 2: verify undo validity if (nCheckLevel >= 2 && pindex) { CBlockUndo undo; if (!pindex->GetUndoPos().IsNull()) { if (!UndoReadFromDisk(undo, pindex)) { - return error("VerifyDB(): *** found bad undo data at %d, hash=%s\n", pindex->nHeight, pindex->GetBlockHash().ToString()); + LogError("VerifyDB(): *** found bad undo data at %d, hash=%s\n", pindex->nHeight, pindex->GetBlockHash().ToString()); + return false; } } } @@ -4780,7 +4804,8 @@ bool CVerifyDB::VerifyDB( assert(coins.GetBestBlock() == pindex->GetBlockHash()); DisconnectResult res = chainstate.DisconnectBlock(block, pindex, coins); if (res == DISCONNECT_FAILED) { - return error("VerifyDB(): *** irrecoverable inconsistency in block data at %d, hash=%s", pindex->nHeight, pindex->GetBlockHash().ToString()); + LogError("VerifyDB(): *** irrecoverable inconsistency in block data at %d, hash=%s\n", pindex->nHeight, pindex->GetBlockHash().ToString()); + return false; } if (res == DISCONNECT_UNCLEAN) { nGoodTransactions = 0; @@ -4795,7 +4820,8 @@ bool CVerifyDB::VerifyDB( if (ShutdownRequested()) return true; } if (pindexFailure) { - return error("VerifyDB(): *** coin database inconsistencies found (last %i blocks, %i good transactions before that)\n", chainstate.m_chain.Height() - pindexFailure->nHeight + 1, nGoodTransactions); + LogError("VerifyDB(): *** coin database inconsistencies found (last %i blocks, %i good transactions before that)\n", chainstate.m_chain.Height() - pindexFailure->nHeight + 1, nGoodTransactions); + return false; } if (skipped_l3_checks) { LogPrintf("Skipped verification of level >=3 (insufficient database cache size). Consider increasing -dbcache.\n"); @@ -4818,10 +4844,14 @@ bool CVerifyDB::VerifyDB( uiInterface.ShowProgress(_("Verifying blocks…").translated, percentageDone, false); pindex = chainstate.m_chain.Next(pindex); CBlock block; - if (!ReadBlockFromDisk(block, pindex, consensus_params)) - return error("VerifyDB(): *** ReadBlockFromDisk failed at %d, hash=%s", pindex->nHeight, pindex->GetBlockHash().ToString()); - if (!chainstate.ConnectBlock(block, state, pindex, coins)) - return error("VerifyDB(): *** found unconnectable block at %d, hash=%s (%s)", pindex->nHeight, pindex->GetBlockHash().ToString(), state.ToString()); + if (!ReadBlockFromDisk(block, pindex, consensus_params)) { + LogError("VerifyDB(): *** ReadBlockFromDisk failed at %d, hash=%s\n", pindex->nHeight, pindex->GetBlockHash().ToString()); + return false; + } + if (!chainstate.ConnectBlock(block, state, pindex, coins)) { + LogError("VerifyDB(): *** found unconnectable block at %d, hash=%s (%s)\n", pindex->nHeight, pindex->GetBlockHash().ToString(), state.ToString()); + return false; + } if (ShutdownRequested()) return true; } } @@ -4841,14 +4871,15 @@ bool Chainstate::RollforwardBlock(const CBlockIndex* pindex, CCoinsViewCache& in // TODO: merge with ConnectBlock CBlock block; if (!ReadBlockFromDisk(block, pindex,m_chainman.GetConsensus())) { - return error("RollforwardBlock(): ReadBlockFromDisk failed at %d, hash=%s", pindex->nHeight, pindex->GetBlockHash().ToString()); + LogError("RollforwardBlock(): ReadBlockFromDisk failed at %d, hash=%s\n", pindex->nHeight, pindex->GetBlockHash().ToString()); + return false; } // MUST process special txes before updating UTXO to ensure consistency between mempool and block processing BlockValidationState state; std::optional mnlist_updates_opt{std::nullopt}; if (!m_chain_helper->special_tx->ProcessSpecialTxsInBlock(*this, block, pindex, inputs, false /*fJustCheck*/, false /*fScriptChecks*/, state, mnlist_updates_opt)) { - return error("RollforwardBlock(DASH): ProcessSpecialTxsInBlock for block %s failed with %s", + LogError("RollforwardBlock(DASH): ProcessSpecialTxsInBlock for block %s failed with %s\n", pindex->GetBlockHash().ToString(), state.ToString()); } @@ -4876,7 +4907,10 @@ bool Chainstate::ReplayBlocks() std::vector hashHeads = db.GetHeadBlocks(); if (hashHeads.empty()) return true; // We're already in a consistent state. - if (hashHeads.size() != 2) return error("ReplayBlocks(): unknown inconsistent state"); + if (hashHeads.size() != 2) { + LogError("ReplayBlocks(): unknown inconsistent state\n"); + return false; + } uiInterface.ShowProgress(_("Replaying blocks…").translated, 0, false); LogPrintf("Replaying blocks\n"); @@ -4886,20 +4920,23 @@ bool Chainstate::ReplayBlocks() const CBlockIndex* pindexFork = nullptr; // Latest block common to both the old and the new tip. if (m_blockman.m_block_index.count(hashHeads[0]) == 0) { - return error("ReplayBlocks(): reorganization to unknown block requested"); + LogError("ReplayBlocks(): reorganization to unknown block requested\n"); + return false; } pindexNew = &(m_blockman.m_block_index[hashHeads[0]]); if (!hashHeads[1].IsNull()) { // The old tip is allowed to be 0, indicating it's the first flush. if (m_blockman.m_block_index.count(hashHeads[1]) == 0) { - return error("ReplayBlocks(): reorganization from unknown block requested"); + LogError("ReplayBlocks(): reorganization from unknown block requested\n"); + return false; } pindexOld = &(m_blockman.m_block_index[hashHeads[1]]); pindexFork = LastCommonAncestor(pindexOld, pindexNew); assert(pindexFork != nullptr); const bool fDIP0003Active = DeploymentActiveAt(*pindexOld, m_chainman.GetConsensus(), Consensus::DEPLOYMENT_DIP0003); if (fDIP0003Active && !m_evoDb.VerifyBestBlock(EvoDbIdentity(), pindexOld->GetBlockHash())) { - return error("ReplayBlocks(DASH): %s", EvoDbInconsistencyMessage()); + LogError("ReplayBlocks(DASH): %s\n", EvoDbInconsistencyMessage()); + return false; } } @@ -4910,12 +4947,14 @@ bool Chainstate::ReplayBlocks() if (pindexOld->nHeight > 0) { // Never disconnect the genesis block. CBlock block; if (!ReadBlockFromDisk(block, pindexOld, m_chainman.GetConsensus())) { - return error("ReplayBlocks(): ReadBlockFromDisk() failed at %d, hash=%s", pindexOld->nHeight, pindexOld->GetBlockHash().ToString()); + LogError("ReplayBlocks(): ReadBlockFromDisk() failed at %d, hash=%s\n", pindexOld->nHeight, pindexOld->GetBlockHash().ToString()); + return false; } LogPrintf("Rolling back %s (%i)\n", pindexOld->GetBlockHash().ToString(), pindexOld->nHeight); DisconnectResult res = DisconnectBlock(block, pindexOld, cache); if (res == DISCONNECT_FAILED) { - return error("ReplayBlocks(): DisconnectBlock failed at %d, hash=%s", pindexOld->nHeight, pindexOld->GetBlockHash().ToString()); + LogError("ReplayBlocks(): DisconnectBlock failed at %d, hash=%s\n", pindexOld->nHeight, pindexOld->GetBlockHash().ToString()); + return false; } // If DISCONNECT_UNCLEAN is returned, it means a non-existing UTXO was deleted, or an existing UTXO was // overwritten. It corresponds to cases where the block-to-be-disconnect never had all its operations @@ -5018,7 +5057,8 @@ bool Chainstate::AddGenesisBlock(const CBlock& block, BlockValidationState& stat { FlatFilePos blockPos{m_blockman.SaveBlockToDisk(block, 0, nullptr)}; if (blockPos.IsNull()) { - return error("%s: writing genesis block to disk failed (%s)", __func__, state.ToString()); + LogError("AddGenesisBlock: writing genesis block to disk failed (%s)\n", state.ToString()); + return false; } CBlockIndex* pindex = m_blockman.AddToBlockIndex(block, block.GetHash(), m_chainman.m_best_header); m_chainman.ReceivedBlockTransactions(block, pindex, blockPos); @@ -5054,7 +5094,8 @@ bool Chainstate::LoadGenesisBlock() return false; } } catch (const std::runtime_error &e) { - return error("%s: failed to initialize block database: %s", __func__, e.what()); + LogError("%s: failed to initialize block database: %s\n", __func__, e.what()); + return false; } return true; From 189a6248894509474e8d00ccb2d5481769f98ae0 Mon Sep 17 00:00:00 2001 From: glozow Date: Thu, 14 Mar 2024 11:14:56 +0000 Subject: [PATCH 3/6] Merge bitcoin/bitcoin#29459: test: check_mempool_result negative feerate bf264e05981e3809715f34f548138d53991db6f2 test: check_mempool_result negative feerate (kevkevin) Pull request description: Adds test coverage in `mempool_accept.py` to check if a negative `maxfeerate` is input into `check_mempool_result` Asserts "Amount out of range" error message and `-3` error code Motivated by this [comment](https://github.com/bitcoin/bitcoin/pull/29434/files#r1491112250) ACKs for top commit: maflcko: lgtm ACK bf264e05981e3809715f34f548138d53991db6f2 brunoerg: nice, utACK bf264e05981e3809715f34f548138d53991db6f2 davidgumberg: Looks great, ACK https://github.com/bitcoin/bitcoin/pull/29459/commits/bf264e05981e3809715f34f548138d53991db6f2 Tree-SHA512: 58931b774cc887c616f2fd91af3ee65cc5db55acd8e2875c76de448c80bd4e020b057c5f4f85556431377f0d0e7553771fb285d1ec20cf64f64ec92a47776b78 --- test/functional/mempool_accept.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/test/functional/mempool_accept.py b/test/functional/mempool_accept.py index 32fff0585371..4cdc1ce8874e 100755 --- a/test/functional/mempool_accept.py +++ b/test/functional/mempool_accept.py @@ -76,6 +76,12 @@ def run_test(self): txid_in_block = self.wallet.sendrawtransaction(from_node=node, tx_hex=raw_tx_in_block) self.generate(node, 1) self.mempool_size = 0 + # Check negative feerate + assert_raises_rpc_error(-3, "Amount out of range", lambda: self.check_mempool_result( + result_expected=None, + rawtxs=[raw_tx_in_block], + maxfeerate=-0.01, + )) self.check_mempool_result( result_expected=[{'txid': txid_in_block, 'allowed': False, 'reject-reason': 'txn-already-known'}], rawtxs=[raw_tx_in_block], From 83abb79dba41506a506dacad0da1da779eb86f16 Mon Sep 17 00:00:00 2001 From: fanquake Date: Wed, 27 Mar 2024 11:38:11 +0000 Subject: [PATCH 4/6] Merge bitcoin/bitcoin#29479: test: Refactor subtree exclusion in lint tests 80fa7da21c470302165c47cc4a6a62fb44f997ef test: Refactor subtree exclusion in lint tests (Brandon Odiwuor) Pull request description: Fixes https://github.com/bitcoin/bitcoin/issues/17413 Refactor subtree exclusion in lint tests to one place Second attempt after PR: https://github.com/bitcoin/bitcoin/pull/24435 ACKs for top commit: fjahr: re-ACK 80fa7da21c470302165c47cc4a6a62fb44f997ef maflcko: lgtm ACK 80fa7da21c470302165c47cc4a6a62fb44f997ef davidgumberg: ACK https://github.com/bitcoin/bitcoin/commit/80fa7da21c470302165c47cc4a6a62fb44f997ef Tree-SHA512: deff7457dd19ca5ea440d3d53feae047e8863b9ddeb6494a3c94605a5d16edc91db8f99a435b4fab2ef89aedee42439562be006da647fb85bbf3def903a3ce50 --- test/lint/README.md | 5 +++++ test/lint/lint-include-guards.py | 8 +++----- test/lint/lint-includes.py | 13 +++---------- test/lint/lint-spelling.py | 5 ++++- test/lint/lint_ignore_dirs.py | 8 ++++++++ 5 files changed, 23 insertions(+), 16 deletions(-) create mode 100644 test/lint/lint_ignore_dirs.py diff --git a/test/lint/README.md b/test/lint/README.md index 704922d7abe3..b2f56bda3d62 100644 --- a/test/lint/README.md +++ b/test/lint/README.md @@ -59,3 +59,8 @@ git remote add --fetch secp256k1 https://github.com/bitcoin-core/secp256k1.git all-lint.py =========== Calls other scripts with the `lint-` prefix. + + +lint_ignore_dirs.py +=================== +Add list of common directories to ignore when running tests diff --git a/test/lint/lint-include-guards.py b/test/lint/lint-include-guards.py index 6e59c83f7dbc..c5de3b4394a5 100755 --- a/test/lint/lint-include-guards.py +++ b/test/lint/lint-include-guards.py @@ -13,15 +13,13 @@ from subprocess import check_output from typing import List +from lint_ignore_dirs import SHARED_EXCLUDED_SUBTREES + HEADER_ID_PREFIX = 'BITCOIN_' HEADER_ID_SUFFIX = '_H' EXCLUDE_FILES_WITH_PREFIX = ['src/crypto/ctaes', - 'src/leveldb', - 'src/crc32c', - 'src/secp256k1', - 'src/minisketch', 'src/tinyformat.h', 'src/bench/nanobench.h', 'src/test/fuzz/FuzzedDataProvider.h', @@ -30,7 +28,7 @@ 'src/ctpl_stl.h', 'src/dashbls', 'src/gsl', - 'src/immer'] + 'src/immer'] + SHARED_EXCLUDED_SUBTREES def _get_header_file_lst() -> List[str]: diff --git a/test/lint/lint-includes.py b/test/lint/lint-includes.py index 9a0cfa127dde..9cf5e2d99c8a 100755 --- a/test/lint/lint-includes.py +++ b/test/lint/lint-includes.py @@ -14,14 +14,7 @@ from subprocess import check_output, CalledProcessError - -EXCLUDED_DIRS = ["src/leveldb/", - "src/crc32c/", - "src/secp256k1/", - "src/minisketch/", - "src/dashbls/", - "src/immer/", - "src/crypto/x11/"] +from lint_ignore_dirs import SHARED_EXCLUDED_SUBTREES EXPECTED_BOOST_INCLUDES = ["boost/date_time/posix_time/posix_time.hpp", "boost/hana/for_each.hpp", @@ -47,7 +40,7 @@ def get_toplevel(): def list_files_by_suffix(suffixes): - exclude_args = [":(exclude)" + dir for dir in EXCLUDED_DIRS] + exclude_args = [":(exclude)" + dir for dir in SHARED_EXCLUDED_SUBTREES] files_list = check_output(["git", "ls-files", "src"] + exclude_args, text=True, encoding="utf8").splitlines() @@ -105,7 +98,7 @@ def find_extra_boosts(): def find_quote_syntax_inclusions(): - exclude_args = [":(exclude)" + dir for dir in EXCLUDED_DIRS] + exclude_args = [":(exclude)" + dir for dir in SHARED_EXCLUDED_SUBTREES] quote_syntax_inclusions = list() try: diff --git a/test/lint/lint-spelling.py b/test/lint/lint-spelling.py index fb4d2495c691..e10d2368dfc5 100755 --- a/test/lint/lint-spelling.py +++ b/test/lint/lint-spelling.py @@ -11,8 +11,11 @@ from subprocess import check_output, STDOUT, CalledProcessError +from lint_ignore_dirs import SHARED_EXCLUDED_SUBTREES + IGNORE_WORDS_FILE = 'test/lint/spelling.ignore-words.txt' -FILES_ARGS = ['git', 'ls-files', '--', ":(exclude)build-aux/m4/", ":(exclude)contrib/seeds/*.txt", ":(exclude)depends/", ":(exclude)doc/release-notes/", ":(exclude)src/dashbls/", ":(exclude)src/crc32c/", ":(exclude)src/crypto/", ":(exclude)src/ctpl_stl.h", ":(exclude)src/cxxtimer.hpp", ":(exclude)src/immer/", ":(exclude)src/leveldb/", ":(exclude)src/qt/locale/", ":(exclude)src/qt/*.qrc", ":(exclude)src/secp256k1/", ":(exclude)src/minisketch/", ":(exclude)contrib/builder-keys/", ":(exclude)contrib/guix/patches", ":(exclude)src/util/subprocess.hpp", ":(exclude)src/wallet/bip39_english.h"] +FILES_ARGS = ['git', 'ls-files', '--', ":(exclude)build-aux/m4/", ":(exclude)contrib/seeds/*.txt", ":(exclude)depends/", ":(exclude)doc/release-notes/", ":(exclude)src/dashbls/", ":(exclude)src/crypto/", ":(exclude)src/ctpl_stl.h", ":(exclude)src/cxxtimer.hpp", ":(exclude)src/immer/", ":(exclude)src/qt/locale/", ":(exclude)src/qt/*.qrc", ":(exclude)contrib/builder-keys/", ":(exclude)contrib/guix/patches", ":(exclude)src/util/subprocess.hpp", ":(exclude)src/wallet/bip39_english.h"] +FILES_ARGS += [f":(exclude){dir}" for dir in SHARED_EXCLUDED_SUBTREES] def check_codespell_install(): diff --git a/test/lint/lint_ignore_dirs.py b/test/lint/lint_ignore_dirs.py new file mode 100644 index 000000000000..54440d824bd7 --- /dev/null +++ b/test/lint/lint_ignore_dirs.py @@ -0,0 +1,8 @@ +SHARED_EXCLUDED_SUBTREES = ["src/leveldb/", + "src/crc32c/", + "src/secp256k1/", + "src/minisketch/", + "src/immer/", + "src/crypto/x11/", + "src/dashbls/", + ] From 0041a6d167ae8e58f291cbe1a626da0d96702386 Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Thu, 25 Apr 2024 13:43:54 -0400 Subject: [PATCH 5/6] Merge bitcoin/bitcoin#29615: test: fix accurate multisig sigop count (BIP16), add unit test 3e9c736a26724ffe3b70b387995fbf48c06300e2 test: fix accurate multisig sigop count (BIP16), add unit test (Sebastian Falbesoner) Pull request description: In the course of reviewing #29589 I noticed the following buggy call-site of `CScriptOp.decode_op_n` in the CScript's `GetSigOpCount` method: https://github.com/bitcoin/bitcoin/blob/4cc99df44aec4d104590aee46cf18318e22a8568/test/functional/test_framework/script.py#L591-L593 This should be `lastOpcode` rather than `opcode`. The latter is either OP_CHECKMULTISIG or OP_CHECKMULTISIGVERIFY at this point, so `decode_op_n` would result in an error. Also, in `CScript.raw_iter`, we have to return the op as `CScriptOp` type instead of a bare integer, otherwise we can't call the decode method on it. To prevent this in the future, add some simple unit tests for `GetSigOpCount`. Note that this was unnoticed, as the code part was never hit so far in the test framework. ACKs for top commit: achow101: ACK 3e9c736a26724ffe3b70b387995fbf48c06300e2 Christewart: ACK 3e9c736a26724ffe3b70b387995fbf48c06300e2 rkrux: tACK [3e9c736](https://github.com/bitcoin/bitcoin/pull/29615/commits/3e9c736a26724ffe3b70b387995fbf48c06300e2) hernanmarino: tACK 3e9c736a26724ffe3b70b387995fbf48c06300e2 Tree-SHA512: 51647bb6d462fbd101effd851afdbd6ad198c0567888cd4fdcac389a9fb4bd3d7e648095c6944fd8875d36272107ebaabdc62d0e2423289055588c12294d05a7 --- test/functional/test_framework/script.py | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/test/functional/test_framework/script.py b/test/functional/test_framework/script.py index bb078591b9f4..bdc127b104a9 100644 --- a/test/functional/test_framework/script.py +++ b/test/functional/test_framework/script.py @@ -479,7 +479,7 @@ def raw_iter(self): i = 0 while i < len(self): sop_idx = i - opcode = self[i] + opcode = CScriptOp(self[i]) i += 1 if opcode > OP_PUSHDATA4: @@ -586,7 +586,7 @@ def GetSigOpCount(self, fAccurate): n += 1 elif opcode in (OP_CHECKMULTISIG, OP_CHECKMULTISIGVERIFY): if fAccurate and (OP_1 <= lastOpcode <= OP_16): - n += opcode.decode_op_n() + n += lastOpcode.decode_op_n() else: n += 20 lastOpcode = opcode @@ -704,3 +704,17 @@ def test_cscriptnum_encoding(self): values = [0, 1, -1, -2, 127, 128, -255, 256, (1 << 15) - 1, -(1 << 16), (1 << 24) - 1, (1 << 31), 1 - (1 << 32), 1 << 40, 1500, -1500] for value in values: self.assertEqual(CScriptNum.decode(CScriptNum.encode(CScriptNum(value))), value) + + def test_legacy_sigopcount(self): + # test repeated single sig ops + for n_ops in range(1, 100, 10): + for singlesig_op in (OP_CHECKSIG, OP_CHECKSIGVERIFY): + singlesigs_script = CScript([singlesig_op]*n_ops) + self.assertEqual(singlesigs_script.GetSigOpCount(fAccurate=False), n_ops) + self.assertEqual(singlesigs_script.GetSigOpCount(fAccurate=True), n_ops) + # test multisig op (including accurate counting, i.e. BIP16) + for n in range(1, 16+1): + for multisig_op in (OP_CHECKMULTISIG, OP_CHECKMULTISIGVERIFY): + multisig_script = CScript([CScriptOp.encode_op_n(n), multisig_op]) + self.assertEqual(multisig_script.GetSigOpCount(fAccurate=False), 20) + self.assertEqual(multisig_script.GetSigOpCount(fAccurate=True), n) From 374a470c443e5e7a53061f96cb8d73ae97ae8c72 Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Thu, 25 Apr 2024 13:55:37 -0400 Subject: [PATCH 6/6] Merge bitcoin/bitcoin#29433: contrib: rpcauth.py - Add new option (-json) to output text in json format 9adf949d2aa6d199b85295b18c08967395b5570a contrib: rpcauth.py - Add new option (-j/--json) to output text in json format (bstin) Pull request description: This is a simple change to rpcauth.py utility in order to output as json instead raw text. This is beneficial because integrating json output is simpler with multiple different forms of automation and tooling ACKs for top commit: maflcko: ACK 9adf949d2aa6d199b85295b18c08967395b5570a achow101: ACK 9adf949d2aa6d199b85295b18c08967395b5570a willcl-ark: tACK 9adf949d2aa6d199b85295b18c08967395b5570a tdb3: ACK for 9adf949d2aa6d199b85295b18c08967395b5570a Tree-SHA512: 2cdc3b2071fbe4fb32a84ce42ee8ad216cff96ed82aaef58daeb3991953ac137ae42d6898a7fdb6cbd1800e1f61ff8d292f0b150eaebdd2a3fd9d37ed7450787 --- share/rpcauth/README.md | 1 + share/rpcauth/rpcauth.py | 12 +++++++++--- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/share/rpcauth/README.md b/share/rpcauth/README.md index 6f627b867bca..1b3acb1dac16 100644 --- a/share/rpcauth/README.md +++ b/share/rpcauth/README.md @@ -15,4 +15,5 @@ positional arguments: optional arguments: -h, --help show this help message and exit + -j, --json output data in json format ``` diff --git a/share/rpcauth/rpcauth.py b/share/rpcauth/rpcauth.py index 6f94f8fe770c..70b3706f3c0c 100755 --- a/share/rpcauth/rpcauth.py +++ b/share/rpcauth/rpcauth.py @@ -7,6 +7,7 @@ from getpass import getpass from secrets import token_hex, token_urlsafe import hmac +import json def generate_salt(size): """Create size byte hex salt""" @@ -24,6 +25,7 @@ def main(): parser = ArgumentParser(description='Create login credentials for a JSON-RPC user') parser.add_argument('username', help='the username for authentication') parser.add_argument('password', help='leave empty to generate a random password or specify "-" to prompt for password', nargs='?') + parser.add_argument("-j", "--json", help="output to json instead of plain-text", action='store_true') args = parser.parse_args() if not args.password: @@ -35,9 +37,13 @@ def main(): salt = generate_salt(16) password_hmac = password_to_hmac(salt, args.password) - print('String to be appended to dash.conf:') - print(f'rpcauth={args.username}:{salt}${password_hmac}') - print(f'Your password:\n{args.password}') + if args.json: + odict={'username':args.username, 'password':args.password, 'rpcauth':f'{args.username}:{salt}${password_hmac}'} + print(json.dumps(odict)) + else: + print('String to be appended to dash.conf:') + print(f'rpcauth={args.username}:{salt}${password_hmac}') + print(f'Your password:\n{args.password}') if __name__ == '__main__': main()