💬 instagibbs commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1813115879)
```Suggestion
if (i == 0 && m_ancestors.contains(tx_entry)) {
```

💬 instagibbs commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1813400339)
at commit 1c33e133db5f876f2ba6eeb28877a5dcf3f6ac04 are we not applying priority values when computing RBFs, since they're only being applied during `Apply()` stage?

subsequent commit 2b5268ebac8daee0521ea82c2688e5e4fac885bc pulls it into changeset, meaning it can act on it?

Do we have coverage of package rbf with priority?

💬 instagibbs commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1813404075)
This shouldn't be necessary, but shouldn't hurt either. `AcceptSubPackage` calls it already for each subpackage.

💬 l0rinc commented on pull request "coins: Add move operations to Coin and CCoinsCacheEntry":
(https://github.com/bitcoin/bitcoin/pull/30643#issuecomment-2433259083)
Thanks for checking @davidgumberg!

> Also, could you point me in the direction of reproducing this CI warning locally? I can't find the warning on corecheck ([corecheck.dev/bitcoin/bitcoin/pulls/28280](https://corecheck.dev/bitcoin/bitcoin/pulls/28280))

I also had a hard time reproducing it on CI ([sonarcould](https://sonarcloud.io/project/issues?id=aureleoules_bitcoin&branch=28280-e1076541e62972464193afb1f8d90ff178665ecb&resolved=false&open=AZCVwk_WMNEdWlV7XnLo) seems to have expired), bu
...

📝 mzumsande opened a pull request: "test: fix intermittent failure in p2p_seednode.py, don't connect to random IPs"
(https://github.com/bitcoin/bitcoin/pull/31142)
Fixes #31103

On some CI runs, the seed node timer in `ThreadOpenConnection` was only started *after* the mocktime was set.
Fix this by waiting for the first connection attempt, which happens after the timer was started.

Also I noticed that the "unreachable" connections are not in fact unreachable, so that the functional test could attempt connections
to random nodes on the internet. This was already noted in https://github.com/bitcoin/bitcoin/pull/29605#discussion_r1701616675 but the sug
...

💬 mzumsande commented on issue "p2p_seednode.py intermittent issue: Expected messages "["Couldn't connect to peers from addrman after 10 seconds. Adding seednode (0.0.0.2) to addrfetch"]" does not partially match log:":
(https://github.com/bitcoin/bitcoin/issues/31103#issuecomment-2433268022)
See #31142 for a fix.

💬 edilmedeiros commented on pull request "RFC: build: support for pre-compiled headers.":
(https://github.com/bitcoin/bitcoin/pull/31053#issuecomment-2433286818)
Concept ACK
Code LGTM.

---
`rm -rf build && cmake -B build -DBoost_INCLUDE_DIR=/opt/local/libexec/boost/1.78/include -DCMAKE_BUILD_TYPE=Debug -DWITH_CCACHE=OFF -DBUILD_BENCH=ON -DBUILD_FUZZ_BINARY=ON -DBUILD_UTIL_CHAINSTATE=ON && /usr/bin/time cmake --build build -j 11`

Master at ffe4261cb0669b1e1a926638e0498ae5b63f3599
1m56,95s real 16m57,22s user 1m31,97s sys

This PR at 3cec3508a79c08db5b97e93bc4225ccdd7d32ad8
1m32,76s real 12m59,11s user 1m6,45s sys

---
With ccache 4.10:
...

👍 theStack approved a pull request: "wallet: optimize migration process, batch db transactions"
(https://github.com/bitcoin/bitcoin/pull/28574#pullrequestreview-2390086731)
re-ACK c98fc36d094a08d44f3c95431db2c5f34a96cc73
(as per `$ git range-diff a4e1ba7...c98fc36`)

💬 edilmedeiros commented on pull request "build: speed up by flattening the dependency graph":
(https://github.com/bitcoin/bitcoin/pull/30911#issuecomment-2433310624)
> I noticed a bit of a "gap" in our compilation on master the other day."

I have been noticing this "gap" all the time with cmake. I'll check with this PR.

👍 TheCharlatan approved a pull request: "fees: Remove CLIENT_VERSION serialization"
(https://github.com/bitcoin/bitcoin/pull/29702#pullrequestreview-2390495638)
Re-ACK fa1c5cc9df116411edca172f8b80fc225c776554

💬 sdaftuar commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1813525410)
My thought process here was to mimic the old behavior of having the in-mempool ancestors calculated once and then cached for any further needs, such as adding the transaction to the mempool (in the single transaction case).

In the package setting, we calculate the in-mempool ancestors of each transaction in a package, and I believe we use that information for some policy checks, but we throw it away as transactions get added.

This all goes away with cluster mempool, so I didn't spend a lot
...

💬 sdaftuar commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1813525748)
My intention was to allow calling `StageAddition` and `StageRemoval` in any order. I can add comments to that effect.

💬 1440000bytes commented on pull request "rest: Support transaction broadcast in REST interface":
(https://github.com/bitcoin/bitcoin/pull/31065#issuecomment-2433416053)
What is the use of REST interface that doesn't have enough endpoints?

💬 sdaftuar commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1813527740)
Yes this matches the old behavior, I believe.

💬 darosior commented on pull request "Drop miniupnp dependency":
(https://github.com/bitcoin/bitcoin/pull/31130#discussion_r1813555299)
Done.

💬 darosior commented on pull request "Drop miniupnp dependency":
(https://github.com/bitcoin/bitcoin/pull/31130#issuecomment-2433449973)
Kept the option as a hidden arg which will return an error explaining UPnP support was dropped and encouraging users to set `-natpmp` instead.

Added a release note.

🤔 theStack reviewed a pull request: "refactor: TxDownloadManager + fuzzing"
(https://github.com/bitcoin/bitcoin/pull/30110#pullrequestreview-2390493996)
Re-reviewed up to 190d87f912737c46668072ea9a460c520f778c92, lgtm. The introduced minor behaviour changes to address https://github.com/bitcoin/bitcoin/pull/30110#issuecomment-2391859626 and https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1801661898 seem reasonable and harmless. Planning to finish reviewing unit test and fuzzer tomorrow (not opposed to defer the one_honest_peer fuzz test for a future PR at all :)).

💬 theStack commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1813507907)
```suggestion
* downloaded, whether and how to validate them. It is also responsible for
```

💬 achow101 commented on pull request "bench: add support for custom data directory":
(https://github.com/bitcoin/bitcoin/pull/31000#issuecomment-2433458355)
In 221410bc14616eaf7017e5097cd6848fcb4f8e1b "bench: specialize working directory name" , the commit message says

> After this commit, benchmarks are contained within its own directory:
> /<OS_tmp_dir>/test_common bitcoin/<benchmark_name>/<random_uint256>/

However I am not observing this behavior. It omits the benchmark name.

The benchmark name is included when `-testdatadir` is used.

💬 sipa commented on pull request "Drop miniupnp dependency":
(https://github.com/bitcoin/bitcoin/pull/31130#discussion_r1813583243)
This is perhaps a bit confusing, given that from the user's perspective these two are the same thing (`-natpmp` controls both).