💬 maflcko commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2236548912)
nit in 35a9dd7352d84292fbc2dcf2e544b380be49b4c3: Unused? Maybe you wanted this to be the container name, or the job id instead?

💬 maflcko commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2236658377)
nit in 67429ff9ae0ad7236d01fee7d25dceb965741b2f: Please use `-o xtrace`, so that non-Bash experts can also understand the intention, without having to <strike>ask an LLM</strike> read the manpage

💬 maflcko commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2236595677)
nit in c7aa954564ceaf0800be461115a568d3c919228a: This seems somewhat redundant with the container name now. It would be less yaml code to use container name instead of job id here?

💬 maflcko commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2236643414)
nit in a3789f2056a76e72fcafbae7396c35f62fb93f56: wrong (unused) suffix

💬 maflcko commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2236619074)
nit in 88816a4ed3f5d528ed7b997241df96fb1f51df44: `apt-get install` will fail with a stale Ubuntu IP list from a cached image. You'll have to call `apt-get update` before every install for each image layer.

💬 maflcko commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2236533948)
nit in 35a9dd7352d84292fbc2dcf2e544b380be49b4c3: Could just be a global env var, instead of repeating it thrice?

💬 maflcko commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2236668156)
Just as a note: This is fine to remove without replacement, because the cirrus cache now runs outside the container. Here, it ran inside it.

💬 maflcko commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2236626008)
Also, I wonder if this can use `$PACKAGES`?

💬 maflcko commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2236739388)
693d3615cdb25c5f185c1e6097a6e647eca7153b: This commit isn't doing anything, because the MAKEJOBS are set globally in the env in GHA (same for the other "perf" commits below).

💬 pinheadmz commented on pull request "rpc: use CScheduler for relocking wallet and remove RPCTimer":
(https://github.com/bitcoin/bitcoin/pull/32862#issuecomment-3127567379)
Ok got a reliable crash on debian after upgrading to clang-20:

```
~/bitcoin$ rm -rf ./bld-cmake && \
cmake -B ./bld-cmake \
-DCMAKE_C_COMPILER='clang-20' \
-DCMAKE_CXX_COMPILER='clang++-20;-stdlib=libc++' \
-DBUILD_GUI=OFF \
-DBUILD_TESTS=OFF \
-DSANITIZERS=thread && \
cmake --build ./bld-cmake --parallel $( nproc ) && \
TSAN_OPTIONS="suppressions=$(pwd)/test/sanitizer_suppressions/tsan:halt_on_error=1:second_deadlock_stack=1" ./bld-cmake/test/functional/wallet_multiwallet.py

...

💬 maflcko commented on pull request "rpc: use CScheduler for relocking wallet and remove RPCTimer":
(https://github.com/bitcoin/bitcoin/pull/32862#issuecomment-3127574724)
It looks like a false positive, so I guess if you really want to fix it, you'll have to do it upstream.

💬 darosior commented on pull request "Introduce per-txin sighash midstate cache for legacy/p2sh/segwitv0 scripts":
(https://github.com/bitcoin/bitcoin/pull/32473#issuecomment-3127602406)
I ran two identically compiled nodes on two identical VMs on the same host, one on top of fb7e9decf3f12ebae786e0cecf2f24a91c6e9d4c (this PR modified to enable caching for block validation too) and one on top of 2cad7226c2d02ec67bbac7ec909030f8bae593f8 (the commit it is based on).

Over 4 days of blocks (from height 906875 (`00000000000000000000264374e6d9b26be206793712e94266a62ddb0e9551e9`) to height 907450 (`00000000000000000001d85ba0ff861fd2499bd72fd436e2c37e11e0c28caf74`)), the average input
...

💬 ryanofsky commented on pull request "ipc: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-3127615193)
Thanks for the reviews!

Updated 3d099384a60c8f64c8d67a9f1e7b8649a9c54313 -> e1f139bd5fc9ee737d2b08307fca2b33354c5747 ([`pr/mine.29`](https://github.com/ryanofsky/bitcoin/commits/pr/mine.29) -> [`pr/mine.30`](https://github.com/ryanofsky/bitcoin/commits/pr/mine.30), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/mine.29..pr/mine.30)) just fixing comments to clarify things.

---

re: https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-3116367414

> It would be better to
...

💬 ajtowns commented on pull request "refactor: GenTxid type safety followups":
(https://github.com/bitcoin/bitcoin/pull/33005#discussion_r2236858410)
Sounds fine.

Another option would be to move `info_for_relay` from txmempool.h into net_processing.cpp:

```c++
/** Returns info for a transaction if its entry_sequence < last_sequence */
static TxMempoolInfo info_for_relay(const CTxMemPool& mempool, const GenTxid& gtxid, uint64_t last_sequence)
{
return std::visit([&](const auto& id) {
LOCK(mempool.cs);
auto i{mempool.GetIter(id)};
return (i.has_value() && i.value()->GetSequence() < last_sequence) ? mempo
...

👍 ryanofsky approved a pull request: "rpc: Handle -named argument parsing where '=' character is used"
(https://github.com/bitcoin/bitcoin/pull/32821#pullrequestreview-3063259562)
Code review ACK fa37153288ca420420636046ef6b8c4ba7e5a478. Thanks for the updates! Only change since last review was clarifying a comment and some variable names and adding new tests to cover cases where non-string positional parameters containing '=' are passed.

👍 ryanofsky approved a pull request: "util: Abort on failing CHECK_NONFATAL in debug builds"
(https://github.com/bitcoin/bitcoin/pull/32588#pullrequestreview-3063286442)
Code review ACK fa37153288ca420420636046ef6b8c4ba7e5a478, just catching subprocess.CalledProcessError in test fixing up a comment since last review

👍 pablomartin4btc approved a pull request: "doc: Add legacy wallet removal release notes"
(https://github.com/bitcoin/bitcoin/pull/33075#pullrequestreview-3063302337)
ACK fa45ccc15dfc52e798da62548dc43d1bd7889c9a

🤔 janb84 reviewed a pull request: "doc: Add legacy wallet removal release notes"
(https://github.com/bitcoin/bitcoin/pull/33075#pullrequestreview-3063334927)
re ACK fa45ccc15dfc52e798da62548dc43d1bd7889c9a

changes since last ACK:
- removed double mention of `newkeypool `

💬 l0rinc commented on pull request "log: rate limiting followups":
(https://github.com/bitcoin/bitcoin/pull/33011#issuecomment-3127754875)
Concept ACK

⚠️ enirox001 opened an issue: "Build warnings from deprecated std::get_temporary_buffer in std::stable_sort calls"
(https://github.com/bitcoin/bitcoin/issues/33080)
### Is there an existing issue for this?

- [x] I have searched the existing issues

### Current behaviour

Clang 20 generates deprecation warnings when std::stable_sort internally calls std::get_temporary_buffer (deprecated in C++17, removed in C++26).

### Expected behaviour

Clean builds without deprecation warnings.

### Steps to reproduce

Compile with Clang 20 (observed during fuzz build):

```
cmake --preset=libfuzzer
cmake --build build_fuzz
```

### Relevant log output

```
warning: 'ge
...