💬 optout21 commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2444750382)
This way both `block` and `blockpart` api's take the same paramaters, including `"offset"` and `"size"`, right? Is that intended?

📝 optout21 opened a pull request: "test: Add test on skip heights in CBlockIndex"
(https://github.com/bitcoin/bitcoin/pull/33661)
The skip height values, used by `CBlockIndex::BuildSkip()` and `GetSkipHeight` are not tested and not well documented. (noticed while reviewing #33515, recently merged).

The motivation is to document the skip value computation through a test.

The first commit adds a test for the properties of the distribution of skip values, namely that they have non-uniform distribution: most values are small but there are some large ones as well.

The second commit adds low-level tests to the `GetSkipH
...

🤔 maflcko reviewed a pull request: "fix: invalid plist format and update values to meet macOS 1.0 standard"
(https://github.com/bitcoin/bitcoin/pull/33654#pullrequestreview-3356260624)
is this llm generated?

💬 maflcko commented on pull request "fix: invalid plist format and update values to meet macOS 1.0 standard":
(https://github.com/bitcoin/bitcoin/pull/33654#discussion_r2444851592)
Isn't 10.14 different from 14.0?

💬 stickies-v commented on pull request "Improve fatal error message for block read failures in BaseIndex::ProcessBlock":
(https://github.com/bitcoin/bitcoin/pull/33659#issuecomment-3421841236)
NACK, feels like busywork, don't see how this improves things.

✅ halilyucel closed a pull request: "Improve fatal error message for block read failures in BaseIndex::ProcessBlock"
(https://github.com/bitcoin/bitcoin/pull/33659)

💬 famouswizard commented on pull request "fix: invalid plist format and update values to meet macOS 1.0 standard":
(https://github.com/bitcoin/bitcoin/pull/33654#discussion_r2444875498)
[maflcko](https://github.com/maflcko), yes, exactly - the previous value `14` was incorrect because it refers to macOS 14 (Sonoma). the correct minimum version is `10.14.0` (Mojave), using the proper X.Y.Z format required by LSMinimumSystemVersion.

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