💬 l0rinc commented on pull request "coins: Add move operations to Coin and CCoinsCacheEntry":
(https://github.com/bitcoin/bitcoin/pull/30643#discussion_r1813384094)
[Pushed](https://github.com/bitcoin/bitcoin/compare/cd400acd8360144b8c7daa4fd2c6bea8267f9ba9..db91623c5241b3795a45940cff0b6e79be6f265f), thank you

💬 davidgumberg commented on pull request "coins: Add move operations to Coin and CCoinsCacheEntry":
(https://github.com/bitcoin/bitcoin/pull/30643#issuecomment-2433238490)
Am I right in understanding that this PR's expected changes are:

1. Deletion of the CCoinsCacheEntry reference copy constructors
2. Update `CoinsMapEntry` in the coins unit tests `to `try_emplace` over `emplace`.

And we are otherwise just explicitly declaring the move constructors that we expect to already be used implicitly in order to silence a spurious CI warning?

Also, could you point me in the direction of reproducing this CI warning locally? I can't find the warning on corecheck
...

💬 achow101 commented on pull request "wallet: optimize migration process, batch db transactions":
(https://github.com/bitcoin/bitcoin/pull/28574#issuecomment-2433250689)
ACK c98fc36d094a08d44f3c95431db2c5f34a96cc73

🤔 instagibbs reviewed a pull request: "cluster mempool: Implement changeset interface for mempool"
(https://github.com/bitcoin/bitcoin/pull/31122#pullrequestreview-2389151891)
first pass through, concept ACK

💬 instagibbs commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1813013432)
s/RemoveStaged/Apply/

💬 instagibbs commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1813066539)
note: circa 88e109f1091561639aca323cb455180317a12b32 this field is set but never read from making the `CalculateMemPoolAncestors` wrapper a bit mysterious

💬 instagibbs commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1813100135)
I think "cleanup" of `m_subpackage` can now all be managed inside `ClearSubPackageState`, including the `m_all_conflicts` clear below.

💬 instagibbs commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1813069337)
what order should caller call things, e.g. `StageAddition`? I think some notes for this class would help with assumptions for caller.

💬 instagibbs commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1813383434)
noting that this isn't actually required to get variable vsize transactions since `ConsumeTxMemPoolEntry` injects a fuzz-selected sigops value. Simpler if you just build a minimal tx then allow `ConsumeTxMemPoolEntry` to choose how big it is in vbytes?

💬 instagibbs commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1813104310)
as of 32101330ec6f6e9c4d467dec370c848cf4d14f23 , now the *first* entry of m_entry_vec now uses `m_ancestors` but no others.

Am I understanding the extend of its use?

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