💬 achow101 commented on issue "Revisiting us self-hosting parts of our CI":
(https://github.com/bitcoin/bitcoin/issues/31965#issuecomment-3152073848)
> Any ideas on how to handle this?

From https://docs.github.com/en/actions/reference/workflows-and-actions/events-that-trigger-workflows#pull_request, it looks like there are other activity types that we can enable to trigger a workflow to run. Maybe if we have a workflow that is also triggered by the `labeled` activity when some "periodic CI" label is added. Then DrahtBot can add that label to PRs to run a particular task again?

Maybe we could use https://docs.github.com/en/actions/reference/
...

💬 0xB10C commented on pull request "rpc: fix getpeerinfo ping duration unit docs":
(https://github.com/bitcoin/bitcoin/pull/33133#issuecomment-3152076777)
> Would it make sense to add "minping" here? e.g. `getpeerinfo (pingtime, minping, and pingwait fields)`

IMO it makes more sense to remove pingtime and min_ping from the ping docs. Just having `Requests that a ping be sent to all other nodes, to measure ping time. Results provided in getpeerinfo.` should be enough and ensures that it doesn't become outdated somewhere down the line?

💬 glozow commented on pull request "validation: detect witness stripping early on":
(https://github.com/bitcoin/bitcoin/pull/33105#discussion_r2252427850)
Oh I see. I would prefer returning witness-stripped in that scenario. It feels like this function returns a not-quite-accurate result in order to accommodate a different function that must run after it to catch a different error... a bit brittle.

💬 mzumsande commented on pull request "p2p: Advance pindexLastCommonBlock early after connecting blocks":
(https://github.com/bitcoin/bitcoin/pull/32180#issuecomment-3152118533)
Thanks! I'll address the nits if / when I have to push again.

💬 glozow commented on pull request "validation: detect witness stripping early on":
(https://github.com/bitcoin/bitcoin/pull/33105#discussion_r2252433061)
We still support txid relay, so I don't think code implementing it is outdated.

✅ w0xlt closed a pull request: "refactor: Update `XOnlyPubKey::GetKeyIDs()` to return a pair of pubkeys"
(https://github.com/bitcoin/bitcoin/pull/32332)

🤔 w0xlt reviewed a pull request: "kernel: improve BlockChecked ownership semantics"
(https://github.com/bitcoin/bitcoin/pull/33078#pullrequestreview-3085546991)
reACK https://github.com/bitcoin/bitcoin/pull/33078/commits/1d9f1cb4bd6b119e1d56cbdd7f6ce4d4521fffa3

💬 jonatack commented on pull request "rpc: fix getpeerinfo ping duration unit docs":
(https://github.com/bitcoin/bitcoin/pull/33133#issuecomment-3152140560)
> IMO it makes more sense to remove pingtime and min_ping from the ping docs

Indeed, that is better. Fewer ducks to keep in a row.

💬 darosior commented on pull request "validation: detect witness stripping early on":
(https://github.com/bitcoin/bitcoin/pull/33105#discussion_r2252446293)
I am not saying code implementing txid relay is outdated.

🤔 achow101 reviewed a pull request: "index: Fix coinstats overflow"
(https://github.com/bitcoin/bitcoin/pull/30469#pullrequestreview-3085549849)
I strongly prefer to have the changes from #33134 be combined with this PR. IMO it would make the change in this PR easier to review.

💬 achow101 commented on pull request "index: Fix coinstats overflow":
(https://github.com/bitcoin/bitcoin/pull/30469#discussion_r2252442694)
In 4fc1b2214baf10d0481c11ab0fc1de2b5d55d687 "index: Fix coinstatsindex overflow issue"

`total_unspendable_amount` is still a cumulative value, not per block.

💬 darosior commented on pull request "net, validation: don't punish peers for consensus-invalid txs":
(https://github.com/bitcoin/bitcoin/pull/33050#issuecomment-3152209934)
> I don't see why all standardness rules can't be validated prior to doing any costly hashing or signature validation

I think it's non-trivial but should be possible in Tapscript to validate all standardness rules before executing the leaf script. It would effectively be a tightening of standardness (runtime upgrade hook checks like the key version would be statically inspected instead), but not one that should cause any issue to non-pathological transactions. For non-Tapscript scripts, short
...

🤔 l0rinc reviewed a pull request: "refactor, index: Remove member variables in coinstatsindex"
(https://github.com/bitcoin/bitcoin/pull/33134#pullrequestreview-3085337044)
Concept ACK

Left a few notes, I think we could split this into smaller, more focused changes, explaining the motivation and decisions in the commit messages and since we're already cleaning up, there are a few more steps that we could do.

💬 l0rinc commented on pull request "refactor, index: Remove member variables in coinstatsindex":
(https://github.com/bitcoin/bitcoin/pull/33134#discussion_r2252318499)
What's the reason for doing these calculations in the middle of the method - is it to avoid calculating the subsidy when there's a read error?
I wouldn't optimize for that usecase: leaving the `block_subsidy` declaration at the beginning and only updating `total_subsidy` at the very end would avoid repeating the `block.height > 0` condition and the diff would also be smaller.

Also, since we're reading and writing a `DBVal` anyway, could use that to hold the state instead of keeping track of
...

💬 l0rinc commented on pull request "refactor, index: Remove member variables in coinstatsindex":
(https://github.com/bitcoin/bitcoin/pull/33134#discussion_r2252407121)
After the refactor every path returns `true`, the function can now be `void`.

💬 l0rinc commented on pull request "refactor, index: Remove member variables in coinstatsindex":
(https://github.com/bitcoin/bitcoin/pull/33134#discussion_r2252400005)
I know you didn't write this, but maybe this is a good time to clarify that we're not strictly changing the order of something (i.e. [reversing](https://www.merriam-webster.com/dictionary/reverse)), but [undoing](https://github.com/bitcoin/bitcoin/blob/master/src/index/coinstatsindex.cpp#L413)/[rewinding](https://github.com/bitcoin/bitcoin/blob/master/src/index/base.h#L125)/[reverting](https://github.com/bitcoin/bitcoin/blob/master/src/rpc/blockchain.cpp#L3135)/rolling back.

💬 l0rinc commented on pull request "refactor, index: Remove member variables in coinstatsindex":
(https://github.com/bitcoin/bitcoin/pull/33134#discussion_r2252430795)
nit: these can also be a `const` now:
```C++
const COutPoint outpoint{tx->GetHash(), j};
const Coin coin{out, block.height, is_coinbase};
```

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