*** Please remove the following help text before submitting: ***#1
Open
Zaidalamari wants to merge 3368 commits into
Open
*** Please remove the following help text before submitting: ***#1Zaidalamari wants to merge 3368 commits into
Zaidalamari wants to merge 3368 commits into
Conversation
The increment was originally added so that mocked time would not appear to go backward relative to real-clock timestamps captured before construction, since Now<NodeSeconds>() rounds the current time down to a whole second. In practice the tests do not mix real and mocked timestamps in a way that exposes this, so the increment is unnecessary.
This refactor is a follow-up to commit faad08e and does not change any behavior. These call sites are clean mechanical swaps. The remaining ones require non-trivial test refactoring and are left for future follow-ups.
The previous name did not indicate the type was intended for testing. Renaming to FakeNodeClock makes this explicit and allows call sites to drop the ctx suffix on the variable name. Suggested in #34858 review feedback. -BEGIN VERIFY SCRIPT- s() { git grep -l "$1" -- src | xargs sed -i "s/$1/$2/g"; } s '\<NodeClockContext\>' 'FakeNodeClock' s '\<clock_ctx\>' 'clock' -END VERIFY SCRIPT-
SteadyClockContext and FakeNodeClock assume they are the only active instance. Overlapping them in the same scope would silently clobber each other. Add a CRTP base class, LimitOne, that asserts at construction if another instance already exists.
The cs_LastBlockFile mutex is redundant: all critical sections are already covered by cs_main. This is demonstrated in this patch by replacing all instances of locking cs_LastBlockFile with pairs of `AssertLockHeld(::cs_main)` and `EXCLUSIVE_LOCKS_REQUIRED(::cs_main)` annotations. No additional `::cs_main` LOCK(...)s are introduced. It is also not clear for which sections `cs_LastBlockFile` is responsible for. It is annotated for `m_blockfile_cursors`, but sporadically and inconsistently also covers `m_blockfile_info`. Since it has no semantic meaning, and seems confusing to developers, remove it.
ec6cf49 blockstorage: Remove cs_LastBlockFile recursive mutex (sedited) Pull request description: The `cs_LastBlockFile` mutex is redundant: all critical sections are already covered by cs_main. This is demonstrated in this patch by replacing all instances of locking `cs_LastBlockFile` with pairs of `AssertLockHeld(::cs_main)` and `EXCLUSIVE_LOCKS_REQUIRED(::cs_main)` annotations. No additional `::cs_main` LOCK(...)s are introduced (besides for test-only code). It is also not clear for which sections `cs_LastBlockFile` is responsible for. It is annotated for `m_blockfile_cursors`, but sporadically and inconsistently also covers `m_blockfile_info` (e.g. in `LoadBlockIndexDB`). Since it has no semantic meaning, and seems confusing to developers, remove it. An alternative to this patch would be expanding the scope of what `cs_LastBlockFile` covers and turning it into a non-recursive mutex. I prepared such a patch some time ago, but found it unsatisfactory. It was not clear to me if the lock was now covering too much or too little, and its purpose remained unclear. If this patch is accepted, I would expect the project to eventually implement a separate, narrowly-scoped block storage lock to allow for a more parallelizable block processing routine. ACKs for top commit: stickies-v: re-ACK ec6cf49 janb84: re- ACK ec6cf49 pablomartin4btc: ACK ec6cf49 Tree-SHA512: e5942bc87300b0db9a0b91d5fe26dab455049e6cef7c96bb12b28141fa04711d46c6af105c0e1a83a9f261edde2c8b8b43ecf577a27d54b4610d784676a85627
The `nb30.h` Windows header defines `UNIQUE_NAME` as a macro. This introduces a fragile dependency on header inclusion order: if Windows headers happen to be included before `UNIQUE_NAME` is used, the preprocessor expands it into a numeric literal, causing syntax errors. Rename the macro to `BITCOIN_UNIQUE_NAME` to remove this fragility and avoid the collision entirely. -BEGIN VERIFY SCRIPT- sed -i 's/\<UNIQUE_NAME\>/BITCOIN_UNIQUE_NAME/g' $(git grep -l 'UNIQUE_NAME' ./src/) -END VERIFY SCRIPT-
…cast fail if privatebroadcast is not enabled 0bfc5e4 add release notes (Pol Espinasa) fdc9fc1 test: check getprivatebroadcast and abortprivatebroadcast throw if the node is running without -privatebroadcast set (Pol Espinasa) 7b821ef rpc: getprivatebroadcastinfo and abortprivatebroadcast throw if -privatebroadcast is disabled (Pol Espinasa) Pull request description: Makes `getprivatebroadcast` and `abortprivatebroadcast` throw if `-privatebroadcast=0`. This is motivated by: sparrowwallet/sparrow#1989 Knowing if `privatebroadcast` is set can be useful for some external software like Sparrow to avoid call `getprivatebroadcastinfo` each time to see if broadcast was done through that. ACKs for top commit: stickies-v: ACK 0bfc5e4 sedited: ACK 0bfc5e4 rkrux: code review ACK 0bfc5e4 andrewtoth: ACK 0bfc5e4 Tree-SHA512: 3bdb3909e93fc3835d801e1efc2bbec673a75a1ff089debd59e8970a0ff2b44d4e00b7ac26f10c972dcb50bf042521921370e1ec57885d67cd8459b3831da898
…id passing 21M BTC d0b76c7 rpc+bitcoin-tx: Specify correct type for ParseFixedPoint() (Hodlinator) 43ca54c refactor(test): Make CAmount arg explicit for BuildCreditingTransaction() (Hodlinator) b5e91e9 wallet: Remove CoinsResult::Clear() (Hodlinator) Pull request description: The *knapsack_solver_test* in *coinselector_tests.cpp* was accumulating satoshi amounts beyond 21M BTC. This was uncovered while experimenting with adding checks to `CAmount`. Fix that by fully resetting the `CoinsResult` object accumulating those amounts, inspired by #35449 (comment). Also, while we're at it, add 2 commits which correct some `int64_t`/`CAmount` confusion. Fixes #35449 ACKs for top commit: sedited: ACK d0b76c7 furszy: utACK d0b76c7 brunoerg: code review ACK d0b76c7 Tree-SHA512: 6d989ded6f6327dc657f437dc256d4adf42a34a1252621421ee38d7851c6cdc97a462f033a4728e3aa7d5514deee4db6e83646105633f9cf7ed6e7e90406b67d
…criptor_psbt 5b65e31 test: remove two unnecessary nodes from the test (rkrux) Pull request description: A discussion in the review of #35443 PR brought this test to my attention. The test needs multiple wallets that can be created on a single node, multiple nodes are not required. As there is a cost associated with setting-up and tearing-down nodes, this patch helps in reducing the test time as well. ACKs for top commit: ekzyis: ACK 5b65e31 polespinasa: lgtm ACK 5b65e31 sedited: ACK 5b65e31 Tree-SHA512: f6b4a96b9beee968ef5438fd9db582a48834ff36ba27c19dd7012902528fa713424212530e34cc16b58c19c023f1accd2b89fe846ef2cc36677c24e160c5b817
fba713a scripted-diff: Rename UNIQUE_NAME to BITCOIN_UNIQUE_NAME (Hennadii Stepanov) Pull request description: #34454 (comment): > ... it is annoying that we keep running into the same bug over and over again (IIRC it happened in the past at least once for Bitcoin Core). Surely this is going to happen again in the future. And here we go again. --- The `nb30.h` Windows header [defines](https://github.com/mingw-w64/mingw-w64/blob/b536c4fdb038a9c59a7e5fb36e7d1293c4dc61d6/mingw-w64-headers/include/nb30.h#L78) `UNIQUE_NAME` as a macro. This introduces a fragile dependency on header inclusion order: if Windows headers happen to be included before `UNIQUE_NAME` is used, the preprocessor expands it into a numeric literal, causing syntax errors. Rename the macro to `BITCOIN_UNIQUE_NAME` to remove this fragility and avoid the collision entirely. --- Noticed while doing a Guix build of the [QML repo](https://github.com/bitcoin-core/gui-qml) for Windows. Recent similar PRs: #34454 and #34868. ACKs for top commit: maflcko: lgtm ACK fba713a sedited: ACK fba713a w0xlt: ACK fba713a Tree-SHA512: 7a63b99a754e797eb8fa5d6a598606150f47ae1130d1d26067c509830e6575f0378ce63fe0ca35c69dce9a394451a34ddadd8b3d5f6f9a7e4c529108af546fb6
Document why we use LIBCXXABI_USE_LLVM_UNWINDER=OFF.
…oAll fa2afba p2p: Release m_peer_mutex early in InitiateTxBroadcastToAll (MarcoFalke) Pull request description: The `InitiateTxBroadcastToAll` method holds the `m_peer_mutex` while updating the bloom filters for all peers. This is perfectly fine, because updating the bloom filters is fast. Though, from a style-perspective, the lock does not need to be held for the whole function. Also, holding the lock longer, may confuse Tsan into a lock-order inversion false-positive (ref: #19303 (comment)). So "fix" both issues in this style-refactor. ACKs for top commit: xyzconstant: Code review ACK fa2afba shuv-amp: ACK fa2afba danielabrozzoni: Code Review ACK fa2afba sedited: ACK fa2afba Tree-SHA512: c47849a4c3cc11c74b61fec3425db8ec7f78db4ca43d7bf3145ce640f7b0872701c09495f0dfe77109d09d5716d920ad3d7308483fe41564c30867b3e80432e7
087f02c ci: skip libunwind runtime in LLVM build (fanquake) 6d47f7c ci: use llvm 22.1.7 (fanquake) Pull request description: Also document why we use `LIBCXXABI_USE_LLVM_UNWINDER=OFF`. Upstream issue is llvm/llvm-project#84348. ACKs for top commit: maflcko: lgtm ACK 087f02c sedited: ACK 087f02c Tree-SHA512: b93c798fd5a016cad40db9d24cb36cb72e531b284aee5458de41e062960514783e30c6f1413c0e62fa261758d783d0004a0973541cbb36bd34b77800c629bd7a
35a814a test: Limit clocks to one active instance (MarcoFalke) 55e402f scripted-diff: Rename NodeClockContext to FakeNodeClock (seduless) 1e9546f test: Use NodeClockContext in more call sites (seduless) 758fea5 test: Drop ++ from NodeClockContext default constructor (seduless) 7c2ec39 test: Enter mocktime before peer creation in block_relay_only_eviction (seduless) Pull request description: Follow-up to #34858 Updates remaining `SetMockTime` call sites that are clean, mechanical swaps fitting the spirit of the original PR (see: #34858 (review) and #34858 (comment)). Further updates to `SetMockTime` are more complex and deserve separate, isolated PRs. The default constructor for `NodeClockContext` increments to the next tick, which is a defensive measure to prevent time going backwards on construction. This has caused some confusion (see thread: #34858 (comment)) and can be safely removed after updating the only test where this is load-bearing (b3c9bd7) (see: #34858 (comment)). The removal also tightens the `addrman_tests/addrman_evictionworks` test to sit exactly on the `ADDRMAN_REPLACEMENT` boundary (4h), catching mutations such as: ```diff diff --git a/src/addrman.cpp b/src/addrman.cpp index d3dae59..d0929c62cb 100644 --- a/src/addrman.cpp +++ b/src/addrman.cpp @@ -920,3 +920,3 @@ void AddrManImpl::ResolveCollisions_() // Has successfully connected in last X hours - if (current_time - info_old.m_last_success < ADDRMAN_REPLACEMENT) { + if (current_time - info_old.m_last_success <= ADDRMAN_REPLACEMENT) { erase_collision = true; ``` The last follow-up item is updating `NodeClockContext` to `FakeNodeClock` to make it clear it is intended for testing (motivated by #34858 (review) and supported in #34858 (comment)). ACKs for top commit: maflcko: re-ACK 35a814a 🛒 sedited: ACK 35a814a Tree-SHA512: ade776e288a4b7bbc4c8855c14d61381b5b20329fe1e72fee87f773e47a9519975d58c277fbacda37dd73c0c1d4ce358c92dcdc4ca049d58cb3453ddf751b45b
This avoids implicit conversions from string literals to `std::string`, eliminating the need to include `<string>` everywhere the `BENCHMARK()` macro is used.
Currently, all test cases using TestChain100Setup or a derived class like BuildChainTestingSetup are using mocktime by default due to the SetMockTime call in the TestChain100Setup ctor. This is confusing, because test cases using mocktime explicitly seem to imply that before they set the mocktime, real time was used. E.g. index_reorg_crash claimed in a comment to "Enable mock time". Fix this issue by adding a FakeNodeClock m_clock field to TestChain100Setup. Then, use the m_clock instead of explicit calls to SetMockTime or to a (now) shadowing local FakeNodeClock variable.
The context is easier to reason about: E.g., * in TestBasicMining it allows to drop manual SetMockTime(0) calls, * in connections_desirable_service_flags it allows to drop manual calls to SetMockTime(GetTime<std::chrono::seconds>() + _n_) and replace them by operator+=(_n_) * in wallet_tests it clarifies that the mocktime does not persist outside the AddTx function
This is easier to reason about, because it will automatically take care of properly setting INITIAL_MOCK_TIME in the ctor. Also, it allows to drop the ElapseTime and replace it with a call to operator+=()
54de023 guix: add setup.sh (fanquake) Pull request description: This is the first change in #25573, which splits out the setup & tarball generation code from `build.sh`, so that it can be re-used, from multiple (future) build scripts. ACKs for top commit: willcl-ark: ACK 54de023 hebasto: ACK 54de023. Tree-SHA512: 9a7f2fe322d281b9867414511af5243f4dd659ea42637f4eb8cc0c8629c94dab842669bb7c503f9fa67cab3fac65561364f07b5c0fda8e6d8c24e7bf161025ef
1. FetchBlock runs in a http worker thread. It acquires a PeerRef, locks cs_main, then may later call BlockRequested which asserts that CNodeState exists for the peer. 2. FinalizeNode may run in either the bitcoind or b-net threads. It locks cs_main, fetches a PeerRef from RemovePeer, fetches a CNodeState, and later removes it from m_node_states. Because of the lock placement in FetchBlock, the http worker thread in 1) can acquire a valid PeerRef and block while the b-net thread in 2) is cleaning up the peer in FinalizeNode. When the worker thread later acquires cs_main, it may crash in BlockRequested since no CNodeState exists. Fix this by acquiring the lock earlier in FetchBlock. The lock can be replaced with a net-specific lock when the remaining CNodeState fields are moved to Peer.
Their clang version is pinned, so the only relevant change should be a more recent cmake, more recent C++ stdlib, and more recent Python version.
Now that the C++ std-lib was bumped in the CI task in the prior commit, some unused includes can (and must) be dropped.
…gle assert_equal with 3 args instead of multiple assert_equals
Co-authored-by: w0xlt <woltx@protonmail.com>
Co-authored-by: w0xlt <woltx@protonmail.com>
Co-authored-by: Daniel Pfeifer <daniel@pfeifer-mail.de>
8ebfff0 doc: add send RPC release note (Sjors Provoost) 5884f5a wallet: remove experimental warning from send RPCs (Sjors Provoost) Pull request description: The `send` RPC was introduced in v21 an initially marked experimental. The `sendall` RPC was added in v24, based on `send` and also marked experimental. I'm not aware of any proposed breaking changes, except #35433 which follows the regular deprecation flow. Time to mark them as no longer experimental. ACKs for top commit: w0xlt: ACK 8ebfff0 achow101: ACK 8ebfff0 polespinasa: ACK 8ebfff0 pablomartin4btc: ACK 8ebfff0 Tree-SHA512: beb5321adaf871157bda396c8e5740daff95ffe342416914340ae4197accebe60236032d1329876b42405437b99f59079a56ec1e5ac592b753031ba2ebd36cfb
The perf counter read buffer comment says the vectors start with three metadata slots, but brace initialization creates one element with value 3. Normal benchmark construction usually hides this because successful `monitor()` calls resize the buffers before `updateResults()`, while failed setup sets `mHasError` before the indexed reads. Initialize the vectors with the size constructor so a default-constructed `LinuxPerformanceCounters` object satisfies the `read_format` invariant instead of relying on that construction path. Reproducer: https://godbolt.org/z/scE8rMd8Y
df9eb72 test: ensure group data cluster pointers are live (Greg Sanders) Pull request description: Belt-and-suspenders check inside `SanityCheck` to avoid dangling pointers. ACKs for top commit: l0rinc: code review ACK df9eb72 marcofleon: lgtm ACK df9eb72 sipa: Cannot-hurt ACK df9eb72 sedited: ACK df9eb72 Tree-SHA512: a666b07a56401182aac156bf575ebe3e0a7fc89cd885ccb0b0e65d64da4df673be2b00cf1643a930da1cd65806924ecb6cd94cb18bcead5761aeae775fa5f2e3
1a3cfdf fuzz: connman: cover AddLocalServices/RemoveLocalServices (Bruno Garcia) c507fb3 fuzz: connman: add outbound-bytes invariants (Bruno Garcia) 4a6fce4 fuzz: connman: add AddNode/RemoveAddedNode invariants (Bruno Garcia) a5859ed fuzz: connman: set m_local_services/m_use_addrman_outgoing/m_max_automatic_connections (Bruno Garcia) 4b84c91 fuzz: connman: add network activity invariants (Bruno Garcia) Pull request description: This PR improves the `connman` fuzz target by replacing some "`(void)`" calls with actual invariant checks, adding coverage for previously uncovered methods, and exercising more initialization states. - Set `m_local_services`, `m_use_addrman_outgoing`, and `m_max_automatic_connections` via fuzzed values before `Init()` to explore more startup configurations. - Add network activity and outbound-bytes invariants. - Add `AddNode`/`RemoveAddedNode` invariants: e.g. a successful `AddNode` increases `GetAddedNodeInfo()` by one; adding the same node again must fail; a subsequent `RemoveAddedNode` must succeed and restore the original count. - Add coverage for `AddLocalServices`/`RemoveLocalServices`. ACKs for top commit: nervana21: re-ACK 1a3cfdf frankomosh: reACK 1a3cfdf . Change from the diff is the restoring `(void)connman.RemoveAddedNode(random_string)` arm sedited: ACK 1a3cfdf Tree-SHA512: c7b6799ca65d2e639d8ab9ab0cc77bae663f24fbda934446a8ee2e8ce9e8e36624d16b4f492b1714e2d67375edd35907cb9392d21f368d3d5298275ff1d05c72
…requirements fb8a103 doc: Clarify build docs about `pkgconf` / `pkg-config` requirements (Hennadii Stepanov) Pull request description: Since #34411, `pkgconf`/`pkg-config` is no longer strictly required. It is currently required for ZeroMQ on several major distributions and operating systems, including Debian, Ubuntu, the BSD derivatives, and Gentoo. Regarding QRencode, our [`FindQRencode.cmake`](https://github.com/bitcoin/bitcoin/blob/master/cmake/module/FindQRencode.cmake) module treats `pkgconf`/`pkg-config` as optional and can successfully locate the package without it. This PR amends #34411 and updates the build notes accordingly. Addresses #34411 (review). ACKs for top commit: purpleKarrot: ACK fb8a103 sedited: ACK fb8a103 Tree-SHA512: 0f740c954f058ce69fe936a51cfe1dc36dc374d392a94343d5c9f6b0be9565e4163c01015b00fef4e58eb180a8a520e6e81ea8b206868b3bcdf077caf0d24d65
The local `static constexpr auto ERR` shadowed the `Sock::ERR` static data member. Rename it to `accept_error`, per the Developer Notes' shadowing guidance. Co-authored-by: Hodlinator <172445034+hodlinator@users.noreply.github.com>
…y classes 8791c47 test: use ExtendedPrivateKey in wallet_taproot.py (rkrux) 89ceafa test: use ExtendedPrivateKey in wallet_listdescriptors.py (rkrux) bbfffca test: use ExtendedPrivateKey in wallet_send.py (rkrux) 2ab6e59 test: use ExtendedPrivateKey in wallet_keypool.py (rkrux) 9e20118 test: use ExtendedPrivateKey in wallet_fundrawtransaction.py (rkrux) 06af0cd test: use ExtendedPrivateKey in wallet_descriptor.py (rkrux) 4100fac test: use ExtendedPrivateKey in wallet_createwallet.py (rkrux) ff3f6de test: use ExtendedPrivateKey in wallet_bumpfee.py (rkrux) 003f2a0 test: use ExtendedPrivateKey in feature_notifications.py (rkrux) f988e6d test: use ExtendedPrivateKey in wallet_importdescriptors.py (rkrux) d2a03d5 test: add extendedkey.py unit tests by using BIP32 test vectors (rkrux) afdb378 test: introduce ExtendedPrivateKey and ExtendedPublicKey classes (rkrux) 4dbaa7c test: generalise byte_to_base58 utility function to allow more version types (rkrux) Pull request description: Many a times there has been a need to come up with dynamic xprvs and xpubs in the functional tests, but the lack of code that creates them dynamically has led to the presence of several hardcoded keys in the testing framework. This is not developer friendly and not self-documenting, clutters the testing code, and makes it difficult to update the tests in the future. This PR introduces two utility classes ExtendedPrivateKey and ExtendedPublicKey that allows the developer to create them on the fly to be used in the tests. I have intentionally not introduced any library for this purpose and have reused the existing libraries and functions in the framework. The implementation is supposed to provide basic functionality for creating xprv randomly or from a fixed seed, creating corresponding xpub, and deriving child xprvs and xpubs at custom derivation paths. I've updated many tests to show how these can be used, there are more tests as well that can be updated in the future to completely remove such non-deterministic hardcoded keys. ACKs for top commit: achow101: ACK 8791c47 w0xlt: ACK 8791c47 Tree-SHA512: f8ec4e09eaa6cc44b0f1c9a91337e570b12fb882c258be89b470de1a8cecf9d2fd40d9f02ee739dcbf639462ea7710aa145a3726f0f537f5a1f1e7772e5b019d
…ock instances 6fa4132 fuzz: share a single mocked steady clock across FuzzedSock instances (Hao Xu) Pull request description: This is a follow-up of #35478 (comment), inspired by maflcko . Each FuzzedSock used to own its mocked steady clock and call MockableSteadyClock::SetMockTime() directly. Hold the clock by reference to an externally provided SteadyClockContext instead, so that several FuzzedSock instances sharing a test case (e.g. one per peer, or one created via Accept()) advance a single mocked clock, and the mocking goes through the SteadyClockContext RAII helper that resets mocktime on destruction. SteadyClockContext is a LimitOne type, so each fuzz target constructs one instance per iteration and passes it to ConsumeSock / ConsumeNode / the FuzzedSock constructor. ACKs for top commit: maflcko: review ACK 6fa4132 🌕 marcofleon: crACK 6fa4132 Tree-SHA512: 3c773b5c0c3ba42a8245c9ea6042b0bc767df4fad506305f3c200310616b48a59deb1542086eb4ce3e8a1407c4d6b42cef3b37cd84bfe80d4821972b8d3b4286
The `ERR` macro is defined on illumos-based systems in the `regset.h`
header included by the Boost.Test framework, which causes a compilation
error.
-BEGIN VERIFY SCRIPT-
ren1() { sed -i "s/\<$1\>/$2/g" $( git grep -l "$1" ./src/util/sock.* ./src/httpserver.h ) ; }
ren1 RECV RecvEvent
ren1 SEND SendEvent
ren1 ERR ErrorEvent
ren2() { sed -i "s/\<$1\>/$2/g" $( git grep -l "$1" ./src/ ) ; }
ren2 Sock::RECV Sock::RecvEvent
ren2 Sock::SEND Sock::SendEvent
ren2 Sock::ERR Sock::ErrorEvent
-END VERIFY SCRIPT-
The `ERR` macro is defined on illumos-based systems in the `regset.h`
header included by the Boost.Test framework, which may cause a
compilation error.
-BEGIN VERIFY SCRIPT-
ren() { sed -i "s/\<$1\>/$2/g" $( git grep -l "$1" ./src/qt/psbtoperationsdialog.* ) ; }
ren StatusLevel::INFO StatusLevel::Info
ren INFO Info
ren StatusLevel::WARN StatusLevel::Warn
ren WARN Warn
ren StatusLevel::ERR StatusLevel::Error
ren ERR Error
-END VERIFY SCRIPT-
41ceea4 scripted-diff: Rename `StatusLevel::{INFO,WARN,ERR}` (Hennadii Stepanov) f395acd scripted-diff: Rename `Sock::{RECV,SEND,ERR}` (Hennadii Stepanov) 7ac25c9 util, refactor: Rename local `ERR` in `Sock::Accept` (Hennadii Stepanov) Pull request description: The `ERR` macro is [defined](https://github.com/illumos/illumos-gate/blob/10a869258e300c530ae56b29aa3bf43461ca98ff/usr/src/uts/intel/sys/regset.h#L101) on illumos-based systems in the `regset.h` header included by the Boost.Test framework, which causes a compilation error: - on [OmniOS](https://github.com/hebasto/bitcoin-core-nightly/actions/runs/28006958700): ``` [484/792] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/main.cpp.o FAILED: [code=1] src/test/CMakeFiles/test_bitcoin.dir/main.cpp.o /usr/bin/g++ -DBOOST_MULTI_INDEX_DISABLE_SERIALIZATION -DBOOST_NO_CXX98_FUNCTION_BASE -I/home/runner/work/bitcoin-core-nightly/bitcoin-core-nightly/build/src -I/home/runner/work/bitcoin-core-nightly/bitcoin-core-nightly/src -I/home/runner/work/bitcoin-core-nightly/bitcoin-core-nightly/src/univalue/include -I/home/runner/work/bitcoin-core-nightly/bitcoin-core-nightly/src/minisketch/include -I/home/runner/work/bitcoin-core-nightly/bitcoin-core-nightly/src/secp256k1/include -isystem /usr/gnu/include -pthread -O2 -g -std=c++20 -fno-extended-identifiers -fmacro-prefix-map=/home/runner/work/bitcoin-core-nightly/bitcoin-core-nightly/src=. -fstack-reuse=none -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=3 -Wstack-protector -fstack-protector-all -fcf-protection=full -fstack-clash-protection -Wall -Wextra -Wformat -Wformat-security -Wvla -Wredundant-decls -Wdate-time -Wduplicated-branches -Wduplicated-cond -Wlogical-op -Woverloaded-virtual -Wsuggest-override -Wimplicit-fallthrough -Wunreachable-code -Wbidi-chars=any -Wundef -Wno-unused-parameter -Werror -MD -MT src/test/CMakeFiles/test_bitcoin.dir/main.cpp.o -MF src/test/CMakeFiles/test_bitcoin.dir/main.cpp.o.d -o src/test/CMakeFiles/test_bitcoin.dir/main.cpp.o -c /home/runner/work/bitcoin-core-nightly/bitcoin-core-nightly/src/test/main.cpp In file included from /usr/include/sys/procfs_isa.h:39, from /usr/include/sys/procfs.h:66, from /usr/include/procfs.h:45, from /usr/local/include/boost/test/impl/debug.ipp:85, from /usr/local/include/boost/test/included/unit_test.hpp:20, from /home/runner/work/bitcoin-core-nightly/bitcoin-core-nightly/src/test/main.cpp:10: /home/runner/work/bitcoin-core-nightly/bitcoin-core-nightly/src/util/sock.h:162:28: error: expected unqualified-id before numeric constant 162 | static constexpr Event ERR = 0b100; | ^~~ ``` - on [OpenIndiana](https://github.com/hebasto/bitcoin-core-nightly/actions/runs/28006958536/job/82891036242): ``` [44](https://github.com/hebasto/bitcoin-core-nightly/actions/runs/28006958536/job/82891036242#step:7:545) [ 59%] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/main.cpp.o [ 59%] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/addrman_tests.cpp.o In file included from /usr/include/sys/procfs_isa.h:39, from /usr/include/sys/procfs.h:66, from /usr/include/procfs.h:45, from /usr/include/boost/test/impl/debug.ipp:85, from /usr/include/boost/test/included/unit_test.hpp:20, from /home/runner/work/bitcoin-core-nightly/bitcoin-core-nightly/src/test/main.cpp:10: /home/runner/work/bitcoin-core-nightly/bitcoin-core-nightly/src/util/sock.h:162:28: error: expected unqualified-id before numeric constant 162 | static constexpr Event ERR = 0b100; | ^~~ ``` --- `StatusLevel::{INFO,WARN,ERR}` has also been renamed to ensure future-proofing and consistency. ACKs for top commit: maflcko: review ACK 41ceea4 💭 sedited: ACK 41ceea4 hodlinator: re-ACK 41ceea4 Tree-SHA512: bf3c76468f9b0167e22356c140306a626bf4f769076c260931d35047724970dd2c97985bde2ef3ceb192d560710832b535ef9bba2ea133c0bfe4eb6b2e8e447f
…URCE bbbbab8 ci: Bump tsan config to ubuntu:26.04 with -U_FORTIFY_SOURCE (MarcoFalke) Pull request description: The `-U_FORTIFY_SOURCE` should be harmless in the Tsan CI, and is required to work around #30586 ACKs for top commit: fanquake: ACK bbbbab8 Tree-SHA512: 7c4b434342861fabc52eda7b551ebe08fcde6079cd39d54a21938c88dcfd80cdb876dd7de859bac1201a2b928163c19704c24d5b5a5e76f4935e33cb3e6ae2bd
829255c cmake: Remove `SelectLibraryConfigurations` from `FindQRencode` module (Hennadii Stepanov) 5c55606 depends: Remove unused `lib/pkgconfig` in `qrencode` package (Hennadii Stepanov) 402ba10 cmake: Drop optional `PkgConfig` use in `FindQRencode` module (Hennadii Stepanov) Pull request description: This PR addresses: - [this](#35602 (comment)) comment: > If it works without pkg-config, then we should remove the usage of pkgconfig entirely, rather than say it will work without it, and leave the dependency in the code? - and [this](#35602 (comment)) one: > While cleaning up this module, you may also get rid of `SelectLibraryConfigurations`. We don't want that `QRencode_LIBRARY` or `QRencode_LIBRARIES` is used anywhere; there is no need to set a variable with that name. ACKs for top commit: purpleKarrot: ACK 829255c Tree-SHA512: fd87a5afd7c0b3271a008df8adcb2ff809053799a0658fbb5a1f15ed11f6abad25836c8e1b7507d0d4c35c027dcbba88b136ac827e17b283ee4ffbbe55d6f4ee
2ee4faf test: add fuzz test for private broadcast (kevkevinpal) 08b7c61 private broadcast: enforce sending to unique node ids (Vasil Dimov) Pull request description: Add a fuzz test that exercises the public methods of the `PrivateBroadcast` class from `src/private_broadcast.h` and checks for correctness. ACKs for top commit: instagibbs: ACK 2ee4faf nervana21: re-ACK 2ee4faf frankomosh: Code Review ACK 2ee4faf. Tree-SHA512: 35f9efcf9e7ea8bd071f6b607fd7c901b60ccf14610f501e6cf06f0d05c389424ab8a0c25a2aaffd5755c8f920af4834e0979cae72a474680170de2b94d06e1c
a318f43 bitcoin-util: Add netmagic command (ekzyis) Pull request description: This adds a `netmagic` command to `bitcoin-util`. It will return the network magic bytes of the selected chain: ``` $ bitcoin-util netmagic f9beb4d9 $ bitcoin-util -regtest netmagic fabfb5da $ bitcoin-util -testnet4 netmagic 1c163f28 $ bitcoin-util -signet netmagic 0a03cf40 # default challenge $ bitcoin-util -signet -signetchallenge=512103ad5e0edad18cb1f0fc0d28a3d4f1f3e445640337489abb10404f2d1e086be430210359ef5021964fe22d6f8e05b2463c9540ce96883fe3b278760f048f5189f2e6c452ae netmagic 0a03cf40 $ bitcoin-util -signet -signetchallenge=51 netmagic 54d26fbd ``` This will be particularly useful for #34566 to determine the datadir path of a custom signet, before starting bitcoind, since #34566 will add the network magic as a suffix. This was mentioned in #34566 (comment). It uses the same code from init.cpp to print the signet derived magic: https://github.com/bitcoin/bitcoin/blob/ea9afb61a1c52979d0e64e4dae9b3a611f7b9a5e/src/init.cpp#L967-L969 Since it does not depend on #34566, and the changes are quite simple, I created a separate PR for this for easier review and discussion. I have tested this by invoking the command with the options above. ACKs for top commit: stickies-v: ACK a318f43 sedited: ACK a318f43 Tree-SHA512: bcb5a1b14260e13a35f5682e4dac735f472ab6ff7781e7e77aef5895ddbecc03a2fc72a26419ba9a28354736a1eaaeddb047bafe276440e339f17ddfe7841862
a0e5e30 fuzz: restore CreateSock in PCP targets (Hao Xu) Pull request description: #35536 (comment) Restore CreateSock in PCP targets to avoid dangling references. ACKs for top commit: marcofleon: ACK a0e5e30 sedited: ACK a0e5e30 Tree-SHA512: 81c04bf0adbceec8cd4ac430fc26c356bb87b5aba10af3bf20301b12256eb5fb63b7e90a7d80579a94a3e00c51622f653eda20d1ac21c8ec3845ac1524dc723b
3765b42 logging: More fully remove libevent log category (Ryan Ofsky) Pull request description: Libevent log category was partially removed in 39e9099, and this commit extends that with the following changes: - Stops showing libevent in the list of supported log categories in `bitcoind -help` and `bitcoin-cli help logging` output. - Stops returning `"libevent": false` in `logging` RPC output. It's not good to treat libevent as a supported log category when it can't be enabled and trying to enable it results in warnings. There's also no need to define an unused LIBEVENT constant value and keep more complicated logic for dealing with deprecated log categories, so this change also simplifies code internally. ACKs for top commit: l0rinc: code review ACK 3765b42 pinheadmz: ACK 3765b42 sedited: ACK 3765b42 Tree-SHA512: 09e9514f905bb0a79d870689af491886baaa31fa19f2ad6aef4283fa20c2fa6ce8d384178139227aeeabffabff6e83d254114daaeefbbfe2c6172b9da8871298
d64ea15 ci: add openBSD cross CI job (fanquake) 5404b62 depends: add openbsd_LDFLAGS (fanquake) Pull request description: This adds a Linux cross job for OpenBSD; similar to #34491 (FreeBSD). ACKs for top commit: hebasto: ACK d64ea15. willcl-ark: ACK d64ea15 Tree-SHA512: 0353c0ae8dd49c861a9100ebd5044fc39227c29859ca68e6454ebdf469c90ff5471e29733613606ee8ba72037f7fca5086275f794e90864a6456ffee73d9113b
b6b1d06 nanobench: fix perf counter buffer init (Lőrinc) Pull request description: **Problem:** The Linux perf counter buffers are documented as starting with three `read_format` metadata slots, but `std::vector<uint64_t>{3}` creates a one-element vector containing `3`. In normal benchmark runs this is usually hidden because successful `monitor()` calls resize the buffers before `updateResults()`, while failed setup sets `mHasError` before the indexed reads. **Fix:** Use the vector size constructor so the buffers start with three zero-initialized metadata slots. This should not change the usual successful benchmark path, but it makes the default object state match the code's indexing assumptions. **Reproducer:** https://godbolt.org/z/scE8rMd8Y **Upstream:** martinus/nanobench#138 ACKs for top commit: dergoegge: Code review ACK b6b1d06 sedited: ACK b6b1d06 Tree-SHA512: f3055e3eaf567e46e99c02497563249a292162606945365a277beacfd95086b5645f67396bfa4f1e08ca109ddd48a6feef3ead6d59b8de8ae898607493ee13ca
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
20 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.