0ea84bc362f395fd247623c22942eb5ca3d1b874 test: explicitly check boolean verbosity is disallowed (tdb3)
7a2e6b68cd928a32dd307273727a85890a74c7da doc: add rpc guidance for boolean verbosity avoidance (tdb3)
698f302df8b7cc6e4077c911d3c129960bdb5e07 rpc: disallow boolean verbosity in getorphantxs (tdb3)
63f5e6ec795f3d5ddfed03f3c51f79ad7a51db1e test: add entry and expiration time checks (tdb3)
808a708107e65e52f54373d2e26f807cf1e444e1 rpc: add entry time to getorphantxs (tdb3)
56bf3027144b4fa6ce9586d3d249b275acb7bcce refactor: rename rpc_getorphantxs to rpc_orphans (tdb3)
7824f6b07703463707bb4f10577ff6d34118e248 test: check that getorphantxs is hidden (tdb3)
ac68fcca701e0b3b90c6bb81d66bfa38b57f39bf 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 0ea84bc362f395fd247623c22942eb5ca3d1b874
glozow:
utACK 0ea84bc362f395fd247623c22942eb5ca3d1b874
rkrux:
tACK 0ea84bc362f395fd247623c22942eb5ca3d1b874
itornaza:
tACK 0ea84bc362f395fd247623c22942eb5ca3d1b874
Tree-SHA512: e48a088f333ebde132923072da58e970461e74362d0acebbc799c3043d5727cdf5f28e82b43cb38bbed27c603df6710695dba91ff0695e623ad168e985dce08e
0f4bc635854597e15ea6968767fc4e5cf5bdd790 [fuzz] txdownloadman and txdownload_impl (glozow)
699643f23a1bd0346e36bd90c83ba1b0b0a5c3fe [unit test] MempoolRejectedTx (glozow)
fa584cbe727b62853a410623b3d7c738e11cbffd [p2p] add TxDownloadOptions bool to make TxRequestTracker deterministic (glozow)
f803c8ce8dd88d9d0fd7857f63d76045b1e2bcaa [p2p] filter 1p1c for child txid in recent rejects (glozow)
5269d57e6d78e90baa0b40629f60a2d1d63e2992 [p2p] don't process orphan if in recent rejects (glozow)
2266eba43a973345351f2b0a8296523fb7de5576 [p2p] don't find 1p1cs for reconsiderable txns that are AlreadyHaveTx (glozow)
fa7027d0fc1fb2eb4148ba9741e1736f61d7e164 [refactor] add CheckIsEmpty and GetOrphanTransactions, remove access to TxDownloadMan internals (glozow)
969b07237b990b7eb6f3d24914ccc872202d8a0f [refactor] wrap {Have,Get}TxToReconsider in txdownload (glozow)
f150fb94e7dbb3c1f4fca32a0abf063943ca676d [refactor] make AlreadyHaveTx and Find1P1CPackage private to TxDownloadImpl (glozow)
1e08195135bc54f7a8b28560ae10943b1fef0d83 [refactor] move new tx logic to txdownload (glozow)
257568eab5baba07571fe2c68759e843d215d4a9 [refactor] move invalid package processing to TxDownload (glozow)
c4ce0c1218d0a3a2e9b22701f26391b8a9107196 [refactor] move invalid tx processing to TxDownload (glozow)
c6b21749ca0aea70908773d865e67511ca141ae6 [refactor] move valid tx processing to TxDownload (glozow)
a8cf3b6e845741e4b992beced564397779bfb7da [refactor] move Find1P1CPackage to txdownload (glozow)
f497414ce76a4cf44fa669e3665746cc17710fc6 [refactor] put peerman tasks at the end of ProcessInvalidTx (glozow)
6797bc42a762f431a986852fa74b1775aea8ba38 [p2p] restrict RecursiveDynamicUsage of orphans added to vExtraTxnForCompact (glozow)
798cc8f5aac9bf2111ea88d4a4c3817d34e089e2 [refactor] move Find1P1CPackage into ProcessInvalidTx (glozow)
416fbc952b209817a37e76c09fff5d17be7a72d0 [refactor] move new orphan handling to ProcessInvalidTx (glozow)
c8e67b9169bddc0bdfefa10e9cf7f9c22847e237 [refactor] move ProcessInvalidTx and ProcessValidTx definitions down (glozow)
3a41926d1b59dc9bbabc38cdc461c169426d94e7 [refactor] move notfound processing to txdownload (glozow)
042a97ce7fc672021cdb1dee62a550ef19c208fb [refactor] move tx inv/getdata handling to txdownload (glozow)
58e09f244b4bf07d31bc8dd4e939c2dc4dc74f3a [p2p] don't log tx invs when in IBD (glozow)
288865338f50d5b00758236aa4a59546a41c88c1 [refactor] rename maybe_add_extra_compact_tx to first_time_failure (glozow)
f48d36cd97e9b27dfa105c35e0fe67cba47056d1 [refactor] move peer (dis)connection logic to TxDownload (glozow)
f61d9e4b4b80842d520c490a1012044c0816679a [refactor] move AlreadyHaveTx to TxDownload (glozow)
84e4ef843db3443278d6eb70ff89fa254fcc6631 [txdownload] add read-only reference to mempool (glozow)
af918349de52e654927d50279de64f548a8b53d6 [refactor] move ValidationInterface functions to TxDownloadManager (glozow)
f6c860efb1221e1eadc3acebd6b0b885b9cc291a [doc] fix typo in m_lazy_recent_confirmed_transactions doc (glozow)
5f9004e1550f726ca9dc9a08c865fa8f2e4b92e8 [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 0f4bc635854597e15ea6968767fc4e5cf5bdd790
theStack:
ACK 0f4bc635854597e15ea6968767fc4e5cf5bdd790
instagibbs:
reACK 0f4bc635854597e15ea6968767fc4e5cf5bdd790
naumenkogs:
ACK 0f4bc635854597e15ea6968767fc4e5cf5bdd790
Tree-SHA512: 84ab8ef8a0fc705eb829d7f7d6885f28944aaa42b03172f256a42605677b3e783919bb900d4e3b8589f85a0c387dfbd972bcd61d252d44a88c6aaa90e4bf920f
915640e191b6a17a245f0502bc399d82a6502ccf depends: zeromq: don't install .pc files and remove patches for them (Cory Fields)
6b8a74463b5ce5d5d22263f220900f3587f730bd 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 915640e191b6a17a245f0502bc399d82a6502ccf
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-
9f243cd7fa6654e3b71ba6bff82cceed547c5d53 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 9f243cd7fa6654e3b71ba6bff82cceed547c5d53 🗜
brunoerg:
crACK 9f243cd7fa6654e3b71ba6bff82cceed547c5d53
marcofleon:
Tested ACK 9f243cd7fa6654e3b71ba6bff82cceed547c5d53
Tree-SHA512: 56e4cad0555dec0c565ea5ecc529628ee4f37d20dc660c647fdc6948fbeed8291e6fe290de514bd4c2c7089654d9ce1add607dc9855462828b62be9ee45e4999
40e5f26a3ff77e50df808f6f850c617aec2df203 mapport: remove dead code in DispatchMapPort (Antoine Poinsot)
38fdf7c1fb1946820236c319ad44c7bcbf0c6a98 mapport: drop outdated comments (Antoine Poinsot)
b7b24352906f1dba64826e7a093069b5bfc504dc doc: add release note for #31130 (Antoine Poinsot)
1b6dec98da3025c19951daf209347cecf1f0c6ab depends: drop miniupnpc (Antoine Poinsot)
953533d0214819a05d36672d295821ef06ced8d6 doc: remove mentions of UPnP (Antoine Poinsot)
94ad614482f4f1f9d207509a209badbc2fb5700d ci: remove UPnP options (Antoine Poinsot)
a9598e5eaab861fd6e6ce279f1282a83eec407d6 build: drop miniupnpc dependency (Antoine Poinsot)
a5fcfb7385c10d83a294cb2bb2248d06b2ab931e interfaces: remove now unused 'use_upnp' arg from 'mapPort' (Antoine Poinsot)
038bbe7b20074cc2201585dcc631e81b9e1e306c daemon: remove UPnP support (Antoine Poinsot)
844770b05ebc34789dc46d70cd6398089539c915 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 40e5f26a3ff77e50df808f6f850c617aec2df203
i-am-yuvi:
Tested ACK 40e5f26a3ff77e50df808f6f850c617aec2df203
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
d855e4cac8303ad4e34ac31cfa7634286589ce99).
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.
552cae243a1bf26bfec03eccd1458f3bf33e01dc fuzz: cover `ASMapHealthCheck` in connman target (brunoerg)
33b0f3ae966ffa50b55489eb867c4d93c0ed3489 fuzz: use `ConsumeNetGroupManager` in connman target (brunoerg)
18c8a0945bda554e121c2a684105dffd55505cd7 fuzz: move `ConsumeNetGroupManager` to util (brunoerg)
fe624631aeb4b5fbad732ad6476c5cd986674b4f fuzz: fuzz `connman` with a non-empty addrman (brunoerg)
0a12cff2a8e54453de1f17e9c0e87e54bbe25a34 fuzz: move `AddrManDeterministic` to util (brunoerg)
Pull request description:
### Motivation
Currently, we fuzz connman with an addrman from `NodeContext`. However,
fuzzing connman with only empty addrman might not be effective, especially
for functions like `GetAddresses` and other ones that plays with addrman. Also,
we do not fuzz connman with ASMap, what would be good for functions that need
`GetGroup`, or even for addrman. Without it, I do not see how effective would be
fuzzing `ASMapHealthCheck`, for example.
### Changes
- Move `AddrManDeterministic` and `ConsumeNetGroupManager` to util.
- Use `ConsumeNetGroupManager` in connman target to construct a netgroupmanager
and use it for `ConnmanTestMsg`.
- Use `AddrManDeterministic` in connman target to create an addrman. It does
not slow down as "filling" the addrman (e.g. with `FillAddrman`).
- Add coverage for `ASMapHealthCheck`.
ACKs for top commit:
maflcko:
review ACK 552cae243a1bf26bfec03eccd1458f3bf33e01dc 🏀
dergoegge:
Code review ACK 552cae243a1bf26bfec03eccd1458f3bf33e01dc
marcofleon:
Code review ACK 552cae243a1bf26bfec03eccd1458f3bf33e01dc. Changes match the PR description.
Tree-SHA512: ba861c839602054077e4bf3649763eeb48357cda83ca3ddd32b02a1b61f4e44a0c5070182f001f9bf531d0d64717876279a7de3ddb9de028b343533b89233851
by passing an additional argument of "outonly" or "o".
This has been requested in order to keep the output within screen limits when running -netinfo
as a live dashboard, i.e. with `watch`.
Also allow passing "h" in addition to "help" to see the help documentation.
fa69a5f4b76a4e2a02db6c32d9c3311ce5fe29bd util: Treat Assume as Assert when evaluating at compile-time (MarcoFalke)
Pull request description:
There is no downside or cost of treating an `Assume` at compile-time as an `Assert` and it may even help to find bugs while compiling without `ABORT_ON_FAILED_ASSUME`.
This is also required for https://github.com/bitcoin/bitcoin/pull/31093
ACKs for top commit:
dergoegge:
ACK fa69a5f4b76a4e2a02db6c32d9c3311ce5fe29bd
brunoerg:
ACK fa69a5f4b76a4e2a02db6c32d9c3311ce5fe29bd
marcofleon:
ACK fa69a5f4b76a4e2a02db6c32d9c3311ce5fe29bd
Tree-SHA512: 17604403f841343a6d5b6e5d777e1760d38e0c27dc1fd4479e3741894fba40cdb1fb659cf24519a51d051bd5884a75992d1227ec9fa40fbf53bc619fbfb304ad
c495731a316d9c97ee05a08cf5087c5535f84bd4 fuzz: wallet: add target for `CreateTransaction` (brunoerg)
3db68e29ec632b29f5417dbef095520e75adc26d wallet: move `ImportDescriptors`/`FuzzedWallet` to util (brunoerg)
Pull request description:
This PR adds a fuzz target for the `CreateTransaction` function. It is a regression target for https://github.com/bitcoin/bitcoin/pull/27271 and can be testing by applying:
```diff
@@ -1110,7 +1110,7 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal(
// This can only happen if feerate is 0, and requested destinations are value of 0 (e.g. OP_RETURN)
// and no pre-selected inputs. This will result in 0-input transaction, which is consensus-invalid anyways
if (selection_target == 0 && !coin_control.HasSelected()) {
- return util::Error{_("Transaction requires one destination of non-0 value, a non-0 feerate, or a pre-selected input")};
+ // return util::Error{_("Transaction requires one destination of non-0 value, a non-0 feerate, or a pre-selected input")};
}
```
Also, it moves `ImportDescriptors` function to `src/wallet/test/util.h` to avoid to duplicate same code.
ACKs for top commit:
marcofleon:
ACK c495731a316d9c97ee05a08cf5087c5535f84bd4
maflcko:
ACK c495731a316d9c97ee05a08cf5087c5535f84bd4 🏻
Tree-SHA512: a439f947b91b01e327e18cd18e63d5ce49f2cb9ca16ca9d56fe337b8cff239b3af4db18fe89478fe5faa5549d37ca935bd321913db7646fbf6818f825cb5d878
The txdownload_impl is similar but allows us to check specific
invariants within its implementation. It will also change a lot more
than the external interface (txdownloadman) will, so we will add more to
this target later.
Avoid the fuzzer situation where:
1. Orphanage has 2 transactions with the same txid, one with witness,
one without witness.
2. The transaction with witness is found to have
`TX_INPUTS_NOT_STANDARD` error. The txid is added to recent rejects
filter, and the tx with witness is deleted from orphanage.
3. A low feerate parent is found. Find1P1CPackage finds the transaction
with no witness in orphanage, and returns the package.
4. net_processing has just been handed a package in which the child is
already in recent rejects.
This is a slight behavior change: if a transaction is in both
reconsiderable rejects and AlreadyHaveTx in another way, we don't try to
return a 1p1c package. This is the correct thing to do, as we don't want
to reconsider transactions that have multiple things wrong with them.
For example, if a transaction is low feerate, and then later found to
have a bad signature, we shouldn't try it again in a package.
