💬 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!

💬 kevkevinpal commented on pull request "test, refactor: Fix MSVC warning C4101 "unreferenced local variable"":
(https://github.com/bitcoin/bitcoin/pull/30464#issuecomment-2232048259)
utACK [44f0878](https://github.com/bitcoin/bitcoin/pull/30464/commits/44f08786f435ed4284d39dc604c2a5fcbde9e602)

lgtm! seems pretty straight forward

🤔 tdb3 reviewed a pull request: "doc: getaddressinfo[isscript] is optional"
(https://github.com/bitcoin/bitcoin/pull/30457#pullrequestreview-2181557526)
Approach ACK
Thanks for increasing clarity in RPC.
Looks like there's a missing `assert` in `test_getaddressinfo()`.
Left a nit.

💬 tdb3 commented on pull request "doc: getaddressinfo[isscript] is optional":
(https://github.com/bitcoin/bitcoin/pull/30457#discussion_r1680220509)
Maybe it's worth adding a line for segwit v0 as well? (e.g. witness program not 20 or 32 bytes long)?

also

nit: might increase clarity
```diff
- BECH32_VALID_UNKNOWN_WITNESS = 'bcrt1p424qxxyd0r'
+ BECH32_VALID_UNKNOWN_WITNESS_PROG = 'bcrt1p424qxxyd0r'
```

👍 tdb3 approved a pull request: "refactor: add coinbase constraints to BlockAssembler::Options"
(https://github.com/bitcoin/bitcoin/pull/30356#pullrequestreview-2181599489)
Code Review ACK 95c2edefe383c2b8a1735c26d8442fd36fa5e339
Good addition and the updates tidy things up nicely.

I would also support a bitcoind config parameter for this.