💬 fjahr commented on pull request "assumeutxo: Don't load a snapshot if it's not in the best header chain":
(https://github.com/bitcoin/bitcoin/pull/30320#discussion_r1665811272)
I think this test also actually resolves the TODOs that https://github.com/bitcoin/bitcoin/pull/29996/commits/556ff68cea1dfa6c444ef9c920dd0ce52ecc9668 aimed to resolve. I will elaborate more there.

🤔 jonatack reviewed a pull request: "[WIP] net: return result from addnode RPC"
(https://github.com/bitcoin/bitcoin/pull/30381#pullrequestreview-2159102244)
Concept ACK, I'd find this useful.

💬 jonatack commented on pull request "[WIP] net: return result from addnode RPC":
(https://github.com/bitcoin/bitcoin/pull/30381#discussion_r1665813596)
Suggest `Unable to open connection to <peer address>`

💬 jonatack commented on pull request "[WIP] net: return result from addnode RPC":
(https://github.com/bitcoin/bitcoin/pull/30381#discussion_r1665816378)
Agree with @stickies-v that returning the peer id would be more useful than the result.

💬 fanquake commented on pull request "depends: build zeromq with CMake":
(https://github.com/bitcoin/bitcoin/pull/29723#issuecomment-2209179140)
> ld64.lld: error: library not found for -lrt

Still at least one other bugfix to upstream here. ZMQ is trying to link against realtime for macOS.

💬 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