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

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

💬 l0rinc commented on pull request "coins: use number of dirty cache entries in flush warnings/logs":
(https://github.com/bitcoin/bitcoin/pull/33512#discussion_r2446031471)
Seems we were wrong, the test assumes that after this call the given `COutPoint` will correspond to the current coins - which isn't the case with `EmplaceCoinInternalDANGER` which never replaces, so I have reverted https://github.com/bitcoin/bitcoin/compare/62cb2a2ed84e695c0959d8e27fc5bedd9b5a135e..54151d612b63f4e42fe03a143540e597728a4cc3

See the previous errors that the suggestions resulted in:
https://github.com/bitcoin/bitcoin/actions/runs/18611576758/job/53070427535?pr=33512#step:7:40482
...

💬 romanz commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#issuecomment-3423601774)
> Some of the PR description is reused from the previous PR, but is now stale.

Good catch, thanks!
Updated PR description: https://github.com/bitcoin/bitcoin/pull/33657#issue-3529967048

💬 l0rinc commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2446074332)
The latest https://maflcko.github.io/b-c-cov/fuzz.coverage/src/txgraph.cpp.gcov.html#L1446 indicates that `SingletonClusterImpl::Split` may not be covered fully.

💬 l0rinc commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#issuecomment-3423668761)
Concept ACK, thanks for helping with the coverage and documentation.
For reference https://maflcko.github.io/b-c-cov/test_bitcoin.coverage/src/chain.cpp.gcov.html is pretty well covered otherwise, but these tests can help with the specifics.

💬 kanzure commented on pull request "doc: add AGENTS.md":
(https://github.com/bitcoin/bitcoin/pull/33662#issuecomment-3423684598)
Using a canary is a clever idea. I think I agree with @l0rinc's reasoning that this is not a good idea, but if there was a decision to insert an attempt at a LLM canary then I would suggest double checking the models and see if offering them a few dollars to donate to a philanthropic cause is more likely to evoke the LLM behavior you are trying to get. This strategy was previously effective, no idea if it's still effective.