💬 Sjors commented on pull request "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange, drop CRPCSignals & g_best_block":
(https://github.com/bitcoin/bitcoin/pull/30409#issuecomment-2325982902)
Rebased after #8230.

💬 fanquake commented on pull request "rpc, rest: Improve block rpc error handling, check header before attempting to read block data.":
(https://github.com/bitcoin/bitcoin/pull/30410#issuecomment-2326003705)
@andrewtoth the CI failure in the test-each-commit job is unrelated to the changes (missing zmq package, fixed in #30749).

👋 fanquake's pull request is ready for review: "guix: (explicitly) build Linux GCC with `--enable-cet`"
(https://github.com/bitcoin/bitcoin/pull/30438)

🤔 TheCharlatan reviewed a pull request: "interpreter: use int32_t instead of int type for risczero compile"
(https://github.com/bitcoin/bitcoin/pull/30794#pullrequestreview-2276872397)
Concept ACK

This can be tested by cloning https://github.com/riscv-collab/riscv-gnu-toolchain, then:
```
./configure --prefix=/opt/riscv-ilp32 --with-arch=rv32gc --with-abi=ilp32
make
```
And then from the root directory:
```
/opt/riscv-ilp32/bin/riscv32-unknown-elf-g++ -std=c++20 -march=rv32i -mabi=ilp32 -I /path/to/bitcoin/src -c src/script/interpreter.cpp -o script.o
```

Without this patch:
```
src/script/interpreter.cpp:1306:24: error: call of overloaded 'Serialize(HashWriter
...

💬 TheCharlatan commented on pull request "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange, drop CRPCSignals & g_best_block":
(https://github.com/bitcoin/bitcoin/pull/30409#issuecomment-2326021511)
> Rebased after https://github.com/bitcoin/bitcoin/pull/8230.

Huh, that was merged 8 years ago? :smile:

👋 fanquake's pull request is ready for review: "depends: build libevent with `-D_GNU_SOURCE`"
(https://github.com/bitcoin/bitcoin/pull/30743)

💬 fanquake commented on pull request "depends: build libevent with `-D_GNU_SOURCE`":
(https://github.com/bitcoin/bitcoin/pull/30743#issuecomment-2326027285)
For now I've opted with matching the prior Autotools behaviour, and always building with `-D_GNU_SOURCE`, which is also easy to backport to `28.x`.

💬 mzumsande commented on pull request "rpc, rest: Improve block rpc error handling, check header before attempting to read block data.":
(https://github.com/bitcoin/bitcoin/pull/30410#issuecomment-2326029369)
> @andrewtoth the CI failure in the test-each-commit job is unrelated to the changes (missing zmq package, fixed in #30749).

Thanks, I also wasn't sure! I've just rebased again.

💬 maflcko commented on pull request "build: Fix linking for `fuzz` target when building with MSan":
(https://github.com/bitcoin/bitcoin/pull/30778#issuecomment-2326034719)
review-only ACK 787dfaf084a3952319778da9cbcda9d7d619e4ee

Only change since last review is linking with libbitcoin_util.

It would be nice to better understand this change, but this can be done later and is not a blocker to for a test-only change.

Also, the depends behavior can be documented better, or be changed, but this is probably best done in a separate issue or pull request, so that this test-only change stays focussed.

👍 fanquake approved a pull request: "doc: update documentation and scripts related to build directories"
(https://github.com/bitcoin/bitcoin/pull/30741#pullrequestreview-2276903597)
ACK 6a68343ffbf3291eb243d90c00df50e672ff3944 - we still need to followup with other scripts/devtools, and likely unify what we are doing in some way, but this is an improvement.

🚀 fanquake merged a pull request: "doc: update documentation and scripts related to build directories"
(https://github.com/bitcoin/bitcoin/pull/30741)

🤔 l0rinc reviewed a pull request: "validation: write chainstate to disk every hour"
(https://github.com/bitcoin/bitcoin/pull/30611#pullrequestreview-2276771072)
Thanks for quick responses, my main concern at this stage is that the commit messages don't explain the changes in enough detail and some symbols might need a rename after the change

💬 l0rinc commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#discussion_r1741658439)
we don't have `m_last_write` anymore

💬 l0rinc commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#discussion_r1741663448)
nit: 1 hour is a *duration* or *interval* or *period*, but technically not a *time*:
```suggestion
// The periodic flush interval is 1 hour
```

💬 l0rinc commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#discussion_r1741667937)
could you please explain the `flush` removal in the corresponding commit message as well?

💬 l0rinc commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#discussion_r1741697957)
Is it even possible for this to be false, given that we skip flush on first run?

nit: just an observation, the starting `!` hides well here, if you think it read better, here's an alternative (though I see that IsNull is used in a few other places):
```suggestion
if (CoinsTip().GetBestBlock() != uint256::ZERO) {
```

💬 l0rinc commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#discussion_r1741685428)
is the name still accurate, now that a periodic write can also trigger it (i.e. not just flushes, like before)?

💬 l0rinc commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#discussion_r1741737264)
I think the convention is:
```suggestion
// Copyright (c) 2024-present The Bitcoin Core developers
```

💬 l0rinc commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#discussion_r1741725078)
I know this line didn't change here, I'm looking for answers:
I find it quite confusing that most of the side-effects of this method are actually done inside if conditions - I wanted to understand whether the name `fDoFullFlush` is accurate, but had a hard time understanding which methods are just checking state (e.g. `CheckDiskSpace`, I assume) and which ones are changing state (e.g. `empty_cache ? !CoinsTip().Flush() : !CoinsTip().Sync()`).
So my question is whether it's fair to call the con
...

💬 l0rinc commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#discussion_r1741741467)
we do more than `add randomness to periodic write interval` in this commit now, could you please split it up a bit more?