💬 l0rinc commented on issue "CI: Failed pulls from docker.io causing jobs to fail":
(https://github.com/bitcoin/bitcoin/issues/31797#issuecomment-2668710356)
Happened again in https://github.com/bitcoin/bitcoin/pull/31904/checks?check_run_id=37470244364

💬 rkrux commented on issue "wallet: wrong balance and crash after reorg and unclean shutdown":
(https://github.com/bitcoin/bitcoin/issues/31824#issuecomment-2668723047)
> I think the root of the problems is that changes to the wallet db are synced immediately, but the best block locator isn't.

Is this not where the chainstate related changes are flushed to disk? https://github.com/bitcoin/bitcoin/blob/28.x/src/validation.cpp#L3069

And here block disconnected notification fired that is listened to by the wallet: https://github.com/bitcoin/bitcoin/blob/28.x/src/validation.cpp#L3087

💬 vasild 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-2668744831)
I would guess the optional is dereferenced when it is nullopt and this is not compiler related. Maybe it only happens on CI due to some timing specifics? CI failures that don't repro locally are the most tedious to nail down. Try adding a lot of `LogError("reached line 123, relevant values %d %s %whatever", ...)` and then inspect the CI log for those.

💬 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-2668747128)
Trying `.value()` instead of `*`...

https://github.com/bitcoin/bitcoin/compare/51aa29ede7bd9a1897f3cf240e70ef180849591d..3b877e6718a916402adf8dcb141da01064289a6b

💬 maflcko commented on issue "CI: Failed pulls from docker.io causing jobs to fail":
(https://github.com/bitcoin/bitcoin/issues/31797#issuecomment-2668753666)
`24.04` is already cached, so it is unclear why `noble` gets rate-limited. I guess that'd be a reason to use the same name consistently.

Absent that, a real fix would still be good.

💬 glozow commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1961743938)
fwiw, here's what I landed on for `test_orphanage_dos_many`:
(1) send 51 of the same orphan from 60 peers (51 orphans, 3060 announcements). `sync_with_ping` and wait until at least one of the orphans is in orphanage
(2) send the 1p1c, which is a small orphan (not 100kvb because prior to increasing `-maxorphantxs`, this peer's memory DoS score could be higher than the other peers' CPU DoS scores and get the tx evicted. Better to compare apples to apples).
(3) send 51 unique orphans from 40 pee
...

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