💬 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#issuecomment-2668757560)
This failure is strange. I tried to reproduce the failure from https://cirrus-ci.com/task/6207546920271872?logs=ci#L2923:

```c++
node0 stderr /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/optional:479: _Tp &std::_Optional_base_impl<interfaces::BlockRef, std::_Optional_base<interfaces::BlockRef>>::_M_get() [_Tp = interfaces::BlockRef, _Dp = std::_Optional_base<interfaces::BlockRef>]: Assertion 'this->_M_is_engaged()' failed.
```

using gcc 13.3.0, checking out commit
...

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

* [bench] TxOrphanage::LimitOrphans

| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 68,742,718.06 | 14.55 | 0.2% | 10.59 | `OrphanageEraseForBlockSinglePeer`
| 8,857.40 | 112,899.93 | 0.3% | 10.86 | `OrphanageEvictionManyPeers`
| 1,544,919.6
...

💬 purpleKarrot commented on pull request "refactor: modernize outdated trait patterns using helper aliases (C++14)":
(https://github.com/bitcoin/bitcoin/pull/31904#discussion_r1961755502)
You can use `std::is_same_v` here!

💬 maflcko commented on pull request "ci: optionally use local docker build cache":
(https://github.com/bitcoin/bitcoin/pull/31545#discussion_r1961758582)
I wonder if any of this could fail? While you claimed that docker can handle an parallel `rm -rf "${DOCKER_BUILD_CACHE_OLD_DIR}"`, I wonder if the `mv` and `rm` could fail when done concurrently. I guess `rm -rf` never fails, and `mv` is atomic. However, running two `mv` at the same time seems like one should fail. And running `mv` and `rm` at the same time, seem like it could corrupt the cache.

💬 l0rinc commented on pull request "refactor: modernize outdated trait patterns using helper aliases (C++14)":
(https://github.com/bitcoin/bitcoin/pull/31904#discussion_r1961764149)
I am, just doing it commit-by-commit: https://github.com/bitcoin/bitcoin/pull/31904/files#diff-4b1ae2ce314ef1f2d4cb4f2dbf7a87a0c299c942782fa734e9e80fed8324faa6R76

👍 vasild approved a pull request: "Add waitNext() to BlockTemplate interface"
(https://github.com/bitcoin/bitcoin/pull/31283#pullrequestreview-2626937531)
ACK 2d65225c4696dcc8312d546eb67f4eae11b9b8fe modulo the comment below

💬 vasild commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1961755597)
This was `return check_tip_changed() || chainman().m_interrupt;` when the lambda `check_tip_changed()` was used. Now that the lambda is expanded here, the `chainman().m_interrupt` condition was lost. I think it should be:

```suggestion
return tip_changed || chainman().m_interrupt;
```

💬 purpleKarrot commented on pull request "refactor: modernize outdated trait patterns using helper aliases (C++14)":
(https://github.com/bitcoin/bitcoin/pull/31904#issuecomment-2668789181)
This change could be automated with clang-tidy's [`modernize-type-traits`](https://clang.llvm.org/extra/clang-tidy/checks/modernize/type-traits.html) check. There are more places that can be modernized with this.

💬 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