👍 tdb3 approved a pull request: "validation: Disable CheckForkWarningConditions for background chainstate"
(https://github.com/bitcoin/bitcoin/pull/30962#pullrequestreview-2328065227)
CR ACK c0a0c72b4d68a4f0c53c2c4b95f4d6e399f8e4ee

🤔 ismaelsadeeq reviewed a pull request: "test: Add missing sync_mempools() to fill_mempool()"
(https://github.com/bitcoin/bitcoin/pull/30948#pullrequestreview-2328108669)
Tested ACK faf801515f8fcd11a3454105cab66c38f6f240fe

💬 theuni commented on pull request "refactor: Replace g_genesis_wait_cv with m_tip_block_cv":
(https://github.com/bitcoin/bitcoin/pull/30967#discussion_r1775163810)
While you're at it, it'd be more idiomatic to use the `wait_for` that takes a predicate, rather than wrapping a `wait_until` in a `while()`.

🤔 theuni reviewed a pull request: "refactor: Replace g_genesis_wait_cv with m_tip_block_cv"
(https://github.com/bitcoin/bitcoin/pull/30967#pullrequestreview-2328154570)
Nice improvements, lgtm.

💬 l0rinc commented on pull request "test: Prove+document ConstevalFormatString/tinyformat parity":
(https://github.com/bitcoin/bitcoin/pull/30933#discussion_r1775177663)
> My aim is to retain the tests of leaving out one arg though (which your suggestion doesn't include)

I didn't include it since I though a single instance of those is enough in the tests.

I would prefer having concrete typed examples over retesting -1 and +1 args (for which explicit examples should likely suffice)

💬 l0rinc commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#issuecomment-2374018307)
I did a `-reindex-chainstate` as well, this seems to be basically the same as before:

<details>
<summary>Benchmark</summary>

```bash
hyperfine \
--runs 3 \
--export-json /mnt/ibd_full-30611.json \
--parameter-list COMMIT 33adc7521cc8bb24b941d959022b084002ba7c60,391c87640d78d9821b87e3f3de755af40a191d24 \
--prepare 'git checkout {COMMIT} && git clean -fxd && git reset --hard && cmake -B build && cmake --build build -j8' \
'COMMIT={COMMIT} ./build/src/bitcoind -datadir=/mnt/BitcoinData
...

👋 kevkevinpal's pull request is ready for review: "[tests] New fuzz target wallet_rpc"
(https://github.com/bitcoin/bitcoin/pull/30570)

💬 maflcko commented on pull request "refactor: Replace g_genesis_wait_cv with m_tip_block_cv":
(https://github.com/bitcoin/bitcoin/pull/30967#discussion_r1775211951)
I don't think this is possible in a refactor, because the code was written to ignore system-time adjustments and always ensure the wait is measured on the steady clock. (No idea if this matters or even was intentional)

If you disagree, I am happy to take and push any diff that is a refactor or a behavior change.

💬 maflcko commented on pull request "refactor: Replace g_genesis_wait_cv with m_tip_block_cv":
(https://github.com/bitcoin/bitcoin/pull/30967#discussion_r1775212650)
Sure, done

📝 TheCharlatan opened a pull request: "init: Remove retry for loop"
(https://github.com/bitcoin/bitcoin/pull/30968)
The for loop around the chain loading logic in `init.cpp` allows users of the GUI to retry once on failure with reindexing without having to manually set the reindex flag on startup. However this current mechanism has problems:

* It is badly documented and has led to confusion among developers and bugs making it into master. Examples:
* https://github.com/bitcoin/bitcoin/pull/28830/files#r1598392660
* https://github.com/bitcoin/bitcoin/pull/30132#issuecomment-2120741121
* It can on
...

💬 kevkevinpal commented on pull request "[tests] New fuzz target wallet_rpc":
(https://github.com/bitcoin/bitcoin/pull/30570#issuecomment-2374061500)
@brunoerg I think this should be ready for review now, I think I just need to currate the list of `WALLET_RPC_COMMANDS_SAFE_FOR_FUZZING` and `WALLET_RPC_COMMANDS_NOT_SAFE_FOR_FUZZING`

💬 andrewtoth commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#issuecomment-2374069851)
> I did a rebased -reindex-chainstate as well, this seems to be basically the same as before:

Nice, so there's no speed regression but crash resistance gained!

Also, I benchmarked only at 484aee0e16aff9c5fcfe66157c67bf29c66baa2a instead of 391c87640d78d9821b87e3f3de755af40a191d24 so that there's no randomness in the benchmarks. It can give us a better picture of what this PR affects.

💬 l0rinc commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#issuecomment-2374081579)
> so that there's no randomness in the benchmarks

I've rebased on master to have up-to-date data. What kind of randomness are you referring to?

> there's no speed regression

3 benchmarks, all showed different things.
We have to find out what causes these differences, that's why I want to try a complete IBD again (which seems like the only downside so far), since ultimately most people won't have blocksdir on the SSD and datadir on the HDD doing a reindex.

💬 andrewtoth commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#issuecomment-2374093681)
> What kind of randomness are you referring to?

The random window for when this PR will flush introduced in the last commit. That is good for steady state operation to not have all nodes converge on the same flush time. But, for benchmarking a reindex or IBD it will just add randomness to the results.

> 3 benchmarks, all showed different things.

Well, they are all different benchmarks with different variables. Changing more variables by running different benchmarks won't necessarily giv
...

💬 maflcko commented on pull request "test: Prove+document ConstevalFormatString/tinyformat parity":
(https://github.com/bitcoin/bitcoin/pull/30933#discussion_r1775253175)
Yeah, seems fine to drop the Args-1, because tinyformat doesn't have to throw on invalid stuff anyway (it does not for many other "invalid" things). Seems fine to just test the happy case.

👍 TheCharlatan approved a pull request: "multiprocess: Add IPC wrapper for Mining interface"
(https://github.com/bitcoin/bitcoin/pull/30510#pullrequestreview-2328340265)
ACK 1a332817665f77f55090fa166930fec0e5500727

Nice changes to the serialization code since my last review, re-using the existing concepts is great. I wonder if the Deserializable concept could just live in the `serialize.h` header. It seems like it could be generally useful?

My `clang-tidy` also wants to indent the `requires` expressions, but I don't think that is necessarily more readable.

💬 stickies-v commented on pull request "util: refactor: add and use run-time safe tinyformat::try_format":
(https://github.com/bitcoin/bitcoin/pull/30928#discussion_r1775276096)
Thanks for looking into this further! I was also exploring the concepts route, but hadn't yet resolved all concerns.

1) Re naming: having `format` overloads with significantly differently behaviour (throwing vs non-throwing) didn't quite sit right with me, so I think your `format_raw` (or `format_throw`) suggestion is preferable, and I agree it makes more sense to rename the throwing variant instead of the non-throwing variant (although the downside is that it makes reviewing this PR less str
...

⚠️ maflcko opened an issue: "win64-cross CI timeout after 2h"
(https://github.com/bitcoin/bitcoin/issues/30969)
https://cirrus-ci.com/task/6567392808009728?logs=ci#L2620

```
...
127/137 Test #131: spend_tests .......................... Passed 6.37 sec
Start 137: db_tests
128/137 Test #135: walletdb_tests ....................... Passed 3.02 sec
129/137 Test #137: db_tests ............................. Passed 2.91 sec
130/137 Test #136: walletload_tests ..................... Passed 5.63 sec
131/137 Test #133: wallet_tests ......................... Passed 14.11 sec

...

💬 maflcko commented on pull request "util: refactor: add and use run-time safe tinyformat::try_format":
(https://github.com/bitcoin/bitcoin/pull/30928#discussion_r1775314729)
> I think c) is the preferable approach. It adds more core-specific LoC to tinyformat but I think it is the cleanest overall?

Yeah, probably. Maybe a large header `// === Everything below was added for Bitcoin Core ===` in the end and then appending the Bitcoin Core stuff would be cleaner than inlining every function into the existing header with a comment by itself.

💬 brunoerg commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1775342441)
Shouldn't we decrease the number of `inbounds_fanout_tx_relay`\`outbounds_fanout_tx_relay` by the number of peers we're going to fanout with ancestors?