👍 tdb3 approved a pull request: "doc: update IBD requirements in doc/README.md"
(https://github.com/bitcoin/bitcoin/pull/30992#pullrequestreview-2348167800)
ACK 36a6d4b0078ebb39ed082c866bf49214a2a01241

Thanks. It's good to keep user-facing docs current, to manage user expectations.

💬 instagibbs commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1787779109)
> Maybe that sort of check would/could be a larger scope than just getorphantxs?

Please don't invalidate the ACKs, but personally find it useful as a reviewer to just enforce expected behavior in tests, even minor ones like this. It's also *documentation* to the future reader as well; the functional tests are often the first place I look for intention of RPCs, without having to dig into git(hub) history to discover it.

🤔 promag reviewed a pull request: "gui: bugfix, decouple WalletModel from RPCExecutor"
(https://github.com/bitcoin-core/gui/pull/841#pullrequestreview-2348181732)
Concept meh, `WalletModel` is coupled to `RPCConsole`, why bother with `RPCExecutor`?

💬 furszy commented on pull request "bugfix: Decouple WalletModel from RPCExecutor":
(https://github.com/bitcoin-core/gui/pull/841#issuecomment-2393888538)
> Concept meh, `WalletModel` is coupled to `RPCConsole`, why bother with `RPCExecutor`?

Thats because the GUI view code and the RPC command parsing and executing functions are entangled right now, but this doesn’t need to remain the case. While the view might require access to the `WalletModel` for retrieving certain information to display (perhaps more so in the future because we don't do much with it now), the underlying procedures for running the commands don’t depend on it. They only need
...

💬 sipa commented on pull request "Cluster linearization: separate tests from tests-of-tests":
(https://github.com/bitcoin/bitcoin/pull/30605#discussion_r1787864440)
Fixed.

💬 sipa commented on pull request "Cluster linearization: separate tests from tests-of-tests":
(https://github.com/bitcoin/bitcoin/pull/30605#discussion_r1787865526)
I have instead added a commit that allows requesting a non-empty subset from `ReadTopologicalSubset`, and added some clarifying comments about why/why not empty sets are requested in all call sites.

💬 sipa commented on pull request "Cluster linearization: separate tests from tests-of-tests":
(https://github.com/bitcoin/bitcoin/pull/30605#discussion_r1787866904)
And the same response applies.

The goal of fuzz testing is testing as many possible valid inputs as possible. Fixing the subset being compared with, and the subset being removed is reducing the number of combinations of tested inputs.

💬 ismaelsadeeq commented on pull request "Cluster linearization: separate tests from tests-of-tests":
(https://github.com/bitcoin/bitcoin/pull/30605#discussion_r1787886486)
I see your point now.
both comments can be resolved.

💬 hebasto commented on pull request "bugfix: Decouple WalletModel from RPCExecutor":
(https://github.com/bitcoin-core/gui/pull/841#issuecomment-2393970280)
@furszy 3f34c04628f1cb0f59afb5176dae3a0e8d414d7d "gui: bugfix, decouple WalletModel from RPCExecutor"

Maybe remove "bugfix" from the commit message, as there is no reproducible bug in the master branch?

👍 BrandonOdiwuor approved a pull request: "ci: set a ctest test timeout of 1200 (20 minutes)"
(https://github.com/bitcoin/bitcoin/pull/31026#pullrequestreview-2348367460)
ACK 93dda4c70254750a5b5c0e7f7d2d89032519b999

LGTM

👍 ismaelsadeeq approved a pull request: "Cluster linearization: separate tests from tests-of-tests"
(https://github.com/bitcoin/bitcoin/pull/30605#pullrequestreview-2348390455)
Code review ACK 46b6a67eccb411903deb09af3301c2499d2c0217

All comments were addressed.

💬 marcofleon commented on pull request "net: fuzz: bypass network magic and checksum validation":
(https://github.com/bitcoin/bitcoin/pull/31012#issuecomment-2394020712)
Approach ACK. The use of `FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION` in `net.cpp` looks good to me. Generated a coverage report to verify that both branches of both checks are hit.

I haven't yet worked through the honggfuzz instructions to confirm that they work. I'll circle back to this to give it a try.

💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2394036343)
Guix builds for the recent push:
- `aarch64`:
```
76be0bebead7ffa05e22746e402f327c26faaa8fc48fb5759c1bb0ef7831623e guix-build-b9d6f1981083/output/aarch64-linux-gnu/SHA256SUMS.part
2dfb35cd8c56a427bb8c8d838c746640c365035dec6e6a59779b26492a3ca109 guix-build-b9d6f1981083/output/aarch64-linux-gnu/bitcoin-b9d6f1981083-aarch64-linux-gnu-debug.tar.gz
18f349d0f1fd0da1ae8c659e0b5e5b10e6c0a349062be1864110382592e337d2 guix-build-b9d6f1981083/output/aarch64-linux-gnu/bitcoin-b9d6f1981083-aarch64-lin
...

👋 hebasto's pull request is ready for review: "build: Switch to Qt 6"
(https://github.com/bitcoin/bitcoin/pull/30997)

💬 ismaelsadeeq commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1787938791)
+1 on adding this in a follow-up.
I can create a follow for it see https://github.com/ismaelsadeeq/bitcoin/commit/9350732b17915d315cf3ce9c744d1491c1b5d386 with the description @sdaftuar provided

💬 ismaelsadeeq commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#issuecomment-2394040665)
post merge ACK 9ad2fe7e69e9e69949ebbb280a15756dc3301f09
My bench result locally was consistent with what @glozow and sipa has provided.

💬 marcofleon commented on pull request "fuzz: wallet: add target for `CreateTransaction`":
(https://github.com/bitcoin/bitcoin/pull/29936#issuecomment-2394049086)
ACK c495731a316d9c97ee05a08cf5087c5535f84bd4

Coverage report: [wallet_create_transaction](https://marcofleon.github.io/coverage/wallet_create_transaction/)

🤔 ismaelsadeeq reviewed a pull request: "test: use context managers and add file existence checks in feature_fee_estimation.py"
(https://github.com/bitcoin/bitcoin/pull/31030#pullrequestreview-2348455824)
Thanks for your interest in contributing to this project @AgusR7

I am unsure whether this PR meets the refactoring standards outlined in the contributing guidelines https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#refactoring, and I'm also not convinced that the advertised advantages justify the review effort. Therefore, I am tending towards NACK.


If you are still willing to contribute check out the [Good first issues](https://github.com/bitcoin/bitcoin/issues?q=is%3Aopen+
...

🤔 ismaelsadeeq reviewed a pull request: "cluster mempool: extend DepGraph functionality"
(https://github.com/bitcoin/bitcoin/pull/30857#pullrequestreview-2348460911)
Concept ACK

📝 itornaza converted_to_draft a pull request: "depends: fix for llvm-ranlib (etc): 'No such file or directory' macOS 15.0"
(https://github.com/bitcoin/bitcoin/pull/30994)
This is an attempt to fix the issue https://github.com/bitcoin/bitcoin/issues/30978 .

To briefly summarize, @Sjors found that while compiling dependencies on macOS 15.0 the following "No such file or directory" are generated while these tools are installed.

```
$ cd depends
$ make
/bin/sh: command -v llvm-ranlib: No such file or directory
/bin/sh: command -v llvm-strip: No such file or directory
/bin/sh: command -v llvm-nm: No such file or directory
/bin/sh: command -v llvm-objdump:
...