🤔 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.

💬 glozow commented on pull request "qa: Functional test improvements":
(https://github.com/bitcoin/bitcoin/pull/30463#issuecomment-2242695352)
ACK a8e3af1a82dd584a1cc3ffbe587e66889f72e3c7, tested with the steps in op

🚀 glozow merged a pull request: "qa: Functional test improvements"
(https://github.com/bitcoin/bitcoin/pull/30463)

💬 ismaelsadeeq commented on pull request "Fee Estimation: change `estimatesmartfee` default mode to `economical`":
(https://github.com/bitcoin/bitcoin/pull/30275#issuecomment-2242703454)
> Slightly surprised no tests need to be changed.

Because the fee estimation functional test for `estimatesmartfee` is not deterministic (transaction fee rates are generated randomly), the `check_raw_estimates` function tests for a range, ensuring the returned fee rate is reasonable.

> A boolean flip where the intended behavior isn't followed and we are conservative again?

I added a simple deterministic test which test the default mode is `economical` in 307ceb46587c83a7ab0f587915cdcbf9
...

📝 maflcko opened a pull request: " lint: Use consistent out-of-tree build for python and test_runner "
(https://github.com/bitcoin/bitcoin/pull/30499)
Fixes https://github.com/bitcoin/bitcoin/issues/30496

Seems odd to sometimes do an out-of-tree build (via `./ci/lint_imagefile`, see `test/lint/README.md`) and sometimes not (via Cirrus CI, see `./ci/lint_run_all.sh`).

Fix it by doing an out-of-tree build consistently in the same location.

Also, fix `$CI_RETRY_EXE`, while touching this.