💬 davidgumberg commented on pull request "coins: Add move operations to Coin and CCoinsCacheEntry":
(https://github.com/bitcoin/bitcoin/pull/30643#discussion_r1813370731)
nit: deleted functions don't need `noexcept`

💬 l0rinc commented on pull request "coins: Add move operations to Coin and CCoinsCacheEntry":
(https://github.com/bitcoin/bitcoin/pull/30643#discussion_r1813377457)
Thanks, if I edit again, I'll remove them

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