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

💬 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/)