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

💬 hebasto commented on pull request "bench: [refactor] iwyu":
(https://github.com/bitcoin/bitcoin/pull/30716#issuecomment-2310537762)
Concept ACK.

Maybe check `src/bench` in the CI separately and treat warnings as errors?

💬 maflcko commented on pull request "bench: [refactor] iwyu":
(https://github.com/bitcoin/bitcoin/pull/30716#issuecomment-2310541118)
> Maybe check `src/bench` in the CI separately and treat warnings as errors?

Yes, this will be a follow-up, because it requires other changes as well.

👍 ryanofsky approved a pull request: "Have createNewBlock() return a BlockTemplate interface"
(https://github.com/bitcoin/bitcoin/pull/30440#pullrequestreview-2261045238)
Code review ACK 23374a22f442939802c0296d6447e0db02d513da. Just reined in clang-tidy since last review so there are fewer changes

💬 stratospher commented on pull request "cli: restrict multiple exclusive argument usage in bitcoin-cli":
(https://github.com/bitcoin/bitcoin/pull/30148#issuecomment-2310604676)
reACK c8e6771.

💬 Sjors commented on pull request "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange and drop CRPCSignals":
(https://github.com/bitcoin/bitcoin/pull/30409#discussion_r1731519643)
Nice! I split the changes to `rpc/mining.cpp` into its own commit, that looks correct.

Running tests before pushing...

🤔 ismaelsadeeq reviewed a pull request: "bench: [refactor] iwyu"
(https://github.com/bitcoin/bitcoin/pull/30716#pullrequestreview-2261138334)
Concept ACK

💬 ismaelsadeeq commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#issuecomment-2310653344)
post merge ACK 647fa37cdbadbeebba147ca6b24e138559cffaaf

🤔 ismaelsadeeq reviewed a pull request: "cluster mempool: optimized candidate search"
(https://github.com/bitcoin/bitcoin/pull/30286#pullrequestreview-2260608958)
Concept ACK