💬 hodlinator commented on pull request "test: [refactor] Use m_rng directly":
(https://github.com/bitcoin/bitcoin/pull/30571#discussion_r1729526826)
One could argue `ZEROS` is also "fixed", but I still agree `FIXED_SEED` with the documentation would be a step a good direction.

💬 hodlinator commented on pull request "test: [refactor] Use m_rng directly":
(https://github.com/bitcoin/bitcoin/pull/30571#discussion_r1726503732)
(Thread https://github.com/bitcoin/bitcoin/pull/30571#discussion_r1724794173)

Seems that this is not an issue (based on my fallible reading):

`SeedRandomStateForTest` -> `MakeRandDeterministicDANGEROUS` -> `GetRNGState().MakeDeterministic(seed)` -> which initializes the `std::optional` `RNGState::m_deterministic_prng`.

`GetRandHash` -> `GetRandBytes` -> `ProcRand(..., RNGLevel::FAST, /*always_use_real_rng=*/false)` -> which will end up sampling hardware-based entropy via `SeedFast` but
...

💬 hebasto commented on pull request "guix: (explicitly) build Linux GCC with `--enable-cet`":
(https://github.com/bitcoin/bitcoin/pull/30438#issuecomment-2310056894)
> The point of this PR has never been to add any additional metadata to the binaries/make any other changes, the point is just to explictly use the hardening option when configuring our compiler (like we do with other options).

I see. Perhaps mention in the PR description that the resulting release binaries are not affected?

👍 pablomartin4btc approved a pull request: "qt: 28.0 translations update"
(https://github.com/bitcoin/bitcoin/pull/30715#pullrequestreview-2260572607)
ACK a0cdf43c4dde4f8d1983311fee2393fcde9123fe

No diff running `make -C src translate` on this PR.

👍 brunoerg approved a pull request: "test: Avoid intermittent block download timeout in p2p_ibd_stalling"
(https://github.com/bitcoin/bitcoin/pull/30705#pullrequestreview-2260594620)
utACK fa5b58ea01fac1adb6336b8b6b5217193295c695

🤔 furszy reviewed a pull request: "wallet: Write best block to disk before backup"
(https://github.com/bitcoin/bitcoin/pull/30678#pullrequestreview-2260723558)
I'm not seeing any drawback from doing it prior to a backup so far.

But the `Flush()` discussion (https://github.com/bitcoin/bitcoin/pull/30678#discussion_r1724059212) made me think about the migration process. We aren't updating the best locator before unloading the wallet. So, might also want to add https://github.com/furszy/bitcoin-core/commit/5b618f1b024f16640a2a05f1e269f61b33b86577.

👍 brunoerg approved a pull request: "test: XORed blocks test follow up"
(https://github.com/bitcoin/bitcoin/pull/30669#pullrequestreview-2260728801)
reACK e1d5dd732d5dc641faf1dde316275c84b6bb224b

🤔 MarnixCroes reviewed a pull request: "build: Introduce CMake-based build system"
(https://github.com/bitcoin/bitcoin/pull/30454#pullrequestreview-2260753659)
41051290ab3b6c36312cec26a27f787cea9961b4 succesfully built on debian12 and fedora40

- the build requirements for Fedora at `doc/build.unix.md` are incorrect/not updated
- disable-wallet mode is not working without sqlite dependency installed? Using `cmake -B build -DENABLE_WALLET=OFF`, as per the instructions, will not work without sqlite installed. (error that sqlite is missing)

💬 hebasto commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2310296649)
> * the build requirements for Fedora at `doc/build.unix.md` are incorrect/not updated

Thanks! Will update them in a follow-up along with other doc amendments.

💬 hebasto commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2310321280)
> [4105129](https://github.com/bitcoin/bitcoin/commit/41051290ab3b6c36312cec26a27f787cea9961b4) succesfully built on debian12 and fedora40
>
> * disable-wallet mode is not working without sqlite dependency installed? Using `cmake -B build -DENABLE_WALLET=OFF`, as per the instructions, will not work without sqlite installed. (error that sqlite is missing)

I cannot reproduce the issue on Fedora 40:
```
$ dnf list installed sqlite-devel
Error: No matching Packages to list
$ cmake -B b
...

👍 ryanofsky approved a pull request: "refactor: Replace ParseHex with consteval ""_hex literals"
(https://github.com/bitcoin/bitcoin/pull/30377#pullrequestreview-2260843182)
Code review ACK 92e599d57c507f35d889c3a804d2a7020dcc6b1d, just small suggested tweaks since last review (comments, constexpr, MakeByteSpan)

👍 ryanofsky approved a pull request: "assumeutxo: Add dumptxoutset height param, remove shell scripts"
(https://github.com/bitcoin/bitcoin/pull/29553#pullrequestreview-2260860543)
Code review ACK ac9d53d0dee26db58ab125aa369f476c232da660. Nice documentation improvements since last review, and a new log statement

👍 ryanofsky approved a pull request: "Have createNewBlock() return a BlockTemplate interface"
(https://github.com/bitcoin/bitcoin/pull/30440#pullrequestreview-2260895880)
Code review ACK 33f5ab32b2651a18c9d56bef1e5b3c69dd26e199. Only changes since last review were include changes and reformatting changes.

I have to say I am not a fan of running clang-format-diff in same commits that introduce real code changes and making unrelated changes to surrounding lines of code. It particularly adds a lot of noise to this PR in the ThresholdState switch statement and makes it harder to review. IMO it is preferable to use clang-format-diff only to format code that is actu
...

💬 MarnixCroes commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2310426838)
> > [4105129](https://github.com/bitcoin/bitcoin/commit/41051290ab3b6c36312cec26a27f787cea9961b4) succesfully built on debian12 and fedora40
> >
> > * disable-wallet mode is not working without sqlite dependency installed? Using `cmake -B build -DENABLE_WALLET=OFF`, as per the instructions, will not work without sqlite installed. (error that sqlite is missing)
>
> I cannot reproduce the issue on Fedora 40:
>
> ```
> $ dnf list installed sqlite-devel
> Error: No matching Packages to l
...

💬 Sjors commented on pull request "Have createNewBlock() return a BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/30440#issuecomment-2310483256)
@ryanofsky I used the incantation suggested here: https://github.com/bitcoin/bitcoin/blob/master/contrib/devtools/README.md#clang-format-diffpy

I assumed this would only edit lines impacted by the commit. But I see it touched a few other places in `rpc/mining.cpp`. I removed unrelated changes from the commit.

🤔 ryanofsky reviewed a pull request: "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange and drop CRPCSignals"
(https://github.com/bitcoin/bitcoin/pull/30409#pullrequestreview-2260994997)
Code review 93176016322687f3ad9ade8f32c1ff1392ee6470, but I'm concerned code is failing to signal m_tip_block_cv on shutdown so it could make shutdown slower. Suggested a fix below. It also seems like it would be easy to eliminate g_best_block completely, so suggested a way to do that too.

💬 ryanofsky commented on pull request "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange and drop CRPCSignals":
(https://github.com/bitcoin/bitcoin/pull/30409#discussion_r1731440200)
If we are going to add a new `m_tip_block` variable to replace `g_best_block` seems like it would be good to make replacement complete and eliminate `g_best_block`. I think following could work (compiles but untested):

```diff
--- a/src/rpc/mining.cpp
+++ b/src/rpc/mining.cpp
@@ -739,7 +739,6 @@ static RPCHelpMan getblocktemplate()
{
// Wait to respond until either the best block changes, OR a minute has passed and there are more transactions
uint256 hashWatchedCh
...

💬 ryanofsky commented on pull request "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange and drop CRPCSignals":
(https://github.com/bitcoin/bitcoin/pull/30409#discussion_r1731432419)
In commit "Add waitTipChanged to Mining interface" (078fc60f198eb7a3bcccb557416fe1d4498f26e7)

One problem with switching from g_best_block_cv to m_tip_block_cv is that this m_tip_block_cv is not signaled when StopRPC is called, which could mean shutdown hangs for a second instead of being instantaneous. I believe this could be fixed in earlier commit 8400d49ffec47add2dc27267ae13ede5d2fabeea with:

```diff
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -282,7 +282,7 @@ void Shutdown(NodeContext
...

📝 maflcko opened a pull request: "bench: [refactor] iwyu"
(https://github.com/bitcoin/bitcoin/pull/30716)
Missing includes are problematic, because:

* Upcoming releases of the C++ standard library often minimize their internal header dependencies. For example, `_LIBCPP_REMOVE_TRANSITIVE_INCLUDES` (https://libcxx.llvm.org/DesignDocs/HeaderRemovalPolicy.html). This can lead to compile failures, which are easy to fix for developers, but may not for users. For example, https://github.com/bitcoin/bitcoin/pull/30612/commits/39d1ca4bc9d29f683108c800bdfe59d2dc90f3c7 had to add missing includes to accommo
...

👍 ryanofsky approved a pull request: "test: [refactor] Use m_rng directly"
(https://github.com/bitcoin/bitcoin/pull/30571#pullrequestreview-2261031461)
Code review ACK 948238a683b6c99f4e91114aa75680c6c2d73714. Only changes since last review were changing a comments a little bit.