73e3fa10b4 doc + test: Correct uint256 hex string endianness (Hodlinator)
Pull request description:
This PR is a follow-up to #30436.
Only changes test-code and modifies/adds comments.
Byte order of hex string representation was wrongfully documented as little-endian, but are in fact closer to "big-endian" (endianness is a memory-order concept rather than a numeric concept). `[arith_]uint256` both store their data in arrays with little-endian byte order (`arith_uint256` has host byte order within each `uint32_t` element).
**uint256_tests.cpp** - Avoid using variable from the left side of the condition in the right side. Credits to @maflcko: https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1688273553
**setup_common.cpp** - Skip needless ArithToUint256-conversion. Credits to @stickies-v: https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1688621638
---
<details>
<summary>
## Logical reasoning for endianness
</summary>
1. Comparing an `arith_uint256` (`base_uint<256>`) to a `uint64_t` compares the beginning of the array, and verifies the remaining elements are zero.
```C++
template <unsigned int BITS>
bool base_uint<BITS>::EqualTo(uint64_t b) const
{
for (int i = WIDTH - 1; i >= 2; i--) {
if (pn[i])
return false;
}
if (pn[1] != (b >> 32))
return false;
if (pn[0] != (b & 0xfffffffful))
return false;
return true;
}
```
...that is consistent with little endian ordering of the array.
2. They have the same endianness (but `arith_*` has host-ordering of each `uint32_t` element):
```C++
arith_uint256 UintToArith256(const uint256 &a)
{
arith_uint256 b;
for(int x=0; x<b.WIDTH; ++x)
b.pn[x] = ReadLE32(a.begin() + x*4);
return b;
}
```
### String conversions
The reversal of order which happens when converting hex-strings <=> uint256 means strings are actually closer to big-endian, see the end of `base_blob<BITS>::SetHexDeprecated`:
```C++
unsigned char* p1 = m_data.data();
unsigned char* pend = p1 + WIDTH;
while (digits > 0 && p1 < pend) {
*p1 = ::HexDigit(trimmed[--digits]);
if (digits > 0) {
*p1 |= ((unsigned char)::HexDigit(trimmed[--digits]) << 4);
p1++;
}
}
```
Same reversal here:
```C++
template <unsigned int BITS>
std::string base_blob<BITS>::GetHex() const
{
uint8_t m_data_rev[WIDTH];
for (int i = 0; i < WIDTH; ++i) {
m_data_rev[i] = m_data[WIDTH - 1 - i];
}
return HexStr(m_data_rev);
}
```
It now makes sense to me that `SetHexDeprecated`, upon receiving a shorter hex string that requires zero-padding, would pad as if the missing hex chars where towards the end of the little-endian byte array, as they are the most significant bytes. "Big-endian" string representation is also consistent with the case where `SetHexDeprecated` receives too many hex digits and discards the leftmost ones, as a form of integer narrowing takes place.
### How I got it wrong in #30436
Previously I used the less than (`<`) comparison to prove endianness, but for `uint256` it uses `memcmp` and thereby gives priority to the *lower* bytes at the beginning of the array.
```C++
constexpr int Compare(const base_blob& other) const { return std::memcmp(m_data.data(), other.m_data.data(), WIDTH); }
```
`arith_uint256` is different in that it begins by comparing the bytes from the end, as it is using little endian representation, where the bytes toward the end are more significant.
```C++
template <unsigned int BITS>
int base_uint<BITS>::CompareTo(const base_uint<BITS>& b) const
{
for (int i = WIDTH - 1; i >= 0; i--) {
if (pn[i] < b.pn[i])
return -1;
if (pn[i] > b.pn[i])
return 1;
}
return 0;
}
```
(The commit documents that `base_blob::Compare()` is doing lexicographic ordering unlike the `arith_*`-variant which is doing numeric ordering).
</details>
ACKs for top commit:
paplorinc:
ACK 73e3fa10b4
ryanofsky:
Code review ACK 73e3fa10b4
Tree-SHA512: 121630c37ab01aa7f7097f10322ab37da3cbc0696a6bbdbf2bbd6db180dc5938c7ed91003aaa2df7cf4a4106f973f5118ba541b5e077cf3588aa641bbd528f4e
Follow-up to #30436.
uint256 string representation was wrongfully documented as little-endian due to them being reversed by GetHex() etc, and base_blob::Compare() giving most significance to the beginning of the internal array. They are closer to "big-endian", but this commit tries to be even more precise than that.
uint256_tests.cpp - Avoid using variable from the left side of the condition in the right side.
setup_common.cpp - Skip needless ArithToUint256-conversion.
75648cea5a test: add P2A ProduceSignature coverage (Greg Sanders)
7998ce6b20 Add release note for P2A output feature (Greg Sanders)
71c9b02a04 test: add P2A coverage for decodescript (Greg Sanders)
1349e9ec15 test: Add anchor mempool acceptance test (Greg Sanders)
9d89209937 policy: stop 3rd party wtxid malleability of anchor spend (Greg Sanders)
b60aaf8b23 policy: make anchor spend standard (Greg Sanders)
455fca86cf policy: Add OP_1 <0x4e73> as a standard output type (Greg Sanders)
Pull request description:
This is a sub-feature taken out of the original proposal for ephemeral anchors #30239
This PR makes *spending* of `OP_1 <0x4e73>` (i.e. `bc1pfeessrawgf`) standard. Creation of this output type is already standard.
Any future witness output types are considered relay-standard to create, but not to spend. This preserves upgrade hooks, such as a completely new output type for a softfork such as BIP341. It also gives us a bit of room to use a new output type for policy uses.
This particular sized witness program has no other known use-cases (https://bitcoin.stackexchange.com/a/110664/17078), s it affords insufficient cryptographic security for a secure commitment to data, such as a script or a public key. This makes this type of output "keyless", or unauthenticated.
As a witness program, the `scriptSig` of the input MUST be blank, by BIP141. This helps ensure txid-stability of the spending transaction, which may be required for smart contracting wallets. If we do not use segwit, a miner can simply insert an `OP_NOP` in the `scriptSig` without effecting the result of program execution.
An additional relay restriction is to disallow non-empty witness data, which an adversary may use to penalize the "honest" transactor when RBF'ing the transaction due to the incremental fee requirement of RBF rules.
The intended use-case for this output type is to "anchor" the transaction with a spending child to bring exogenous CPFP fees into the transaction package, encouraging the inclusion of the package in a block. The minimal size of creation and spending of this output makes it an attractive contrast to outputs like `p2sh(OP_TRUE)` and `p2wsh(OP_TRUE)` which
are significantly larger in vbyte terms.
Combined with TRUC transactions which limits the size of child transactions significantly, this is an attractive option for presigned transactions that need to be fee-bumped after the fact.
ACKs for top commit:
sdaftuar:
utACK 75648cea5a
theStack:
re-ACK 75648cea5a
ismaelsadeeq:
re-ACK 75648cea5a via [diff](e7ce6dc070..75648cea5a)
glozow:
ACK 75648cea5a
tdb3:
ACK 75648cea5a
Tree-SHA512: d529de23d20857e6cdb40fa611d0446b49989eaafed06c28280e8fd1897f1ed8d89a4eabbec1bbf8df3d319910066c3dbbba5a70a87ff0b2967d5205db32ad1e
f553e6d86f refactor: remove TxidFromString (stickies-v)
285ab50ace test: replace WtxidFromString with Wtxid::FromHex (stickies-v)
9a0b2a69c4 fuzz: increase FromHex() coverage (stickies-v)
526a87ba6b test: add uint256::FromHex unittest coverage (stickies-v)
Pull request description:
Since fab6ddbee6, `TxidFromString()` has been deprecated because it is less robust than the `transaction_identifier::FromHex()` introduced in [the same PR](https://github.com/bitcoin/bitcoin/pull/30482). Specifically, it tries to recover from length-mismatches, recover from untrimmed whitespace, 0x-prefix and garbage at the end, instead of simply requiring exactly 64 hex-only characters.
In this PR, `TxidFromString` is removed completely to clean up the code and prevent further unsafe usage. Unit and fuzz test coverage on `uint256::FromHex()` and functions that wrap it is increased.
Note: `TxidFromSring` allowed users to prefix strings with "0x", this is no longer allowed for `transaction_identifier::FromHex()`, so a helper function for input validation may prove helpful in the future _(this overlaps with the `uint256::uint256S()` vs `uint256::FromHex()` future cleanup)_. It is not relevant to this PR, though, besides the fact that this unused (except for in tests) functionality is removed.
The only users of `TxidFromString` are:
- `test`, where it is straightforward to drop in the new `FromHex()` methods without much further concern
- `qt` coincontrol. There is no need for input validation here, but txids are not guaranteed to be 64 characters. This is already handled by the existing code, so again, using `FromHex()` here seems quite straightforward.
Addresses @maflcko's suggestion: https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1691826934
Also removes `WtxidFromString()`, which is a test-only helper function.
### Testing GUI changes
To test the GUI coincontrol affected lines, `regtest` is probably the easiest way to quickly get some test coins, you can use e.g.
```
alias cli="./src/bitcoin-cli -regtest"
cli createwallet "coincontrol"
# generate 10 spendable outputs on 1 address
cli generatetoaddress 10 $(cli -rpcwallet=coincontrol getnewaddress)
# generate 10 spendable outputs on another address
cli generatetoaddress 10 $(cli -rpcwallet=coincontrol getnewaddress)
# make previous outputs spendable
cli generatetoaddress 100 $(cli -rpcwallet=coincontrol getnewaddress)
```
ACKs for top commit:
maflcko:
re-ACK f553e6d86f🔻
hodlinator:
ACK f553e6d86f
paplorinc:
ACK f553e6d86f
TheCharlatan:
Nice, ACK f553e6d86f
Tree-SHA512: c1c7e6ea4cbf05cf660ba178ffc4f35f0328f7aa6ad81872e2462fb91a6a22e4681ff64b3d0202a5a9abcb650c939561585cd309164a69ab6081c0765ee271ef
These outputs are called anchors, and allow
key-less anchor spends which are vsize-minimized
versus keyed anchors which require larger outputs
when creating and inputs when spending.
b4dd7ab43e logging: use std::string_view (Anthony Towns)
558df5c733 logging: Apply formatting to early log messages (Anthony Towns)
6cf9b34440 logging: Limit early logging buffer (Anthony Towns)
0b1960f1b2 logging: Add DisableLogging() (Anthony Towns)
6bbc2dd6c5 logging: Add thread safety annotations (Anthony Towns)
Pull request description:
In order to cope gracefully with `Log*()` calls that are invoked prior to logging being fully configured (indicated by calling `StartLogging()` we buffer early log messages in `m_msgs_before_open`. This has a couple of minor issues:
* if there are many such log messages the buffer can become arbitrarily large; this can be a problem for users of libkernel that might not wish to worry about logging at all, and as a result never invoke `StartLogging()`
* early log messages are formatted before the formatting options are configured, leading to inconsistent output
Fix those issues by buffering the log info prior to formatting it, and setting a limit on the size of the buffer (dropping the oldest lines, and reporting the number of lines skipped).
Also adds some thread safety annotations, and the ability to invoke `LogInstance().DisableLogging()` if you want to disable logging entirely, for a minor efficiency improvement.
ACKs for top commit:
maflcko:
re-ACK b4dd7ab43e 🕴
ryanofsky:
Code review ACK b4dd7ab43e
TheCharlatan:
Nice, ACK b4dd7ab43e
Tree-SHA512: 966660181276939225a9f776de6ee0665e44577d2ee9cc76b06c8937297217482e6e426bdc5772d1ce533a0ba093a8556b6a50857d4c876ad8923e432a200440
fae0db0360 fuzz: Deglobalize signature cache in sigcache test (TheCharlatan)
Pull request description:
The body of the fuzz test should ideally be a pure function. If data is persisted in the cache over many iterations, and there is a crash, reproducing it from the input might be difficult. Solve this by getting rid of the global state. This is a follow-up from #30425.
ACKs for top commit:
dergoegge:
utACK fae0db0360
ryanofsky:
Code review ACK fae0db0360
Tree-SHA512: 93dcbb9f2497f13856970469042d6870f04de10fe206827a8db1aae7fc8f3ac7fd900bee7945b5fe4c9e33883268dabb15be7e7bc91cf353ffc0d118cd60e97d
It encapsulates a given linearization in chunked form, permitting arbitrary
subsets of transactions to be removed from the linearization. Its purpose
is adding the Intersect function, which is a crucial operation that will
be used in a further commit to make Linearize improve existing linearizations.
This adds a first version of the overall linearization interface, which given
a DepGraph constructs a good linearization, by incrementally including good
candidate sets (found using AncestorCandidateFinder and SearchCandidateFinder).
This introduces a bespoke fuzzing-focused serialization format for DepGraphs,
and then tests that this format can represent any graph, roundtrips, and then
uses that to test the correctness of DepGraph itself.
This forms the basis for future fuzz tests that need to work with interesting
graphs.
f46b220256 fuzz: Use BasicTestingSetup for coins_view target (TheCharlatan)
9e2a723d5d test: Add arguments for creating a slimmer setup (TheCharlatan)
Pull request description:
This adds arguments to some of the testing setup constructors for creating an environment without networking and a validation interface. This is useful for improving the performance of the utxo snapshot fuzz test, which constructs a new TestingSetup on each iteration.
Using this slimmed down `TestingSetup` in future might also make the tests a bit faster when run in aggregate.
ACKs for top commit:
maflcko:
review ACK f46b220256
dergoegge:
utACK f46b220256
Tree-SHA512: 9dc62512b127b781fc9e2d8ef2b5a9b06ebb927a8294b6d872001c553984a7eb1f348e0257b32435b34b5505b5d0323f73bdd572a673da272d3e1e8538ab49d6
fac0c3d4bf doc: Add release notes for two pull requests (MarcoFalke)
fa7b57e5f5 refactor: Replace ParseHashStr with FromHex (MarcoFalke)
fa90777245 rest: Reject truncated hex txid early in getutxos parsing (MarcoFalke)
fab6ddbee6 refactor: Expose FromHex in transaction_identifier (MarcoFalke)
fad2991ba0 refactor: Implement strict uint256::FromHex() (MarcoFalke)
fa103db2bb scripted-diff: Rename SetHex to SetHexDeprecated (MarcoFalke)
fafe4b8051 test: refactor: Replace SetHex with uint256 constructor directly (MarcoFalke)
Pull request description:
In `rest_getutxos` truncated txids such as `aa` or `ff` are accepted. This is brittle at best.
Fix it by rejecting any truncated (or overlarge) input.
----
Review note: This also starts a major refactor to rework hex parsing in Bitcoin Core, meaning that a few refactor commits are included as well. They are explained individually in the commit message and the work will be continued in the future.
ACKs for top commit:
stickies-v:
re-ACK fac0c3d4bf - only doc and test updates to address review comments, thanks!
hodlinator:
ACK fac0c3d4bf
Tree-SHA512: 473feb3fcf6118443435d1dd321006135b0b54689bfbbcb1697bb5811a449bef51f475c715de6911ff3c4ea3bdb75f601861ff93347bc4414d6b9e5298105dd7
This is a safe replacement of the previous SetHex, which now returns an
optional to indicate success or failure.
The code is similar to the ParseHashStr helper, which will be removed in
a later commit.
c85accecaf [refactor] delete EraseTxNoLock, just use EraseTx (glozow)
6ff84069a5 remove obsoleted TxOrphanage::m_mutex (glozow)
61745c7451 lock m_recent_confirmed_transactions using m_tx_download_mutex (glozow)
723ea0f9a5 remove obsoleted hashRecentRejectsChainTip (glozow)
18a4355250 update recent_rejects filters on ActiveTipChange (glozow)
36f170d879 add ValidationInterface::ActiveTipChange (glozow)
3eb1307df0 guard TxRequest and rejection caches with new mutex (glozow)
Pull request description:
See #27463 for full project tracking.
This contains the first few commits of #30110, which require some thinking about thread safety in review.
- Introduce a new `m_tx_download_mutex` which guards the transaction download data structures including `m_txrequest`, the rolling bloom filters, and `m_orphanage`. Later this should become the mutex guarding `TxDownloadManager`.
- `m_txrequest` doesn't need to be guarded using `cs_main` anymore
- `m_recent_confirmed_transactions` doesn't need its own lock anymore
- `m_orphanage` doesn't need its own lock anymore
- Adds a new `ValidationInterface` event, `ActiveTipChanged`, which is a synchronous callback whenever the tip of the active chainstate changes.
- Flush `m_recent_rejects` and `m_recent_rejects_reconsiderable` on `ActiveTipChanged` just once instead of checking the tip every time `AlreadyHaveTx` is called. This should speed up calls to that function (no longer comparing a block hash each time) and removes the need to lock `cs_main` every time it is called.
Motivation:
- These data structures need synchronization. While we are holding `m_tx_download_mutex`, these should hold:
- a tx hash in `m_txrequest` is not also in `m_orphanage`
- a tx hash in `m_txrequest` is not also in `m_recent_rejects` or `m_recent_confirmed_transactions`
- In the future, orphan resolution tracking should also be synchronized. If a tx has an entry in the orphan resolution tracker, it is also in `m_orphanage`, and not in `m_txrequest`, etc.
- Currently, `cs_main` is used to e.g. sync accesses to `m_txrequest`. We should not broaden the scope of things it locks.
- Currently, we need to know the current chainstate every time we call `AlreadyHaveTx` so we can decide whether we should update it. Every call compares the current tip hash with `hashRecentRejectsChainTip`. It is more efficient to have a validation interface callback that updates the rejection filters whenever the chain tip changes.
ACKs for top commit:
instagibbs:
reACK c85accecaf
dergoegge:
Code review ACK c85accecaf
theStack:
Light code-review ACK c85accecaf
hebasto:
ACK c85accecaf, I have reviewed the code and it looks OK.
Tree-SHA512: c3bd524b5de1cafc9a10770dadb484cc479d6d4c687d80dd0f176d339fd95f73b85cb44cb3b6b464d38a52e20feda00aa2a1da5a73339e31831687e4bd0aa0c5
SetHex is fragile, because it accepts any non-hex input or any length of
input, without error feedback. This can lead to issues when the input is
truncated or otherwise corrupted.
Document the problem by renaming the method.
In the future, the fragile method should be removed from the public
interface.
-BEGIN VERIFY SCRIPT-
sed -i 's/SetHex/SetHexDeprecated/g' $( git grep -l SetHex ./src )
-END VERIFY SCRIPT-
09ce3501fa fix: Make TxidFromString() respect string_view length (Hodlinator)
01e314ce0a refactor: Change base_blob::SetHex() to take std::string_view (Hodlinator)
2f5577dc2e test: uint256 - Garbage suffixes and zero padding (Hodlinator)
f11f816800 refactor: Make uint256_tests no longer use deprecated BOOST_CHECK() (Hodlinator)
f0eeee2dc1 test: Add test for TxidFromString() behavior (Ryan Ofsky)
Pull request description:
### Problem
Prior to this, `TxidFromString()` was passing `string_view::data()` into `uint256S()` which meant it would only receive the a naked `char*` pointer and potentially scan past the `string_view::length()` until it found a null terminator (or some other non-hex character).
Appears to have been a fully dormant bug as callers were either passing a string literal or `std::string` directly to `TxidFromFromString()`, meaning a null terminator always existed at `pointer[length()]`. Bug existed since original merge of `TxidFromString()`.
### Solution
Make `uint256S()` (and `base_blob::SetHex()`) take and operate on `std::string_view` instead of `const char*` and have `TxidFromString()` pass that in.
(PR was prompted by comment in https://github.com/bitcoin/bitcoin/pull/30377#issuecomment-2208857200 (referring to https://github.com/bitcoin/bitcoin/pull/28922#discussion_r1404437378)).
ACKs for top commit:
maflcko:
re-ACK 09ce3501fa🕓
paplorinc:
ACK 09ce3501fa
ryanofsky:
Code review ACK 09ce3501fa. I think the current code changes are about as small as you could make to fix the bug without introducing a string copy, and the surrounding test improvements are all very nice and welcome.
Tree-SHA512: c2c10551785fb6688d1e2492ba42a8eee4c19abbe8461bb0774d56a70c23cd6b0718d2641632890bee880c06202dee148126447dd2264eaed4f5fee7e1bcb581
Prior to this, passing string_view::data() into uint256S() meant the latter would only receive the a naked char* pointer and potentially scan past the string_view::length() until it found a null terminator (or some other non-hex character).
Appears to have been a fully dormant bug as callers were either passing a string literal or std::string directly to TxidFromFromString(), meaning null terminator always existed at pointer[length()]. Bug existed since original merge of TxidFromString(), discussed in https://github.com/bitcoin/bitcoin/pull/28922#discussion_r1404437378.
The body of the fuzz test should ideally be a pure function. If data is
persisted in the cache over many iterations, and there is a crash,
reproducing it from the input might be difficult.
Adds more testing options for creating an environment without networking
and a validation interface. This is useful for improving the performance
of the utxo snapshot fuzz test, which constructs a new TestingSetup on
each iteration.
8607773750 Add fuzz test for FSChaCha20Poly1305 (stratospher)
c807f33228 Add fuzz test for AEADChacha20Poly1305 (stratospher)
Pull request description:
This PR adds fuzz tests for `AEADChaCha20Poly1305` and `FSChaCha20Poly1305` introduced in #28008.
Run using:
```
$ FUZZ=crypto_aeadchacha20poly1305 src/test/fuzz/fuzz
$ FUZZ=crypto_fschacha20poly1305 src/test/fuzz/fuzz
```
ACKs for top commit:
dergoegge:
tACK 8607773750
marcofleon:
Tested ACK 8607773750. Ran both targets for ~200 CPU hours. Coverage of intended targets looks good to me. The simulation of damaged keys and checks that follow seem useful as well.
Tree-SHA512: b6b85661d896e653caeed330f941fde665fc2bbd97ecd340808a3f365c469fe9134aa77316569a771dc36d1158cac1a5f76700bcfc45fff12aef07562e48feb9
16bd283b3a Reapply "test: p2p: check that connecting to ourself leads to disconnect" (Sebastian Falbesoner)
0dbcd4c148 net: prevent sending messages in `NetEventsInterface::InitializeNode` (Sebastian Falbesoner)
66673f1c13 net: fix race condition in self-connect detection (Sebastian Falbesoner)
Pull request description:
This PR fixes a recently discovered race condition in the self-connect detection (see #30362 and #30368).
Initiating an outbound network connection currently involves the following steps after the socket connection is established (see [`CConnman::OpenNetworkConnection`](bd5d1688b4/src/net.cpp (L2923-L2930)) method):
1. set up node state
2. queue VERSION message (both steps 1 and 2 happen in [`InitializeNode`](bd5d1688b4/src/net_processing.cpp (L1662-L1683)))
3. add new node to vector `m_nodes`
If we connect to ourself, it can happen that the sent VERSION message (step 2) is received and processed locally *before* the node object is added to the connection manager's `m_nodes` vector (step 3). In this case, the self-connect remains undiscovered, as the detection doesn't find the outbound peer in `m_nodes` yet (see `CConnman::CheckIncomingNonce`).
Fix this by swapping the order of 2. and 3., by taking the `PushNodeVersion` call out of `InitializeNode` and doing that in the `SendMessages` method instead, which is only called for `CNode` instances in `m_nodes`.
The temporarily reverted test introduced in #30362 is readded. Fixes #30368.
Thanks go to vasild, mzumsande and dergoegge for suggestions on how to fix this (see https://github.com/bitcoin/bitcoin/issues/30368#issuecomment-2200625017 ff. and https://github.com/bitcoin/bitcoin/pull/30394#discussion_r1668290789).
ACKs for top commit:
naiyoma:
tested ACK [16bd283b3a), built and tested locally, test passes successfully.
mzumsande:
ACK 16bd283b3a
tdb3:
ACK 16bd283b3a
glozow:
ACK 16bd283b3a
dergoegge:
ACK 16bd283b3a
Tree-SHA512: 5b8aced6cda8deb38d4cd3fe4980b8af505d37ffa0925afaa734c5d81efe9d490dc48a42e1d0d45dd2961c0e1172a3d5b6582ae9a2d642f2592a17fbdc184445
fa690c8e53 test: [refactor] Pass TestOpts (MarcoFalke)
Pull request description:
Currently optional test context setup settings are passed by adding a new optional argument to the constructors. For example `extra_args`. This is problematic, because:
* Adding more optional settings in the future requires touching all affected constructors, increasing their verbosity.
* Setting only a later option requires setting the earlier ones.
* Clang-tidy named args passed to `std::make_unique` are not checked.
Fix all issues by adding a new struct `TestOpts`, which holds all options. Notes:
* The chain type is not an option in the struct for now, because the default values vary.
* The struct holds all possible test options globally. Not all fields may be used by all constructors. Albeit harmless, it is up to the test author to not set a field that is unused.
ACKs for top commit:
kevkevinpal:
utACK [fa690c8](fa690c8e53)
dergoegge:
utACK fa690c8e53
TheCharlatan:
Nice, ACK fa690c8e53
Tree-SHA512: 8db8efa5dff854a73757d3f454f8f902e41bb4358f5f9bae29dbb3e251e20ee93489605de51d0822ba31d97835cd15526a29c075278dd6a8bbde26134feb4f49
bc34bc2888 fuzz: limit the number of nested wrappers in descriptors (Antoine Poinsot)
8d7340105f fuzz: limit the number of sub-fragments per fragment for descriptors (Antoine Poinsot)
Pull request description:
Some of the logic in the miniscript module is quadratic. It only becomes an issue for very large uninteresting descriptors (like a `thresh` with 130k sub-fragments or a fragment with more than 60k nested `j:` wrappers).
This PR fixes the two types of fuzz timeouts reported by Marco in https://github.com/bitcoin/bitcoin/issues/28812 by trying to pinpoint the problematic descriptors through a simple analysis of the string, without limiting the size of the string itself. This is the same approach as was adopted for limiting the depth of derivation paths.
ACKs for top commit:
dergoegge:
utACK bc34bc2888
stickies-v:
Light ACK bc34bc2888
marcofleon:
Code review ACK bc34bc2888. The added comments are useful, thanks for those. Tested on the three inputs in https://github.com/bitcoin/bitcoin/issues/28812 that caused the timeouts.
Tree-SHA512: 8811c7b225684c5ecc1eb1256cf39dfa60d4518161e70210086c8a01b38927481ebe747af86aa5f4803187672d43fadabcfdfbf4e3b049738d629a25143f0e77
09370529fb fuzz: mini_miner_selection fixups. (glozow)
de273d5300 MiniMiner: use FeeFrac in AncestorFeerateComparator (glozow)
Pull request description:
Closes #30284. Closes #30367, see https://github.com/bitcoin/bitcoin/issues/30367#issuecomment-2217459257
Previously, we were only comparing feerates up to 1/1000 precision, since CFeeRate comparison just looks at their respective nSatoshisPerK. This could lead to MiniMiner selecting packages in the wrong order (i.e. by txid) if their feerates were less than 0.001sat/vB different. Fix this by creating + comparing `FeeFrac`s instead.
Also, `FeeFrac::Mul` doesn't have the overflow problem.
Also added a few minor fuzzer fixups that caught my eye while I was debugging this.
ACKs for top commit:
ismaelsadeeq:
Tested ACK 09370529fb
murchandamus:
ACK 09370529fb with nits
dergoegge:
tACK 09370529fb
Tree-SHA512: e5b6d6c3f7289f30cd8280d0a47cd852d0180b83d1b27ff9514f50c97103b0f069484e48cba2ca3a57419beadc1996c1b9dd8d0a0f34bc4f4223d8adaf414ce5
The script building logic performs a quadratic number of copies in the
number of nested wrappers in the miniscript. Limit the number of nested
wrappers to avoid fuzz timeouts.
Thanks to Marco Falke for reporting the fuzz timeouts and providing a
minimal input to reproduce.
