💬 hebasto commented on pull request "test, refactor: Fix MSVC warning C4101 "unreferenced local variable"":
(https://github.com/bitcoin/bitcoin/pull/30464#issuecomment-2231884900)
It is a part of upfront PR'ed commits from https://github.com/bitcoin/bitcoin/pull/30454, as suggested by @fanquake offline.

👍 theuni approved a pull request: "test, refactor: Fix MSVC warning C4101 "unreferenced local variable""
(https://github.com/bitcoin/bitcoin/pull/30464#pullrequestreview-2181381380)
trivial ACK 44f08786f435ed4284d39dc604c2a5fcbde9e602.

Seems weird to be catching a non-const ref, but it really doesn't matter here :)

🤔 ismaelsadeeq reviewed a pull request: "cluster mempool: cluster linearization algorithm"
(https://github.com/bitcoin/bitcoin/pull/30126#pullrequestreview-2181382355)
Reviewed:

- [x] Commit 4409a282d7fe7a0ebee67c8fb9fb4ef157ed5883
- [x] Commit f5fa49f589f477caa5a4ca41d8331acdca6d7298

I spent time understanding the [cluster linearization theory](https://delvingbitcoin.org/t/introduction-to-cluster-linearization/1032) and the serialization and de-serialization code in commit f5fa49f589f477caa5a4ca41d8331acdca6d7298 are well documented 💯 .

📝 hebasto opened a pull request: "depends: Set `CMAKE_SYSTEM_VERSION` for CMake builds"
(https://github.com/bitcoin/bitcoin/pull/30465)
From CMake [docs](https://cmake.org/cmake/help/latest/variable/CMAKE_SYSTEM_VERSION.html):
> When the [CMAKE_SYSTEM_NAME](https://cmake.org/cmake/help/latest/variable/CMAKE_SYSTEM_NAME.html#variable:CMAKE_SYSTEM_NAME) variable is set explicitly to enable [cross compiling](https://cmake.org/cmake/help/latest/manual/cmake-toolchains.7.html#cross-compiling-toolchain) then the value of CMAKE_SYSTEM_VERSION must also be set explicitly to specify the target system version.

This PR is split from ht
...

💬 hebasto commented on pull request "depends: Set `CMAKE_SYSTEM_VERSION` for CMake builds":
(https://github.com/bitcoin/bitcoin/pull/30465#issuecomment-2231918125)
It is a part of upfront PR'ed commits from https://github.com/bitcoin/bitcoin/pull/30454, as suggested by @fanquake offline.

cc @theuni @TheCharlatan @ryanofsky

🤔 hodlinator reviewed a pull request: "blockstorage: XOR blocksdir *.dat files"
(https://github.com/bitcoin/bitcoin/pull/28052#pullrequestreview-2181323398)
Concept ACK fae8e0a9825d107431f3f33e4f70fe54d02ab3e3
(Haven't had time to look at the tests yet).

Appreciate the robustness of `InitBlocksdirXorKey()` after some testing of removing the `blocks/xor.dat` file, re-running `bitcoind`. `hexdump`ing it, removing the whole `blocks/` directory, rerunning, `hexdump`ing again etc.

💬 hodlinator commented on pull request "blockstorage: XOR blocksdir *.dat files":
(https://github.com/bitcoin/bitcoin/pull/28052#discussion_r1680128246)
Tested `bitcoind -blocksxor=01234567`, which does not throw. Evaluates to true. Discussion seems to have stalled since #12713 https://github.com/bitcoin/bitcoin/blob/6f9db1ebcab4064065ccd787161bf2b87e03cc1f/src/common/args.cpp#L43-L63

💬 hodlinator commented on pull request "blockstorage: XOR blocksdir *.dat files":
(https://github.com/bitcoin/bitcoin/pull/28052#discussion_r1680076253)
Error messages should be as specific as possible to aid troubleshooting.
```suggestion
return InitError(strprintf(Untranslated("Failed to initialize ChainstateManager: %s"), e.what()));
```

💬 hodlinator commented on pull request "blockstorage: XOR blocksdir *.dat files":
(https://github.com/bitcoin/bitcoin/pull/28052#discussion_r1680074476)
Including "KEY" makes me think the variable would itself contain the default XOR pattern.
```suggestion
static constexpr bool DEFAULT_XOR_BLOCKSDIR{true};
```
Same goes for `bool xor_key` below.. would call it `bool xored_on_disk` or something.

💬 hodlinator commented on pull request "blockstorage: XOR blocksdir *.dat files":
(https://github.com/bitcoin/bitcoin/pull/28052#discussion_r1680120967)
(https://cplusplus.com/reference/cstdio/fopen/ stated that "x" was only part of C2011, not C++. But it is included from C++17 according to https://en.cppreference.com/w/cpp/io/c/fopen).

💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1680152503)
Done.

💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1680152672)
The loop uses `idx` below, unfortunately.

💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1680152726)
Done.

💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1680152763)
Done.

💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1680154549)
The function is just for testing, so it doesn't matter much, but sure, added.

💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1680154751)
Done.

💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1680155259)
Fair enough, but style-wise I still like to have a comment on each function. I've expanded the comment a bit here to point out it's for testing.

💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1680157731)
It's not clear to me what you're asking for. Are you asking what I changed to address this, or saying that it's still an issue, or saying that more explanatory comments are needed?

💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1680159369)
@ismaelsadeeq Done.

💬 mzumsande commented on pull request "rpc, rest: Improve getblock error when only header is available":
(https://github.com/bitcoin/bitcoin/pull/30410#issuecomment-2231992542)
> Ok, I am done, but I think it is easier to review this pull request.

OK - I'm working on addressing the feedback, plus adding more test coverage for the different types of errors. Will update soon!