💬 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

🚀 achow101 merged a pull request: "[IBD] coins: reduce lookups in dbcache layer propagation"
(https://github.com/bitcoin/bitcoin/pull/33602)

💬 achow101 commented on pull request "merkle: migrate `path` arg to reference and drop unused args":
(https://github.com/bitcoin/bitcoin/pull/33805#issuecomment-3639323671)
ACK 24ed820d4f0d8f7fa2f69e1909c2d98f809d2f94

🤔 hodlinator reviewed a pull request: "refactor: Add util::Result failure types and ability to merge result values"
(https://github.com/bitcoin/bitcoin/pull/25665#pullrequestreview-2343976961)
re: https://github.com/bitcoin/bitcoin/pull/25665#issuecomment-3617394520

Excuse me, I somehow glossed over the fact that you had 3 template parameters in this PR, not 2. :facepalm: I missed that you allowed for a `FailureType` apart from `MessageType`, the former defaulting to `void` but messages being default-on. I see a greater justification for the distinct `Result` type. However, it seems the either/or aspect remains, if I'm not mistaken.

Inspired by your reply, I nerd-sniped myself i
...

💬 hodlinator commented on pull request "refactor: Add util::Result failure types and ability to merge result values":
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r2593959586)
in e78cfb399e5e59805557992c32d34080bfca6eee "refactor: Add util::Result failure values":

Isn't it closer to:
```suggestion
//! tuple<optional<SuccessType>, unique_ptr<tuple<FailureType, MessagesType>>>
```

💬 hodlinator commented on pull request "refactor: Add util::Result failure types and ability to merge result values":
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r2594047330)
Would anything be gained from making the template parameter an r-value? (Earlier commits do explicit `std::move()`s, justifying the function's name, but that justification appears to have dissipated).
```suggestion
static void Move(DstResult& dst, SrcResult&& src)
```