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

👍 vasild approved a pull request: "multiprocess: Add libmultiprocess git subtree"
(https://github.com/bitcoin/bitcoin/pull/31741#pullrequestreview-2626370787)
ACK 66c318abe1b7df3e2ff1035376bc5f4b2059626a

💬 vasild commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#discussion_r1961422233)
style: Our CMake files use 2-space for indentation and no space after `if` like this: `if(FOO`. When we were switching to CMake I assumed this is ok because it was all consistent. Now I realize that people will keep adding changes that use 4-space indentation and a space after `if` because the rest of the code base uses that.

Maybe just ignore this and accept that our CMake files will end up with an inconsistent mixture of different styles?

🤔 stickies-v reviewed a pull request: "Fix delimeter in `package-msg` field of `submitpackage` RPC"
(https://github.com/bitcoin/bitcoin/pull/31900#pullrequestreview-2626465092)
The developer notes [state](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#rpc-interface-guidelines):

> Argument and field naming: please consider whether there is already a naming style or spelling convention in the API for the type of object in question (blockhash, for example), and if so, try to use that. If not, use snake case fee_delta (and not, e.g. feedelta or camel case feeDelta).
>
> Rationale: Consistency with the existing interface.

So it seems like if
...

💬 zaidmstrr commented on pull request "rpc: combinerawtransaction now rejects unmergeable transactions":
(https://github.com/bitcoin/bitcoin/pull/31298#discussion_r1961477434)
```suggestion
for (unsigned int idx{0}; idx < txs.size(); idx++) {
```
nit: don't need to add the extra space

💬 zaidmstrr commented on pull request "rpc: combinerawtransaction now rejects unmergeable transactions":
(https://github.com/bitcoin/bitcoin/pull/31298#discussion_r1961489882)
```suggestion
for (unsigned int i{0}; i < mergedTx.vin.size(); ++i) {
```
nit: same for here `++i` should be prioritised.

💬 zaidmstrr commented on pull request "rpc: combinerawtransaction now rejects unmergeable transactions":
(https://github.com/bitcoin/bitcoin/pull/31298#discussion_r1961478912)
```suggestion
for (unsigned int k{0}; k < txVariantsCopy.size(); ++k) {
```
nit: `++k` should be given more prefrence then `k++ `

💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-2668392827)
Rebased a604321c3e4bd50b52fa28e8567f6b068b2d2fb3 -> 251a55f2f0cc3cdfb7fa0015b76772586134cde3 ([kernelApi_24](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_24) -> [kernelApi_25](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_25), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelApi_24..kernelApi_25))

💬 maflcko commented on pull request "test, refactor: Add TestNode.binaries to hold binary paths":
(https://github.com/bitcoin/bitcoin/pull/31866#discussion_r1961532157)
> > It would be better to fully move all touched code of this pull into `TestNode` and only allow getting binaries through the `TestNode` interface.
>
> This doesn't make much sense to me. I wouldn't want tests to be using `self.nodes[0].binaries` if they aren't actually interacting with node 0, and when a node 0 object might not even exist. If tests just need to call some binaries, they should be able to use a standalone binaries object without creating a fake node or going through an unrela
...