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.
- Modify `SipHash_32b` benchmark to use `FastRandomContext` for generating initial values.
- Cycle through and modify each byte of the `uint256` value to ensure no part of it can be optimized away.
The lack of "recursion" (where the method call overwrites the used inputs partially) and the systematic modification of each input byte makes the benchmark usage more reliable and thorough.
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
This is needed in the next commit to add compile-time checking to strprintf
calls, because bitcoin-cli.cpp uses dynamic width in many format strings.
This change is easiest to review ignoring whitespace.
Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
Co-authored-by: Hodlinator <172445034+hodlinator@users.noreply.github.com>
Co-authored-by: l0rinc <pap.lorinc@gmail.com>
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).
Since there is now only two options in the MapPortProtoFlag enum, the
four possible combinations of current and enabled are already covered in
the four `if` branches.
