💬 l0rinc commented on pull request "refactor, index: Remove member variables in coinstatsindex":
(https://github.com/bitcoin/bitcoin/pull/33134#discussion_r2252451393)
Seems to me we don't actually need the whole coin anymore, only its `vprevout`.
And is there a particular reason for copying the outpoint via `COutPoint outpoint{tx->vin[j].prevout.hash, tx->vin[j].prevout.n};` instead of just `tx_undo.vprevout[j]`?
And a few lines above we used `continue` to avoid extra indentation, maybe we can apply those here as well:
```C++
if (is_coinbase) continue;

const auto& vprevout{block.undo_data->vtxundo.at(i - 1).vprevout};
for (size_t j{0}; j < vprevout.si
...

💬 l0rinc commented on pull request "refactor, index: Remove member variables in coinstatsindex":
(https://github.com/bitcoin/bitcoin/pull/33134#discussion_r2252285727)
Is it still an *init* now that it just updates `m_muhash`? The method doc claims that: "/// Initialize internal state from the database and block index" - is that still accurate?

Since this change is not strictly inlining of fields anymore, please consider doing it in a separate commit where the commit message gives more context.

💬 l0rinc commented on pull request "refactor, index: Remove member variables in coinstatsindex":
(https://github.com/bitcoin/bitcoin/pull/33134#discussion_r2252428121)
Please consider extracting the repeated coinbase check (since it's not just a trivial getter), which would enable short-circuiting the other non-trivial-and-rare BIP30 check:
```suggestion
const bool is_coinbase{tx->IsCoinBase()};
if (is_coinbase && IsBIP30Unspendable(block.hash, block.height)) {
```

💬 l0rinc commented on pull request "refactor, index: Remove member variables in coinstatsindex":
(https://github.com/bitcoin/bitcoin/pull/33134#discussion_r2252380027)
it's not immediately obvious how these changes are related to inlining of fields (and why we don't need to keep the accounting at all) - could you please extract it to a separate commit which explains what's happening here?
Bundling low-risk changes in the same commit as high-risk changes makes review harder.

💬 0xB10C commented on pull request "rpc: fix getpeerinfo ping duration unit docs":
(https://github.com/bitcoin/bitcoin/pull/33133#issuecomment-3152330339)
- removed the `(s)` unit from the docs: https://github.com/bitcoin/bitcoin/pull/33133#discussion_r2251809033
- removed the `getpeerinfo` field documentation from the `ping` RPC: https://github.com/bitcoin/bitcoin/pull/33133#issuecomment-3152076777
- added a bit more details to the commit message: https://github.com/bitcoin/bitcoin/pull/33133#pullrequestreview-3085116958

💬 jonatack commented on pull request "init: make `-blockmaxweight` startup option debug only":
(https://github.com/bitcoin/bitcoin/pull/32654#discussion_r2252548042)
It seems this change ought to have a release note to explain why the option is no longer available by default and how to enable it.

🤔 jonatack reviewed a pull request: "rpc: fix getpeerinfo ping duration unit docs"
(https://github.com/bitcoin/bitcoin/pull/33133#pullrequestreview-3085715524)
ACK 1252eeb997df2eb12c33d92eb1a5c9d6643a67ff

💬 jonatack commented on pull request "rpc: fix getpeerinfo ping duration unit docs":
(https://github.com/bitcoin/bitcoin/pull/33133#discussion_r2252556648)
pico-nit, if you have to retouch

```suggestion
"Results are provided in RPC getpeerinfo.\n"
```

👍 theStack approved a pull request: "rpc: fix getpeerinfo ping duration unit docs"
(https://github.com/bitcoin/bitcoin/pull/33133#pullrequestreview-3085748775)
ACK 1252eeb997df2eb12c33d92eb1a5c9d6643a67ff

💬 achow101 commented on pull request "allocators: Apply manual ASan poisoning to `PoolResource`":
(https://github.com/bitcoin/bitcoin/pull/32581#issuecomment-3152410690)
ACK ad132761fc49c38769c09653a265fdbc3b93eda5

💬 luke-jr commented on pull request "Adding alert for failure to prevent dead-end user crash":
(https://github.com/bitcoin/bitcoin/pull/33127#discussion_r2252594171)
There already is, in src/init.cpp ("Do you want to rebuild the databases now?")

(But I'm not sure we can detect this condition that early)

🤔 jonatack reviewed a pull request: "test: Fix 'getdescriptoractivity' RPCHelpMan, add test to verify fix"
(https://github.com/bitcoin/bitcoin/pull/33119#pullrequestreview-3085779061)
ACK 3543bfdfec345cf2c952143c31674ef02de2a64b

Suggest updating the PR title prefix from "test" to "rpc"

💬 jonatack commented on pull request "wallet: Add `exportwatchonlywallet` RPC to export a watchonly version of a wallet":
(https://github.com/bitcoin/bitcoin/pull/32489#issuecomment-3152447889)
> For anyone who's interested, I'm hosting a review club meeting about this PR on Wednesday: [bitcoincore.reviews/32489](https://bitcoincore.reviews/32489)

There is `Review Club` label in this repo that can be added, too.

💬 luke-jr commented on pull request "Allowing multi client support in guix-build":
(https://github.com/bitcoin/bitcoin/pull/33126#discussion_r2252606990)
```suggestion
if any(' bitcoin-util utility version ' in s for s in binary.strings):
```

💬 luke-jr commented on pull request "Allowing multi client support in guix-build":
(https://github.com/bitcoin/bitcoin/pull/33126#discussion_r2252612225)
```suggestion
mv build/dist/*.zip "${OUTDIR}/${DISTNAME}-${HOST}-unsigned.zip"
```

💬 luke-jr commented on pull request "Allowing multi client support in guix-build":
(https://github.com/bitcoin/bitcoin/pull/33126#issuecomment-3152466438)
and ftr, Concept ACK. Core has always had a policy of using CLIENT_NAME correctly and not breaking needlessly when it's changed. PR title could be updated to appease naysayers.

💬 jonatack commented on pull request "cli: return local services in -netinfo":
(https://github.com/bitcoin/bitcoin/pull/31886#issuecomment-3152474568)
Rebased to re-run the CI, no other change: `git range-diff d1b5831 caea5f0 721a051`

💬 luke-jr commented on pull request "Adding alert for failure to prevent dead-end user crash":
(https://github.com/bitcoin/bitcoin/pull/33127#issuecomment-3152486579)
Looking at the relevant code, this feels like a symptom of a problem we could/should detect earlier somehow (which may also be early enough to follow the reindex GUI path)

💬 l0rinc commented on pull request "cli: return local services in -netinfo":
(https://github.com/bitcoin/bitcoin/pull/31886#issuecomment-3152490929)
Redid the rebase, reran the test, reACK 721a051320f2c10d2e9c89c985f180da81d64dca

💬 jonatack commented on pull request "test: headers sync timeout":
(https://github.com/bitcoin/bitcoin/pull/32677#issuecomment-3152491438)
Post-merge ACK, thanks @stringintech!