💬 stickies-v commented on pull request "[WIP] net: return result from addnode RPC":
(https://github.com/bitcoin/bitcoin/pull/30381#issuecomment-2209184357)
> The rationale is directly from https://github.com/bitcoin/bitcoin/issues/20552

Sorry, I should have phrased this more clearly, the initial thoughts I mentioned were numbered to be addressing the same numbered elements from the paragraph above. So specifically the rationale for "behaviour change: allow removing a v2 connection when !node_v2transport", which I now understand is just to get it in line with the documentation.

> Do you have any suggestions on another way to determine success
...

🤔 fjahr reviewed a pull request: "Assumeutxo: bugfix on loadtxoutset with a divergent chain + tests"
(https://github.com/bitcoin/bitcoin/pull/29996#pullrequestreview-2159051199)
The first two commits look very good but I am not sure the third commit is still needed, but I am not completely sure yet and would like to hear what others think.

💬 fjahr commented on pull request "Assumeutxo: bugfix on loadtxoutset with a divergent chain + tests":
(https://github.com/bitcoin/bitcoin/pull/29996#discussion_r1665782174)
nit: `n3` is a pretty meaningless variable name that I wouldn't include in the logs. The comment above seems to be enough to explain what is going on so I think this can just be removed.

💬 fjahr commented on pull request "Assumeutxo: bugfix on loadtxoutset with a divergent chain + tests":
(https://github.com/bitcoin/bitcoin/pull/29996#discussion_r1665838469)
This goes back to some of my earlier comments but I think it got lost because there were multiple things discussed at the same time: I think this is not the error we want to hit in this case (as I interpret the TODO) and I think we will not hit it in the future either, because once #30320 is merged this will hit the error message added there and not this one. This error message here mentions "work" but this is the actual work of the active chainstate, not some headers that the node has received.
...

💬 hebasto commented on pull request "kernel: Streamline util library":
(https://github.com/bitcoin/bitcoin/pull/29015#issuecomment-2209258843)
It seems that `libbitcoin_consensus` still depends on `libbitcoin_util` via the `ParseHex()` call in `pubkey.cpp`.

However, the `"consensus util"` dependency is not allowed in `contrib/devtools/check-deps.sh`.

💬 Sjors commented on pull request "Stratum v2 Template Provider (take 3)":
(https://github.com/bitcoin/bitcoin/pull/29432#issuecomment-2209289584)
There's also the `RequestTransactionData(.Success)` message flow to consider. This message is separate from `NewTemplate` so that a miner can immediately start hashing while the back and forth the Job Declarator Server is happening.

In the approach where the Template Provider software only gets the details it needs to send a `NewTemplate` message, we'll need an additional RPC method of ZMQ feed to provide the information needed in a `RequestTransactionData.Success` message. In the approach w
...

💬 maflcko commented on pull request "depends: build libevent with CMake":
(https://github.com/bitcoin/bitcoin/pull/29835#issuecomment-2209314790)
```
/src/bitcoin-core/depends/work/download/libevent-2.1.12-stable/libevent-2.1.12-stable.tar.gz.temp: OK
Extracting libevent...
/src/bitcoin-core/depends/sources/libevent-2.1.12-stable.tar.gz: OK
Preprocessing libevent...
patching file CMakeLists.txt
patching file cmake/AddEventLibrary.cmake
Configuring libevent...
-- The C compiler identification is Clang 18.0.0
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - failed
-- Check for working C compiler: /usr/local/bin
...

💬 maflcko commented on pull request "depends: build libevent with CMake":
(https://github.com/bitcoin/bitcoin/pull/29835#issuecomment-2209314974)
https://github.com/google/oss-fuzz/actions/runs/9797043349/job/27052782296#step:7:9340

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