update bitcoin to 0.18.1 #1
Open
Conversation
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
The `py310-zmq` binary package is not available by default on NetBSD 10.1. It has been updated to `py313-zmq`, and the `python310` package is updated accordingly. See: https://ftp.netbsd.org/pub/pkgsrc/current/pkgsrc/index-all.html.
… property 0dd8d5c cmake: Specify Windows plugin path in `test_bitcoin-qt` property (Hennadii Stepanov) Pull request description: This PR simplifies testing on Windows by removing the need to set the `QT_PLUGIN_PATH` environment variable for different build configurations. For example, the paths might otherwise be: - `C:/Users/hebasto/dev/bitcoin/build/vcpkg_installed/x64-windows/Qt6/plugins/` for "Release" - `C:/Users/hebasto/dev/bitcoin/build/vcpkg_installed/x64-windows/debug/Qt6/plugins/` for "Debug" ACKs for top commit: purpleKarrot: ACK 0dd8d5c Tree-SHA512: 0418b8fa4d74ca500aae9e36e56ebcefb566d2ac04a9d22e17d309400ad38dd5a6e75f0195c680796b761fb145444c33336b64180f15c6b1125fe190d58396b6
c29eaee doc: Update NetBSD Build Guide (Hennadii Stepanov) Pull request description: The `py310-zmq` binary package is not available by default on NetBSD 10.1. It has been updated to `py313-zmq`, and the `python310` package is updated accordingly. See: https://ftp.netbsd.org/pub/pkgsrc/current/pkgsrc/index-all.html. ACKs for top commit: fanquake: ACK c29eaee Tree-SHA512: 6924a974d6ed494609c789cc3f28cf173af3a37b940520ad7b169eff87e30af8346fec07e46f8bffe14a329060c41ac46d46b91910a00994cdf8a7ace8391d1c
96963b8 depends: static libxcb (fanquake) ad06843 depends: avoid qdbusviewer in Qt build (fanquake) 6848ed5 depends: apply Qt patches to fix static libxcb use (fanquake) 5f1b016 depends: static libxcb-util-image (fanquake) 98a2fbb depends: static libxkbcommon (fanquake) 1412baf depends: static libxcb-util-wm (fanquake) a4009da depends: static libxcb-keysyms (fanquake) bcfb867 depends: static libxcb-render-util (fanquake) Pull request description: Related to #33434. Tested on: * Fedora 42: #33537 (review). * Ubuntu 24.04: #33537 (comment). * Debian 13.x: #33537 (comment). ACKs for top commit: hebasto: re-ACK 96963b8, rebased, addressed my comments and adjusted formatting in `symbol-check.py` since my recent [review](#33537 (review)). willcl-ark: utACK 96963b8 TheCharlatan: ACK 96963b8 Tree-SHA512: e947bc5b5cb0ec97963bc3f451f8fa6afb2e3699435370798d7a2aaefea7445cbe031d3b642f946f936829fa4cbe4efd2bfacd6b15739da15c3596cc4776b362
…TH settings 2594d5a build: Remove CMAKE_SKIP_BUILD_RPATH and SKIP_BUILD_RPATH settings (Henry Romp) Pull request description: Remove CMAKE_SKIP_BUILD_RPATH and SKIP_BUILD_RPATH settings that are no longer needed after reordering the Guix build script to perform binary checks after installation. This PR also removes the unused CMake maintenance targets (`check-security` and `check-symbols`) and updates the Guix security checks to include binaries in the `libexec/` directory (added in PR #31679). ACKs for top commit: purpleKarrot: ACK 2594d5a hebasto: ACK 2594d5a. Tree-SHA512: ed451a298f5aae05c177b0033b092faaa7536caeaa3d84da9b8b611e2aa905e1dd337e57aef0efd69ce6ce6ac0cf77dc57adf175079b95bf53dd96d5d0c8118b
fa95353 ci: Run macos tasks in a git archive, not git checkout (MarcoFalke) faf99ae refactor: Avoid -W*-whitespace in git archive (MarcoFalke) Pull request description: Otherwise, compilation with GCC-15+ will warn about it: ``` src/clientversion.cpp:33:79: error: trailing whitespace [-Werror=trailing-whitespace=] 33 | //! git will put "#define GIT_COMMIT_ID ..." on the next line inside archives. ``` Follow-up to #32482 (comment) Can be tested via `git archive --output=/tmp/a.tar HEAD` ACKs for top commit: fanquake: ACK fa95353 Tree-SHA512: 73940ffc0fd83db557275bd5e993a3c47c5397682a1188447c48e077ead597ba0fc3e5ef9da7b746746ff04a26022ce35ac10768888bbd4707f25b799af43e45
fae3618 ci: Annotate all check runs with the pull request number (MarcoFalke) faf05d6 ci: Retry lint image building once after failure (MarcoFalke) fac4f6d ci: Rewrite lint task Bash snippet to Python (MarcoFalke) fa0d37a ci: Rewrite Bash to check inputs to Python (MarcoFalke) Pull request description: This contains a few follow-ups to #33744: * Rewrite the actions Bash snippet to Python. I've confirmed it still works in https://github.com/maflcko/bitcoin-core-with-ci/actions/runs/19067932430 (scroll down). * Add a lint-build retry to avoid issues such as #33640 for the lint task as well. * Finally, run the `debug_pull_request_number_str` annotation on all checks, to ensure they are present even when GitHub deletes annotations on a re-run. For example, the initial attempt https://github.com/bitcoin/bitcoin/actions/runs/19041534107/attempts/1?pr=33772 has the annotations, and the lint re-run has them removed: https://github.com/bitcoin/bitcoin/actions/runs/19041534107?pr=33772 ACKs for top commit: m3dwards: ACK fae3618 willcl-ark: ACK fae3618 Tree-SHA512: 6db147ccee622b7a640703f7e916ea662a8e42978f633046f22f8540017196250ef7771b28cd6e502368f1f3fe52b7524de0a3443f25c9659f524b4c9286ad0d
fadb4f6 test: Remove tests violating hardened std::span (MarcoFalke) Pull request description: Also, add a test for creating a CScript from an empty byte vector. To test: `rm -rf ./bld-cmake && cmake -B ./bld-cmake -DCMAKE_C_COMPILER='clang' -DCMAKE_CXX_COMPILER='clang++;-stdlib=libc++;-D_LIBCPP_HARDENING_MODE=_LIBCPP_HARDENING_MODE_DEBUG' -DBUILD_GUI=OFF -DBUILD_FUZZ_BINARY=OFF -DBUILD_BENCH=OFF -DBUILD_KERNEL_LIB=ON -DENABLE_WALLET=OFF -DENABLE_IPC=OFF && cmake --build ./bld-cmake --parallel $( nproc ) && valgrind --tool=none ./bld-cmake/bin/test_kernel --catch_system_error=no` Before: ``` /cxx_build/include/c++/v1/span:451: libc++ Hardening assertion __count == 0 || std::to_address(__first) != nullptr failed: passed nullptr with non-zero length in span's constructor (iterator, len) ``` After: (Passes) ACKs for top commit: TheCharlatan: ACK fadb4f6 stickies-v: ACK fadb4f6 Tree-SHA512: 47c2ee975b82978bbb226b47cde337dce5a7e25bc1d70c31f34b9a9ff38477609764c267e47ac5fd71a578fb2b2b135c698bb02dae1777a87bcc4079dcd278ef
Moving the python code out of the yaml string makes it easier to lint, format, and edit. This can be reviewed with the git options: --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space
…on flows d31158d psbt: clarify PSBT, PSBTInput, PSBTOutput unserialization flows (rkrux) Pull request description: The unserialization flows of the PSBT types work based on few underlying assumptions of functions from `serialize.h` & `stream.h` that takes some to understand when read the first time. Add few comments that highlight these assumptions hopefully making it easier to grasp. Also, mention key/value format types as per BIP 174. ACKs for top commit: achow101: ACK d31158d theStack: ACK d31158d Tree-SHA512: 45111ef7f0258ebbc41d058b3ef2a72472774ab2878caf2d71d7b57b27549c46a51ccbeda5fe164bcf4f7ec10627bbae6e7763aa80b1e66912703a2088682817
0aebdac init: completely remove `-maxorphantx` option (Sebastian Falbesoner) Pull request description: This is a small follow-up for #32941 (commit 1384dba), removing the `-maxorphantx` option completely, now that v30 has been released. If removing it for v31 is seen as controversial/premature (I personally don't think it is), the merge can be delayed for a future release. ACKs for top commit: maflcko: lgtm ACK 0aebdac achow101: ACK 0aebdac w0xlt: ACK 0aebdac rkrux: lgtm ACK 0aebdac stickies-v: ACK 0aebdac Tree-SHA512: 818633b903174387ae259acb1d1e8ce07f78e158de2c150742ef0950b0f5d62af553e4e35ab962432306e04e07c45b1be11dbae459a8b62c4b9a6b5ef1746d26
…onnman::GetAddresses()` 4d893c0 net: Remove unused `local_socket_bytes` variable in `CConnman::GetAddresses()` (WakeTrainDev) Pull request description: The local_socket_bytes variable was never used. Removed it to clean up dead code. ACKs for top commit: mzumsande: ACK 4d893c0 theStack: ACK 4d893c0 Tree-SHA512: f423bcf975aa2602464fcb96db323cbd6007a7491ddbe119f1d20e890c883dd351a55976151c5d25f5d26267b0efe1f0836fbd65e540c920dac931ed8d67846a
Would hopefully have helped me in this case: #31176 (comment) Since then however, fd813bf also made sure a minimal environment is used.
Should help in cases such as: #31144 (comment)
Effectively this is treating all transactions in txgraph as being in a cluster of size 1.
Preparatory commit to the rbf functional test, before changes are made to the rbf rules as part of cluster mempool.
Include an adjustment to mempool_tests.cpp due to the additional memory used by txgraph. Includes a temporary change to the mempool_ephemeral_dust.py functional test, due to validation checks being reordered. This change will revert once the RBF rules are changed in a later commit.
Rather than evicting the transactions with the lowest descendant feerate, instead evict transactions that have the lowest chunk feerate. Once mining is implemented based on choosing transactions with highest chunk feerate (see next commit), mining and eviction will be opposites, so that we will evict the transactions that would be mined last.
After cluster mempool, the mini_miner will no longer match the miner's block construction. Eventually mini_miner should be reworked to directly use linearizations done in the mempool.
Co-Authored-By: Gregory Sanders <gsanders87@gmail.com>
fa6db79 test: Avoid shutdown race in NetworkThread (MarcoFalke) Pull request description: Locally, I am seeing rare intermittent exceptions in the network thread: ``` stderr: Exception in thread NetworkThread: Traceback (most recent call last): File "/python3.10/threading.py", line 1016, in _bootstrap_inner self.run() File "./test/functional/test_framework/p2p.py", line 744, in run self.network_event_loop.run_forever() File "/python3.10/asyncio/base_events.py", line 603, in run_forever self._run_once() File "/python3.10/asyncio/base_events.py", line 1871, in _run_once event_list = self._selector.select(timeout) AttributeError: 'NoneType' object has no attribute 'select' ``` I can reproduce this intermittently via `while ./bld-cmake/test/functional/test_runner.py $(for i in {1..400}; do echo -n "tool_rpcauth "; done) -j 400 ; do true ; done`. I suspect this is a race where the shutdown starts the close of the network thread while it is starting. A different exception showing this race can be reproduced via: ```diff diff --git a/test/functional/test_framework/p2p.py b/test/functional/test_framework/p2p.py index 610aa4c..64561e157c 100755 --- a/test/functional/test_framework/p2p.py +++ b/test/functional/test_framework/p2p.py @@ -741,6 +741,7 @@ class NetworkThread(threading.Thread): def run(self): """Start the network thread.""" + import time;time.sleep(.1) self.network_event_loop.run_forever() def close(self, *, timeout=10): ``` It is trivial to reproduce via any test (e.g. `./bld-cmake/test/functional/tool_rpcauth.py`) and shows a similar traceback to the one above: ``` Exception in thread NetworkThread: Traceback (most recent call last): File "/python3.10/threading.py", line 1016, in _bootstrap_inner self.run() File "./test/functional/test_framework/p2p.py", line 745, in run self.network_event_loop.run_forever() File "/python3.10/asyncio/base_events.py", line 591, in run_forever self._check_closed() File "/python3.10/asyncio/base_events.py", line 515, in _check_closed raise RuntimeError('Event loop is closed') RuntimeError: Event loop is closed ``` So fix the second runtime error in hope of fixing the first one as well. ACKs for top commit: brunoerg: code review ACK fa6db79 Tree-SHA512: ca352ebf7929456ea2bbfcfe4f726adcbfcfb3dc0edeaddae7f6926f998888f0bd8b987ddef60308266eeab6bffa7ebdc32f5908db9de5404df95635dae4a8f6
e9536fa contrib: fix manpage generation (fanquake) Pull request description: 0972f55 from #33229 broke manpage generation, because the assumption that the last word in the line containing the version number, was the version number, no-longer holds for some binaries. i.e `bitcoind`. ACKs for top commit: janb84: re ACK e9536fa rkrux: re-ACK e9536fa Tree-SHA512: 471b1800beeec3ea70d722ac2dcc26073805c8fcdf0418ceb79728cc001eb7c2f11a3d832b54a7ae68d26fe5c97934a9c87eedae7601515857e660fac7532c0a
…::Init() facd01e refactor: remove redundant locator cleanup in BaseIndex::Init() (Hao Xu) Pull request description: Leverage locator.IsNull() to simplify ReadBestBlock() and remove unnecessary SetNull(). ACKs for top commit: l0rinc: ACK facd01e furszy: utACK facd01e sedited: ACK facd01e Tree-SHA512: ca418a3d4c72b9bfdb194e03d5acfacf7b567850ab385ad3a1ec6d070eedef4aea2bb8335f8bfe0316e84c22583111fc202f7603044c5d4d05754db23f75b47f
…EADME.md` ec8eb01 doc: Add `x86_64-w64-mingw32ucrt` triplet to `depends/README.md` (Hennadii Stepanov) 48496ca ci: Remove redundant `DEP_OPTS` from “Windows-cross UCRT” job (Hennadii Stepanov) Pull request description: This PR is part of the ongoing effort to migrate to the modern UCRT runtime for cross-compiled Windows binaries, including release builds. For more details about this migration, see: - #30210 - #33593 - #33764 Can be tested on the following systems: - Debian Trixie x86_64 (requires the [`g++-mingw-w64-ucrt64`](https://packages.debian.org/trixie/g++-mingw-w64-ucrt64) package, as documented). - Fedora 42 or 43 (requires the [`ucrt64-gcc-c++`](https://packages.fedoraproject.org/pkgs/mingw-gcc/ucrt64-gcc-c++/) package). Also see related upstream issues: - https://bugs.launchpad.net/ubuntu/+source/gcc-mingw-w64/+bug/2132114 - https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1121403 ACKs for top commit: m3dwards: ACK ec8eb01 hodlinator: ACK ec8eb01 Tree-SHA512: 26e6e0354fedd8cc79d186b519124c306a7de328ced7247804c6e6d546e9c71cd5b92af878ab0f9a3c1e506a339944acaeb0ac99cd56bed896fd77b952788ae0
…stors dcd42d6 [test] wallet send 3 generation TRUC (glozow) e753fad [wallet] never try to spend from unconfirmed TRUC that already has ancestors (glozow) Pull request description: Addresses #33368 (comment) There is not an explicit check that the to-be-created wallet transaction would be within the {TRUC, normal} ancestor limits. This means that the wallet may create a transaction that violates these limits, but fail to broadcast it in `CommitTransaction`. This appears to be expected behavior for the normal ancestor limits (and any other situation in which the wallet creates a tx that was rejected by mempool) and AFAIK the transaction will be rebroadcast at some point after the ancestors confirm. https://github.com/bitcoin/bitcoin/blob/1ed00a0d39d5190d8ad88a0dd705a09b56d987aa/test/functional/wallet_basic.py#L502-L506 It's a bit complex to address this for the normal ancestor limit, and probably unrealistic for the wallet to check all possible mempool policies in coin selection, but it's quite trivial for TRUC: just skip any unconfirmed UTXOs that have any ancestors. I think it would be much more helpful to the user to say there are insufficient funds. ACKs for top commit: achow101: ACK dcd42d6 monlovesmango: ACK dcd42d6 rkrux: lgtm ACK dcd42d6 Tree-SHA512: b4cf9685bf0593c356dc0d6644835d53e3d7089f42b65f647795257dc7f5dac90c5ee493b41ee30a1c1beb880a859db8e049d3c64a43d5ca9b3e6482ff6bddd5
866bbb9 cmake, test: Improve locality of `bitcoin_ipc_test` library description (Hennadii Stepanov) ae2e438 cmake: Move IPC tests to `ipc/test` (Hennadii Stepanov) Pull request description: This PR follows up on #33445 and: 1. Organizes the IPC tests in the same way as the wallet tests. 2. Removes no longer needed `src/test/.clang-tidy.in`. See the previous discussion: - #33445 (comment) - #33445 (review) Additionally, the locality of the `bitcoin_ipc_test` build target description has been improved. ACKs for top commit: Sjors: ACK 866bbb9 janb84: ACK 866bbb9 ryanofsky: Code review ACK 866bbb9, just adding back the suggested comment, and also fixing bad include arguments passed to target_capnp_sources. It would probably be a little better if the include fix was done in an earlier commit, since it's not really related to the other changes in the last commit, but would also be ok to make both changes at the same time. Tree-SHA512: ed7cc817ccb88595d8516978bff0ea2560048d35b3f548e7913aec7d58b8d6ac550e230e992c527fb747bef175580be92dc4df6342e4485f3a9870dba0a25cba
…nodes.us b0c7067 Remove unreliable seed from chainparams.cpp, and the associated README (SatsAndSports) Pull request description: The DNS seed `dnsseed.bitcoin.dashjr-list-of-p2p-nodes.us.` is not returning a representative sample of bitcoin nodes. It currently returns nothing later than 28.1.0, breaching the policy. This PR removes that seed from the list of DNS seeds ### Rationale The [policy for seeds](https://github.com/bitcoin/bitcoin/blob/master/doc/dnsseed-policy.md) includes this: > The DNS seed results must consist exclusively of fairly selected and functioning Bitcoin nodes from the public network A number of comments below, in response to this PR, include apparent breaches of this policy: [1](#33723 (comment)) [2](#33723 (comment)), [3](#33723 (comment)), in particular the first linked comment ([1](#33723 (comment))) comparing the distribution at this seed to other seeds. This seed is not including anything later than 28.2.0, breaching this policy. To ensure the policy is followed, and the seeds include a representative sample of Bitcoin nodes, this PR removes this seed from the list ### Data I ran this: ``` # Get some ip address from that seed: # Repeated multiple times, to get many different IPs: dig +short dnsseed.bitcoin.dashjr-list-of-p2p-nodes.us >> dnsseed.bitcoin.dashjr-list-of-p2p-nodes.us # For each distinct ip gathered from the seed, get basic info about the node, including it's User Agent string: cat dnsseed.bitcoin.dashjr-list-of-p2p-nodes.us | sort -u | while read ip; do echo ===; echo $ip; nmap -p 8333 --script bitcoin-info "$ip"; done > seed_versions.txt ``` and then summarized the agents with `egrep 'User Agent' seed_versions.txt | sort | uniq -c` and got: ``` 1 User Agent: /Satoshi:22.0.0/ 1 User Agent: /Satoshi:22.1.0/ 5 User Agent: /Satoshi:24.0.1/ 1 User Agent: /Satoshi:25.1.0/ 30 User Agent: /Satoshi:27.0.0/ 1 User Agent: /Satoshi:27.1.0/ 1 User Agent: /Satoshi:27.1.0/Knots:20240801/ 1 User Agent: /Satoshi:28.0.0/ 7 User Agent: /Satoshi:28.1.0/ 2 User Agent: /Satoshi:28.1.0/Knots:20250305/ ``` ACKs for top commit: l0rinc: reACK b0c7067 delta1: reACK b0c7067 Crypt-iQ: crACK b0c7067 laanwj: ACK b0c7067 murchandamus: ACK b0c7067 RandyMcMillan: ACK b0c7067 wiz: ACK b0c7067 dergoegge: ACK b0c7067 stickies-v: re-ACK b0c7067 mzumsande: ACK b0c7067 instagibbs: ACK b0c7067 Tree-SHA512: 7230b8dd24560ce6f8247e2e82ae7846ded8b91e230c59cc3643da3f5b9c12b5f025c1bb14490c19ca55f3794e81ce08106b31b3bf883d5c2dced05017123ac4
This refactor does not change behavior. -BEGIN VERIFY SCRIPT- sed --in-place 's/\<LogPrintf\>/LogInfo/g' \ $( git grep -l '\<LogPrintf\>' -- ./contrib/ ./src/ ./test/ ':(exclude)src/logging.h' ) -END VERIFY SCRIPT-
167df7a net: fix use-after-free with v2->v1 reconnection logic (Eugene Siegel) Pull request description: `CConnman::Stop()` resets `semOutbound`, yet `m_reconnections` is not cleared in `Stop`. Each `ReconnectionInfo` contains a `grant` member that points to the memory that `semOutbound` pointed to and `~CConnman` will attempt to access the grant field (memory that was already freed) when destroying `m_reconnections`. Fix this by calling `m_reconnections.clear()` in `CConnman::Stop()` and add appropriate annotations. I was able to reproduce the original issue #33615 with the following diff by randomly stopping my node while it was attempting to reconnect (and verified that this patch fixes the issue, at least in my ~40-50 runs): <details> <summary> diff </summary> ```diff diff --git a/src/net.cpp b/src/net.cpp index ef1c630..9c1d161d8b 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -1918,8 +1918,8 @@ void CConnman::DisconnectNodes() { LOCK(m_nodes_mutex); - const bool network_active{fNetworkActive}; - if (!network_active) { +// const bool network_active{fNetworkActive}; +// if (!network_active) { // Disconnect any connected nodes for (CNode* pnode : m_nodes) { if (!pnode->fDisconnect) { @@ -1927,7 +1927,7 @@ void CConnman::DisconnectNodes() pnode->fDisconnect = true; } } - } +// } // Disconnect unused nodes std::vector<CNode*> nodes_copy = m_nodes; @@ -1941,7 +1941,7 @@ void CConnman::DisconnectNodes() // Add to reconnection list if appropriate. We don't reconnect right here, because // the creation of a connection is a blocking operation (up to several seconds), // and we don't want to hold up the socket handler thread for that long. - if (network_active && pnode->m_transport->ShouldReconnectV1()) { + if (true) { reconnections_to_add.push_back({ .addr_connect = pnode->addr, .grant = std::move(pnode->grantOutbound), ``` </details> I'm curious to see if others can reproduce as well. ACKs for top commit: dergoegge: Code review ACK 167df7a darosior: utACK 167df7a mzumsande: ACK 167df7a Tree-SHA512: 33fdfb110a7cdae182b5cd5400eea8a271308a62dd56491e0aef8865eff24a9ea908be74e4e2e2ee00ac1cb698e46f270f56dffffe34cf2cfd79e9b1079d6531
…in MerkleComputation ffcae82 test: exercise TransactionMerklePath with empty block; targets the MerkleComputation empty-leaves path that was only reached by fuzz tests (frankomosh) Pull request description: As noted in [#32243 (comment)](#32243 (comment)), the early return inside `MerkleComputation` when `leaves.size() == 0` was only exercised by fuzz tests. The existing `merkle_test_empty_block` calls `BlockMerkleRoot`, which uses `ComputeMerkleRoot`, but does not exercise the `TransactionMerklePath` → `ComputeMerklePath` → `MerkleComputation` code path. Coverage before adding test: <img width="2459" height="66" alt="before" src="https://github.com/user-attachments/assets/ca94015a-d7c2-4281-ac60-13b22f177b67" /> Coverage after adding test: <img width="2459" height="66" alt="after" src="https://github.com/user-attachments/assets/b1d4e1bb-af72-46ab-8898-f18db39dd2fb" /> ACKs for top commit: kevkevinpal: ACK [ffcae82](ffcae82) maflcko: lgtm ACK ffcae82 brunoerg: code review ACK ffcae82 sedited: ACK ffcae82 Tree-SHA512: d2499d91269c4f4f9a86011f7ad13f675834662a5bd37b0e7cbe887a7d9acf4170e53f0bdc528011fc82866b9c1dec34f4e7e9cd64cc3100591c1580a4df5d00
`ParseByteUnits()` is the only parsing function in `strencodings.cpp` lacking a fuzz test. Add a test case to check the function against arbitrary strings and randomized default_multiplier's.
…mary fd4ce55 contrib: Count entry differences in asmap-tool diff summary (Fabian Jahr) Pull request description: Currently the output of `asmap-tool.py diff` returns the total number of addresses that has changed at the end of the list. Example output currently: ``` 2602:feda:c0::/48 AS1029 # was AS43126 2604:7c00:100::/40 AS29802 # was AS40244 # 0 IPv4 addresses changed; 79552154633921058212365205504 (2^96.01) IPv6 addresses changed ``` This is good indicator but in case of a longer list I would like the number of changed entries as well, since that is an easier number to parse and for debugging of certain issues also the more relevant value. This PR adds the count of changed entries to this summary output at the end. There as also a bit more structure so it's easier to parse as well. Example new output: ``` 2602:feda:c0::/48 AS1029 # was AS43126 2604:7c00:100::/40 AS29802 # was AS40244 # Summary IPv4: 0 entries with 0 addresses changed IPv6: 12 entries with 79552154633921058212365205504 (2^96.01) addresses changed ``` ACKs for top commit: jurraca: utACK [`fd4ce55121`](fd4ce55) janb84: utACK fd4ce55 hodlinator: ACK fd4ce55 Tree-SHA512: 97cc543eaba80a33f0291b20630411bda869d3b8d1b35ed7f36792064cb1edccc8fe4740b7229b5451a88b7bd8d68c42f96829ce4255ecac3e29d70b68061608
710031e Revert "guix: sqlite wants tcl" (Hennadii Stepanov) 4cf5ea6 depends: Propagate native C compiler to `sqlite` package (Hennadii Stepanov) Pull request description: This PR: 1. Ensures that autosetup can build the local bootstrap `jimsh0` when neither `jimsh` nor `tclsh` is available on the system. 2. Removes the `tcl` package from the Guix manifest. This is an alternative to #33975. ACKs for top commit: fanquake: ACK 710031e sedited: ACK 710031e Tree-SHA512: bdaa29af977799669bfc2aa3a8d0a4a688263b99c5f06b1582fbefb71ef77be0ee6223903e8357e51a9e0a7744807174b94262c2f4a3afd9f39737b61b00863e
This requires some small refactors to silence false-positive warnings. Also, expand the bugprone-unused-return-value.CheckedReturnTypes option to include util::Result, and util::Expected.
57b888c fuzz: Add a test case for `ParseByteUnits()` (Chandra Pratap) Pull request description: `ParseByteUnits()` is the only parsing function in `strencodings.cpp` lacking a fuzz test. Add a test case to check the function against arbitrary strings and randomized `default_multiplier`. ACKs for top commit: maflcko: lgtm ACK 57b888c dergoegge: utACK 57b888c marcofleon: crACK 57b888c Tree-SHA512: c16557442987437e5e0c9d9a8b016df93e513e34acb78242a1f73dabc4482632ec57eb35cb4c84f9a1ea838fa6bda2094f2a8b52ace431f8064a79fad96e9a52
fa4395d refactor: Remove unused LogPrintf (MarcoFalke) fa05181 scripted-diff: LogPrintf -> LogInfo (MarcoFalke) Pull request description: `LogPrintf` has many issues: * It does not mention the log severity (info). * It is a deprecated alias for `LogInfo`, according to the dev notes. * It wastes review cycles, because reviewers sometimes point out that it is deprecated. * It makes the code inconsistent, when both versions of the alias are used. Fix all issues by removing the deprecated alias. ACKs for top commit: ajtowns: ACK fa4395d stickies-v: ACK fa4395d rkrux: lgtm ACK fa4395d Tree-SHA512: de95d56df27b9ee33548cc7ee7595e2d253474094473089ee67787ddb171384383c683142672c3e2c1984e19eee629b2c469dc85713640a73391610581edbdbe
7b90b4f guix: reduce allowed exported symbols (fanquake) Pull request description: Need to double-check, but pretty sure this is atleast partly from #33181. ACKs for top commit: sedited: Nice, ACK 7b90b4f Tree-SHA512: 538c03dc32aab9b3e18100e8ffa0d664aea5ceba6aafee9e8e0894c2d02eea3b3fb09733cf7b5bd0aefb6b56d0ac3b92f28da932e135b23f55404efd8f43664a
…n-qt` runtime libs 41e657a guix: add bitcoin-qt runtime libs doc in symbol-check (fanquake) ef4ce19 depends: freetype 2.11.1 (fanquake) Pull request description: Update freetype to `2.11.1`. Updating fontconfig (currently `2.12.6`) to `2.13.1` requires what looks like a hard dep on gperf; leaving that as-is for now. Document expectations in `symbol-check.py`. Closes #29977 (changes are based on discussion there). ACKs for top commit: sedited: ACK 41e657a Tree-SHA512: 71c4ccc442df0b90bebc475003eb325564111b8312c42bc7d7a9c81a2fc166fdc0814c9ddde3cfe562c3c835556e7f97107458b02a07b981b1a199bf65d5ac1d
48840bf refactor: Prefer `<=>` over multiple relational operators (Daniel Pfeifer) 5a0f49b refactor: Remove all `operator!=` definitions (Daniel Pfeifer) Pull request description: Remove all `operator!=` definitions and provide `operator<=>` as a replacement where all relational comparison operators were defined before. The compiler is able to deduce missing comparison operators from `operator!=` and `operator<=>`. The compiler provided operators have the following advantages: 1. less code 2. guaranteed consistency Refactoring that changes the implementation, or replaces it with `= default` is left for a separate PR. ACKs for top commit: optout21: utACK 48840bf Chand-ra: tACK [`48840bf`](48840bf). Built the PR and ran unit tests; everything passes. maflcko: review ACK 48840bf 🌖 stickies-v: utACK 48840bf. Pretty straightforward cleanup taking advantage of C++20 improvements, nice. janb84: ACK 48840bf sipa: ACK 48840bf Tree-SHA512: 7fedc4abc451c7ad611e3a960ff939a35580667222009cb30ca546e564dc9161e3e8d4d1d7d44c538d961cc8f7adba6e6dbcebcd1be370bf33aef294d06f236b
faa2373 refactor: Enable clang-tidy bugprone-unused-return-value (MarcoFalke) fa114be Add util::Expected (std::expected) (MarcoFalke) Pull request description: Some low-level code could benefit from being able to use `std::expected` from C++23: * Currently, some code is using `std::optional<E>` to denote an optional error. This is fine, but a bit confusing, because `std::optional` is normally used for values, not errors. Using `std::expected<void, E>` is clearer. * Currently, some code is using `std::variant<V, E>` to denote either a value or an error. This is fine, but a bit verbose, because `std::variant` requires a visitor or get_if/holds_alternative instead of a simple call of the `operator bool` for `std::expected`. In theory, `util::Result` could be taught to behave similar to `std::expected` (see #34005). However, it is unclear if this is the right approach: * `util::Result` is mostly meant for higher level code, where errors come with translated error messages. * `std::expected` is mostly meant for lower level code, where errors could be an enum, or any other type. * #25665 aims to minimize the memory footprint of the error by wrapping it in a unique_ptr internally. `std::expected` requires the value and error to be "nested within it" (https://cplusplus.github.io/LWG/issue4141). So from a memory-layout perspective, the two are not compatible. * `std::expected` also comes with `std::unexpected`, which also does not map cleanly to `util::Result`. So just add a minimal drop-in port of `std::expected`. ACKs for top commit: romanz: tACK faa2373 sedited: Re-ACK faa2373 hodlinator: ACK faa2373 rkrux: light Code Review ACK faa2373 ryanofsky: Code review ACK faa2373, only adding `value_or` implementation and `CheckedReturnTypes` clang-tidy commit since last review. stickies-v: ACK faa2373 Tree-SHA512: fdbd0f6bf439738ffe6a68da5522f1051537f8df9c308eb90bef6bd2e06931d79f1c5da22d5500765e9cb1d801d5be39e11e10d47c9659fec1a8c8804cb7c872
Previously, when `debug=net` is enabled, we log "New [..] peer connected" for new inbound peers with `LogInfo`. However, `LogInfo` will get rate-limited since #32604. When we specifically turn on `debug=net`, we don't want these log messages to be rate-limited. To fix this, use `LogDebug(BCLog::NET, ...)` for potentially high- rate inbound connections. Otherwise use `LogInfo`. This means we don't rate-limit the messages for inbound peers when `debug=net` is turned on but will rate-limit if we created outbound at a high rate as these are logged via `LogInfo`. -- I ran into this message getting rate-limited on one of my monitoring nodes with `-logsourcelocations=1`: With logsourcelocations, one of these lines is about 338 chars (or 338 bytes) long. We rate-limit after more than 1048576 bytes per hour, which results in about 3100 in- and outbound connections per hour. With evicted and instantly reconnecting connections from an entity like LinkingLion, this can be reached fairly quickly. Co-Authored-By: Eugene Siegel <elzeigel@gmail.com> Co-Authored-By: Anthony Towns <aj@erisian.com.au>
ff06e24 init: point out -stopatheight may be imprecise (brunoerg) Pull request description: `-stopatheight` is used to stop running bitcoind after reaching a given height. However, this feature is imprecise since some blocks can still be processed during the shutdown. There are some previous discussions around it in #13713, #13490 and #13477. However, I'm not sure if it will get fixed since it's undesirable to burden the validation code further with this and we can bypass this behavior by using `invalidateblock` to wind back. Anyway, since at this moment its behavior is imprecise I think worth mentioning it in documentation. ACKs for top commit: rkrux: re-ACK ff06e24 stickies-v: ACK ff06e24 pablomartin4btc: ACK ff06e24 jaonoctus: re-ACK ff06e24 Tree-SHA512: 222d5e89021d5f9a7ce0edca44c4ce20b13f71832413dccea78ad40a01f2a615a061f8cf446d7290ed911023922adbc6fa22f0c88cff306dcd8b4ae14194e9b8
b5a7a68 ci: Make the max number of commits tested explicit (Hodlinator) Pull request description: Gives less of a false sense of security. ACKs for top commit: maflcko: lgtm ACK b5a7a68 rkrux: crACK b5a7a68 janb84: ACK b5a7a68 glozow: lgtm ACK b5a7a68 Tree-SHA512: 9f50a86f440d6a551a0c1ff547e61b61b829e98cd0cd2d5ca65966af0b48d40582f698bcb039a7467c4b71166920413c334eac0e9e4f0141c3e02cd68555865b
…countered in Tapscript 9d5021a script: add SCRIPT_ERR_TAPSCRIPT_EMPTY_PUBKEY (billymcbip) Pull request description: We currently have two callsites for `SCRIPT_ERR_PUBKEYTYPE`: - A pre-tapscript policy error behind the `SCRIPT_VERIFY_STRICTENC` flag: https://github.com/bitcoin/bitcoin/blob/4de26b111f4d9b116d0d4930f8bbd69889b3cf75/src/script/interpreter.cpp#L220 - A [consensus error](https://github.com/bitcoin/bips/blob/master/bip-0342.mediawiki?plain=1#L93) in Tapscript: https://github.com/bitcoin/bitcoin/blob/4de26b111f4d9b116d0d4930f8bbd69889b3cf75/src/script/interpreter.cpp#L368 It would be good for readability and testability to have separate errors for both cases, as they are quite distinct (policy vs. consensus, format vs. emptiness). **This PR adds `SCRIPT_ERR_TAPSCRIPT_EMPTY_PUBKEY` for the consensus error path.** This change would make our error handling more consistent. We have more granular errors for other pubkey error paths already: `SCRIPT_ERR_WITNESS_PUBKEYTYPE`, `SCRIPT_ERR_DISCOURAGE_UPGRADABLE_PUBKEYTYPE`. We also have separate errors for MINIMAL_IF: `SCRIPT_ERR_MINIMALIF` for the policy error pre-tapscript, and `SCRIPT_ERR_TAPSCRIPT_MINIMALIF` for the consensus error post-tapscript. Tests: Added a test case to `script_tests` and ran `build/bin/test_bitcoin --run_test=script_tests --log_level=success`. ``` test/script_tests.cpp:144: info: check '[["aa","#SCRIPT# 0 CHECKSIG","#CONTROLBLOCK#",0.00000001],"","0x51 0x20 #TAPROOTOUTPUT#","P2SH,WITNESS,TAPROOT","TAPSCRIPT_EMPTY_PUBKEY","TAPSCRIPT: OP_CHECKSIG with empty pubkey must fail"] (with flags 165d5d)' has passed ... ``` Ran `DIR_UNIT_TEST_DATA="$(pwd)/../qa-assets/unit_test_data" build/bin/test_bitcoin --run_test=script_assets_tests --log_level=success`. Updated `feature_taproot.py` and ran `build/test/functional/feature_taproot.py`. Looking forward to your feedback. ACKs for top commit: sedited: ACK 9d5021a darosior: utACK 9d5021a sipa: ACK 9d5021a Tree-SHA512: bc0b7f64454313fe392ffb2d23aa4eca3deadc5ea1d10b3fba0b3ab4cb0575a5ddcb002dc27b4fa7aa3c180840a83d1b3e5c89351009ce7ffe684d58e1980ace
…saction in `FullBlockTest.update_block()` a7c96f8 tests: Add witness commitment if we have a witness transaction in FullBlockTest.update_block() (Chris Stewart) Pull request description: This is useful for test cases where we want to test logic invalid blocks that contain witness transactions. If we don't add the witness commitment as per [BIP141](https://github.com/bitcoin/bips/blob/master/bip-0141.mediawiki#user-content-Commitment_structure), blocks will be rejected with the error [`Block mutated`](https://github.com/bitcoin/bitcoin/blob/fb0ada982a73687520c43b8fde480fa5d456f3e1/src/validation.cpp#L4180). This change was needed in ajtowns#13 which is a soft fork proposal to disallow 64 byte transactions. We want to test that 64 byte transactions serialized without the witness are invalid. If we do not have this change, we cannot directly test the logic that rejects 64 byte transactions. I decided to PR this upstream as many soft fork proposals may not see the light of day, but this functionality seems strictly additive to the test framework. ACKs for top commit: theStack: ACK a7c96f8 sedited: ACK a7c96f8 glozow: ACK a7c96f8 Tree-SHA512: 7c185838abaf068bc96b425c3c971b73f75dfcb41dacc8b2f2543c7602f23f19d908633278b93738f18049e6bd8c845c152cfb93b289bef501c7e86ed8dae0ab
4b47113 validation: Reword CheckForkWarningConditions and call it also during IBD and at startup (Martin Zumsande) 2f51951 p2p: Add warning message when receiving headers for blocks cached as invalid (Martin Zumsande) Pull request description: In case of corruption that leads to a block being marked as invalid that is seen as valid by the rest of the network, the user currently doesn't receive good error messages, but will often be stuck in an endless headers-sync loop with no explanation (#26391). This PR improves warnings in two ways: - When we receive a header that is already saved in our disk, but invalid, add a warning. This will happen repeatedly during the headerssync loop (see #26391 (comment) on how to trigger it artificially). - Removes the IBD check from `CheckForkWarningConditions` and adds a call to the function during init (`LoadChainTip()`). The existing check was added in 55ed3f1 a long time ago when we had more sophisticated fork detection that could lead to false positives during IBD, but that logic was removed in fa62304 so that I don't see a reason to suppress the warning anymore. Fixes #26391 (We'll still do the endless looping, trying to find a peer with a headers that we can use, but will now repeatedly log warnings while doing so). ACKs for top commit: glozow: ACK `git range-diff 6d2c8ea...4b47113` theStack: re-ACK 4b47113 sedited: ACK 4b47113 Tree-SHA512: 78bc53606374636d616ee10fdce0324adcc9bcee2806a7e13c9471e4c02ef00925ce6daef303bc153b7fcf5a8528fb4263c875b71d2e965f7c4332304bc4d922
d4d184e log: don't rate-limit "new peer" with -debug=net (0xb10c) Pull request description: Previously, when `debug=net` is enabled, we log "New [..] peer connected" for new inbound peers with `LogInfo`. However, `LogInfo` will get rate-limited since #32604. When we specifically turn on `debug=net`, we don't want these log messages to be rate-limited. To fix this, use `LogDebug(BCLog::NET, ...)` for potentially high-rate inbound connections. Otherwise use `LogInfo`. This means we don't rate-limit the messages for inbound peers when `debug=net` is turned on but will rate-limit if we created outbound at a high rate as these are logged via `LogInfo`. The new log messages look similar to: ``` 2025-12-08T00:00:00Z [net] New inbound peer connected: transport=v2 version=70016 blocks=0 peer=1 2025-12-08T00:00:00Z New outbound-full-relay peer connected: transport=v2 version=70016 blocks=281738 peer=5 ``` -- I ran into this message getting rate-limited on one of my monitoring nodes with `-logsourcelocations=1`: With logsourcelocations, one of these lines is about 338 chars (or 338 bytes) long. We rate-limit after more than 1048576 bytes per hour, which results in about 3100 in- and outbound connections per hour. With evicted and instantly reconnecting connections from an entity like LinkingLion, this can be reached fairly quickly. ACKs for top commit: stickies-v: utACK d4d184e Crypt-iQ: tACK d4d184e maflcko: review ACK d4d184e 🚲 rkrux: lgtm code review ACK d4d184e glozow: lgtm ACK d4d184e Tree-SHA512: 14dbf693fa44a74c9822590e7a08167d2deeb1bc6f4b8aeb00c1b035c0df7101087d5c80a3c0d637879d5c52f88b30f0cb4c0577cff6f647d2eb3300f49d8ea3
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.