This is required for a future commit. Can be reviewed via the git
options --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space
Also move util::detail::Hex to a proper namespace instead of an inline
namespace so it doesn't conflict with the new util::detail namespace, and
won't create other problems for callers trying to use the inline namespaces.
Also fix a misleading comment in util_string_tests.cpp.
Co-Authored-By: Ryan Ofsky <ryan@ofsky.org>
Pass literal format strings instead of std::string so formats can be
checked at compile time.
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
Co-authored-by: stickies-v <stickies-v@protonmail.com>
fe39acf88f tinyformat: Add compile-time checking for literal format strings (Ryan Ofsky)
184f34f2d0 util: Support dynamic width & precision in ConstevalFormatString (Ryan Ofsky)
Pull request description:
Add compile-time checking for literal format strings passed to `strprintf` and `tfm::format` to make sure the right number of format arguments are passed.
There is still no compile-time checking if non-literal `std::string` or `bilingual_str` format strings are passed, but this is improved in other PRs:
- [#31061](https://github.com/bitcoin/bitcoin/pull/31061) implements compile-time checking for translated strings
- [#31072](https://github.com/bitcoin/bitcoin/pull/31072) increases compile-time checking by using literal strings as format strings, instead of `std::string` and `bilingual_str`
- [#31149](https://github.com/bitcoin/bitcoin/pull/31149) may drop the `std::string` overload for `strprintf` to [require](https://github.com/bitcoin/bitcoin/pull/31149#issuecomment-2444579999) compile-time checking
ACKs for top commit:
maflcko:
re-ACK fe39acf88f🕐
l0rinc:
ACK fe39acf88f
hodlinator:
re-ACK fe39acf88f
Tree-SHA512: f1ddef0c96b9468c5ffe31b42dc19f1922583dd14f2e180b618d992c98614c5cc7db9f9cd917ef503f833bbc7dbec78e4489d0035416dce6840837e1d66d87cb
e80e4c6ff9 validation: Remove RECENT_CONSENSUS_CHANGE validation result (TheCharlatan)
Pull request description:
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 be exposed by the kernel library keeping them around may also be confusing to future users of the library.
ACKs for top commit:
sipa:
ACK e80e4c6ff9
naumenkogs:
ACK e80e4c6ff9
dergoegge:
ACK e80e4c6ff9
ajtowns:
ACK e80e4c6ff9
Tree-SHA512: 0af17c4435bb1b5a4f43600da30545cbbe95a7d642419cabdefabfb82b9335d92262c1c48be7ca2f2a024078ae9447161228b6f951d2f508a51159a31947fb54
fa66e0887c bench: add support for custom data directory (furszy)
ad9c2cceda test, bench: specialize working directory name (furszy)
Pull request description:
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.
A good use case is #28574, where we are benchmarking the wallet
migration process on an HDD.
ACKs for top commit:
maflcko:
re-ACK fa66e0887c
achow101:
ACK fa66e0887c
tdb3:
re ACK fa66e0887c
hodlinator:
re-ACK fa66e0887c
pablomartin4btc:
re-ACK fa66e0887c
Tree-SHA512: 4e87206c07e26fe193c07074ae9eb0cc9c70a58aeea8cf27d18fb5425d77e4b00dbe0e6d6a75c17b427744e9066458b9a84e5ef7b0420f02a4fccb9c5ef4dacc
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 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
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.
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.
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
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
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>
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.
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
552cae243a fuzz: cover `ASMapHealthCheck` in connman target (brunoerg)
33b0f3ae96 fuzz: use `ConsumeNetGroupManager` in connman target (brunoerg)
18c8a0945b fuzz: move `ConsumeNetGroupManager` to util (brunoerg)
fe624631ae fuzz: fuzz `connman` with a non-empty addrman (brunoerg)
0a12cff2a8 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 552cae243a🏀
dergoegge:
Code review ACK 552cae243a
marcofleon:
Code review ACK 552cae243a. Changes match the PR description.
Tree-SHA512: ba861c839602054077e4bf3649763eeb48357cda83ca3ddd32b02a1b61f4e44a0c5070182f001f9bf531d0d64717876279a7de3ddb9de028b343533b89233851
c495731a31 fuzz: wallet: add target for `CreateTransaction` (brunoerg)
3db68e29ec 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 c495731a31
maflcko:
ACK c495731a31 🏻
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.
