💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1787644002)
Here is the change:

<details>
<summary>[patch] only remove if accepted in mempool</summary>

```diff
--- i/src/net_processing.cpp
+++ w/src/net_processing.cpp
@@ -4803,27 +4803,12 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
const uint256& txid = ptx->GetHash();
const uint256& wtxid = ptx->GetWitnessHash();

const uint256& hash = peer->m_wtxid_relay ? wtxid : txid;
AddKnownTx(*peer, hash);

- if (a
...

💬 sipa commented on issue "[Testnet] Insufficient data or no feerate found":
(https://github.com/bitcoin/bitcoin/issues/31032#issuecomment-2393598722)
Fee estimation needs data, which consists of seeing transactions arrive, and seeing them be mined. Testnet just doesn't have much data.

💬 ismaelsadeeq commented on pull request "Cluster linearization: separate tests from tests-of-tests":
(https://github.com/bitcoin/bitcoin/pull/30605#discussion_r1787649203)
Thanks for clarifying, initially I thought the aim is to remove the subset found by the search continuously until there are no transactions left in `m_todo`.

Maybe it will be helpful to document the point you gave as comment?

💬 hebasto commented on pull request "qt6: Handle different signatures of `QANEF::nativeEventFilter`":
(https://github.com/bitcoin-core/gui/pull/840#issuecomment-2393637703)
> If Qt 6.7.0 is going to be required for Windows after the switchover; why change this code now just to immediately drop it after migrating to Qt 6?

To make https://github.com/bitcoin/bitcoin/pull/30997 focused on the build system changes only.

We already have:
```
$ git grep 'QT_VERSION_CHECK(6, 0, 0)'
src/qt/addressbookpage.cpp:#if (QT_VERSION >= QT_VERSION_CHECK(6, 0, 0))
src/qt/addressbookpage.cpp:#if (QT_VERSION >= QT_VERSION_CHECK(6, 0, 0))
src/qt/bitcoin.cpp:#if (QT_VERSION <
...

⚠️ 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.