From 21efd265174cd887ec90bb9ee363597ee9ad5e2f Mon Sep 17 00:00:00 2001 From: merge-script Date: Thu, 11 Apr 2024 16:18:39 +0200 Subject: [PATCH 1/7] Merge bitcoin/bitcoin#29849: Fix typos in `subprocess.hpp` 13f5391bbb45cd8aebc6ae70cad08aff632ebd55 Fix typos in `subprocess.hpp` (Hennadii Stepanov) Pull request description: Resolves one item in the https://github.com/bitcoin/bitcoin/pull/28981#pullrequestreview-1991272752: > - Remove linter exclusions and fix all issues. Based on upstream https://github.com/arun11299/cpp-subprocess/pull/101. ACKs for top commit: fanquake: ACK 13f5391bbb45cd8aebc6ae70cad08aff632ebd55 Tree-SHA512: 2ee27a5b7d1ba6f47a5148add155c918eadaaffb94a4b5dd3edea00e63440b87291c559361bf25a8db1567debff78cf7e9466dc34f14331ca1d426994837df93 --- src/util/subprocess.hpp | 32 +++++++++++++++++--------------- test/lint/lint-spelling.py | 2 +- 2 files changed, 18 insertions(+), 16 deletions(-) diff --git a/src/util/subprocess.hpp b/src/util/subprocess.hpp index dd53a8fbb4fb..0fcc9397ea7a 100644 --- a/src/util/subprocess.hpp +++ b/src/util/subprocess.hpp @@ -1,6 +1,8 @@ +// Based on the https://github.com/arun11299/cpp-subprocess project. + /*! -Documentation for C++ subprocessing libraray. +Documentation for C++ subprocessing library. @copyright The code is licensed under the [MIT License](http://opensource.org/licenses/MIT): @@ -106,7 +108,7 @@ namespace subprocess { // from pipe static const size_t SP_MAX_ERR_BUF_SIZ = 1024; -// Default buffer capcity for OutBuffer and ErrBuffer. +// Default buffer capacity for OutBuffer and ErrBuffer. // If the data exceeds this capacity, the buffer size is grown // by 1.5 times its previous capacity static const size_t DEFAULT_BUF_CAP_BYTES = 8192; @@ -312,8 +314,8 @@ namespace util inline env_map_t MapFromWindowsEnvironment(){ wchar_t *variable_strings_ptr; wchar_t *environment_strings_ptr; - std::wstring delimeter(L"="); - int del_len = delimeter.length(); + std::wstring delimiter(L"="); + int del_len = delimiter.length(); env_map_t mapped_environment; // Get a pointer to the environment block. @@ -335,7 +337,7 @@ namespace util // Create a string from Variable String env_string_t current_line(variable_strings_ptr); // Find the first "equals" sign. - auto pos = current_line.find(delimeter); + auto pos = current_line.find(delimiter); // Assuming it's not missing ... if(pos!=std::wstring::npos){ // ... parse the key and value. @@ -433,8 +435,8 @@ namespace util * Function: join * Parameters: * [in] vec : Vector of strings which needs to be joined to form - * a single string with words seperated by a seperator char. - * [in] sep : String used to seperate 2 words in the joined string. + * a single string with words separated by a separator char. + * [in] sep : String used to separate 2 words in the joined string. * Default constructed to ' ' (space). * [out] string: Joined string. */ @@ -655,7 +657,7 @@ namespace util * Default value is 0. */ struct bufsize { - explicit bufsize(int siz): bufsiz(siz) {} + explicit bufsize(int sz): bufsiz(sz) {} int bufsiz = 0; }; @@ -711,7 +713,7 @@ struct string_arg }; /*! - * Option to specify the executable name seperately + * Option to specify the executable name separately * from the args sequence. * In this case the cmd args must only contain the * options required for this executable. @@ -728,7 +730,7 @@ struct executable: string_arg * Option to set the current working directory * of the spawned process. * - * Eg: cwd{"/som/path/x"} + * Eg: cwd{"/some/path/x"} */ struct cwd: string_arg { @@ -852,7 +854,7 @@ struct error wr_ch_ = fd; } explicit error(IOTYPE typ) { - assert ((typ == PIPE || typ == STDOUT) && "STDERR not aloowed"); + assert ((typ == PIPE || typ == STDOUT) && "STDERR not allowed"); if (typ == PIPE) { #ifndef __USING_WINDOWS__ std::tie(rd_ch_, wr_ch_) = util::pipe_cloexec(); @@ -874,7 +876,7 @@ struct error // ATTN: Can be used only to execute functions with no // arguments and returning void. // Could have used more efficient methods, ofcourse, but -// that wont yield me the consistent syntax which I am +// that won't yield me the consistent syntax which I am // aiming for. If you know, then please do let me know. class preexec_func @@ -1211,10 +1213,10 @@ class Streams * 9. communicate(...) - Get the output/error from the child and close the channels * from the parent side. *10. input() - Get the input channel/File pointer. Can be used for - * cutomizing the way of sending input to child. + * customizing the way of sending input to child. *11. output() - Get the output channel/File pointer. Usually used in case of redirection. See piping examples. - *12. error() - Get the error channel/File poiner. Usually used + *12. error() - Get the error channel/File pointer. Usually used in case of redirection. *13. start_process() - Start the child process. Only to be used when * `defer_spawn` option was provided in Popen constructor. @@ -1358,7 +1360,7 @@ class Popen // Command in string format std::string args_; - // Comamnd provided as sequence + // Command provided as sequence std::vector vargs_; std::vector cargv_; diff --git a/test/lint/lint-spelling.py b/test/lint/lint-spelling.py index fb4d2495c691..79c4533bdf55 100755 --- a/test/lint/lint-spelling.py +++ b/test/lint/lint-spelling.py @@ -12,7 +12,7 @@ from subprocess import check_output, STDOUT, CalledProcessError 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/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/wallet/bip39_english.h"] def check_codespell_install(): From 86f25a85538f84907bf5c5caf2228c99457fedfb Mon Sep 17 00:00:00 2001 From: merge-script Date: Wed, 24 Apr 2024 22:57:50 +0800 Subject: [PATCH 2/7] Merge bitcoin/bitcoin#29910: refactor: Rename `subprocess.hpp` to follow our header name conventions 08f756bd370c3e100b186c7e24bef6a033575b29 Replace locale-dependent `std::strerror` with `SysErrorString` (Hennadii Stepanov) d8e4ba4d0568769782b8c19c82e955c4aee73477 refactor: Rename `subprocess.hpp` to follow our header name conventions (Hennadii Stepanov) Pull request description: This PR renames the header `*.hpp` --> `*.h` and adjusts the header guard name, which makes it available for processing by linters. Fixed the following linter warning: ``` The locale dependent function strerror(...) appears to be used: src/util/subprocess.h: std::runtime_error( err_msg + ": " + std::strerror(err_code) ) Unnecessary locale dependence can cause bugs that are very tricky to isolate and fix. Please avoid using locale-dependent functions if possible. Advice not applicable in this specific case? Add an exception by updating the ignore list in /bitcoin/test/lint/lint-locale-dependence.py ^---- failure generated from lint-locale-dependence.py ``` ACKs for top commit: TheCharlatan: ACK 08f756bd370c3e100b186c7e24bef6a033575b29 Tree-SHA512: 57a2f01c20eb9552481e428a4969bd59e9ada9f784fe1a45cb62aa9c9152c8e950d336854f45af0e2e5dc7c7b2a1fb216c8f832e3d6ccfb457ad71b6e423231e --- src/Makefile.am | 2 +- src/common/run_command.cpp | 2 +- src/test/system_tests.cpp | 2 +- src/util/{subprocess.hpp => subprocess.h} | 10 ++++++---- 4 files changed, 9 insertions(+), 7 deletions(-) rename src/util/{subprocess.hpp => subprocess.h} (99%) diff --git a/src/Makefile.am b/src/Makefile.am index 4f40714c7677..9de99c307476 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -424,7 +424,7 @@ BITCOIN_CORE_H = \ util/sock.h \ util/string.h \ util/spanparsing.h \ - util/subprocess.hpp \ + util/subprocess.h \ util/syserror.h \ util/system.h \ util/time.h \ diff --git a/src/common/run_command.cpp b/src/common/run_command.cpp index e5356490ef7f..347b4860955f 100644 --- a/src/common/run_command.cpp +++ b/src/common/run_command.cpp @@ -12,7 +12,7 @@ #include #ifdef ENABLE_EXTERNAL_SIGNER -#include +#include #endif // ENABLE_EXTERNAL_SIGNER UniValue RunCommandParseJSON(const std::string& str_command, const std::string& str_std_in) diff --git a/src/test/system_tests.cpp b/src/test/system_tests.cpp index ce7ed3912c79..3a11152bf071 100644 --- a/src/test/system_tests.cpp +++ b/src/test/system_tests.cpp @@ -7,7 +7,7 @@ #include #ifdef ENABLE_EXTERNAL_SIGNER -#include +#include #endif // ENABLE_EXTERNAL_SIGNER #include diff --git a/src/util/subprocess.hpp b/src/util/subprocess.h similarity index 99% rename from src/util/subprocess.hpp rename to src/util/subprocess.h index 0fcc9397ea7a..501155dffdcd 100644 --- a/src/util/subprocess.hpp +++ b/src/util/subprocess.h @@ -33,8 +33,10 @@ Documentation for C++ subprocessing library. @version 1.0.0 */ -#ifndef SUBPROCESS_HPP -#define SUBPROCESS_HPP +#ifndef BITCOIN_UTIL_SUBPROCESS_H +#define BITCOIN_UTIL_SUBPROCESS_H + +#include #include #include @@ -150,7 +152,7 @@ class OSError: public std::runtime_error { public: OSError(const std::string& err_msg, int err_code): - std::runtime_error( err_msg + ": " + std::strerror(err_code) ) + std::runtime_error(err_msg + ": " + SysErrorString(err_code)) {} }; @@ -1985,4 +1987,4 @@ namespace detail { } -#endif // SUBPROCESS_HPP +#endif // BITCOIN_UTIL_SUBPROCESS_H From 87c526eeea81b3c7f08ad9cd555503c6e35375c3 Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Thu, 2 May 2024 16:33:18 -0400 Subject: [PATCH 3/7] Merge bitcoin/bitcoin#29961: refactor: remove remaining unused code from cpp-subprocess 8b52e7f628304e83b0e36fd97e617de0f71c5a62 update comments in cpp-subprocess (check_output references) (Sebastian Falbesoner) 97f159776ec06f767df1d4990aa7d0859140f52f remove unused method `Popen::kill` from cpp-subprocess (Sebastian Falbesoner) 908c51fe4afeba0af500c6275027b1afa1b3bd19 remove commented out code in cpp-subprocess (Sebastian Falbesoner) ff79adbe056220202f7a56d67f788c38fc49ef9f remove unused templates from cpp-subprocess (Sebastian Falbesoner) Pull request description: This PR removes remaining code that is unused within the cpp-subprocess module (templates and commented out code). Happy to add more removals if anyone finds more unused parts. Note that there are some API functions of the `Popen` class that we don't use, e.g. `wait()`, `pid()`, `poll()`, `kill()`, but they sound IMHO common enough to be useful in the future, so not sure how deep we should go there. ACKs for top commit: fjahr: Code review ACK 8b52e7f628304e83b0e36fd97e617de0f71c5a62 achow101: ACK 8b52e7f628304e83b0e36fd97e617de0f71c5a62 hebasto: ACK 8b52e7f628304e83b0e36fd97e617de0f71c5a62. Tree-SHA512: 14c1cd2216185d941923f06fdc7acbeed66cd87e2691d9a352f7309b3e07fe4877b580f598a2e4106f9c48395ed6de00a0bfb5d3c3af9c4624d1956a0f543e99 --- src/util/subprocess.h | 84 ++++++++++--------------------------------- 1 file changed, 18 insertions(+), 66 deletions(-) diff --git a/src/util/subprocess.h b/src/util/subprocess.h index 501155dffdcd..a1be9a145f07 100644 --- a/src/util/subprocess.h +++ b/src/util/subprocess.h @@ -172,12 +172,6 @@ using env_vector_t = std::vector; //-------------------------------------------------------------------- namespace util { - template - inline bool is_ready(std::shared_future const &f) - { - return f.wait_for(std::chrono::seconds(0)) == std::future_status::ready; - } - inline void quote_argument(const std::wstring &argument, std::wstring &command_line, bool force) { @@ -919,8 +913,8 @@ class preexec_func * This is basically used to determine the length of the actual * data stored inside the dynamically resized vector. * - * This is what is returned as the output to communicate and check_output - * functions, so, users must know about this class. + * This is what is returned as the output to the communicate + * function, so, users must know about this class. * * OutBuffer and ErrBuffer are just different typedefs to this class. */ @@ -931,22 +925,6 @@ class Buffer explicit Buffer(size_t cap) { buf.resize(cap); } void add_cap(size_t cap) { buf.resize(cap); } -#if 0 - Buffer(const Buffer& other): - buf(other.buf), - length(other.length) - { - std::cout << "COPY" << std::endl; - } - - Buffer(Buffer&& other): - buf(std::move(other.buf)), - length(other.length) - { - std::cout << "MOVE" << std::endl; - } -#endif - public: std::vector buf; size_t length = 0; @@ -1202,25 +1180,24 @@ class Streams * interface to the client. * * API's provided by the class: - * 1. Popen({"cmd"}, output{..}, error{..}, cwd{..}, ....) + * Popen({"cmd"}, output{..}, error{..}, cwd{..}, ....) * Command provided as a sequence. - * 2. Popen("cmd arg1"m output{..}, error{..}, cwd{..}, ....) + * Popen("cmd arg1", output{..}, error{..}, cwd{..}, ....) * Command provided in a single string. - * 3. wait() - Wait for the child to exit. - * 4. retcode() - The return code of the exited child. - * 5. pid() - PID of the spawned child. - * 6. poll() - Check the status of the running child. - * 7. kill(sig_num) - Kill the child. SIGTERM used by default. - * 8. send(...) - Send input to the input channel of the child. - * 9. communicate(...) - Get the output/error from the child and close the channels - * from the parent side. - *10. input() - Get the input channel/File pointer. Can be used for - * customizing the way of sending input to child. - *11. output() - Get the output channel/File pointer. Usually used - in case of redirection. See piping examples. - *12. error() - Get the error channel/File pointer. Usually used - in case of redirection. - *13. start_process() - Start the child process. Only to be used when + * wait() - Wait for the child to exit. + * retcode() - The return code of the exited child. + * pid() - PID of the spawned child. + * poll() - Check the status of the running child. + * send(...) - Send input to the input channel of the child. + * communicate(...) - Get the output/error from the child and close the channels + * from the parent side. + * input() - Get the input channel/File pointer. Can be used for + * customizing the way of sending input to child. + * output() - Get the output channel/File pointer. Usually used + in case of redirection. See piping examples. + * error() - Get the error channel/File pointer. Usually used + in case of redirection. + * start_process() - Start the child process. Only to be used when * `defer_spawn` option was provided in Popen constructor. */ class Popen @@ -1265,14 +1242,6 @@ class Popen if (!defer_process_start_) execute_process(); } -/* - ~Popen() - { -#ifdef __USING_WINDOWS__ - CloseHandle(this->process_handle_); -#endif - } -*/ void start_process() noexcept(false); @@ -1284,10 +1253,6 @@ class Popen int poll() noexcept(false); - // Does not fail, Caller is expected to recheck the - // status with a call to poll() - void kill(int sig_num = 9); - void set_out_buf_cap(size_t cap) { stream_.set_out_buf_cap(cap); } void set_err_buf_cap(size_t cap) { stream_.set_err_buf_cap(cap); } @@ -1478,19 +1443,6 @@ inline int Popen::poll() noexcept(false) #endif } -inline void Popen::kill(int sig_num) -{ -#ifdef __USING_WINDOWS__ - if (!TerminateProcess(this->process_handle_, (UINT)sig_num)) { - throw OSError("TerminateProcess", 0); - } -#else - if (session_leader_) killpg(child_pid_, sig_num); - else ::kill(child_pid_, sig_num); -#endif -} - - inline void Popen::execute_process() noexcept(false) { #ifdef __USING_WINDOWS__ From 55b41012066e82e0ff8de29849112d07c81b2e0b Mon Sep 17 00:00:00 2001 From: merge-script Date: Fri, 3 May 2024 10:47:37 +0800 Subject: [PATCH 4/7] Merge bitcoin/bitcoin#30017: refactor, fuzz: Make 64-bit shift explicit b50d127a7710d790c2ba4a08f01b832c2a0b1203 refactor: Make 64-bit shift explicit (Hennadii Stepanov) Pull request description: This PR fixes MSVC warning [C4334](https://docs.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-3-c4334) in the fuzzing code. Similar to https://github.com/bitcoin/bitcoin/pull/26252. All `DisableSpecificWarnings` dropped from `fuzz.vcxproj` as all remained are inherited from `common.init.vcxproj`. Required to simplify warning suppression porting to the CMake-based build system. ACKs for top commit: maflcko: utACK b50d127a7710d790c2ba4a08f01b832c2a0b1203 sipsorcery: utACK b50d127a7710d790c2ba4a08f01b832c2a0b1203 Tree-SHA512: 18f6082b4234506ad2f9df54e577031b97cdf9f7ef64cad4162f275660716ab73587a97d3af0f778dfd48d2751d8676b5d3381d0aa837fcc60a09704473a9209 --- src/test/fuzz/poolresource.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/test/fuzz/poolresource.cpp b/src/test/fuzz/poolresource.cpp index ce64ef6472cf..f764d9f8dbe7 100644 --- a/src/test/fuzz/poolresource.cpp +++ b/src/test/fuzz/poolresource.cpp @@ -63,9 +63,9 @@ class PoolResourceFuzzer { if (m_total_allocated > 0x1000000) return; size_t alignment_bits = m_provider.ConsumeIntegralInRange(0, 7); - size_t alignment = 1 << alignment_bits; + size_t alignment = size_t{1} << alignment_bits; size_t size_bits = m_provider.ConsumeIntegralInRange(0, 16 - alignment_bits); - size_t size = m_provider.ConsumeIntegralInRange(1U << size_bits, (1U << (size_bits + 1)) - 1U) << alignment_bits; + size_t size = m_provider.ConsumeIntegralInRange(size_t{1} << size_bits, (size_t{1} << (size_bits + 1)) - 1U) << alignment_bits; Allocate(size, alignment); } From 252795bb8fe2dee476b9192afc9ae5b96958b302 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Fri, 3 May 2024 09:44:00 -0400 Subject: [PATCH 5/7] Merge bitcoin/bitcoin#30026: refactor, test: Always initialize pointer bd2de7ac591d7704b79304089ad1fb57e085da8b refactor, test: Always initialize pointer (Hennadii Stepanov) Pull request description: This change fixes MSVC warning [C4703](https://learn.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-4-c4703). All `DisableSpecificWarnings` dropped from `test_bitcoin.vcxproj` as all remained are inherited from `common.init.vcxproj`. Required to simplify warning suppression porting to the CMake-based build system. ACKs for top commit: maflcko: utACK bd2de7ac591d7704b79304089ad1fb57e085da8b sipsorcery: utACK bd2de7ac591d7704b79304089ad1fb57e085da8b. ryanofsky: Code review ACK bd2de7ac591d7704b79304089ad1fb57e085da8b Tree-SHA512: 006db041d3c3697a77d9df14de86cf7c8a10804b45789df01268b2236cf6452e77dc57e89f5d5a6bc26d4b5cd483f0722d6035649c8a523b57954bb1fc810d0c --- src/test/validation_chainstate_tests.cpp | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/test/validation_chainstate_tests.cpp b/src/test/validation_chainstate_tests.cpp index 73920bda6743..3d7d9e50e368 100644 --- a/src/test/validation_chainstate_tests.cpp +++ b/src/test/validation_chainstate_tests.cpp @@ -14,6 +14,7 @@ #include #include #include +#include #include #include @@ -96,14 +97,14 @@ BOOST_FIXTURE_TEST_CASE(chainstate_update_tip, TestChain100Setup) BOOST_CHECK_EQUAL(chainman.GetAll().size(), 2); - CChainState& background_cs{*[&] { + CChainState& background_cs{*Assert([&]() -> CChainState* { for (CChainState* cs : chainman.GetAll()) { if (cs != &chainman.ActiveChainstate()) { return cs; } } - assert(false); - }()}; + return nullptr; + }())}; // Create a block to append to the validation chain. std::vector noTxns; From 2ec48c5ab367411efc530fd2d656fd7d6f6533d0 Mon Sep 17 00:00:00 2001 From: merge-script Date: Wed, 29 May 2024 09:22:24 +0100 Subject: [PATCH 6/7] Merge bitcoin/bitcoin#30170: refactor: Use type-safe time in txorphanage fa6d4891c7815025ab1cd58d0906466af31bb648 refactor: Use type-safe time in txorphanage (MarcoFalke) Pull request description: This allows to remove manual conversions like multiplication by `60`, and uses a type-safe type instead of a raw `int64_t`. ACKs for top commit: epiccurious: utACK fa6d4891c7815025ab1cd58d0906466af31bb648. dergoegge: Code review ACK fa6d4891c7815025ab1cd58d0906466af31bb648 brunoerg: utACK fa6d4891c7815025ab1cd58d0906466af31bb648 Tree-SHA512: c187d0e579b1131afcef8c901f5662c18ab867fa2a99fbb13b67bb1e10b2047128194bfef8329cde0d51e1c35d6227ae292b823968f37ea9422975e46e01846a --- src/txorphanage.cpp | 17 +++++++++-------- src/txorphanage.h | 3 ++- 2 files changed, 11 insertions(+), 9 deletions(-) diff --git a/src/txorphanage.cpp b/src/txorphanage.cpp index d03924c2fe7d..50c0758f32f1 100644 --- a/src/txorphanage.cpp +++ b/src/txorphanage.cpp @@ -8,13 +8,14 @@ #include #include #include +#include #include -/** Expiration time for orphan transactions in seconds */ -static constexpr int64_t ORPHAN_TX_EXPIRE_TIME = 20 * 60; -/** Minimum time between orphan transactions expire time checks in seconds */ -static constexpr int64_t ORPHAN_TX_EXPIRE_INTERVAL = 5 * 60; +/** Expiration time for orphan transactions */ +static constexpr auto ORPHAN_TX_EXPIRE_TIME{20min}; +/** Minimum time between orphan transactions expire time checks */ +static constexpr auto ORPHAN_TX_EXPIRE_INTERVAL{5min}; bool TxOrphanage::AddTx(const CTransactionRef& tx, NodeId peer) @@ -39,7 +40,7 @@ bool TxOrphanage::AddTx(const CTransactionRef& tx, NodeId peer) return false; } - auto ret = m_orphans.emplace(hash, OrphanTx{tx, peer, GetTime() + ORPHAN_TX_EXPIRE_TIME, m_orphan_list.size(), sz}); + auto ret = m_orphans.emplace(hash, OrphanTx{tx, peer, Now() + ORPHAN_TX_EXPIRE_TIME, m_orphan_list.size(), sz}); assert(ret.second); m_orphan_list.push_back(ret.first); for (const CTxIn& txin : tx->vin) { @@ -121,12 +122,12 @@ void TxOrphanage::LimitOrphans(unsigned int max_orphans_size) LOCK(m_mutex); unsigned int nEvicted = 0; - static int64_t nNextSweep; - int64_t nNow = GetTime(); + static NodeSeconds nNextSweep; + auto nNow{Now()}; if (nNextSweep <= nNow) { // Sweep out expired orphan pool entries: int nErased = 0; - int64_t nMinExpTime = nNow + ORPHAN_TX_EXPIRE_TIME - ORPHAN_TX_EXPIRE_INTERVAL; + auto nMinExpTime = nNow + ORPHAN_TX_EXPIRE_TIME - ORPHAN_TX_EXPIRE_INTERVAL; std::map::iterator iter = m_orphans.begin(); while (iter != m_orphans.end()) { diff --git a/src/txorphanage.h b/src/txorphanage.h index bd68ea091627..0bd364d672f6 100644 --- a/src/txorphanage.h +++ b/src/txorphanage.h @@ -9,6 +9,7 @@ #include #include #include +#include #include #include @@ -70,7 +71,7 @@ class TxOrphanage { struct OrphanTx { CTransactionRef tx; NodeId fromPeer; - int64_t nTimeExpire; + NodeSeconds nTimeExpire; size_t list_pos; size_t nTxSize; }; From 52b803389bdd207e75d5379f92f7988709a66d82 Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Tue, 23 Apr 2024 12:58:50 -0400 Subject: [PATCH 7/7] Merge bitcoin/bitcoin#29865: util: remove unused cpp-subprocess options 13adbf733f09c73c3cf0025d94c52f9cec5dba3b remove unneeded environment option from cpp-subprocess (Sebastian Falbesoner) 2088777ba0f9ad3f6d4ab8b0b6ff8aad71117307 remove unneeded cwd option from cpp-subprocess (Sebastian Falbesoner) 03ffb09c31aa04cc296c0ce10d07109e22a8dd75 remove unneeded bufsize option from cpp-subprocess (Sebastian Falbesoner) 79c30363733503a1fb7d4c98aa0d56ced0be6e32 remove unneeded close_fds option from cpp-subprocess (Sebastian Falbesoner) 62db8f8e5a6cfe19d905afc91731d6bc8a665f61 remove unneeded session_leader option from cpp-subprocess (Sebastian Falbesoner) 80d008c66d00d3496cd8549daee6775cf2c6b782 remove unneeded defer_spawn option from cpp-subprocess (Sebastian Falbesoner) cececad7b29e2ca3de1216db1c541dba6dc81bfa remove unneeded preexec function option from cpp-subprocess (Sebastian Falbesoner) 633e45b2e2728efcb0637afa94fcbd5756dfbe76 remove unneeded shell option from cpp-subprocess (Sebastian Falbesoner) Pull request description: The newly introduced cpp-subprocess library provides a good number of options for the `Popen` class: https://github.com/bitcoin/bitcoin/blob/0de63b8b46eff5cda85b4950062703324ba65a80/src/util/subprocess.hpp#L1009-L1020 Some of them are either not fully implemented (`shell`, missing an implementation on Windows), implemented in an ugly way (e.g. using "Impoverished, meager, needy, truly needy version of type erasure" for `preexec_func` according to the author's own words) or simply unlikely to be ever needed for our external signer use-case (`defer_spawn`). Instead of maintaining incomplete and/or unneeded code, I'd suggest to get rid of it and only keep support for options if there is a strong reason for it. ACKs for top commit: achow101: ACK 13adbf733f09c73c3cf0025d94c52f9cec5dba3b hebasto: re-ACK 13adbf733f09c73c3cf0025d94c52f9cec5dba3b. Tree-SHA512: 8270da27891cb659da2ef6062a23f4b86331859b15ac27b79ae7433b14f5bd7efaba621f2b3ba1953708d0f38377a8bd23ef1cc0f28b9c152ac8958dd9eec6b0 --- src/util/subprocess.h | 393 +----------------------------------------- 1 file changed, 9 insertions(+), 384 deletions(-) diff --git a/src/util/subprocess.h b/src/util/subprocess.h index a1be9a145f07..25ba2c438f93 100644 --- a/src/util/subprocess.h +++ b/src/util/subprocess.h @@ -156,19 +156,6 @@ class OSError: public std::runtime_error {} }; -//-------------------------------------------------------------------- - -//Environment Variable types -#ifndef _MSC_VER - using env_string_t = std::string; - using env_char_t = char; -#else - using env_string_t = std::wstring; - using env_char_t = wchar_t; -#endif -using env_map_t = std::map; -using env_vector_t = std::vector; - //-------------------------------------------------------------------- namespace util { @@ -301,100 +288,6 @@ namespace util if (!SetHandleInformation(*child_handle, HANDLE_FLAG_INHERIT, 0)) throw OSError("SetHandleInformation", 0); } - - // env_map_t MapFromWindowsEnvironment() - // * Imports current Environment in a C-string table - // * Parses the strings by splitting on the first "=" per line - // * Creates a map of the variables - // * Returns the map - inline env_map_t MapFromWindowsEnvironment(){ - wchar_t *variable_strings_ptr; - wchar_t *environment_strings_ptr; - std::wstring delimiter(L"="); - int del_len = delimiter.length(); - env_map_t mapped_environment; - - // Get a pointer to the environment block. - environment_strings_ptr = GetEnvironmentStringsW(); - // If the returned pointer is NULL, exit. - if (environment_strings_ptr == NULL) - { - throw OSError("GetEnvironmentStringsW", 0); - } - - // Variable strings are separated by NULL byte, and the block is - // terminated by a NULL byte. - - variable_strings_ptr = (wchar_t *) environment_strings_ptr; - - //Since the environment map ends with a null, we can loop until we find it. - while (*variable_strings_ptr) - { - // Create a string from Variable String - env_string_t current_line(variable_strings_ptr); - // Find the first "equals" sign. - auto pos = current_line.find(delimiter); - // Assuming it's not missing ... - if(pos!=std::wstring::npos){ - // ... parse the key and value. - env_string_t key = current_line.substr(0, pos); - env_string_t value = current_line.substr(pos + del_len); - // Map the entry. - mapped_environment[key] = value; - } - // Jump to next line in the environment map. - variable_strings_ptr += std::wcslen(variable_strings_ptr) + 1; - } - // We're done with the old environment map buffer. - FreeEnvironmentStringsW(environment_strings_ptr); - - // Return the map. - return mapped_environment; - } - - // env_vector_t WindowsEnvironmentVectorFromMap(const env_map_t &source_map) - // * Creates a vector buffer for the new environment string table - // * Copies in the mapped variables - // * Returns the vector - inline env_vector_t WindowsEnvironmentVectorFromMap(const env_map_t &source_map) - { - // Make a new environment map buffer. - env_vector_t environment_map_buffer; - // Give it some space. - environment_map_buffer.reserve(4096); - - // And fill'er up. - for(auto kv: source_map){ - // Create the line - env_string_t current_line(kv.first); current_line += L"="; current_line += kv.second; - // Add the line to the buffer. - std::copy(current_line.begin(), current_line.end(), std::back_inserter(environment_map_buffer)); - // Append a null - environment_map_buffer.push_back(0); - } - // Append one last null because of how Windows does it's environment maps. - environment_map_buffer.push_back(0); - - return environment_map_buffer; - } - - // env_vector_t CreateUpdatedWindowsEnvironmentVector(const env_map_t &changes_map) - // * Merges host environment with new mapped variables - // * Creates and returns string vector based on map - inline env_vector_t CreateUpdatedWindowsEnvironmentVector(const env_map_t &changes_map){ - // Import the environment map - env_map_t environment_map = MapFromWindowsEnvironment(); - // Merge in the changes with overwrite - for(auto& it: changes_map) - { - environment_map[it.first] = it.second; - } - // Create a Windows-usable Environment Map Buffer - env_vector_t environment_map_strings_vector = WindowsEnvironmentVectorFromMap(environment_map); - - return environment_map_strings_vector; - } - #endif /*! @@ -427,26 +320,6 @@ namespace util } - /*! - * Function: join - * Parameters: - * [in] vec : Vector of strings which needs to be joined to form - * a single string with words separated by a separator char. - * [in] sep : String used to separate 2 words in the joined string. - * Default constructed to ' ' (space). - * [out] string: Joined string. - */ - static inline - std::string join(const std::vector& vec, - const std::string& sep = " ") - { - std::string res; - for (auto& elem : vec) res.append(elem + sep); - res.erase(--res.end()); - return res; - } - - #ifndef __USING_WINDOWS__ /*! * Function: set_clo_on_exec @@ -647,56 +520,6 @@ namespace util * ------------------------------- */ -/*! - * The buffer size of the stdin/stdout/stderr - * streams of the child process. - * Default value is 0. - */ -struct bufsize { - explicit bufsize(int sz): bufsiz(sz) {} - int bufsiz = 0; -}; - -/*! - * Option to defer spawning of the child process - * till `Popen::start_process` API is called. - * Default value is false. - */ -struct defer_spawn { - explicit defer_spawn(bool d): defer(d) {} - bool defer = false; -}; - -/*! - * Option to close all file descriptors - * when the child process is spawned. - * The close fd list does not include - * input/output/error if they are explicitly - * set as part of the Popen arguments. - * - * Default value is false. - */ -struct close_fds { - explicit close_fds(bool c): close_all(c) {} - bool close_all = false; -}; - -/*! - * Option to make the child process as the - * session leader and thus the process - * group leader. - * Default value is false. - */ -struct session_leader { - explicit session_leader(bool sl): leader_(sl) {} - bool leader_ = false; -}; - -struct shell { - explicit shell(bool s): shell_(s) {} - bool shell_ = false; -}; - /*! * Base class for all arguments involving string value. */ @@ -722,34 +545,6 @@ struct executable: string_arg executable(T&& arg): string_arg(std::forward(arg)) {} }; -/*! - * Option to set the current working directory - * of the spawned process. - * - * Eg: cwd{"/some/path/x"} - */ -struct cwd: string_arg -{ - template - cwd(T&& arg): string_arg(std::forward(arg)) {} -}; - -/*! - * Option to specify environment variables required by - * the spawned process. - * - * Eg: environment{{ {"K1", "V1"}, {"K2", "V2"},... }} - */ -struct environment -{ - environment(env_map_t&& env): - env_(std::move(env)) {} - explicit environment(const env_map_t& env): - env_(env) {} - env_map_t env_; -}; - - /*! * Used for redirecting input/output/error */ @@ -866,44 +661,6 @@ struct error int wr_ch_ = -1; }; -// Impoverished, meager, needy, truly needy -// version of type erasure to store function pointers -// needed to provide the functionality of preexec_func -// ATTN: Can be used only to execute functions with no -// arguments and returning void. -// Could have used more efficient methods, ofcourse, but -// that won't yield me the consistent syntax which I am -// aiming for. If you know, then please do let me know. - -class preexec_func -{ -public: - preexec_func() {} - - template - explicit preexec_func(Func f): holder_(new FuncHolder(std::move(f))) - {} - - void operator()() { - (*holder_)(); - } - -private: - struct HolderBase { - virtual void operator()() const = 0; - virtual ~HolderBase(){}; - }; - template - struct FuncHolder: HolderBase { - FuncHolder(T func): func_(std::move(func)) {} - void operator()() const override { func_(); } - // The function pointer/reference - T func_; - }; - - std::unique_ptr holder_ = nullptr; -}; - // ~~~~ End Popen Args ~~~~ @@ -987,17 +744,9 @@ struct ArgumentDeducer ArgumentDeducer(Popen* p): popen_(p) {} void set_option(executable&& exe); - void set_option(cwd&& cwdir); - void set_option(bufsize&& bsiz); - void set_option(environment&& env); - void set_option(defer_spawn&& defer); - void set_option(shell&& sh); void set_option(input&& inp); void set_option(output&& out); void set_option(error&& err); - void set_option(close_fds&& cfds); - void set_option(preexec_func&& prefunc); - void set_option(session_leader&& sleader); private: Popen* popen_ = nullptr; @@ -1148,9 +897,6 @@ class Streams HANDLE g_hChildStd_ERR_Wr = nullptr; #endif - // Buffer size for the IO streams - int bufsiz_ = 0; - // Pipes for communicating with child // Emulates stdin @@ -1180,9 +926,9 @@ class Streams * interface to the client. * * API's provided by the class: - * Popen({"cmd"}, output{..}, error{..}, cwd{..}, ....) + * Popen({"cmd"}, output{..}, error{..}, ....) * Command provided as a sequence. - * Popen("cmd arg1", output{..}, error{..}, cwd{..}, ....) + * Popen("cmd arg1", output{..}, error{..}, ....) * Command provided in a single string. * wait() - Wait for the child to exit. * retcode() - The return code of the exited child. @@ -1197,8 +943,6 @@ class Streams in case of redirection. See piping examples. * error() - Get the error channel/File pointer. Usually used in case of redirection. - * start_process() - Start the child process. Only to be used when - * `defer_spawn` option was provided in Popen constructor. */ class Popen { @@ -1216,7 +960,7 @@ class Popen // Setup the communication channels of the Popen class stream_.setup_comm_channels(); - if (!defer_process_start_) execute_process(); + execute_process(); } template @@ -1228,7 +972,7 @@ class Popen // Setup the communication channels of the Popen class stream_.setup_comm_channels(); - if (!defer_process_start_) execute_process(); + execute_process(); } template @@ -1239,12 +983,10 @@ class Popen // Setup the communication channels of the Popen class stream_.setup_comm_channels(); - if (!defer_process_start_) execute_process(); + execute_process(); } - void start_process() noexcept(false); - int pid() const noexcept { return child_pid_; } int retcode() const noexcept { return retcode_; } @@ -1314,16 +1056,7 @@ class Popen std::future cleanup_future_; #endif - bool defer_process_start_ = false; - bool close_fds_ = false; - bool has_preexec_fn_ = false; - bool shell_ = false; - bool session_leader_ = false; - std::string exe_name_; - std::string cwd_; - env_map_t env_; - preexec_func preexec_fn_; // Command in string format std::string args_; @@ -1358,20 +1091,6 @@ inline void Popen::populate_c_argv() cargv_.push_back(nullptr); } -inline void Popen::start_process() noexcept(false) -{ - // The process was started/tried to be started - // in the constructor itself. - // For explicitly calling this API to start the - // process, 'defer_spawn' argument must be set to - // true in the constructor. - if (!defer_process_start_) { - assert (0); - return; - } - execute_process(); -} - inline int Popen::wait() noexcept(false) { #ifdef __USING_WINDOWS__ @@ -1446,17 +1165,6 @@ inline int Popen::poll() noexcept(false) inline void Popen::execute_process() noexcept(false) { #ifdef __USING_WINDOWS__ - if (this->shell_) { - throw OSError("shell not currently supported on windows", 0); - } - - void* environment_string_table_ptr = nullptr; - env_vector_t environment_string_vector; - if(this->env_.size()){ - environment_string_vector = util::CreateUpdatedWindowsEnvironmentVector(env_); - environment_string_table_ptr = (void*)environment_string_vector.data(); - } - if (exe_name_.length()) { this->vargs_.insert(this->vargs_.begin(), this->exe_name_); this->populate_c_argv(); @@ -1502,8 +1210,8 @@ inline void Popen::execute_process() noexcept(false) NULL, // process security attributes NULL, // primary thread security attributes TRUE, // handles are inherited - creation_flags, // creation flags - environment_string_table_ptr, // use provided environment + creation_flags, // creation flags + NULL, // use parent's environment NULL, // use parent's current directory &siStartInfo, // STARTUPINFOW pointer &piProcInfo); // receives PROCESS_INFORMATION @@ -1542,14 +1250,6 @@ inline void Popen::execute_process() noexcept(false) int err_rd_pipe, err_wr_pipe; std::tie(err_rd_pipe, err_wr_pipe) = util::pipe_cloexec(); - if (shell_) { - auto new_cmd = util::join(vargs_); - vargs_.clear(); - vargs_.insert(vargs_.begin(), {"/bin/sh", "-c"}); - vargs_.push_back(new_cmd); - populate_c_argv(); - } - if (exe_name_.length()) { vargs_.insert(vargs_.begin(), exe_name_); populate_c_argv(); @@ -1616,30 +1316,6 @@ namespace detail { popen_->exe_name_ = std::move(exe.arg_value); } - inline void ArgumentDeducer::set_option(cwd&& cwdir) { - popen_->cwd_ = std::move(cwdir.arg_value); - } - - inline void ArgumentDeducer::set_option(bufsize&& bsiz) { - popen_->stream_.bufsiz_ = bsiz.bufsiz; - } - - inline void ArgumentDeducer::set_option(environment&& env) { - popen_->env_ = std::move(env.env_); - } - - inline void ArgumentDeducer::set_option(defer_spawn&& defer) { - popen_->defer_process_start_ = defer.defer; - } - - inline void ArgumentDeducer::set_option(shell&& sh) { - popen_->shell_ = sh.shell_; - } - - inline void ArgumentDeducer::set_option(session_leader&& sleader) { - popen_->session_leader_ = sleader.leader_; - } - inline void ArgumentDeducer::set_option(input&& inp) { if (inp.rd_ch_ != -1) popen_->stream_.read_from_parent_ = inp.rd_ch_; if (inp.wr_ch_ != -1) popen_->stream_.write_to_child_ = inp.wr_ch_; @@ -1662,15 +1338,6 @@ namespace detail { if (err.rd_ch_ != -1) popen_->stream_.err_read_ = err.rd_ch_; } - inline void ArgumentDeducer::set_option(close_fds&& cfds) { - popen_->close_fds_ = cfds.close_all; - } - - inline void ArgumentDeducer::set_option(preexec_func&& prefunc) { - popen_->preexec_fn_ = std::move(prefunc); - popen_->has_preexec_fn_ = true; - } - inline void Child::execute_child() { #ifndef __USING_WINDOWS__ @@ -1717,41 +1384,8 @@ namespace detail { if (stream.err_write_ != -1 && stream.err_write_ > 2) close(stream.err_write_); - // Close all the inherited fd's except the error write pipe - if (parent_->close_fds_) { - int max_fd = sysconf(_SC_OPEN_MAX); - if (max_fd == -1) throw OSError("sysconf failed", errno); - - for (int i = 3; i < max_fd; i++) { - if (i == err_wr_pipe_) continue; - close(i); - } - } - - // Change the working directory if provided - if (parent_->cwd_.length()) { - sys_ret = chdir(parent_->cwd_.c_str()); - if (sys_ret == -1) throw OSError("chdir failed", errno); - } - - if (parent_->has_preexec_fn_) { - parent_->preexec_fn_(); - } - - if (parent_->session_leader_) { - sys_ret = setsid(); - if (sys_ret == -1) throw OSError("setsid failed", errno); - } - // Replace the current image with the executable - if (parent_->env_.size()) { - for (auto& kv : parent_->env_) { - setenv(kv.first.c_str(), kv.second.c_str(), 1); - } - sys_ret = execvp(parent_->exe_name_.c_str(), parent_->cargv_.data()); - } else { - sys_ret = execvp(parent_->exe_name_.c_str(), parent_->cargv_.data()); - } + sys_ret = execvp(parent_->exe_name_.c_str(), parent_->cargv_.data()); if (sys_ret == -1) throw OSError("execve failed", errno); @@ -1794,16 +1428,7 @@ namespace detail { for (auto& h : handles) { if (h == nullptr) continue; - switch (bufsiz_) { - case 0: - setvbuf(h, nullptr, _IONBF, BUFSIZ); - break; - case 1: - setvbuf(h, nullptr, _IONBF, BUFSIZ); - break; - default: - setvbuf(h, nullptr, _IOFBF, bufsiz_); - }; + setvbuf(h, nullptr, _IONBF, BUFSIZ); } #endif }