🤔 naiyoma reviewed a pull request: "net_processing: rename RelayTransaction to better describe what it does"
(https://github.com/bitcoin/bitcoin/pull/33565#pullrequestreview-3356353069)
utACK 84b2ad03349bf6af3d6057f3dbcdf84c17f11bcb

💬 w0xlt commented on pull request "wallet/rpc: fix listdescriptors RPC fails to return descriptors with private key information when wallet contains descriptors missing any key":
(https://github.com/bitcoin/bitcoin/pull/32471#discussion_r2444943716)
nit: Could reuse one temporary provider to reduce alloc churn.

```suggestion
FlatSigningProvider tmp_provider;
for (const auto& pubkey : m_pubkey_args) {
tmp_provider.keys.clear();
pubkey->GetPrivKey(0, arg, tmp_provider);
if (tmp_provider.keys.empty()) return false;
}
```

🤔 cedwies reviewed a pull request: "ci: Only write docker build images to Cirrus cache"
(https://github.com/bitcoin/bitcoin/pull/33639#pullrequestreview-3356419572)
utACK fa29ecd
Reviewed the code and the new CI flow. The change to limit cache writes to Cirrus (while still reading cache on GHA) and moving the build step into Python with `shlex.split(os.getenv("DOCKER_BUILD_CACHE_ARG", ""))` looks good to me. I don’t see any unintended behavior changes beyond what is described in the PR.

💬 ajtowns commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2445023085)
CTxMemPoolEntry to only need to be movable for `mempool_entries.emplace_back(ConsumeTxMempoolEntry(...))` in test/fuzz/policy_estimator.cpp.

I'm a bit surprised `Ref::operator=(Ref&&)` isn't virtual -- moving a Ref that might actually a mempool entry seems dangerous; but otoh, moving by operator= only seems to be used in fuzz/txgraph.cpp, which means we don't actually move anything that's not just a Ref.

💬 laanwj commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2445067273)
Mind that `codecvt_utf8_utf16` (and the entirety of codecvt) is deprecated in C++17 and will be removed in C++26. We might not want to introduce another instance.
(see [codecvt_utf8_utf16](https://en.cppreference.com/w/cpp/locale/codecvt_utf8_utf16.html) and #32361)

💬 Eunovo commented on pull request "validation: ensure assumevalid is always used during reindex":
(https://github.com/bitcoin/bitcoin/pull/31615#discussion_r2445073249)
Done.

✅ l0rinc closed a pull request: "log: unify `UpdateTip` values"
(https://github.com/bitcoin/bitcoin/pull/32996)

💬 TheCharlatan commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2445190158)
Looks like all we need to do here is add the appropriate manifest same as in #32380? If so, I would just wait for that.

💬 ajtowns commented on pull request "refactor: Construct g_verify_flag_names on first use":
(https://github.com/bitcoin/bitcoin/pull/33600#issuecomment-3422388238)
ACK faa9d10c84bc6b465cbca266468990cc716b4300

Didn't realise this wasn't already merged. This adds a trivial implicit mutex when this variable is accessed which is a little lame, but it's not accessed in performance critical places, and even if it were, it's fast anyway.

💬 TheCharlatan commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2445236632)
You are correctly describing what I want this test to do.

📝 Sjors opened a pull request: "doc: add AGENTS.md"
(https://github.com/bitcoin/bitcoin/pull/33662)
Suggest disclosure via commit message:

```
Assisted-by: GitHub Copilot
Assisted-by: OpenAI GPT-5-Codex
```

This can be useful information for reviewers, especially when the PR is from a new contributor.

In my experience using Visual Studio Code in agent mode (e.g. on https://github.com/sjors/sv2-tp) it usually picks up and follows this instruction.

Of course anyone who wishes to hide the fact they're using an LLM can simply modify the commit message.

See https://agents.md

💬 m3dwards commented on pull request "doc: add AGENTS.md":
(https://github.com/bitcoin/bitcoin/pull/33662#issuecomment-3422505220)
Concept ACK

nit: The instruction could tell the LLM to add itself rather than the current literal instruction of adding the two hardcoded values. Perhaps they are clever enough to figure out what's being asked.

💬 laanwj commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2445321889)
Yes,sgtm.
imo we should go with the manifest change and rip out all UTF-8 conversion.
Now that windows 10 is EOL i think it's very fair to depend on a windows 10 feature, we should get that merged soon.

💬 TheCharlatan commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-3422547071)
Thank you for the review @stringintech!

Updated a755e00a13541b3b5a707cf385f1cbec0449c6a9 -> edf99b88e644c7d2a2db434c8db298c9c5303cf9 ([kernelApi_74](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_74) -> [kernelApi_75](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_75), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelApi_74..kernelApi_75))

* Addressed @stringintech's comments [1](https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2440887703), [2](https
...

💬 laanwj commented on pull request "Modernize use of UTF-8 in Windows code":
(https://github.com/bitcoin/bitcoin/pull/32380#issuecomment-3422576068)
re-ACK 53e4951a5b5b9d166d278db4240513d09b447f58
Especially now that Windows 10 is EOL, it's fine to depend on a Windows 10 feature.

👍 l0rinc approved a pull request: "Fix windows libc++ `fs::path` `fstream` compile errors"
(https://github.com/bitcoin/bitcoin/pull/33550#pullrequestreview-3357116782)
Code review ACK c864a4c1940d682f7eb6fdb3b91b18d638b59330

It bothers me a bit that the first commit doesn't compile, but it seems fixed in the second one - I'm fine with merging as long as the second commit is part of the PR.

I have tried reproducing the error of removing `make_preferred` and `filename` on `master`, but I'm not actually getting any failure - but it doesn't look dangerous, I'm personally fine with removing those, they don't seem to be necessary anymore.

<details>
<summar
...

🤔 enirox001 reviewed a pull request: "test: [move-only] binary utils to utils.py"
(https://github.com/bitcoin/bitcoin/pull/33633#pullrequestreview-3357335396)
ACK fa75ef4

Reviewed the commits, and this is a solid refactor. Moving the standalone binary helpers out of the large test_framework.py and into util.py is a welcome change for modularity and separation of concerns

💬 enirox001 commented on pull request "test: [move-only] binary utils to utils.py":
(https://github.com/bitcoin/bitcoin/pull/33633#discussion_r2445659129)
nano nit:

consider adding a docstring to the new export_env_build_path function. Its purpose was clear in the old context, but a comment would be helpful now that it's a standalone utility.

📝 waketraindev opened a pull request: "addrman, net: filter during address selection via AddrPolicy to avoid underfill"
(https://github.com/bitcoin/bitcoin/pull/33663)
This PR introduces `AddrMan::AddrPolicy`, a predicate that allows callers to
exclude addresses during selection. The policy returns true to skip a given
entry and is evaluated while holding `AddrMan::cs`.

The mechanism is used in `CConnman::GetAddressesUnsafe()` to filter out
banned and discouraged peers before address selection instead of removing
them afterward. This prevents `getnodeaddresses` and other RPCs from
returning fewer results than requested when large portions of the addres
...

👋 waketraindev's pull request is ready for review: "addrman, net: filter during address selection via AddrPolicy to avoid underfill"
(https://github.com/bitcoin/bitcoin/pull/33663)