👍 ryanofsky approved a pull request: "fix: Make TxidFromString() respect string_view length"
(https://github.com/bitcoin/bitcoin/pull/30436#pullrequestreview-2186195988)
Code review ACK c3a9c71c7077324bf0aa40f834f7537a14619340

I think the changes are all good, but the PR and the most important commit "fix: Make TxidFromString() respect string length" (c3a9c71c7077324bf0aa40f834f7537a14619340) are lacking a good description that actually says what the bug is.

I think they should say that currently `TxidFromString` takes a string_view parameter, but internally it is calling the `uint256S` function which expects a nul-terminated string. If `TxidFromString` is
...

💬 ryanofsky commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1683055614)
In commit "refactor: Change base_blob::SetHex() to take std::string_view" (f77d961d926405637dfbdfb9a9baea1fab4f1b7b)

Note: This line isn't directly equivalent to previous code because it trims whitespace from the end of the string, not just the beginning, but the net effect is the same because any whitespace at the end of the string would be ignored anyway.

I'm surprised looking at the implementation of SetHex to see how unreliable it is in general. Since it can't report errors it winds up
...

💬 ryanofsky commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1683086730)
In commit "refactor: Change base_blob::SetHex() to take std::string_view" (f77d961d926405637dfbdfb9a9baea1fab4f1b7b)

It seems not great, especially in the light of the unterminated string bug, that this will iterate over the entire string when only the beginning portion of the string may be needed. For example, if the string is 1MB and the blob is 32 bytes, this will iterate over 1MB. Would changing this to:
```c++
while (digits < str.size() && digits < WIDTH*2 && ::HexDigit(str[digits]) !=
...

🚀 glozow merged a pull request: "test: Non-Shy version sender"
(https://github.com/bitcoin/bitcoin/pull/30453)

💬 glozow commented on pull request "refactor: add coinbase constraints to BlockAssembler::Options":
(https://github.com/bitcoin/bitcoin/pull/30356#issuecomment-2237009418)
post merge ACK

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1683160882)
Of course

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1683164631)
Ah, my mistake, this was an intermediary commit - fine either way

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-2237035374)
ACK d7f94e0fa9ec3641405e6909ab7da4e48865e840

👍 itornaza approved a pull request: "crypto, refactor: add new KeyPair class"
(https://github.com/bitcoin/bitcoin/pull/30051#pullrequestreview-2186375491)
re tACK 5e9f4f41de98c2cdcc74e31fe925b2f5cf9f6aec

Thank you for your commit redo in a more granular way. Just a couple of questions really in the discussion above. To be thorough, rebuilt and retested the commit and all tests included the extended suite pass.

🤔 glozow reviewed a pull request: "test: expand LimitOrphan and EraseForPeer coverage"
(https://github.com/bitcoin/bitcoin/pull/30082#pullrequestreview-2186380821)
reACK 172c1ad026cc38c6f52679e74c14579ecc77c48e

💬 dergoegge commented on pull request "fuzz: Version handshake":
(https://github.com/bitcoin/bitcoin/pull/30413#discussion_r1683190037)
Measuring exec/s without lazy init:


With:


💬 dergoegge commented on pull request "fuzz: Version handshake":
(https://github.com/bitcoin/bitcoin/pull/30413#discussion_r1683143984)
There are more message types that are allowed after the version message and before the verack (some of them are also allowed post verack), essentially all of the ones processed in [this block](https://github.com/bitcoin/bitcoin/blob/20ccb30b7af1c30cece2b0579ba88aa101583f07/src/net_processing.cpp#L3894-L4061) (`wtxidrelay`, `sendaddrv2`, `sendtxrcncl`, `sendheaders`, `sendcmpct`).

I used `ALL_NET_MESSAGE_TYPES`, so that we can avoid touching this test if we add more handshake related message t
...

💬 dergoegge commented on pull request "fuzz: Version handshake":
(https://github.com/bitcoin/bitcoin/pull/30413#discussion_r1683123261)
My idea was that this might help a fuzzer to more realistically simulate how time would change between messages. Setting it directly would of course also work but I thought it might result in more mutations that are completely bogus (the time isn't attacker controlled so this seemed less useful). Happy to change it, probably doesn't make a huge difference. What do you think?

💬 Sjors commented on pull request "Introduce waitFeesChanged() mining interface":
(https://github.com/bitcoin/bitcoin/pull/30443#issuecomment-2237077920)
Rebased after #30356 landed. Changed `fees_before` to be a reference to `CAmount` based on the above experience of using it.

💬 Sjors commented on pull request "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange and drop CRPCSignals":
(https://github.com/bitcoin/bitcoin/pull/30409#issuecomment-2237085603)
Rebased after #30356 (`#include` order).

💬 Sjors commented on pull request "guix: bump time-machine to e6e70abe65850da0ae69428654375d367088ef5e":
(https://github.com/bitcoin/bitcoin/pull/30452#issuecomment-2237090936)
guix hashes:

```
365b4e5e4f876a949834d6f444edb079851e7e77b4fc77f7d6bafd1958f87a10 guix-build-314407a2fb80/output/aarch64-linux-gnu/SHA256SUMS.part
d8f7f8a9d84e37df272a53e5a69f4325196dfc7d43b1a2c12415e81a140f80ad guix-build-314407a2fb80/output/aarch64-linux-gnu/bitcoin-314407a2fb80-aarch64-linux-gnu-debug.tar.gz
aaafb96f75597be1206329b5632e32d1f844e15e69415184e8aada3397789bc6 guix-build-314407a2fb80/output/aarch64-linux-gnu/bitcoin-314407a2fb80-aarch64-linux-gnu.tar.gz
d3eec1297168c6104
...

💬 Sjors commented on pull request "Have createNewBlock() return a BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/30440#issuecomment-2237092200)
Rebased after https://github.com/bitcoin/bitcoin/pull/30356 landed.

💬 theuni commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1683210749)
There are a few places here that seem to be changing behavior from setting flags to adding flags. Are the outcomes guaranteed to be the same?

👍 maflcko approved a pull request: "Early logging improvements"
(https://github.com/bitcoin/bitcoin/pull/30386#pullrequestreview-2185698999)
ACK f3632d53a5be7efe61be8181ff481d30f7313bd4 🚲

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: ACK f3632d53a5be7efe61be8181ff
...

💬 maflcko commented on pull request "Early logging improvements":
(https://github.com/bitcoin/bitcoin/pull/30386#discussion_r1682767124)
q in 0b1960f1b29cfe5209ac68102c8643fc9553f247: Any reason to assert this but not the that the other `m_print_to_*` fields are false? What is the difference of a developer writing `PushBackCallback(...); DisableLogging()` to crash the program vs a developer writing `m_print_to_console=true;DisableLogging()` to achieve the same?