Rather than individually calling addUnchecked for each transaction added in a
changeset (after removing all the to-be-removed transactions), instead we can
take advantage of boost::multi_index's splicing features to extract and insert
entries directly from the staging multi_index into mapTx.
This has the immediate advantage of saving allocation overhead for mempool
entries which have already been allocated once. This also means that the memory
locations of mempool entries will not change when transactions go from staging
to the main mempool.
Additionally, eliminate addUnchecked and require all new transactions to enter
the mempool via a CTxMemPoolChangeSet.
9c5775c331 addrman: cap the `max_pct` to not exceed the maximum number of addresses (brunoerg)
Pull request description:
Fixes#31234
This PR fixes a bad alloc issue in `GetAddresses` by capping the value `max_pct`. In practice, values greater than 100 should be treated as 100 since it's the percentage of addresses to return. Also, it limites the value `max_pct` in connman target to exercise values between 0 and 100.
ACKs for top commit:
adamandrews1:
Code Review ACK 9c5775c331
marcofleon:
Tested ACK 9c5775c331. Reproduced the crash on master and checked that this fixed it. The checks added to `GetAddr_` look reasonable.
mzumsande:
Code Review ACK 9c5775c331
vasild:
ACK 9c5775c331
Tree-SHA512: 2957ae561ccc37df71f43c1863216d2e563522ea70b9a4baee6990e0b4a1ddadccabdcb9115c131a9a57480367b5ebdd03e0e3d4c8583792e2b7d1911a0a06d3
The previous error message for non-existent wallets of "Already a
descriptor wallet" is misleading. Return a more specific error when a
non-existent wallet is specified.
The hardcoded nBits range would occasionally produce values for
the difficulty target that were too low, causing the total work
of the test chain to exceed MinimumChainWork. This fix uses
ConsumeArithUInt256InRange to properly generate targets that
will produce header chains with less work than MinimumChainWork.
Also known as Ephemeral Dust.
We try to ensure that dust is spent in blocks by requiring:
- ephemeral dust tx is 0-fee
- ephemeral dust tx only has one dust output
- If the ephemeral dust transaction has a child,
the dust is spent by by that child.
0-fee requirement means there is no incentive to mine
a transaction which doesn't have a child bringing its
own fees for the transaction package.
Expands the benchmark framework with the existing '-testdatadir' arg,
enabling the ability to change the benchmark data directory.
This is useful for running benchmarks on different storage devices, and
not just under the OS /tmp/ directory.
Since G_TEST_GET_FULL_NAME is not initialized in the benchmark framework,
benchmarks using the unit test setup run in the same directory without
any clear distinction between them.
This poses an extra complication for locating any specific benchmark
directory during a failure.
In master, unit tests and benchmarks run in the following path:
/<OS_tmp_dir>/test_common bitcoin/<random_uint256>/
After this commit, unit tests and benchmarks are contained within its
own directory:
/<OS_tmp_dir>/test_common bitcoin/<test_name>/<time_in_nanoseconds>/
This makes it easier to find any benchmark run when a failure occurs.
5a96767e3f depends, libevent: Do not install *.pc files and remove patches for them (Hennadii Stepanov)
ffda355b5a cmake, refactor: Move `HAVE_EVHTTP_...` to `libevent` interface (Hennadii Stepanov)
b619bdc330 cmake: Revamp `FindLibevent` module (Hennadii Stepanov)
Pull request description:
This PR generalizes the use of `find_package` / `pkg_check_modules`, prioritizing the former.
Addresses https://github.com/bitcoin/bitcoin/pull/30903#issuecomment-2444700876:
> We should also follow up with refactoring the libevent module, to more generically use CMake/pkg-config, rather than restricting the CMake usage to `vcpkg`. At that point, we'd likely be able to dump pkg-config for the depends path entirely.
Similar to https://github.com/bitcoin/bitcoin/pull/30903.
ACKs for top commit:
fanquake:
ACK 5a96767e3f
Tree-SHA512: 181020c16ccd2821e718c73f264badcdc5e62980c4a8d9691e759efe2ea00da2326e26308d1dcfdeac01e9e27930428ecace9f36941deee951b751b138d7266c
fa729ab4a2 doc: Fixup bitcoin-wallet manpage chain selection args (MarcoFalke)
Pull request description:
The sentence is missing `-testnet4` and `-chain`. Instead of duplicating the full list (and having to keep it in sync), just refer to them as `(test)chain selection arguments`.
ACKs for top commit:
willcl-ark:
utACK fa729ab4a2
tdb3:
Code Review ACK fa729ab4a2
rkrux:
crACK fa729ab4a2
Tree-SHA512: e2cb6e2dd778a34e6c7e8ccde9794ab601e68bad68fe110f41cd73ac12ac3c5d0632fb59a48355f03ef0909f77ec5afd7ea50f301a998cb3ec76e115969f3e7e
4120c7543e scripted-diff: get rid of remaining "command" terminology in protocol.{h,cpp} (Sebastian Falbesoner)
Pull request description:
The confusing "command" terminology for the 12-byte field in the (v1) p2p message header was replaced with the more proper term "message type" in other modules already years ago, see eg #18533, #18937, #24078, #24141. This PR does the same for the protocol.{h,cpp} module to complete the replacements. Note that "GetCommand" is a method name also used in the `ArgsManager` (there it makes much more sense), so the scripted-diff lists for this replacement the files explicitly, rather than using `$(git grep -l ...)`.
ACKs for top commit:
maflcko:
review ACK 4120c7543e🛒
fjahr:
Code review ACK 4120c7543e
rkrux:
tACK 4120c7543e
Tree-SHA512: 7b4dd30136392a145da95d2f3ba181c18c155ba6f3158e49e622d76811c6a45ef9b5c7539a979a04d8404faf18bb27f11457aa436d4e2998ece3deb2c9e59748
0de3e96e33 tracing: use bitcoind pid in bcc tracing examples (0xb10c)
411c6cfc6c tracing: only prepare tracepoint args if attached (0xb10c)
d524c1ec06 tracing: dedup TRACE macros & rename to TRACEPOINT (0xb10c)
Pull request description:
Currently, if the tracepoints are compiled (e.g. in depends and release builds), we always prepare the tracepoint arguments regardless of the tracepoints being used or not. We made sure that the argument preparation is as cheap as possible, but we can almost completely eliminate any overhead for users not interested in the tracepoints (the vast majority), by gating the tracepoint argument preparation with an `if(something is attached to this tracepoint)`. To achieve this, we use the optional semaphore feature provided by SystemTap.
The first commit simplifies and deduplicates our tracepoint macros from 13 TRACEx macros to a single TRACEPOINT macro. This makes them easier to use and also avoids more duplicate macro definitions in the second commit.
The Linux tracing tools I'm aware of (bcc, bpftrace, libbpf, and systemtap) all support the semaphore gating feature. Thus, all existing tracepoints and their argument preparation is gated in the second commit. For details, please refer to the commit messages and the updated documentation in `doc/tracing.md`.
Also adding unit tests that include all tracepoint macros to make sure there are no compiler problems with them (e.g. some varadiac extension not supported).
Reviewers might want to check:
- Do the tracepoints still work for you? Do the examples in `contrib/tracing/` run on your system (as bpftrace frequently breaks on every new version, please test master too if it should't work for you)? Do the CI interface tests still pass?
- Is the new documentation clear?
- The `TRACEPOINT_SEMAPHORE(event, context)` macros places global variables in our global namespace. Is this something we strictly want to avoid or maybe move to all `TRACEPOINT_SEMAPHORE`s to a separate .cpp file or even namespace? I like having the `TRACEPOINT_SEMAPHORE()` in same file as the `TRACEPOINT()`, but open for suggestion on alternative approaches.
- Are newly added tracepoints in the unit tests visible when using `readelf -n build/src/test/test_bitcoin`? You can run the new unit tests with `./build/src/test/test_bitcoin --run_test=util_trace_tests* --log_level=all`.
<details><summary>Two of the added unit tests demonstrate that we are only processing the tracepoint arguments when attached by having a test-failure condition in the tracepoint argument preparation. The following bpftrace script can be used to demonstrate that the tests do indeed fail when attached to the tracepoints.</summary>
`fail_tests.bt`:
```c
#!/usr/bin/env bpftrace
usdt:./build/src/test/test_bitcoin:test:check_if_attached {
printf("the 'check_if_attached' test should have failed\n");
}
usdt:./build/src/test/test_bitcoin:test:expensive_section {
printf("the 'expensive_section' test should have failed\n");
}
```
Run the unit tests with `./build/src/test/test_bitcoin` and start `bpftrace fail_tests.bt -p $(pidof test_bitcoin)` in a separate terminal. The unit tests should fail with:
```
Running 594 test cases...
test/util_trace_tests.cpp(31): error: in "util_trace_tests/test_tracepoint_check_if_attached": check false has failed
test/util_trace_tests.cpp(51): error: in "util_trace_tests/test_tracepoint_manual_tracepoint_active_check": check false has failed
*** 2 failures are detected in the test module "Bitcoin Core Test Suite"
```
</details>
These links might provide more contextual information for reviewers:
- [How SystemTap Userspace Probes Work by eklitzke](https://eklitzke.org/how-sytemtap-userspace-probes-work) (actually an example on Bitcoin Core; mentions that with semaphores "the overhead for an untraced process is effectively zero.")
- [libbpf comment on USDT semaphore handling](1596a09b5d/src/usdt.c (L83-L92)) (can recommend the whole comment for background on how the tracepoints and tracing tools work together)
- https://sourceware.org/systemtap/wiki/UserSpaceProbeImplementation#Semaphore_Handling
ACKs for top commit:
willcl-ark:
utACK 0de3e96e33
laanwj:
re-ACK 0de3e96e33
jb55:
utACK 0de3e96e33
vasild:
ACK 0de3e96e33
Tree-SHA512: 0e5e0dc5e0353beaf5c446e4be03d447e64228b1be71ee9972fde1d6fac3fac71a9d73c6ce4fa68975f87db2b2bf6eee2009921a2a145e24d83a475d007a559b
The *_RECENT_CONSENSUS_CHANGE variants in the validation result
enumerations were always unused. They seem to have been kept around
speculatively for a soft fork after segwit, however they were never used
for taproot either. This points at them not having a clear purpose.
Based on the original pull requests' comments their usage was never
entirely clear:
https://github.com/bitcoin/bitcoin/pull/11639#issuecomment-370234133https://github.com/bitcoin/bitcoin/pull/15141#discussion_r271039747
Since they are part of the validation interface and need to exposed by
the kernel library keeping them around may also be confusing to future
users of the library.
c189eec848 doc: release note for mempoolrullrbf removal (Greg Sanders)
d47297c6aa rpc: Mark fullrbf and bip125-replaceable as deprecated (Greg Sanders)
04a5dcee8a docs: remove requirement to signal bip125 (Greg Sanders)
111a23d9b3 Remove -mempoolfullrbf option (Greg Sanders)
Pull request description:
Given https://github.com/bitcoin/bitcoin/pull/30493 and the related discussion on network uptake it's probably not helpful to have an option for a feature that will not be respected by the network in any meaningful way.
Wallet changes can be done in another PR on its own cadence to account for possible fingerprinting, waiting for fullrbf logic to permeate the network, etc.
ACKs for top commit:
stickies-v:
re-ACK c189eec848
achow101:
ACK c189eec848
murchandamus:
ACK c189eec848
rkrux:
reACK c189eec848
Tree-SHA512: 9447f88f8f291c56c5bde70af0a91b0a4f5163aaaf173370fbfdaa3c3fd0b44120b14d3a1977f7ee10e27ffe9453f8a70dd38aad0ffb8c39cf145049d2550730
fa461d7a43 fuzz: Limit wallet_notifications iterations (MarcoFalke)
Pull request description:
I don't think the fuzz target has ever found a real issue. The closest being https://github.com/bitcoin/bitcoin/pull/25869
It is also, by far, the slowest fuzz target. For example, looking at https://cirrus-ci.com/task/5533338067271680?logs=ci#L3974, it takes more than one hour:
```
Run wallet_notifications with args ['/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/test/fuzz/fuzz', '-runs=1', PosixPath('/ci_container_base/ci/scratch/qa-assets/fuzz_corpora/wallet_notifications')]INFO: Running with entropic power schedule (0xFF, 100).
INFO: Seed: 1096115652
INFO: Loaded 1 modules (625824 inline 8-bit counters): 625824 [0x5628396d9138, 0x562839771dd8),
INFO: Loaded 1 PC tables (625824 PCs): 625824 [0x562839771dd8,0x56283a0fe7d8),
INFO: 1287 files found in /ci_container_base/ci/scratch/qa-assets/fuzz_corpora/wallet_notifications
INFO: -max_len is not provided; libFuzzer will not generate inputs larger than 1047827 bytes
INFO: seed corpus: files: 1287 min: 1b max: 1047827b total: 11616898b rss: 172Mb
#16pulse cov: 14328 ft: 25341 corp: 14/239b exec/s: 5 rss: 204Mb
#64pulse cov: 19179 ft: 58412 corp: 61/3587b exec/s: 5 rss: 320Mb
#128pulse cov: 19692 ft: 85738 corp: 125/16Kb exec/s: 3 rss: 544Mb
#256pulse cov: 19923 ft: 107490 corp: 253/72Kb exec/s: 2 rss: 556Mb
#512pulse cov: 20107 ft: 124704 corp: 509/330Kb exec/s: 2 rss: 590Mb
Slowest unit: 10 s:
artifact_prefix='./'; Test unit written to ./slow-unit-9fa5f7d7e4afa1626622ef1b3c70a7563eecf11d
#1024pulse cov: 20360 ft: 136324 corp: 1009/2488Kb exec/s: 0 rss: 726Mb
Slowest unit: 23 s:
artifact_prefix='./'; Test unit written to ./slow-unit-5d99a20de2c2b6bedb0cbaf0ba3743ae3ba13c7c
Slowest unit: 26 s:
artifact_prefix='./'; Test unit written to ./slow-unit-8889ecb61bdc0650355e0d0d27c012f3239d07a4
Slowest unit: 42 s:
artifact_prefix='./'; Test unit written to ./slow-unit-d16c084282ac1a85fcdc43c48e49836b08446686
#1289INITED cov: 20409 ft: 138281 corp: 1245/10323Kb exec/s: 0 rss: 880Mb
#1289DONE cov: 20409 ft: 138281 corp: 1245/10323Kb lim: 1047827 exec/s: 0 rss: 880Mb
Done 1289 runs in 3813 second(s)
```
Looking at the flame graphs, it looks like the slow runs spend most of their time in the Knapsack solver. This seems reasonable, because it may run 1000 inner Knapsack iterations 200 times. So reduce the fuzz iterations from 200 to 20 to avoid fuzz timeouts and wasted resources.
ACKs for top commit:
brunoerg:
code review ACK fa461d7a43
dergoegge:
lgtm ACK fa461d7a43
Tree-SHA512: bee707a3398ab0c729f335f00d8cad63135939831454dd863830fc957b4b51b27064224be0ed15eb76cfcc39de972e4e79b0802940934fbac516840ddc475ab9
d22a234ed2 net: Use actual memory size in receive buffer accounting (laanwj)
047b5e2af1 streams: add DataStream::GetMemoryUsage (laanwj)
c3a6722f34 net: Use DynamicUsage(m_type) in CSerializedNetMsg::GetMemoryUsage (laanwj)
c6594c0b14 memusage: Add DynamicUsage for std::string (laanwj)
7596282a55 memusage: Allow counting usage of vectors with different allocators (laanwj)
Pull request description:
Add a method `CNetMessage::GetMemoryUsage` and use this for accounting of the size of the process receive queue instead of the raw message size (like we already do for the send buffer and `CSerializedNetMsg`).
This ensures that allocation and deserialization overhead is better taken into account.
On average, this counts about ~100 extra bytes per packet on x86_64:
```
2024-10-27T09:50:12Z [net] 24 bytes -> 112 bytes
2024-10-27T10:36:37Z [net] 61 bytes -> 176 bytes
2024-10-27T10:36:38Z [net] 1285 bytes -> 1392 bytes
2024-10-27T09:50:21Z [net] 43057 bytes -> 43168 bytes
```
ACKs for top commit:
l0rinc:
ACK d22a234ed2
achow101:
ACK d22a234ed2
i-am-yuvi:
ACK d22a234ed2
danielabrozzoni:
Light ACK d22a234ed2 - code looks good to me, but I'm not very familiar with C++ memory management specifics
Tree-SHA512: ef09707e77b67bdbc48e9464133e4fccfa5c05051c1022e81ad84f20ed41db83ac5a9b109ebdb8d38f70785c03c5d6bfe51d32dc133d49e52d1e6225f6f8e292
Previously this assertion checked MAX_PEER_TX_REQUEST_IN_FLIGHT was not
exceeded. However, this property is not actually enforced; it is just
used to determine when a peer is overloaded.
9e5089dbb0 build, msvc: Enable `libqrencode` vcpkg package (Hennadii Stepanov)
30089b0cb6 cmake: Add `FindQRencode` module (Hennadii Stepanov)
Pull request description:
This PR introduces the `FindQRencode` CMake module, following the official CMake [guidelines](https://cmake.org/cmake/help/latest/manual/cmake-developer.7.html#find-modules) for managing [upstream libraries](https://github.com/fukuchi/libqrencode) that lack a config file package. This module enhances flexibility in locating the `libqrencode` library by making the use of `pkg-config` optional.
With this update, `libqrencode` can be detected on systems where either `pkg-config` or the `libqrencode.pc` file is unavailable, such as Windows environments using the vcpkg package manager. However, if `libqrencode.pc` is available, it remains beneficial as the only direct source of the library's version information.
Additionally, the `libqrencode` vcpkg package is enabled for MSVC builds.
Here is a diff for configuration output on Ubuntu 24.10:
```diff
-- Detecting CXX compile features - done
-- Found SQLite3: /usr/include (found suitable version "3.46.1", minimum required is "3.7.17")
-- Found PkgConfig: /usr/bin/pkg-config (found version "1.8.1")
--- Checking for module 'libqrencode'
--- Found libqrencode, version 4.1.1
+-- Found QRencode: /usr/lib/x86_64-linux-gnu/libqrencode.so (found version "4.1.1")
-- Found Qt: /usr/lib/x86_64-linux-gnu/cmake/Qt5 (found suitable version "5.15.15", minimum required is "5.11.3")
-- Performing Test CXX_SUPPORTS__WERROR
-- Performing Test CXX_SUPPORTS__WERROR - Success
```
ACKs for top commit:
fanquake:
ACK 9e5089dbb0
Tree-SHA512: bb9baca64386772f2f4752b1cbff1230792562ca6b2e37c56ad28580b55b1ae6ff65c2cf0d8ab026111d7b5a056d7ac672496a3cfd1a81e4fdd2b84c8cf75fff
97235c446e build: Disable secp256k1 musig module (Ava Chow)
2d46a89386 Squashed 'src/secp256k1/' changes from 2f2ccc46954..0cdc758a563 (Ava Chow)
Pull request description:
v0.6.0 was just released, main change is that it has the musig module which #29675 needs.
ACKs for top commit:
hebasto:
ACK 97235c446e, verified by updating the secp256k1 subtree locally.
laanwj:
ACK 97235c446e
Tree-SHA512: af92da26fc9afb55399b73d80198c0d2aa1adfae7b91f0ad20ffeb519135baf7e78243049b9bd45a2027943931b2d657c944f93151e5200d95a6f3c90b831f31
