💬 theStack commented on issue "TestFramework: TestShell.reset() will always fail":
(https://github.com/bitcoin/bitcoin/issues/31131#issuecomment-2433056070)
It seems that the TestShell instructions generally don't work anymore since we switched to CMake. For me they fail already earlier at the `TestShell` instantiation, as the generated `config.ini` file is searched in a path that still assumes in-tree builds:
```
$ git rev-parse HEAD
ffe4261cb0669b1e1a926638e0498ae5b63f3599
$ python3
Python 3.10.12 (main, Sep 11 2024, 15:47:36) [GCC 11.4.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import sys
>>> sy
...

💬 l0rinc commented on pull request "tinyformat: refactor: increase compile-time checks and don't throw for tfm::format_error":
(https://github.com/bitcoin/bitcoin/pull/30928#discussion_r1813088395)
might as well put the whole test suite inside instead of having an empty passing test

💬 l0rinc commented on pull request "tinyformat: refactor: increase compile-time checks and don't throw for tfm::format_error":
(https://github.com/bitcoin/bitcoin/pull/30928#discussion_r1813079479)
nit: we're inside `namespace tinyformat`:
```suggestion
} catch (format_error& fmterr) {
```

(same for `formatImpl`)

📝 laanwj opened a pull request: "doc: Make list of targets in depends README consistent"
(https://github.com/bitcoin/bitcoin/pull/31141)
The description of `i686-pc-linux-gnu` and `x86_64-pc-linux-gnu` is incomplete and inconsistent with the others. Fix this. Also use "64 bit" consistently instead of "64-bit".

💬 maflcko commented on pull request "doc: Make list of targets in depends README consistent":
(https://github.com/bitcoin/bitcoin/pull/31141#issuecomment-2433122900)
lgtm ACK a0c9595810c7d8bb17d8b5bea8d916db194b5239

💬 edilmedeiros commented on pull request "doc: replace `-?` with `-h` and `-help`":
(https://github.com/bitcoin/bitcoin/pull/31118#issuecomment-2433144931)
tACK 33a28e252a7349c0aa284005aee97873b965fcfe

I would prefer everything in a single commit, but it's OK.

👍 hebasto approved a pull request: "doc: Make list of targets in depends README consistent"
(https://github.com/bitcoin/bitcoin/pull/31141#pullrequestreview-2389771410)
ACK a0c9595810c7d8bb17d8b5bea8d916db194b5239.

💬 hodlinator commented on pull request "fees: Remove CLIENT_VERSION serialization":
(https://github.com/bitcoin/bitcoin/pull/29702#issuecomment-2433178606)
re-ACK fa1c5cc9df116411edca172f8b80fc225c776554

Thanks for adjusting the version error for too high values!

Makes sense to remove explicit newlines given #30929.

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