💬 l0rinc commented on pull request "refactor: modernize outdated trait patterns using helper aliases (C++14)":
(https://github.com/bitcoin/bitcoin/pull/31904#discussion_r1961806436)
Thanks, fixed

💬 ryanofsky commented on pull request "test, refactor: Add TestNode.binaries to hold binary paths":
(https://github.com/bitcoin/bitcoin/pull/31866#discussion_r1961806652)
re: https://github.com/bitcoin/bitcoin/pull/31866#discussion_r1961532157

> I see your point about not wanting to use `self.nodes[0].binaries`, so an alternative might be to remove `TestFramework::binary_paths` and instead add an array of the same size as the versions array to store the result of a call to `[Binaries(self.get_binary_paths(), bin_dir_from_version(v)) for v in versions]`.

I think I don't fully understand the suggestion and the problem this is trying to solve. If the goal is t
...

💬 l0rinc commented on pull request "refactor: modernize outdated trait patterns using helper aliases (C++14)":
(https://github.com/bitcoin/bitcoin/pull/31904#issuecomment-2668850930)
@purpleKarrot, you can check the PR with clang-tidy if it helps, but it won't find all of the values changed here.
And even if it does, we need to simplify the review, we can't just rely on clang-tidy blindly.
Added the command that I used to check the changes via `modernize-type-traits` - let me know if you find other ones that I haven't covered.

👍 TheCharlatan approved a pull request: "cmake: Exclude generated sources from translation"
(https://github.com/bitcoin/bitcoin/pull/31899#pullrequestreview-2627041699)
ACK ff4ddd3d2e3b08156fd0a0aeb954fde0a5f4cb03

💬 purpleKarrot commented on pull request "refactor: modernize outdated trait patterns using helper aliases (C++14)":
(https://github.com/bitcoin/bitcoin/pull/31904#issuecomment-2668883277)
Changes LGTM. You may want to drop the reference to C++14 from the title, as `std::is_same_v` is actually not available until C++17. I you want, you could go full C++20 by replacing `std::is_same_v` with `std::same_as`.

📝 maflcko opened a pull request: "ci: Switch to gcr.io mirror to avoid rate limits"
(https://github.com/bitcoin/bitcoin/pull/31906)
dockerhub seems to have recently started to increase their rate limits further, beyond what is documented, even to the extent where pulling the same image twice at the same time results in a ban. See https://github.com/bitcoin/bitcoin/issues/31797#issuecomment-2656374222

Fix all issues by just using another mirror, as documented in https://cloud.google.com/artifact-registry/docs/pull-cached-dockerhub-images

Fixes https://github.com/bitcoin/bitcoin/issues/31797

💬 l0rinc commented on pull request "refactor: modernize outdated trait patterns using helper aliases (C++14/C++17)":
(https://github.com/bitcoin/bitcoin/pull/31904#issuecomment-2668905168)
Thanks for the review and the C++17 suggestion @purpleKarrot.
If you agree with the changes, please add the latest hash after your ack, otherwise it's just a concept ack.

💬 purpleKarrot commented on pull request "refactor: modernize outdated trait patterns using helper aliases (C++14/C++17)":
(https://github.com/bitcoin/bitcoin/pull/31904#issuecomment-2668914709)
ACK 7abc6e1f4b4aca4c8c576ff0cc27d68f90e42d28

💬 Sjors commented on pull request "Have createNewBlock() wait for tip, make rpc handle shutdown during long poll and wait methods":
(https://github.com/bitcoin/bitcoin/pull/31785#issuecomment-2668927555)
Perhaps on a slow machine the timeout could go negative here:

```cpp
auto now{std::chrono::steady_clock::now()};
if (now >= deadline) break;
const MillisecondsDouble remaining{deadline - now};
block = miner.waitTipChanged(current_block.hash, remaining);
```

Though that seems unlikely, and I wouldn't expect it to happen consistently. In any case I added better handling for negative timeout.

💬 maflcko commented on pull request "Fix field name styling in `submitpackage` RPC results":
(https://github.com/bitcoin/bitcoin/pull/31900#discussion_r1961857705)
reject-details was only added in 221c789e91696569fa34dbd162d26e98cf9cab64 (master), so changing it seems fine

📝 darosior opened a pull request: "qa: clarify and document assumeutxo tests"
(https://github.com/bitcoin/bitcoin/pull/31907)
The `feature_assumeutxo.py` functional test checks various errors with malleated snapshots. Some of these cases are brittle or use confusing and undocumented values. Fix those by making them less brittle and using clear, documented values.

I ran across those when working on an unrelated changeset which affected the snapshot. It took me a while to understand where the seemingly magic values were coming from, so i figured it was worth proposing this patch on its own for the sake of making the t
...

💬 ryanofsky commented on pull request "Have createNewBlock() wait for tip, make rpc handle shutdown during long poll and wait methods":
(https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1961867136)
I reproduced the CI bug and problem happens on this line. Looks like `block->hash` should be replaced by `current_block.hash` here and below

💬 maflcko commented on pull request "refactor: modernize outdated trait patterns using helper aliases (C++14/C++17)":
(https://github.com/bitcoin/bitcoin/pull/31904#discussion_r1961870266)
pls submit those upstream

🤔 l0rinc reviewed a pull request: "fuzz: a new target for the coins database"
(https://github.com/bitcoin/bitcoin/pull/28216#pullrequestreview-2626885053)
I still can't run any fuzzing on my Mac (for the past ~5 months), so I only added code review comments based on what I found.

💬 l0rinc commented on pull request "fuzz: a new target for the coins database":
(https://github.com/bitcoin/bitcoin/pull/28216#discussion_r1961863151)
The file name previously coincided with the only fuzz target - now a `coins_view` contains a `coins_db` target as well.
Given that the type is `CCoinsViewDB`, we could rename this to:
```suggestion
FUZZ_TARGET(coins_view_db, .init = initialize_coins_view)
```

💬 l0rinc commented on pull request "fuzz: a new target for the coins database":
(https://github.com/bitcoin/bitcoin/pull/28216#discussion_r1961742988)
The boolean parameter basically decides if the second parameter is a database view or not.
We could reduce this duplication by deducing it inside the `TestCoinsView` method instead:
```C++
void TestCoinsView(FuzzedDataProvider& fuzzed_data_provider, CCoinsView& backend_coins_view)
{
const bool is_db{!!dynamic_cast<CCoinsViewDB*>(&backend_coins_view)};
```
or if you prefer:
```C++
void TestCoinsView(FuzzedDataProvider& fuzzed_data_provider, CCoinsView& backend_coins_view)
{
con
...

💬 l0rinc commented on pull request "fuzz: a new target for the coins database":
(https://github.com/bitcoin/bitcoin/pull/28216#discussion_r1961864110)
This is already obvious from two lines below:
```suggestion
.path = "",
```

💬 l0rinc commented on pull request "fuzz: a new target for the coins database":
(https://github.com/bitcoin/bitcoin/pull/28216#discussion_r1961749796)
nit: I know this was just copied over, but technically this is
```suggestion
.cache_bytes = 1 << 20, // 1MiB.
```

💬 l0rinc commented on pull request "fuzz: a new target for the coins database":
(https://github.com/bitcoin/bitcoin/pull/28216#discussion_r1961871432)
What's the purpose of the comment?
Why not add that to the commit message only?

💬 l0rinc commented on pull request "fuzz: a new target for the coins database":
(https://github.com/bitcoin/bitcoin/pull/28216#discussion_r1961727492)
I thought I commented on this already, but seems I forgot to submit:
👍 for
```suggestion
assert(is_db == !!coins_view_cursor);
```
or maybe even
```suggestion
assert(is_db != !coins_view_cursor);
```