💬 instagibbs commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1640177309)
could leave an assert to this effect right after this

💬 instagibbs commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1638488625)
```Suggestion
LIMITED_WHILE(provider.remaining_bytes() > 0, TestBitSet::Size() * TestBitSet::Size()) {
```

💬 instagibbs commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1638450101)
```Suggestion
// Read (parent, child) pairs, and add them to the cluster and depgraph.
```

💬 instagibbs commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1644945090)
it'd be worth it to have `del_set` be sometimes overcomplete, including random subsets of `~todo` which should be handled internally by being dropped. Alternatively it could be disallowed via `Assume()`?

💬 ajtowns commented on issue "RFC: Misused LogError and LogWarning macros":
(https://github.com/bitcoin/bitcoin/issues/30348#issuecomment-2194982649)
> I just don't see a significant distinction between different syntaxes `LogDebug(...)` or `LogPrint(BCLog::Debug, ...)` or even `log.Debug(...)` so have not been trying to raise syntax as an issue.

The second one is an extra 13 characters of noise each time (20 if you don't do something to avoid having to type `BCLog::Level::Debug`); for the third one, C++ doesn't allow you to have it be a magic macro where the arguments aren't even evaluated when debug logging isn't enabled. Far better to h
...

📝 Sjors opened a pull request: "[WIP] libbitcoin_net"
(https://github.com/bitcoin/bitcoin/pull/30350)

✅ Sjors closed a pull request: "[WIP] libbitcoin_net"
(https://github.com/bitcoin/bitcoin/pull/30350)

💬 Sjors commented on pull request "[WIP] libbitcoin_net":
(https://github.com/bitcoin/bitcoin/pull/30350#issuecomment-2195009062)
Sorry, was trying to open this against my own fork.

💬 ajtowns commented on issue "RFC: Misused LogError and LogWarning macros":
(https://github.com/bitcoin/bitcoin/issues/30348#issuecomment-2195019443)
> Looking at the code I found 58 cases where `LogError` appeared to be use correctly, in conditions that would lead to full or partial shutdowns, and 61 cases where it appeared to be used incorrectly to report errors that would not cause shutdowns and were potentially transient.

#30347 doesn't seem to actually fix any of the 61 incorrect cases? Maybe have a PR to manually fix the incorrect ones, then a scripted diff to convert everything to the new names?

💬 kevkevinpal commented on pull request "test: Added coverage to Block not found error using gettxoutsetinfo":
(https://github.com/bitcoin/bitcoin/pull/30340#issuecomment-2195027831)
thanks @tdb3 and @alfonsoromanz I added a log in [82c454c](https://github.com/bitcoin/bitcoin/pull/30340/commits/82c454c65658439481b50fe71ed097ecb8d70737)

---

Also @tdb3 I tried doing
```
b1hash = index_node.getblockhash(1)
index_node.invalidateblock(b1hash)

self.log.info("Test obtaining info for a non-existent block hash")
assert_raises_rpc_error(-5, "Block not found", index_node.gettxoutsetinfo, hash_type="none", hash_or_height=b1hash, use_index=True)

index_node.reconsiderblock
...

💬 kevkevinpal commented on pull request "test: Added coverage to Block not found error using gettxoutsetinfo":
(https://github.com/bitcoin/bitcoin/pull/30340#discussion_r1657352656)
thanks updated in [6809ffd](https://github.com/bitcoin/bitcoin/pull/30340/commits/6809ffd8a6c589c515af48da2dd8367e4c6c2c26)

also the earlier comment I addressed below https://github.com/bitcoin/bitcoin/pull/30340#issuecomment-2195027831

💬 darosior commented on pull request "fuzz: a new target for the coins database":
(https://github.com/bitcoin/bitcoin/pull/28216#issuecomment-2195058547)
Status here is i'm looking into some determinism issues pointed out by Niklas: https://gnusha.org/bitcoin-core-dev/2024-06-03.log

👍 theuni approved a pull request: "kernel: De-globalize validation caches"
(https://github.com/bitcoin/bitcoin/pull/30141#pullrequestreview-2145715522)
utACK modulo the non-copyable comment.

Looks good. Left some comments with a few nice-to-have's. I'm not sure how much trouble it'd be to pass in the nonces (and whether it's 100% safe to reuse them), so that might make more sense as a follow-up.

💬 theuni commented on pull request "kernel: De-globalize validation caches":
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1657330024)
This function can be const.

💬 theuni commented on pull request "kernel: De-globalize validation caches":
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1657322116)
Move this into `ValidationCache`? It seems unused anywhere else.

💬 theuni commented on pull request "kernel: De-globalize validation caches":
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1657354399)
Same comment here, would be nice to pass this in. Perhaps one nonce to rule them all? I don't think there'd be any loss of security.

💬 theuni commented on pull request "kernel: De-globalize validation caches":
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1657370573)
Similar to @stickies-v's comment in a previous commit, since we're going to be passing around references and pointers to this, it'd be a good idea to make it non-copyable to avoid accidental copies.

💬 theuni commented on pull request "kernel: De-globalize validation caches":
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1657337310)
Might be useful to pass the nonce in rather than generating it here to make the order of rng/ValidationCache instantiation explicit.

Presumably it'd also make for easier testing with a deterministic seed.

💬 theuni commented on pull request "kernel: De-globalize validation caches":
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1657394662)
Oh, I guess the mutex takes care of that for us. Still, wouldn't hurt to be explicit.

💬 murchandamus commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1622850893)
Thanks, fixed