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

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