💬 maflcko commented on pull request "depends: build libevent with CMake":
(https://github.com/bitcoin/bitcoin/pull/29835#issuecomment-2209317804)
Not sure if this is a real issue, because OSS-Fuzz broke their infra today. I'll check back tomorrow.

👍 stickies-v approved a pull request: "kernel: De-globalize validation caches"
(https://github.com/bitcoin/bitcoin/pull/30141#pullrequestreview-2156092162)
ACK c1d6e525131cb9e54bbedb79ea64e2469a77aed8 . Left a few nits but would suggest not to address unless force push is necessary, although of course I'll quickly re-review if you do.

> I guess it shows the dangers of reviewing with range-diff

FYI In my case, too, this wasn't a review failure because of range-diff, as I've done multiple full re-reviews that included the affected line. I think I was blind to it because of the similarity to the `-maxsigcache` halving that is expected.

Thanks
...

💬 stickies-v commented on pull request "kernel: De-globalize validation caches":
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1663978129)
nit: I don't think you meant to introduce this diff in 71774fb9f37110c68f7eed8f0ce9c857d5f196c5

💬 stickies-v commented on pull request "kernel: De-globalize validation caches":
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1664043007)
nit: unnecessary whiteline

💬 stickies-v commented on pull request "kernel: De-globalize validation caches":
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1665894249)
nit: since this line isn't a move-only change anyway, could address clang-tidy fix (+ for `SignatureCache::ComputeEntrySchnorr`)

```suggestion
void SignatureCache::ComputeEntryECDSA(uint256& entry, const uint256& hash, const std::vector<unsigned char>& vchSig, const CPubKey& pubkey) const
```

💬 stickies-v commented on pull request "kernel: De-globalize validation caches":
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1665881816)
in 06e87c2455614041416ab391bfd98b11715343a8: modifying a global inside a constructor seems quite footgunny. The footgun is removed in the next commit 0d41630f1da92d56e6874d9f0c7d7fbcd24fe0a3, but I think the robust thing to do would be to squash the next commit to avoid cherry-pick accidents. I don't practically see this leading to issues, so I'm fine keeping it as is too to minimize the range-diff, so it's probably more of a review note.

💬 stickies-v commented on pull request "kernel: De-globalize validation caches":
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1665861575)
nit: these includes don't seem necessary

💬 maflcko commented on pull request "refactor: Add util::Result failure values, multiple error and warning messages":
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1665942681)
79a970d4f12458a175317c453e251db7846c3561: I am not really sure, what the point of `Update` here is. A simple `return Interrupted{};` compiles as well. I fail to see why it would be wrong, because the default constructed monostate should not be returned either way.

(Same for all code below)

My recommendation would be to keep the code as simple as possible and not add complicated features or complication that isn't required.

👍 itornaza approved a pull request: "Stratum v2 Noise Protocol"
(https://github.com/bitcoin/bitcoin/pull/29346#pullrequestreview-2159282833)
Concept ACK a2436955f5bde920e41e03a5db34d477bfeb04a6

As stated in my previous comments, the implementation of the Stratum v2 Noise Protocol is adding more flexibility to Bitcoin than overhead towards the integration of the Stratum protocol Template Provider and Job Declaration Client roles within bitcoind.

I include some really minor nits that came out during a quick code review against the protocol specs and leaving them here to your discretion.

💬 itornaza commented on pull request "Stratum v2 Noise Protocol":
(https://github.com/bitcoin/bitcoin/pull/29346#discussion_r1665927305)
nit: Is that comment needed? SV2 spec 4.5.2 is not relevant to message length.

💬 itornaza commented on pull request "Stratum v2 Noise Protocol":
(https://github.com/bitcoin/bitcoin/pull/29346#discussion_r1665929102)
styling nit: Consider splitting into multiple lines for clarity and consistency with Serialize above.

💬 itornaza commented on pull request "Stratum v2 Noise Protocol":
(https://github.com/bitcoin/bitcoin/pull/29346#discussion_r1665945833)
nit: Should the `ciphertext` message be the @param[out] in the EncryptWithAdd method?

💬 itornaza commented on pull request "Stratum v2 Noise Protocol":
(https://github.com/bitcoin/bitcoin/pull/29346#discussion_r1665944790)
nit: Should the `plain` message be the @param[in] in the `EncryptWithAdd` method?

💬 itornaza commented on pull request "Stratum v2 Noise Protocol":
(https://github.com/bitcoin/bitcoin/pull/29346#discussion_r1665954584)
nit: Consider removing the commented out code. Same to the following line as well.

💬 itornaza commented on pull request "Stratum v2 Noise Protocol":
(https://github.com/bitcoin/bitcoin/pull/29346#discussion_r1665952724)
styling nit: Consider dropping the `_our` part from the variable's name to be more consistent with the other names and be closer to the Noise protocol naming convention `s` (implicitly our) and `rs` (explicitly remote). Same for the rest of the names below.

💬 alfonsoromanz commented on pull request "Assumeutxo: bugfix on loadtxoutset with a divergent chain + tests":
(https://github.com/bitcoin/bitcoin/pull/29996#discussion_r1666000243)
Thanks @fjahr . I'm open to deleting the third commit. I mentioned this [earlier](https://github.com/bitcoin/bitcoin/pull/29996#issuecomment-2086782948) but kept it because I thought my scenario might be somehow different due to the divergent chain.

Your explanation helped me understand the concept of the headers chain, and I think you're right that #30320 may cover the remaining TODOs. I interpreted the part about `"...loading a snapshot when the current chain tip is: [...]"` as referring to
...

📝 theStack opened a pull request: "net: fix race condition in self-connect detection"
(https://github.com/bitcoin/bitcoin/pull/30394)
This PR fixes a recently discovered race condition in the self-connect detection (see #30362 and #30368).

Initiating an outbound network connection currently involves the following steps after the socket connection is established (see [`CConnman::OpenNetworkConnection`](https://github.com/bitcoin/bitcoin/blob/bd5d1688b4311e21c0e0ff89a3ae02ef7d0543b8/src/net.cpp#L2923-L2930) method):
1. set up node state
2. queue VERSION message (both steps 1 and 2 happen in [`InitializeNode`](https://github
...

💬 TheCharlatan commented on pull request "contrib: use c++ compiler rather than c compiler for binary checks":
(https://github.com/bitcoin/bitcoin/pull/30387#discussion_r1666030464)
Why is this left as `CC`?

💬 theuni commented on pull request "contrib: use c++ compiler rather than c compiler for binary checks":
(https://github.com/bitcoin/bitcoin/pull/30387#discussion_r1666042736)
Whoops, fixed (along with another typo)

💬 furszy commented on pull request "Fix cases of calls to `FillPSBT` errantly returning `complete=true`":
(https://github.com/bitcoin/bitcoin/pull/30357#discussion_r1666047097)
Seems that the PSBT 'outputs' field retains the previous output derivation path after replacement. `walletprocesspsbt` appends the new information without removing/updating the previous one. See https://github.com/furszy/bitcoin-core/commit/698e090ead3d0603146c98bd2ec06e44389f34cc.