⚠️ vasild opened an issue: "Prioritize processing of peers based on their CPU usage"
(https://github.com/bitcoin/bitcoin/issues/31033)
### Please describe the feature you'd like to see added.

Currently, we process messages to/from all peers in a loop where every peer is processed once (has the same weight). The list of peers is shuffled before every loop.

Considering a scenario where we spend considerably more CPU time for some peers compared to others, does it make sense to de-prioritize CPU-hungry peers? This might be happening deliberately (a CPU DoS attack) or not.

For example: if we spent 5 CPU seconds to process ea
...

💬 fanquake commented on pull request "qt6: Handle different signatures of `QANEF::nativeEventFilter`":
(https://github.com/bitcoin-core/gui/pull/840#issuecomment-2393655804)
Not sure we should be having to open and review multiple additional PRs (one now, one later to remove the code again), and be making pointless code changes, just to keep a different PR "focussed". Especially when the changes you're trying to extract are completely relevant.

💬 willcl-ark commented on pull request "ci: set a ctest test timeout of 1200 (20 minutes)":
(https://github.com/bitcoin/bitcoin/pull/31026#issuecomment-2393726440)
> > I tried for quite some time to get this to work using `CTEST_TEST_TIMEOUT` from within _CMakeLists.txt_ but it does not appear to be possible.
>
> According to CMake docs, [`CTEST_TEST_TIMEOUT`](https://cmake.org/cmake/help/latest/variable/CTEST_TEST_TIMEOUT.html)
>
> > Specify the CTest `TimeOut` setting in a `ctest` [dashboard client script](https://cmake.org/cmake/help/latest/manual/ctest.1.html#ctest-script).
>
> which is a use case different from ours.

Huh, for some reason I
...

💬 willcl-ark commented on issue "depends: llvm-ranlib (etc): "No such file or directory" on Intel macOS 15.0":
(https://github.com/bitcoin/bitcoin/issues/30978#issuecomment-2393737848)
I also see this issue on MacOS 15.0.

> This looks odd, because you seem to be hitting code paths for macOS cross-compilation, when building natively. i.e when building on macOS, the tooling should be found by calling xcrun:

I don't think this is correct? These checks are coming from *hosts/darwin.mk* which IIUC is the correct host file to be usedwhen building on a darwin host?:

https://github.com/bitcoin/bitcoin/blob/51c698161b54402f9a8302c9a36f28a4db01fc4f/depends/hosts/darwin.mk#L19-L
...

💬 Sjors commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#issuecomment-2393768768)
@vasild if you rebase, tidy might point out that `sockman.cpp.o depends on i2p.cpp`. So you probably need to either move `i2p.cpp` to common as well, or remove the dependency.

📝 furszy opened a pull request: "gui: bugfix, decouple WalletModel from RPCExecutor"
(https://github.com/bitcoin-core/gui/pull/841)
A more comprehensive fix for the issue described in #837.

Since the `WalletModel` class is unavailable when compiling without wallet support
`(-DENABLE_WALLET=0)`, the RPC executor class should not be coupled to it.
This decoupling ensures GUI compatibility with builds that omit wallet support.

💬 furszy commented on pull request "qt6: Fix linking when configured with `-DENABLE_WALLET=OFF`":
(https://github.com/bitcoin-core/gui/pull/837#issuecomment-2393788867)
Arrived a bit late, but see #841. It drops `WalletModel` linkage and removes an extra `#ifdef ENABLE_WALLET` in the RPC executor class.

💬 instagibbs commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1787775319)
just noting that previously the `MAX_STANDARD_TX_WEIGHT` was enforced via `TxOrphanage::AddTx`, just not the computed memory usage. Honestly seems a bit accidentally asymmetric to begin with.

👍 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.