⚠️ maflcko opened an issue: "fuzz: Apply HasTooManySubFrag (et al) to miniscript_string (et al)"
(https://github.com/bitcoin/bitcoin/issues/30498)
Should the check be applied to miniscript_string, for example the `miniscript_string/ae395bdc087e233d7f8e1844d4814b2c00cc9d21` input, as well?

_Originally posted by @maflcko in https://github.com/bitcoin/bitcoin/issues/30197#issuecomment-2233404249_


Seems useful to at least apply to that fuzz target as well. In theory it could be applied to parse_univalue as well (reverting commit a1b8a917b176ee36961203ccee96457d85102e60).

💬 josibake commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#issuecomment-2242659405)
Update https://github.com/bitcoin/bitcoin/commit/56267cf84281789186035f09898cce3d749748ec -> https://github.com/bitcoin/bitcoin/commit/e998ca2c53bb8320e102a02a06182f8bc49a4f90 ([apply-taptweak-method-05](https://github.com/josibake/bitcoin/tree/apply-taptweak-method-05) -> [apply-taptweak-method-06](https://github.com/josibake/bitcoin/tree/apply-taptweak-method-06) ([compare](https://github.com/josibake/bitcoin/compare/apply-taptweak-method-05..josibake:apply-taptweak-method-06))

* Fix CI fai
...

💬 josibake commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#issuecomment-2242668252)
Rebased on master https://github.com/bitcoin/bitcoin/commit/e998ca2c53bb8320e102a02a06182f8bc49a4f90 -> https://github.com/bitcoin/bitcoin/commit/9afa2c9e50370b2918377f3f3eac0950a4296ec0 ([apply-taptweak-method-06](https://github.com/josibake/bitcoin/tree/apply-taptweak-method-06) -> [apply-taptweak-method-06](https://github.com/josibake/bitcoin/tree/apply-taptweak-method-06-rebase) ([compare](https://github.com/josibake/bitcoin/compare/apply-taptweak-method-05..josibake:apply-taptweak-method-06
...

💬 paplorinc commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1686322412)
We have many different flavors of hex parsers, to document the differences better, let's add a test where e.g. `TryParseHash` would behave differently, e.g. `"0123456789 ABCDEF"` where `TryParseHex` continues, `SetHexFragile` stops:
```C++
TryParseHex: 0123456789abcdef
SetHexFragile: efcdab8967452301000000000000000000000000000000000000000000000000
```

See: https://github.com/bitcoin/bitcoin/pull/30482/commits/801404719dbdbbc327b2f1bc24cb11b6d52e3f27#r1685500849

🤔 paplorinc requested changes to a pull request: "fix: Make TxidFromString() respect string_view length"
(https://github.com/bitcoin/bitcoin/pull/30436#pullrequestreview-2191194232)
Thanks for tackling these, please my comments, most of them originally posted to https://github.com/bitcoin/bitcoin/pull/30482/files

💬 paplorinc commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1686329623)
previous version was cleaner in my opinion.
Since str is random access, we could simplify to:
```suggestion
while (digits < trimmed.size() && ::HexDigit(trimmed[digits]) != -1)
++digits;
```


See: https://github.com/bitcoin/bitcoin/pull/30482/commits/f77d961d926405637dfbdfb9a9baea1fab4f1b7b#r1685498257

💬 paplorinc commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1686320395)
I see that `BOOST_CHECK` was use throughout, but we could use `BOOST_CHECK_EQUAL` in a few places for better error messages - at least for the new code:
```suggestion
BOOST_CHECK_EQUAL(uint256S("\t \n \n \f\n\r\t\v\t 0x"+R1L.ToString()+" \t \n \n \f\n\r\t\v\t "), R1L);
```

We could do the old ones in a different PR, but let's not introduce already deprecated code.

See: https://github.com/bitcoin/bitcoin/pull/30482/commits/801404719dbdbbc327b2f1bc24cb11b6d52e3f27#r1685455576

💬 paplorinc commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1686328446)
we could keep the const here as well (and simplify a few other structures, please see my other comments for details):
```C++
template <unsigned int BITS>
void base_blob<BITS>::SetHex(const std::string_view str)
{
const auto trimmed = util::RemovePrefixView(util::TrimStringView(str), "0x");
size_t digits = 0;
while (digits < trimmed.size() && ::HexDigit(trimmed[digits]) != -1)
++digits;
for (auto it = m_data.begin(); it < m_data.end(); ++it) {
auto hi = (
...

💬 paplorinc commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1686339331)
we should be able to keep the const in most places:
```suggestion
inline uint160 uint160S(const std::string_view str)
```

💬 paplorinc commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1686345800)
We shouldn't need zeroing, if we're processing the values exhaustively - but if we do, we could use
```suggestion
SetNull();
```

See: https://github.com/bitcoin/bitcoin/pull/30482/files#r1685396384

💬 paplorinc commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1686332889)
I see this uses little-endian encoding, while e.g. `TryParseHex` uses big-endian, right?
Do you happen to know what the reason is? Should we maybe document this?
Do we need to keep the endianness in every scenario (since that's why we need the digit counting at the beginning) or could we use big-endian in any of the usages, e.g. if these hex strings aren't persisted or someting (in a different PR, of course)?

See: https://github.com/bitcoin/bitcoin/pull/30482/commits/f77d961d926405637dfbdfb
...

💬 paplorinc commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1686364804)
we could avoid double assignment (and casting) by using a temp variable for the high nibble as such:
```C++
for (auto it = m_data.begin(); it < m_data.end(); ++it) {
auto hi = (digits > 0) ? ::HexDigit(trimmed[--digits]) : 0;
*it = (digits > 0) ? hi | (::HexDigit(trimmed[--digits]) << 4) : hi;
}
```

💬 paplorinc commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1686365416)
we should be able to use `m_data.begin()` and `m_data.end()` instead

💬 paplorinc commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1686370155)
posted to https://github.com/bitcoin/bitcoin/pull/30436, please resolve it here

💬 paplorinc commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1686370312)
posted to https://github.com/bitcoin/bitcoin/pull/30436, please resolve it here

💬 paplorinc commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1686370510)
posted to https://github.com/bitcoin/bitcoin/pull/30436, please resolve it here

💬 paplorinc commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1686370769)
posted to https://github.com/bitcoin/bitcoin/pull/30436, please resolve it here

💬 paplorinc commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1686370868)
posted to https://github.com/bitcoin/bitcoin/pull/30436, please resolve it here

💬 paplorinc commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1686371093)
posted to https://github.com/bitcoin/bitcoin/pull/30436, please resolve it here

💬 hebasto commented on pull request "locks: introduce mutex for tx download, flush rejection filters once per tip change":
(https://github.com/bitcoin/bitcoin/pull/30111#discussion_r1686372826)
As far as I understand, `AssertLockNotHeld(m_mempool.cs);` is an attempt to force the mutex hierarchy mentioned in here: https://github.com/bitcoin/bitcoin/blob/c85accecafc20f6a6ae94bdf6cdd3ba9747218fd/src/net_processing.cpp#L784

As `m_mempool.cs` is not used in this function, I suggest to add a comment to prevent tempting of removing this line on the "clean up" purpose in the future.