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
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
fafbf8acf4 Make G_FUZZING constexpr, require -DBUILD_FOR_FUZZING=ON to execute a fuzz target (MarcoFalke)
fae3cf0ffa ci: Temporarily disable macOS/Windows fuzz step (MarcoFalke)
Pull request description:
`g_fuzzing` is used inside `Assume` at runtime, causing significant overhead in hot paths. See https://github.com/bitcoin/bitcoin/issues/31178
One could simply remove the `g_fuzzing` check from the `Assume`, but this would make fuzzing a bit less useful. Also, it would be unclear if `g_fuzzing` adds a runtime overhead in other code paths today or in the future.
Fix all issues by making `G_FUZZING` equal to the build option `BUILD_FOR_FUZZING`, and for consistency in fuzzing, require it to be set when executing any fuzz target.
Fixes https://github.com/bitcoin/bitcoin/issues/31178
Temporarily this drops fuzzing from two CI tasks, but they can be re-added in a follow-up with something like https://github.com/bitcoin/bitcoin/pull/31073
ACKs for top commit:
marcofleon:
Tested ACK fafbf8acf4
davidgumberg:
I still ACK fafbf8acf4 for fixing the regression measured in #31178.
ryanofsky:
Code review ACK fafbf8acf4 but approach -0, because this approach means libraries built for fuzz testing do not function correctly if used in a release, and libraries built for releases are mostly useless for fuzz testing. So I would like to at least consider other solutions to this problem even if we go with this one.
dergoegge:
utACK fafbf8acf4
Tree-SHA512: 124fc2e8b35e0c4df414436556a7a0a36cd1bec4b3000b40dcf2ab8c85f32e0610bf7f70d2fd79223d62f3c3665b6c09da21241654c7b9859461b8ca340d5421
87532fe558 netinfo: allow setting an outbound-only peer list (Jon Atack)
681ebcceca netinfo: rename and hoist max level constant to use in top-level help (Jon Atack)
e7d307ce8c netinfo: clarify relaytxes and addr_relay_enabled help docs (Jon Atack)
eef2a9d406 netinfo: add peer services column (Jon Atack)
Pull request description:
Been using this since May 2023.
- add a peer services column (considered displaying the p2p_v2 flag as "p" or "2"; proposing "2" here for continuity with the "v" column, but "p" is fine for me as well)
- clarify in the help that "relaytxes" and "addr_relay_enabled" are from getpeerinfo
- hoist (and rename) the max level constant to use in top-level help, to avoid overlooking to update the top-level help if the value of the constant changes (as caught by Larry Ruane in review below)
- add an optional "outonly" (or "o") argument for an outbound-only peer list, as suggested by Vasil Dimov in his review below. Several people have requested this, to keep the output within screen limits when running netinfo as a live dashboard (i.e. with `watch`) on a node with many peers. While doing this, also permit passing "h" for the help in addition to "help".
ACKs for top commit:
achow101:
ACK 87532fe558
rkrux:
tACK 87532fe558
tdb3:
cr re ACK 87532fe558
brunoerg:
crACK 87532fe558
Tree-SHA512: 35b1b0de28dfecaad58bf5af194757a5e0f563553cf69ea4d76f2e1963f8d662717254df2549114c7bba4a041bf5282d5cb3fba8d436b2807f2a00560787d64c
0cdc758a563 Merge bitcoin-core/secp256k1#1631: release: prepare for 0.6.0
39d5dfd542a release: prepare for 0.6.0
df2eceb2790 build: add ellswift.md and musig.md to release tarball
a306bb7e903 tools: fix check-abi.sh after cmake out locations were changed
145868a84d2 Do not export `secp256k1_musig_nonce_gen_internal`
b161bffb8bf Merge bitcoin-core/secp256k1#1579: Clear sensitive memory without getting optimized out (revival of #636)
a38d879a1a6 Merge bitcoin-core/secp256k1#1628: Name public API structs
7d48f5ed02e Merge bitcoin-core/secp256k1#1581: test, ci: Lower default iteration count to 16
694342fdb71 Name public API structs
0f73caf7c62 test, ci: Lower default iteration count to 16
9a8db52f4e9 Merge bitcoin-core/secp256k1#1582: cmake, test: Add `secp256k1_` prefix to test names
765ef53335a Clear _gej instances after point multiplication to avoid potential leaks
349e6ab916b Introduce separate _clear functions for hash module
99cc9fd6d01 Don't rely on memset to set signed integers to 0
97c57f42ba8 Implement various _clear() functions with secp256k1_memclear()
9bb368d1466 Use secp256k1_memclear() to clear stack memory instead of memset()
e3497bbf001 Separate between clearing memory and setting to zero in tests
d79a6ccd43a Separate secp256k1_fe_set_int( . , 0 ) from secp256k1_fe_clear()
1c081262227 Add secp256k1_memclear() for clearing secret data
1464f15c812 Merge bitcoin-core/secp256k1#1625: util: Remove unused (u)int64_t formatting macros
980c08df80a util: Remove unused (u)int64_t formatting macros
9b7c59cbb90 Merge bitcoin-core/secp256k1#1624: ci: Update macOS image
096e3e23f63 ci: Update macOS image
e7d384488e8 Don't clear secrets in pippenger implementation
68b55209f1b Merge bitcoin-core/secp256k1#1619: musig: ctimetests: fix _declassify range for generated nonce points
f0868a9b3d8 Merge bitcoin-core/secp256k1#1595: build: 45839th attempt to fix symbol visibility on Windows
1fae76f50c0 Merge bitcoin-core/secp256k1#1620: Remove unused scratch space from API
8be3839fb2e Remove unused scratch space from API
57eda3ba300 musig: ctimetests: fix _declassify range for generated nonce points
87384f5c0f2 cmake, test: Add `secp256k1_` prefix to test names
e59158b6eb7 Merge bitcoin-core/secp256k1#1553: cmake: Set top-level target output locations
18f9b967c25 Merge bitcoin-core/secp256k1#1616: examples: do not retry generating seckey randomness in musig
5bab8f6d3c4 examples: make key generation doc consistent
e8908221a45 examples: do not retry generating seckey randomness in musig
70b6be1834e extrakeys: improve doc of keypair_create (don't suggest retry)
01b5893389e Merge bitcoin-core/secp256k1#1599: #1570 improve examples: remove key generation loop
cd4f84f3ba8 Improve examples/documentation: remove key generation loops
a88aa935063 Merge bitcoin-core/secp256k1#1603: f can never equal -m
3660fe5e2a9 Merge bitcoin-core/secp256k1#1479: Add module "musig" that implements MuSig2 multi-signatures (BIP 327)
168c92011f5 build: allow enabling the musig module in cmake
f411841a46b Add module "musig" that implements MuSig2 multi-signatures (BIP 327)
0be79660f38 util: add constant-time is_zero_array function
c8fbdb1b972 group: add ge_to_bytes_ext and ge_from_bytes_ext
ef7ff03407f f can never equal -m
c232486d84e Revert "cmake: Set `ENVIRONMENT` property for examples on Windows"
26e4a7c2146 cmake: Set top-level target output locations
4c57c7a5a95 Merge bitcoin-core/secp256k1#1554: cmake: Clean up testing code
447334cb06d include: Avoid visibility("default") on Windows
472faaa8ee6 Merge bitcoin-core/secp256k1#1604: doc: fix typos in `secp256k1_ecdsa_{recoverable_,}signature` API description
292310fbb24 doc: fix typos in `secp256k1_ecdsa_{recoverable_,}signature` API description
85e224dd97f group: add ge_to_bytes and ge_from_bytes
7c987ec89e6 cmake: Call `enable_testing()` unconditionally
6aa576515ef cmake: Delete `CTest` module
git-subtree-dir: src/secp256k1
git-subtree-split: 0cdc758a56360bf58a851fe91085a327ec97685a
Add a method CNetMessage::GetMemoryUsage and use this for accounting of
the size of the process receive queue instead of the raw message size.
This ensures that allocation and deserialization overhead is taken into
account.
a1b3ccae4b init: warn, don't error, when '-upnp' is set (Antoine Poinsot)
Pull request description:
It prevented the GUI from starting when its settings.json had the -upnp option set. This also doesn't prevent the node from running, so this error didn't need to be fatal.
Thanks to Sjors for bringing attention to what i broke and to Maflcko for suggesting a simple short term fix.
Fixes https://github.com/bitcoin-core/gui/issues/843.
ACKs for top commit:
maflcko:
lgtm ACK a1b3ccae4b
kevkevinpal:
Concept ACK [a1b3cca](a1b3ccae4b)
achow101:
ACK a1b3ccae4b
tdb3:
ACK a1b3ccae4b
Tree-SHA512: ceb1513bf532698e5143d64430a065f39626ef0d2708103ffc8ab7f81e8393f488af2350c5a299bc80f966add82a3951b4d81ae8b0e3070c0d15c94e8db4badd
It prevented the GUI from starting when its settings.json had the -upnp option set. This also doesn't
prevent the node from running, so this error didn't need to be fatal.
Thanks to Sjors for bringing attention to what i broke and to Maflcko for suggestion a simple short
term fix.
bbbbaa0d9a Fix unsigned integer overflows in interpreter (MarcoFalke)
Pull request description:
Unsigned integer overflow is well defined by the language and in some cases even useful or necessary. However, I think that it should be avoided in interpreter, as it makes the code harder to read and requires the whole file to be suppressed in the sanitizer. This puts more burden on reviewers to check that any changes to interpreter that involve unsigned integer overflow are sane.
This patch involves a few changes:
* Evaluate the addition in 64-bit "space". Previously, the first argument was `size_t` (unsigned, 32-bit or 64-bit, depending on platform) and the second was `int` (32-bit on all supported platforms). Thus the addition was done in 32-bit or 64-bit "unsigned space". Now the addition is done in 64-bit "signed space" on all platforms. This is safe because signed integer overflow (UB) isn't expected here with 64-bit integers.
* Clarify that the value passed to the "stack macros" always fits in an `int64_t`. This is done with the C++11 syntax `int64_t{i}`, which fails to compile if `i` needs to be narrowed to fit into `int64_t`.
* Explicitly convert the result of the addition to `size_t`. This isn't needed, because the called function already converts the value (see https://en.cppreference.com/w/cpp/container/vector/operator_at), however I have a slight preference for the explicit cast. (Happy to remove if reviewers prefer without)
The patch does not change the bitcoind binary on my 64-bit system with `clang++ -O2`. However, it does change with gcc.
ACKs for top commit:
achow101:
ACK bbbbaa0d9a
ismaelsadeeq:
Code review ACK bbbbaa0d9a
hebasto:
ACK bbbbaa0d9a, I have reviewed the code and it looks OK.
Tree-SHA512: 0e9cbc6a0afd3db0d1d9489fd5e32ff856217604abde370add1f01c2cae8c526f2afedeb372997217c3a70ab0f8f56442e8230f87456f8e21c9abcb7c6578f7c
e60cecc811 doc: add release note for 31156 (Martin Zumsande)
fc7dfb3df5 test: Don't enforce BIP94 on regtest unless specified by arg (Martin Zumsande)
Pull request description:
The added arg `-test=bip94` is only used in a functional test for BIP94. This is done because the default regtest consensus rules should follow mainnet, not testnet.
Fixes #31137.
ACKs for top commit:
achow101:
ACK e60cecc811
tdb3:
cr and light test ACK e60cecc811
rkrux:
tACK e60cecc811
BrandonOdiwuor:
utACK e60cecc811
laanwj:
Code review ACK e60cecc811
Tree-SHA512: ca2f322f89d8808dfc3565fe020d2615cfcc110e188a02128ad7108fef51c735b33d55b5e6a70c505d78f7291f3c635dc7dfbcd78be1348d4d6e483883be4216
559a8dd9c0 key: clear out secret data in `DecodeExtKey` (Sebastian Falbesoner)
Pull request description:
Same as in `DecodeSecret`, we should also clear out the secret data from the vector resulting from the Base58Check parsing for xprv keys. Note that the if condition is needed in order to avoid UB, see #14242 (commit d855e4cac8).
ACKs for top commit:
davidgumberg:
utACK 559a8dd9c0
achow101:
ACK 559a8dd9c0
tdb3:
cr ACK 559a8dd9c0
laanwj:
Code review ACK 559a8dd9c0
Tree-SHA512: c22499fe2899a9a5a58159ec55e94cf961570d8af06358d4a6d1943d567be9b88657af90d060d3083985ea957886a4f91bb762a2fcf3311007e7a535b42b0fde
3a4a788ee0 init: Correct coins db cache size setting (TheCharlatan)
Pull request description:
The chainstate caches are currently re-balanced on startup even in the non-assumeutxo case, leading to the database being needlessly re-opened and its cache re-allocated.
Similar to `InitCoinsCache` and `m_coinstip_cache_size_bytes`, the `m_coinsdb_cache_size_bytes` should be set in `InitCoinsDB`.
Together with only conservatively setting the cache values when a assumeutxo chainstate is present, this allows for skipping the cache re-balance during initialization in the normal non-assumeutxo case.
Before:
```
2024-10-09T21:22:17Z Checking all blk files are present...
2024-10-09T21:22:17Z Initializing chainstate Chainstate [ibd] @ height -1 (null)
2024-10-09T21:22:17Z Opening LevelDB in /home/drgrid/.bitcoin/signet/chainstate
2024-10-09T21:22:17Z Opened LevelDB successfully
2024-10-09T21:22:17Z Using obfuscation key for /home/drgrid/.bitcoin/signet/chainstate: b0a6f4e95fd05c92
2024-10-09T21:22:17Z Loaded best chain: hashBestChain=0000000e119967d4937dad58456885ae43fb1761db686947e2f8e168c9a39a4f height=216852 date=2024-10-09T21:06:16Z progress=0.999989
2024-10-09T21:22:17Z Opening LevelDB in /home/drgrid/.bitcoin/signet/chainstate
2024-10-09T21:22:17Z Opened LevelDB successfully
2024-10-09T21:22:17Z Using obfuscation key for /home/drgrid/.bitcoin/signet/chainstate: b0a6f4e95fd05c92
2024-10-09T21:22:17Z [Chainstate [ibd] @ height 216852 (0000000e119967d4937dad58456885ae43fb1761db686947e2f8e168c9a39a4f)] resized coinsdb cache to 8.0 MiB
2024-10-09T21:22:17Z [Chainstate [ibd] @ height 216852 (0000000e119967d4937dad58456885ae43fb1761db686947e2f8e168c9a39a4f)] resized coinstip cache to 440.0 MiB
2024-10-09T21:22:17Z init message: Verifying blocks…
```
After:
```
2024-10-09T21:21:37Z Checking all blk files are present...
2024-10-09T21:21:37Z Initializing chainstate Chainstate [ibd] @ height -1 (null)
2024-10-09T21:21:37Z Opening LevelDB in /home/drgrid/.bitcoin/signet/chainstate
2024-10-09T21:21:37Z Opened LevelDB successfully
2024-10-09T21:21:37Z Using obfuscation key for /home/drgrid/.bitcoin/signet/chainstate: b0a6f4e95fd05c92
2024-10-09T21:21:37Z Loaded best chain: hashBestChain=0000012c12b48011a7d9150ce96ed6a44bbf32b09eeecaff4a667789dda2a566 height=216850 date=2024-10-09T20:37:05Z progress=0.999971
2024-10-09T21:21:37Z init message: Verifying blocks…
```
The change may also be verified by looking at the `feature_assumeutxo.py` functional test debug logs.
ACKs for top commit:
fjahr:
utACK 3a4a788ee0
achow101:
ACK 3a4a788ee0
laanwj:
Code review ACK 3a4a788ee0
BrandonOdiwuor:
Code Review ACK 3a4a788ee0
Tree-SHA512: 87878d0d196bb426370d4b4bd180ca52a34017a0799ecea651c2532461fd2927b0f7cc8182276a7d9bb1fe0ede7d0ad677e3714ca22f321917d711c643acc578
0ea84bc362 test: explicitly check boolean verbosity is disallowed (tdb3)
7a2e6b68cd doc: add rpc guidance for boolean verbosity avoidance (tdb3)
698f302df8 rpc: disallow boolean verbosity in getorphantxs (tdb3)
63f5e6ec79 test: add entry and expiration time checks (tdb3)
808a708107 rpc: add entry time to getorphantxs (tdb3)
56bf302714 refactor: rename rpc_getorphantxs to rpc_orphans (tdb3)
7824f6b077 test: check that getorphantxs is hidden (tdb3)
ac68fcca70 rpc: disallow undefined verbosity in getorphantxs (tdb3)
Pull request description:
Implements follow-up suggestions from #30793.
- Now disallows undefined verbosity levels (below and above valid values) (https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1786093549)
- Disallows boolean verbosity (https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1788273274) and adds guidance to developer-notes
- Checks that `getorphantxs` is a hidden rpc (https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1786107786)
- Adds a test for `expiration` time
- Adds `entry` time to the returned orphan objects (verbosity >=1) to relieve the user from having to calculate it from `expiration`. Also adds associated test. (https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1743687732)
- Minor cleanup (blank line removal and log message move) (https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1786092641)
Included a commit to rename the test to a more generic `get_orphans` to better accommodate future orphanage-related RPCs (e.g. `getorphanangeinfo`). Can drop the refactor commit from this PR if people feel strongly about it.
ACKs for top commit:
achow101:
ACK 0ea84bc362
glozow:
utACK 0ea84bc362
rkrux:
tACK 0ea84bc362
itornaza:
tACK 0ea84bc362
Tree-SHA512: e48a088f333ebde132923072da58e970461e74362d0acebbc799c3043d5727cdf5f28e82b43cb38bbed27c603df6710695dba91ff0695e623ad168e985dce08e
0f4bc63585 [fuzz] txdownloadman and txdownload_impl (glozow)
699643f23a [unit test] MempoolRejectedTx (glozow)
fa584cbe72 [p2p] add TxDownloadOptions bool to make TxRequestTracker deterministic (glozow)
f803c8ce8d [p2p] filter 1p1c for child txid in recent rejects (glozow)
5269d57e6d [p2p] don't process orphan if in recent rejects (glozow)
2266eba43a [p2p] don't find 1p1cs for reconsiderable txns that are AlreadyHaveTx (glozow)
fa7027d0fc [refactor] add CheckIsEmpty and GetOrphanTransactions, remove access to TxDownloadMan internals (glozow)
969b07237b [refactor] wrap {Have,Get}TxToReconsider in txdownload (glozow)
f150fb94e7 [refactor] make AlreadyHaveTx and Find1P1CPackage private to TxDownloadImpl (glozow)
1e08195135 [refactor] move new tx logic to txdownload (glozow)
257568eab5 [refactor] move invalid package processing to TxDownload (glozow)
c4ce0c1218 [refactor] move invalid tx processing to TxDownload (glozow)
c6b21749ca [refactor] move valid tx processing to TxDownload (glozow)
a8cf3b6e84 [refactor] move Find1P1CPackage to txdownload (glozow)
f497414ce7 [refactor] put peerman tasks at the end of ProcessInvalidTx (glozow)
6797bc42a7 [p2p] restrict RecursiveDynamicUsage of orphans added to vExtraTxnForCompact (glozow)
798cc8f5aa [refactor] move Find1P1CPackage into ProcessInvalidTx (glozow)
416fbc952b [refactor] move new orphan handling to ProcessInvalidTx (glozow)
c8e67b9169 [refactor] move ProcessInvalidTx and ProcessValidTx definitions down (glozow)
3a41926d1b [refactor] move notfound processing to txdownload (glozow)
042a97ce7f [refactor] move tx inv/getdata handling to txdownload (glozow)
58e09f244b [p2p] don't log tx invs when in IBD (glozow)
288865338f [refactor] rename maybe_add_extra_compact_tx to first_time_failure (glozow)
f48d36cd97 [refactor] move peer (dis)connection logic to TxDownload (glozow)
f61d9e4b4b [refactor] move AlreadyHaveTx to TxDownload (glozow)
84e4ef843d [txdownload] add read-only reference to mempool (glozow)
af918349de [refactor] move ValidationInterface functions to TxDownloadManager (glozow)
f6c860efb1 [doc] fix typo in m_lazy_recent_confirmed_transactions doc (glozow)
5f9004e155 [refactor] add TxDownloadManager wrapping TxOrphanage, TxRequestTracker, and bloom filters (glozow)
Pull request description:
Part of #27463.
This PR does 3 things:
(1) It modularizes transaction download logic into a `TxDownloadManager`. Transaction download logic refers to the process of deciding what transactions to request, download, and validate.[1] There should be no behavior changes. Using `--color_moved=dimmed_zebra -w` may help.
(2) It adds unit and fuzz (🪄) testing for transaction download.
(3) It makes a few small behavioral changes:
- Stop (debug-only) logging tx invs during IBD
- Just like all other transactions, require orphans have RecursiveDynamicUsage < 100k before adding to vExtraTxnForCompact
- Don't return a 1p1c that contains a parent or child in recent rejects. Don't process any orphan already in recent rejects. These cases should not happen in actual node operation; it's just to allow tighter sanity checks during fuzzing.
There are several benefits to this interface, such as:
- Unit test coverage and fuzzing for logic that currently isn't feasible to test as thoroughly (without lots of overhead) and/or currently only lightly tested through `assert_debug_log` (not good) in functional tests.
- When we add more functionality (e.g. package relay messages, more robust orphan handling), the vast majority of it will be within `TxDownloadManager` instead of `PeerManager`, making it easier to review and test. See #28031 for what this looks like.
- `PeerManager` will no longer know anything about / have access to `TxOrphanage`, `TxRequestTracker` or the rejection caches. Its primary interface with `TxDownloadManager` would be much simpler:
- Passing on `ValidationInterface` callbacks
- Telling `txdownloadman` when a peer {connects, disconnects}
- Telling `txdownloadman`when a {transaction, package} is {accepted, rejected} from mempool
- Telling `txdownloadman` when invs, notfounds, and txs are received.
- Getting instructions on what to download.
- Getting instructions on what {transactions, packages, orphans} to validate.
- Get whether a peer `HaveMoreWork` for the `ProessMessages` loop
- (todo) Thread-safety can be handled internally.
[1]: This module is concerned with tx *download*, not upload. It excludes transaction announcements/gossip which happens after we download/accept a transaction. Txreconciliation (erlay) is excluded from this module, as it only relates to deciding which `inv`s to send or helping the other peer decide which `inv`s to send. It is independent from this logic.
ACKs for top commit:
achow101:
light ACK 0f4bc63585
theStack:
ACK 0f4bc63585
instagibbs:
reACK 0f4bc63585
naumenkogs:
ACK 0f4bc63585
Tree-SHA512: 84ab8ef8a0fc705eb829d7f7d6885f28944aaa42b03172f256a42605677b3e783919bb900d4e3b8589f85a0c387dfbd972bcd61d252d44a88c6aaa90e4bf920f
915640e191 depends: zeromq: don't install .pc files and remove patches for them (Cory Fields)
6b8a74463b cmake: Add `FindZeroMQ` module (Hennadii Stepanov)
Pull request description:
This PR introduces the `FindZeroMQ` module, which first attempts to find the `libzmq` library using CMake's `find_package()` and falls back to `pkg_check_modules()` if unsuccessful.
Addresses https://github.com/bitcoin/bitcoin/issues/30876 for the ZeroMQ package.
ACKs for top commit:
fanquake:
ACK 915640e191
Tree-SHA512: 2f17bae21be5d3f280a13425d22f5d1b2e23837a8aaf5ec89c433767509de030a42d598b261e102bdb5b860d8ede98013c124c3d25e081e956d4ee3a81b2584f
The added regtest option -test=bip94 is only used in the functional
test for BIP94.
This is done because the default regtest consensus rules
should aim to follow to mainnet, not testnet.
Before this commit, we would always prepare tracepoint arguments
regardless of the tracepoint being used or not. While we already made
sure not to include expensive arguments in our tracepoints, this
commit introduces gating to make sure the arguments are only prepared
if the tracepoints are actually used. This is a win-win improvement
to our tracing framework. For users not interested in tracing, the
overhead is reduced to a cheap 'greater than 0' compare. As the
semaphore-gating technique used here is available in bpftrace, bcc,
and libbpf, users interested in tracing don't have to change their
tracing scripts while profiting from potential future tracepoints
passing slightly more expensive arguments. An example are mempool
tracepoints that pass serialized transactions. We've avoided the
serialization in the past as it was too expensive.
Under the hood, the semaphore-gating works by placing a 2-byte
semaphore in the '.probes' ELF section. The address of the semaphore
is contained in the ELF note providing the tracepoint information
(`readelf -n ./src/bitcoind | grep NT_STAPSDT`). Tracing toolkits
like bpftrace, bcc, and libbpf increase the semaphore at the address
upon attaching to the tracepoint. We only prepare the arguments and
reach the tracepoint if the semaphore is greater than zero. The
semaphore is decreased when detaching from the tracepoint.
This also extends the "Adding a new tracepoint" documentation to
include information about the semaphores and updated step-by-step
instructions on how to add a new tracepoint.
This change allows to the use of the `CLIENT_` namespace without
potential name clashes.
-BEGIN VERIFY SCRIPT-
sed -i "s/\<CLIENT_NAME\>/UA_NAME/g" $( git grep -l "CLIENT_NAME" ./src)
-END VERIFY SCRIPT-
9f243cd7fa Introduce `g_fuzzing` global for fuzzing checks (dergoegge)
Pull request description:
This PR introduces a global `g_fuzzing` that indicates if we are fuzzing.
If `g_fuzzing` is `true` then:
* Assume checks are enabled
* Special fuzzing paths are taken (e.g. pow check is reduced to one bit)
Closes #30950#31057
ACKs for top commit:
maflcko:
review ACK 9f243cd7fa 🗜
brunoerg:
crACK 9f243cd7fa
marcofleon:
Tested ACK 9f243cd7fa
Tree-SHA512: 56e4cad0555dec0c565ea5ecc529628ee4f37d20dc660c647fdc6948fbeed8291e6fe290de514bd4c2c7089654d9ce1add607dc9855462828b62be9ee45e4999
40e5f26a3f mapport: remove dead code in DispatchMapPort (Antoine Poinsot)
38fdf7c1fb mapport: drop outdated comments (Antoine Poinsot)
b7b2435290 doc: add release note for #31130 (Antoine Poinsot)
1b6dec98da depends: drop miniupnpc (Antoine Poinsot)
953533d021 doc: remove mentions of UPnP (Antoine Poinsot)
94ad614482 ci: remove UPnP options (Antoine Poinsot)
a9598e5eaa build: drop miniupnpc dependency (Antoine Poinsot)
a5fcfb7385 interfaces: remove now unused 'use_upnp' arg from 'mapPort' (Antoine Poinsot)
038bbe7b20 daemon: remove UPnP support (Antoine Poinsot)
844770b05e qt: remove UPnP settings (Antoine Poinsot)
Pull request description:
This PR removes UPnP IGD support and drops our [miniupnp](https://github.com/miniupnp/miniupnp) dependency.
Miniupnpc is a C library (somewhat) maintained by a single person which had several vulnerabilities in the past (a couple dozens are listed [here](https://cve.mitre.org/cgi-bin/cvekey.cgi?keyword=miniupnp)), some of which directly affected our software ([RCE in 2015](https://bitcoincore.org/en/2024/07/03/disclose_upnp_rce/), [OOM in 2020](https://bitcoincore.org/en/2024/07/31/disclose-upnp-oom/)).
The main purpose of this functionality is to have more (non-data-center) reachable nodes on the network. For a non-technical user running Bitcoin Core at home, the software would automatically open a port on their router to receive incoming connections. This way, users not able to manually open a port on their router would still provide the network with more resources and enhance its diversity.
However, due to past vulnerabilities (and a worry about unknown future ones) in miniupnpc this feature was disabled by default in https://github.com/bitcoin/bitcoin/pull/6795. Having it disabled by default kills (most of?) the purpose of having this functionality in the first place: someone technical enough to understand the `-upnp` startup option or the "enable UPnP" setting is most likely able to open a port on his box in the first place.
In addition, laanwj implemented PCP with a NAT-PMP fallback directly in Bitcoin Core in https://github.com/bitcoin/bitcoin/pull/30043. If we ever want to re-enable automatic NAT traversal by default in Bitcoin Core, this is the best option (and in my opinion the only sane one). The NAT-PMP fallback makes it so compatibility shouldn't be (much of) an issue.
On balance, i believe that keeping this functionality and this barely maintained C dependency has higher costs than benefits. Therefore i propose that we get rid of it.
ACKs for top commit:
jarolrod:
ACK 40e5f26a3f
1440000bytes:
Code Review ACK 40e5f26a3f
laanwj:
Code review ACK 40e5f26a3f
i-am-yuvi:
Tested ACK 40e5f26a3f
Tree-SHA512: 9ea48662775510f5ec6de7af65790f7c8d211603398e9d8c634a86387be81b28081419a95b4d6680d3d7fe6a9f16cec99f16516548201dc7e49781909899a657
Same as in `DecodeSecret`, we should also clear out the secret data from
the vector resulting from the Base58Check parsing for xprv keys. Note
that the if condition is needed in order to avoid UB, see #14242 (commit
d855e4cac8).
