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

💬 l0rinc commented on pull request "test: Fix TestFlushBehavior when coin has non-zero dynamic memory usage":
(https://github.com/bitcoin/bitcoin/pull/33381#issuecomment-3423519260)
Was a good find, thanks, I agree that it doesn't make a lot of sense anymore to keep this around, if you agree, please consider closing the PR

✅ PiRK closed a pull request: "test: Fix TestFlushBehavior when coin has non-zero dynamic memory usage"
(https://github.com/bitcoin/bitcoin/pull/33381)

💬 l0rinc commented on pull request "doc: add AGENTS.md":
(https://github.com/bitcoin/bitcoin/pull/33662#issuecomment-3423544412)
I'm not sure I understand - why would we do anything like this? The LLM is just a tool, it would be weird to say: "suggestions by: clang-tidy" or "IDE used: CLion" or "Assisted-by: The C++ Programming Language: Special Edition".

Maybe the problem is committing something here that's "substantially generated with automated assistance" - in the same sense as we would discourage copy-pasting something here from StackOverflow without tailoring it to our needs.

So that's a concept NACK from me.
...

💬 sdaftuar commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2445998747)
Agreed, though I think this type of behavior change regarding what packages we accept can happen in a separate PR after cluster mempool.

💬 romanz commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2446013049)
I have used the same implementation for both endpoints for simplicity and to avoid code duplication.
I can add an additional argument to enable `offset` and `size` parameter handling for the `/rest/blockpart` endpoint (and disable it for `/rest/block` endpoint).
WDYT?

💬 sdaftuar commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2446016715)
I'm inclined to implement your suggestion, so that any cluster size limit failures are treated as non-RECONSIDERABLE for now. TRUC transactions would never hit these cluster limits, so users who would be concerned about cluster size limits as a potential pinning vector for CPFP packages already have a mechanism to avoid it.

(I also considered having all cluster size limits being treated as RECONSIDERABLE, but decided that is also not great, partly because we certainly shouldn't let a transac
...

💬 romanz commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2446030515)
Sounds good - fixed in https://github.com/bitcoin/bitcoin/compare/787d6df48af31b42ee9a69ff5c33e73bbe8cf8e9..c4bf7a77c45e297672222a2e1a984ef8d755b471.