💬 hebasto commented on pull request "doc: correct the zmq automatic build info":
(https://github.com/bitcoin/bitcoin/pull/30946#discussion_r1775024220)
pedantic nit: The technically correct term for the first `cmake` invocation is ["generating a project buildsystem"](https://cmake.org/cmake/help/latest/manual/cmake.1.html#generate-a-project-buildsystem). The actual "building" is performed by the generated build system during the following step. Elsewhere in our documentation, we use phrases such as "configuring Bitcoin Core" or "configuring the build system".

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