💬 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

💬 murchandamus commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1657289988)
Made all four tests output messages that indicate what went wrong in the success case.

💬 murchandamus commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1629892844)
Removed the static keyword and the clear as suggested.

💬 murchandamus commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1657397165)
Amended all the functions to directly create OutputGroups instead.

💬 murchandamus commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1622850804)
I have removed `nInput` from the function parameters of `MakeCoin(…)`.

💬 murchandamus commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1622850394)
I was able to remove all except for

```cpp
#include <consensus/amount.h>
#include <wallet/coinselection.h>
#include <wallet/test/wallet_test_fixture.h>

#include <boost/test/unit_test.hpp>
```

I’m not familiar with IOWY and could not turn up anything with a websearch, what is that?

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

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

👍 1alexbc1 approved a pull request: "test: Added coverage to Block not found error using gettxoutsetinfo"
(https://github.com/bitcoin/bitcoin/pull/30340#pullrequestreview-2145858736)
Amadeevantv yimelaja