💬 stickies-v commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#issuecomment-2242614404)
re-ACK 788fe9cc9ab979c5e14f544ae05dc46436892b81

No changes except for commit message update.

📝 maflcko opened a pull request: " rpc: Return precise loadtxoutset error messages "
(https://github.com/bitcoin/bitcoin/pull/30497)
The error messages should never happen in normal operation. However, if
they do, they are helpful to return to the user to debug the issue. For
example, to notice a truncated file.

This fixes https://github.com/bitcoin/bitcoin/issues/28621

Also includes a minor refactor commit.

💬 fanquake commented on pull request "depends: build zeromq with CMake":
(https://github.com/bitcoin/bitcoin/pull/29723#discussion_r1686334519)
> My question was about a change that introduced such non-determinism, given that the source code is the same.

The change is CMake. It's passing the full/path/to/the/source/files during compilation, rather than something like src/file which autotools does, which in turn causes the `__FILE__` macros in the zmq error handling code expand to non-deterministic paths.

💬 vasild commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#issuecomment-2242633991)
`ea89a6e368...4533445fd7`: rebase due to conflicts

💬 Sjors commented on pull request "multiprocess: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-2242645460)
If you want to simplify this PR a bit further, you can drop `waitFeesChanged`, as well as `getCoinbaseMerklePath()` and `submitSolution` from the `BlockTemplate` interface.

I moved those out of the PRs on the repo and into my own fork (https://github.com/Sjors/bitcoin/pull/52 and https://github.com/Sjors/bitcoin/pull/53). I can apply the required changes to `ipc/capnp/mining.capnp` myself in https://github.com/Sjors/bitcoin/pull/48; that's much easier than I expected.

That should remove an
...

🚀 fanquake merged a pull request: "fuzz: Limit parse_univalue input length"
(https://github.com/bitcoin/bitcoin/pull/30473)

💬 paplorinc commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1686350738)
I don't see why silencing warnings and putting them to dead comments that cannot bite back is better than "bloating the logs"

⚠️ 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