💬 maflcko 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-2668799834)
> CI failures that don't repro locally are the most tedious to nail down.

It is not possible to reliably reproduce undefined behavior, because it is undefined. My recommendation would be to just run in valgrind to spot any memory errors (and get a stacktrace). Alternatively, you can re-compile with hardened library flags, but that requires a recompilation with different flags and doesn't give a backtrace either.

💬 glozow commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#issuecomment-2668811890)
> The changes here have surprisingly little effect on the included benchmarks:

My expectation was that the time would go up with these changes, but hopefully not by too much.

I am pretty surprised that "[txorphanage] when full, evict from the DoSiest peers first" doesn't make `OrphanageEvictionManyPeers` slower, but I guess it's because there are only 24 transactions.

💬 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-2668821917)
It's unlikely to be related to starting up the node, because this happens only on the fourth `waitforblockheight` call in the test, where it's waiting for `current_height + 1`. That's the only time when it actually has to wait. It should hit the timeout after 2 milliseconds.

💬 stickies-v commented on pull request "Fix field name styling in `submitpackage` RPC results":
(https://github.com/bitcoin/bitcoin/pull/31900#issuecomment-2668823269)
`submitpackage` is marked as experimental and unstable, so changing that is fine imo. That does not hold for `testmempoolaccept`, so if we make changes to fields you'll have to add a `-deprecatedrpc` option. I don't think breaking a stable API just for stylistic purposes is worth it, so I would postpone that until we need another breaking change.

Would be good to add release notes to document the `submitpackage` breaking changes.

💬 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#discussion_r1961792583)
This might be a problem.

💬 purpleKarrot commented on pull request "refactor: modernize outdated trait patterns using helper aliases (C++14)":
(https://github.com/bitcoin/bitcoin/pull/31904#discussion_r1961800838)
check formatting.

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