💬 maflcko commented on pull request "fuzz: a new target for the coins database":
(https://github.com/bitcoin/bitcoin/pull/28216#discussion_r1961110876)
Wouldn't it be shorter and clearer to write `assert(!!coins_view_cursor == is_db);`?

💬 maflcko commented on pull request "refactor: Remove redundant and confusing calls to IsArgSet":
(https://github.com/bitcoin/bitcoin/pull/31896#discussion_r1961129692)
Not sure. It is not immediately actionable and probably opinionated or unclear, so best to remove. Though, I don't want to do it here. Maybe a follow-up pull could do it?

💬 Sjors commented on pull request "mining: drop unused -nFees and sigops from CBlockTemplate":
(https://github.com/bitcoin/bitcoin/pull/31897#discussion_r1961163057)
I forgot to drop this line in my previous push.

💬 maflcko commented on pull request "scripted-diff: Type-safe settings retrieval":
(https://github.com/bitcoin/bitcoin/pull/31260#discussion_r1961169935)
> Note this change would also be a change in behavior because `-addresstype=` could no longer be used to unset previous value

Why would that be? I haven't tested this, but from reading the code and the test, I get the impression that setting a value to an empty string will set the value to an empty string and not the default value.

Though, maybe I am confused with negation, which, according to the docs also claims to set a string to an empty string, even though I had the impression that it
...

💬 Sjors commented on pull request "mining: drop unused -nFees and sigops from CBlockTemplate":
(https://github.com/bitcoin/bitcoin/pull/31897#issuecomment-2667869182)
`block.vtx` contains a dummy coinbase, `vTxFees` and `vTxSigOpsCost` contain dummy fees and dummy sigop costs for the coinbase. That seems more straight forward to me than having `vTxFees` be shorter and then having to document why these things have different lengths.

> Since the coinbase transaction does not have fees

That's almost a philosophical question, because miners can (and sometimes accidentally did) claim less fees than allowed. In a way such a coinbase has negative fees, which i
...

💬 Sjors commented on pull request "Have createNewBlock() wait for tip, make rpc handle shutdown during long poll and wait methods":
(https://github.com/bitcoin/bitcoin/pull/31785#issuecomment-2667879598)
> I think a dedicated PR like https://github.com/bitcoin/bitcoin/pull/30635 would be a better place to update the RPC interface, if that is what we want to do.

I'll rebase the latter PR on top of this, and then reduce the scope here.

💬 davidgumberg commented on pull request "build: Make config warnings fatal if -DWCONFIGURE_ERROR=ON":
(https://github.com/bitcoin/bitcoin/pull/31665#issuecomment-2667879734)
> Clients can then control via command line flags like [`-Werror=<what>`](https://cmake.org/cmake/help/latest/manual/cmake.1.html#cmdoption-cmake-Werror) what warnings they want to see hand whether they should be interpreted as error.

Thanks for the suggestion, it seems like cmake's `-Werror=<what>` does not support making `WARNING` messages errors, only `AUTHOR_WARNING` with `-Werror=dev` and `DEPRECATION` with `-Werror=deprecated`.

For example:

```cmake
cmake_minimum_required(VERSION
...

📝 Sjors converted_to_draft a pull request: "rpc: add optional blockhash to waitfornewblock, unhide in help"
(https://github.com/bitcoin/bitcoin/pull/30635)
The `waitfornewblock` is inherently racy as the tip may have changed since the last RPC call, and can even change during initial processing of this call.

Add an optional `blockhash` argument so the caller can specify their current tip. Return immediately if our tip is different.

I've made it fail if `LookupBlockIndex` fails. This should never happen if the user got the block hash from our RPC in the first place.

Finally, `waitfornewblock` is no longer hidden in `help`.

💬 Sjors commented on pull request "rpc: add optional blockhash to waitfornewblock, unhide in help":
(https://github.com/bitcoin/bitcoin/pull/30635#issuecomment-2667880812)
#31785 increasingly overlaps with the scope of this PR, so I'm going to rebase this on it.

💬 Sjors commented on pull request "Have createNewBlock() wait for tip, make rpc handle shutdown during long poll and wait methods":
(https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1961207980)
Good point about the comment, I took your suggestion.

👍 vasild approved a pull request: "Have createNewBlock() wait for tip, make rpc handle shutdown during long poll and wait methods"
(https://github.com/bitcoin/bitcoin/pull/31785#pullrequestreview-2626051280)
ACK 6806651d2f64d51094178cbfb23ef2f4a956aa4d

Would be happy to re-review if suggestions from @ryanofsky are applied, especially https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1960150037

Note wrt RPC changing of the behavior when waiting at startup for the tip to be set -- the current code first sets the tip and then starts the RPC, so this is mostly theoretical or about future changes that might swap the startup order. Thus, IMO, no need for release notes.

💬 maflcko commented on pull request "random: Check `GetRNDRRS` is supported in `InitHardwareRand` to avoid infinite loop":
(https://github.com/bitcoin/bitcoin/pull/31826#discussion_r1961315638)
I don't think it is possible in C++ to use a function before the function signature has been given to the compiler.

I fail to see how this compiles on any compiler. See https://godbolt.org/z/cYPda9784

Locally I get:

```
[ 40%] Building CXX object src/util/CMakeFiles/bitcoin_util.dir/__/random.cpp.o
/__w/b-c-nightly/b-c-nightly/src/random.cpp:201:30: error: use of undeclared identifier 'VerifyRNDRRS'
201 | g_rndrrs_supported = VerifyRNDRRS();
|
...

📝 eval-exec opened a pull request: "random: move VerifyRNDRRS above InitHardwareRand, fix https://github.com/bitcoin/bitcoin/pull/31826#discussion_r1961315638"
(https://github.com/bitcoin/bitcoin/pull/31902)
Fix: https://github.com/bitcoin/bitcoin/pull/31826#discussion_r1961315638

💬 maflcko commented on pull request "scripted-diff: Type-safe settings retrieval":
(https://github.com/bitcoin/bitcoin/pull/31260#discussion_r1961337833)
To answer my own question, the change in behavior likely refers to the removal of the empty() check in the diff, not the use of the `DefaultFn`. My understanding is that just using `DefaultFn` would be a refactor.

💬 eval-exec commented on pull request "random: Check `GetRNDRRS` is supported in `InitHardwareRand` to avoid infinite loop":
(https://github.com/bitcoin/bitcoin/pull/31826#discussion_r1961338142)
Thanks for pointing it out. I've submitted a fix https://github.com/bitcoin/bitcoin/pull/31902

💬 maflcko commented on pull request "correct wrong assumptions in the contrib linearize data script":
(https://github.com/bitcoin/bitcoin/pull/31888#issuecomment-2668134487)
It would be good to include a test, or modify the existing one. Otherwise, it will be harder to test and see what behavior actually changed and future changes could break the fix again.

💬 dergoegge commented on pull request "contrib: Add deterministic-unittest-coverage":
(https://github.com/bitcoin/bitcoin/pull/31901#issuecomment-2668163938)
Concept ACK

💬 hebasto commented on issue "cmake: (release) version handling is broken":
(https://github.com/bitcoin/bitcoin/issues/31898#issuecomment-2668199988)
> Since guix builds from this tarball, how is it that the guix build results in binaries the output the correct version, prior to cmake?

The correct version comes from `CLIENT_VERSION_STRING`, which is set in:https://github.com/bitcoin/bitcoin/blob/785649f3977517a4ba45c5d2fedfbda778fb52cb/CMakeLists.txt#L50

> It looks like it's only defined when building inside of a git tree. This is the same behavior as with autotools. My understanding is that the version numbers are also put in the build sys
...

💬 dergoegge commented on pull request "fuzz: Use serial task runner to increase fuzz stability":
(https://github.com/bitcoin/bitcoin/pull/31841#issuecomment-2668230264)
Concept ACK

💬 maflcko commented on pull request "fuzz: Use serial task runner to increase fuzz stability":
(https://github.com/bitcoin/bitcoin/pull/31841#issuecomment-2668236894)
(rebased without changes to make testing easier)