👍 hebasto approved a pull request: "doc: correct the zmq automatic build info"
(https://github.com/bitcoin/bitcoin/pull/30946#pullrequestreview-2327933232)
re-ACK 06e7e83632985bd8b648d24f9a51591d3a3bbec3.

💬 tdb3 commented on pull request "doc: correct the zmq automatic build info":
(https://github.com/bitcoin/bitcoin/pull/30946#discussion_r1775030740)
Yeah, you're right. I was on the fence about naming (since the actual build is done with `cmake --build build -jN`. Better to fix it now, so I pushed a change.

💬 l0rinc commented on pull request "doc: correct the zmq automatic build info":
(https://github.com/bitcoin/bitcoin/pull/30946#issuecomment-2373794712)
ACK commit/06e7e83632985bd8b648d24f9a51591d3a3bbec3

🚀 fanquake merged a pull request: "doc: correct the zmq automatic build info"
(https://github.com/bitcoin/bitcoin/pull/30946)

💬 hebasto commented on issue "cmake: passing options from depends build to main build":
(https://github.com/bitcoin/bitcoin/issues/30813#issuecomment-2373847587)
Additionally, if we remove settings of non-toolchain variables, such as `BUILD_GUI` or `ENABLE_WALLET` from the toolchain file, it allows the reuse of this toolchain file to build depends themselves, instead of relying on this code:https://github.com/bitcoin/bitcoin/blob/39219fe145e5e6e6f079b591e3f4b5fea8e71804/depends/funcs.mk#L173-L199

💬 ryanofsky commented on pull request "multiprocess: Add IPC wrapper for Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30510#issuecomment-2373862834)
Thanks for the review!

> I didn't check if the new `CustomBuildField` and `CustomReadField` to serialize `std::chrono` is identical to that in #30437. If not, then I'll test it next time I rebase [Sjors#48](https://github.com/Sjors/bitcoin/pull/48).

They should be the same except the builder now has static asserts to make sure the capnp number type has enough range to represent the std::chrono duration type (that capnp Float64 type can represent MillisecondsDouble in this case).

💬 maflcko commented on pull request "test: Add missing sync_mempools() to fill_mempool()":
(https://github.com/bitcoin/bitcoin/pull/30948#discussion_r1775104055)
> by the way this was Tested ACK

Reviews in resolved conversations are unlikely to be seen. My recommendation would be to put them in a normal comment that isn't hidden.

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