🤔 l0rinc reviewed a pull request: "streams: replace `std::find` with `memchr` (5x improvement)"
(https://github.com/bitcoin/bitcoin/pull/34044#pullrequestreview-3564063293)
Concept ACK

`FindByte`’s only production use seems to be scanning block files during `-reindex` to find the first byte of the network magic in a circular buffer.
Because of documented historical bugs the data may be jumbled a bit, it's why we need to be able to find the beginning.
It's not something we expect users to do often, but this should speed it up a tiny bit. Left a few suggestions to make it more readable as well to make it easier to sell :)

I would be curious whether a `-reindex -sto
...

💬 l0rinc commented on pull request "streams: replace `std::find` with `memchr` (5x improvement)":
(https://github.com/bitcoin/bitcoin/pull/34044#discussion_r2607867005)
I don't think this needs manual hoisting - it's loop-invariant so the compiler will likely do it.
Inling it will reduce the diff.
And I don't think we need the ugly `to_integer` here: https://godbolt.org/z/7MWzTbMdb

💬 l0rinc commented on pull request "streams: replace `std::find` with `memchr` (5x improvement)":
(https://github.com/bitcoin/bitcoin/pull/34044#discussion_r2608033086)
the old implementation is quite confusing.

`buf_offset += inc` is only executed when `inc >= len` - and given it can't be greater, it's basically:
```C++
buf_offset += len;
```
which makes a lot more sense.

----

`m_read_pos += inc;` is either incremented by the distance of the find or by `len`, so if we do the `hit` check earlier, we can simplify this as well:
```C++
const auto len{std::min<size_t>(vchBuf.size() - buf_offset, nSrcPos - m_read_pos)};
const auto* start{vchBuf.data() + buf_offse
...

💬 w0xlt commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2608266711)
If I’m understanding the API correctly, `m_tx_for_private_broadcast.DidNodeConfirmReception(nodeid)` returns null when there’s no associated transaction even if it was previously removed (in `PrivateBroadcast::Remove()` in a8e23ca6)

If that’s the case, can private broadcast connections with already removed transactions still trigger `NumToOpenAdd(1)`, resulting in pointless connections ?

```suggestion
if (node.IsPrivateBroadcastConn()) {
// Only schedule a replacement private
...

💬 l0rinc commented on pull request "test: Log IP of download server in get_previous_releases.py":
(https://github.com/bitcoin/bitcoin/pull/34045#discussion_r2608312657)
new import seems unnecessary now

💬 achow101 commented on pull request "test: Log IP of download server in get_previous_releases.py":
(https://github.com/bitcoin/bitcoin/pull/34045#discussion_r2608326193)
Removed

💬 romanz commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2608380979)
Thanks, added in https://github.com/bitcoin/bitcoin/pull/33657/commits/44758e8387e66ec49d190b59f8a8fefab84b1e69.

💬 l0rinc commented on pull request "test: Log IP of download server in get_previous_releases.py":
(https://github.com/bitcoin/bitcoin/pull/34045#discussion_r2608382091)
I'm find with this as well, alternatively to document what we're connecting to, we could unpack the `ip` from the `(host, port)`/`(host, port, flowinfo, scope_id)` tuples with something like:
```suggestion
ip, *_ = response.fp.raw._sock.getpeername()
print(f"Connected to {ip}")
```

👍 l0rinc approved a pull request: "test: Log IP of download server in get_previous_releases.py"
(https://github.com/bitcoin/bitcoin/pull/34045#pullrequestreview-3564697400)
untested ACK cdaf25f9c3e56b4e80b90a471bdc0a175160534c

💬 romanz commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2608385566)
No worries, thanks :)

<details>

<summary> git range-diff </summary>

```
1: 1be1c165b3 ! 1: 7a2a6b398b blockstorage: return an error code from `ReadRawBlock()`
@@ src/rest.cpp: static bool rest_block(const std::any& context,
+ const auto block_data{chainman.m_blockman.ReadRawBlock(pos)};
+ if (!block_data) {
+ switch (block_data.error()) {
-+ case node::ReadRawError::IO: return RESTERR(req, HTTP_NOT_FOUND, "I/O error reading " + hashStr)
...

💬 romanz commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2608388301)
Unified in f03f9d7dcd929f73f7b0c7030d9822e54f1621ec

💬 l0rinc commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2608396257)
If we don't have a default, the compiler will warn since we have the `-Wswitch` set - in which case I don't see why the `assert(false)` is needed.
Alternatively, if we add a `default: assert(false)`, we don't need to specify all values and the compiler won't warn, the failure will be on runtime.
Anyway, off topic, I'm fine either way.

💬 l0rinc commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#issuecomment-3639185341)
ACK 44758e8387e66ec49d190b59f8a8fefab84b1e69

Since last push a `HTTP_NOT_FOUND` error code was changed to `HTTP_INTERNAL_SERVER_ERROR`, a test `BOOST_REQUIRE` was changed to a more permissive `BOOST_CHECK`, and in functional tests an assert was adjusted and a missing-block-data case was added for the status of the response.

💬 achow101 commented on pull request "precalculate SipHash constant salt XORs":
(https://github.com/bitcoin/bitcoin/pull/30442#issuecomment-3639185350)
ACK 6eb5ba569141b722a5e27ef36e04994886769c44

📝 l0rinc opened a pull request: "refactor: inline constant `f_obfuscate = false` parameter"
(https://github.com/bitcoin/bitcoin/pull/34048)
Split out of https://github.com/bitcoin/bitcoin/pull/33324 since it makes sense on its own.

Currently the parameter is only called with a single constant parameter, it doesn't make sense to needlessly obfuscate the constructor (pun intended).

💬 stickies-v commented on pull request "logging: API improvements":
(https://github.com/bitcoin/bitcoin/pull/34038#discussion_r2608431613)
I think this should work?

<details>
<summary>git diff on 77f272c094</summary>

```diff
diff --git a/src/rpc/client.cpp b/src/rpc/client.cpp
index 0693b60557..25c313c552 100644
--- a/src/rpc/client.cpp
+++ b/src/rpc/client.cpp
@@ -325,7 +325,7 @@ static const CRPCConvertParam vRPCConvertParams[] =
{ "psbtbumpfee", 1, "replaceable"},
{ "psbtbumpfee", 1, "outputs"},
{ "psbtbumpfee", 1, "original_change_index"},
- { "logging", 0, "include" },
+ { "logging", 0, "d
...

🤔 sipa reviewed a pull request: "precalculate SipHash constant salt XORs"
(https://github.com/bitcoin/bitcoin/pull/30442#pullrequestreview-3564766146)
ACK 6eb5ba569141b722a5e27ef36e04994886769c44

💬 romanz commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2608446338)
Not sure whether `getblock` JSON RPC performance measurement will be precise enough (due to RPC & serialization overhead)...

Maybe it would be easier to inline `GetRawBlockChecked` (similar to how it's done in `rest.cpp`)?

```diff
diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp
index 40761d5cab..e179284fcf 100644
--- a/src/rpc/blockchain.cpp
+++ b/src/rpc/blockchain.cpp
@@ -678,21 +678,6 @@ static CBlock GetBlockChecked(BlockManager& blockman, const CBlockIndex& blockin
...

💬 achow101 commented on pull request "[IBD] coins: reduce lookups in dbcache layer propagation":
(https://github.com/bitcoin/bitcoin/pull/33602#issuecomment-3639270227)
ACK 0ac969cddfdba52f7947e9b140ef36e2b19c2c41

💬 ajtowns commented on pull request "doc: improvements to doc/descriptors.md":
(https://github.com/bitcoin/bitcoin/pull/33986#discussion_r2608498884)
Wrong bracket for bip 384