💬 ryanofsky commented on pull request "init: fix init fatal error on invalid negated option value":
(https://github.com/bitcoin/bitcoin/pull/30684#issuecomment-2299869398)
Nice catch! I think the changes in 744d7242239b8c09c2626e5ef7f90c2a91eed0fa might have some benefits, but might not be the best solution to this problem for a few reasons:

- They are not a complete fix. While they prevent an uncaught exception when you pass `-nowallet=not_a_bool` they do not prevent a similar uncaught exception when you pass `-nowallet=0`
- The new error message "Invalid '-no%s=%s': Value must be a boolean (0/1 or false/true)" seems to suggest that false/true values would be
...

💬 ryanofsky commented on pull request "wallet: Write best block to disk before backup":
(https://github.com/bitcoin/bitcoin/pull/30678#discussion_r1724059212)
In commit "wallet: Write best block to disk before backup" (888a167446901c12960e6776613a3a1fb9789f59)

I wonder if it makes sense to put this in the CWallet::Flush method and call Flush() here. For example, this also seems like a good thing to do when unloading a wallet.

📝 theStack opened a pull request: "test: replace deprecated secp256k1 context flags usage"
(https://github.com/bitcoin/bitcoin/pull/30687)
The flags `SECP256K1_CONTEXT_{SIGN,VERIFY}` have been marked as deprecated since libsecp256k1 version 0.2 (released in December 2022), with the recommendation to use SECP256K1_CONTEXT_NONE instead, see https://github.com/bitcoin-core/secp256k1/pull/1126. Note that in contrast to other deprecated functions/variables, these defines don't have a deprecated attribute and hence don't lead to a compiler warning (see https://github.com/bitcoin-core/secp256k1/pull/1126#discussion_r922105271), so they ar
...

💬 tdb3 commented on pull request "test: check xor.dat creation when missing":
(https://github.com/bitcoin/bitcoin/pull/30669#discussion_r1724133162)
lgtm, added.

💬 tdb3 commented on pull request "test: check xor.dat creation when missing":
(https://github.com/bitcoin/bitcoin/pull/30669#issuecomment-2299992071)
> If you want, you could also tackle the suggestion to move the `read_xor_key` helper to the `TestNode` class, see [#30657 (comment)](https://github.com/bitcoin/bitcoin/pull/30657#discussion_r1717985952)

Thanks for the suggestion! This PR seems to be a reasonable place to perform the move, so I updated it to include this. Happy to make tweaks if you or @maflcko have any further suggestions.

🚀 achow101 merged a pull request: "fuzz: Faster utxo_snapshot fuzz target"
(https://github.com/bitcoin/bitcoin/pull/30644)

💬 tdb3 commented on pull request "fix: handle invalid `-rpcbind` port earlier":
(https://github.com/bitcoin/bitcoin/pull/30679#issuecomment-2300398744)
> I think it would be good to keep the new `return false` and error log in `HTTPBindAddresses`. But a more direct fix for this issue might be to just move the code that checks port numbers on line 1304 above the code that starts the http server on line 1219 in `AppInitMain`:

Thank you for taking a lot. Agreed, rearranging in `AppInitMain` seems to be the better approach. Updated. Also rearranged the commits so that the tests are introduced in the first commit and adjusted with the introduc
...

💬 maflcko commented on pull request "doc: Extend developer-notes with file-name-only debugging fix":
(https://github.com/bitcoin/bitcoin/pull/30670#discussion_r1724457568)
What you link to is about the *build* folder, not the *source* folder. IIUC suggesting to use the build folder `build` is due to the `.gitignores` taking effect. However, users are otherwise free to use any other build folder.

The same does not apply to the source folder, so making them "consistent" doesn't apply here.

Also, even a small LLM like 4o-mini "understands" the previous doc correctly, so I am not sure what can be hard to understand.

I don't see why this change is worth it.

...

💬 GregTonoski commented on issue "bitcoind shouldn't be shutdown automatically despite wallet synchronisation error":
(https://github.com/bitcoin/bitcoin/issues/30671#issuecomment-2301226272)
>
> Wallets contain users' money. We should not silently unload any of them without user interaction. Their importance is greater than completing a node startup. Imagine how would you behave if you start your wallet's node and your coins are not there.

As a user, I want to start bitcoind irrespectively of coins being in a wallet. In this case, I don't even enable RPC which makes it impossible for me to check wallet's balance.

💬 maflcko commented on issue "bitcoind shouldn't be shutdown automatically despite wallet synchronisation error":
(https://github.com/bitcoin/bitcoin/issues/30671#issuecomment-2301244806)
> I want to start bitcoind

As mentioned previously, it is possible to do so in any previous release of Bitcoin Core, and no one is holding you back from doing so.

I agree that the error message can be improved to add more context or suggestions, but reworking the startup (and post-startup) behavior in a breaking change for all users with questionable trade-offs is unlikely to happen.

🤔 maflcko reviewed a pull request: "CMake replaces Autotools"
(https://github.com/bitcoin/bitcoin/pull/30664#pullrequestreview-2249947365)
left a nit

💬 maflcko commented on pull request "CMake replaces Autotools":
(https://github.com/bitcoin/bitcoin/pull/30664#discussion_r1724536921)
style nit: If you are removing the only invocation of this script, wouldn't it be better to remove the other mentions in the docs as well?

```
cmake/script/GenerateBuildInfo.cmake:# This script is a multiplatform port of the share/genbuild.sh shell script.

src/clientversion.cpp:// The <obj/build.h>, which is generated by the build environment (share/genbuild.sh),

💬 maflcko commented on pull request "CMake replaces Autotools":
(https://github.com/bitcoin/bitcoin/pull/30664#issuecomment-2301321844)
> It aims reveal conflicts with other PRs.
>
> For more details, please refer to today's IRC meeting discussion.

It would be good to mention any important key points, otherwise it will be hard to understand whether this is the pull request that will be merged as a follow-up, and whether it should be reviewed, or if there is another place where the review should happen.

👋 maflcko's pull request is ready for review: "Bump python minimum supported version to 3.10"
(https://github.com/bitcoin/bitcoin/pull/30527)

💬 maflcko commented on pull request "Bump python minimum supported version to 3.10":
(https://github.com/bitcoin/bitcoin/pull/30527#issuecomment-2301329179)
Unlocked for review for 29.x

💬 maflcko commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#issuecomment-2301457862)
An O2 godbolt would be https://godbolt.org/z/Kqxe6353P

> +0.04% seems okay if you ask me.

Are you sure? I checked the DrahtBot guix build and everything I checked was smaller, but maybe the build is malicious or I made a mistake.

💬 fanquake commented on pull request "depends: Fix CMake-generated `libzmq.pc` file":
(https://github.com/bitcoin/bitcoin/pull/30508#issuecomment-2301464512)
> I'd rather wait until upstream weighs in though, since we don't need this at least until we switch to CMake.

Looks like upstream either forgot about, or missed this, so I guess someone should followup.

💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#issuecomment-2301467409)
This is how I checked the sizes of current tip vs base:
```
$ ls -al guix-build-c3fa29db1b05/distsrc-c3fa29db1b05-x86_64-linux-gnu/src/bitcoind
-rwxr-xr-x 1 hodlinator users 115048128 aug 20 14:10 guix-build-c3fa29db1b05/distsrc-c3fa29db1b05-x86_64-linux-gnu/src/bitcoind

$ ls -al guix-build-5fdbc8b4ee60/distsrc-5fdbc8b4ee60-x86_64-linux-gnu/src/bitcoind
-rwxr-xr-x 1 hodlinator users 114998544 aug 13 11:29 guix-build-5fdbc8b4ee60/distsrc-5fdbc8b4ee60-x86_64-linux-gnu/src/bitcoind
```

💬 maflcko commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1724656502)
Heh, nice. I didn't know that string literal operator templates are possible since C++20.

💬 l0rinc commented on pull request "doc: Extend developer-notes with file-name-only debugging fix":
(https://github.com/bitcoin/bitcoin/pull/30670#discussion_r1724663372)
Valid points, thanks @glozow & @maflcko, dropped that commit.