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

👍 ryanofsky approved a pull request: "refactor: Replace g_genesis_wait_cv with m_tip_block_cv"
(https://github.com/bitcoin/bitcoin/pull/30967#pullrequestreview-2328408540)
Code review ACK fa61644768c3f6932860d1c5e2c6c0a38d5257e5.

Overall very nice changes, but I think the first commit fa5ac85d19a387c2e12fc0833368e1240a3dcf51 has a drawback because it adds a `NodeContext` argument to `RPCStop` when ideally RPC code should be agnostic to node initialization and not depend on node types. Would suggest following change to clean it up, which could be squashed into the first commit:

- ef271bc75e7bea8c82ce8667fb22e9b25dca5712 refactor: Split up NodeContext shutdown
...

💬 ryanofsky commented on pull request "refactor: Replace g_genesis_wait_cv with m_tip_block_cv":
(https://github.com/bitcoin/bitcoin/pull/30967#discussion_r1775316336)
In commit "refactor: Replace g_genesis_wait_cv with m_tip_block_cv" (faaf888eb275cecc2d2803c5756afac633e0e2a4)

This code is waiting for `m_tip_block` to be non-null, but then code below will be assuming that `chainman.ActiveTip()` will be non-null. These are similar conditions but not exactly the same, so I think it would be good to add an assert below the if statement like `assert(WITH_LOCK(chainman.GetMutex(), return chainman.ActiveChain().Tip() != nullptr));` to make sure this code is func
...

💬 ryanofsky commented on pull request "refactor: Replace g_genesis_wait_cv with m_tip_block_cv":
(https://github.com/bitcoin/bitcoin/pull/30967#discussion_r1775328397)
re: https://github.com/bitcoin/bitcoin/pull/30967#discussion_r1775211951

I'm not sure about the steady clock thing, but I think the reason this code is written to use a while loop is because it wants to wake up every 1000ms to check m_interrupt in case `m_interrupt` was signaled without `m_tip_block_cv` being signalled. Since I posted another change above to ensure that signalling `m_interrupt` will reliably signal `m_tip_block_cv`, I think this code could be changed to use a simple wait pred
...

👍 TheCharlatan approved a pull request: "refactor: Replace g_genesis_wait_cv with m_tip_block_cv"
(https://github.com/bitcoin/bitcoin/pull/30967#pullrequestreview-2328454931)
ACK fa61644768c3f6932860d1c5e2c6c0a38d5257e5

💬 TheCharlatan commented on pull request "refactor: Replace g_genesis_wait_cv with m_tip_block_cv":
(https://github.com/bitcoin/bitcoin/pull/30967#discussion_r1775344259)
Nit: Could forward-declare `NodeContext` here instead.

💬 maflcko commented on issue "win64-cross CI timeout after 2h":
(https://github.com/bitcoin/bitcoin/issues/30969#issuecomment-2374284122)
This is odd, because normally it should pass in less than 15 minutes:https://cirrus-ci.com/task/4920689327603712

Looks like `allocator_tests` never finished.