📝 maflcko opened a pull request: "refactor: Remove redundant and confusing calls to IsArgSet"
(https://github.com/bitcoin/bitcoin/pull/31896)
`IsArgSet` is problematic:

* It returns whether an arg has been set, even if it has been negated. `IsArgSet` is sometimes used to check for a truthy value, which is wrong, but usually harmless. Cleanup of those cases may or may not be done in a follow-up.
* In most other cases, calling it is redundant, because the immediately following `Get*Arg` calls can already return an `std::optional` nullopt value to indicate an unset arg.

So relieve both issues by removing all `IsArgSet` that are re
...

💬 maflcko commented on pull request "scripted-diff: Type-safe settings retrieval":
(https://github.com/bitcoin/bitcoin/pull/31260#discussion_r1959977366)
nit: I find the `!Value(gArgs).isNull()` with the double negation and verbosity over just `Get(gArgs)` a bit ugly and I think it would be better to remove `IsArgSet` as much as possible. In most cases it is not needed anyway, or used incorrectly. About this one, I left a comment here: https://github.com/bitcoin/bitcoin/pull/31887/files#r1959923000

💬 hodlinator commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1959978900)
Agree on double-quotes in more places giving nicer output, but would not rephrase or remove legacy wallets at this point.
```suggestion
{"filename", RPCArg::Type::STR, RPCArg::Optional::NO, "The path to the wallet directory (absolute or relative to the \"wallets\" directory), or a legacy wallet's .dat file (within the \"wallets\" directory). The \"wallets\" directory is set by the -walletdir option and defaults to the \"wallets\" folder within the data directory."},
```
I
...

📝 Sjors opened a pull request: "mining: drop unused -nFees and sigops from CBlockTemplate"
(https://github.com/bitcoin/bitcoin/pull/31897)
For the coinbase `vTxFees` used a dummy value of -nFees.

Similarly the first `vTxSigOpsCost` entry was calculated from
the dummy coinbase transaction.

This was introduced in #2115, but the values were never returned by the RPC or used in a test.

Set both to 0 instead and add code comments to prevent confusion.

💬 jonatack commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1960001093)
I think invalidating ACKs is more a consideration for critical or difficult-to-review changes.

If loading legacy wallets will be disabled in the next release, it may be worth not adding a mention here that would require attention/removal.

🚀 fanquake merged a pull request: "build: remove ENABLE_HARDENING condition from check-security"
(https://github.com/bitcoin/bitcoin/pull/31892)

💬 Sjors commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1960003712)
I opened #31897 to drop this `-nFees` dummy.

💬 jonatack commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1960005999)
> Agree on double-quotes in more places giving nicer output

The suggested diff doesn't only add double quotes but also proposes a rewrite for clarity between wallet directory and wallets directory.

✅ prusnak closed an issue: "rpc: dumptxoutset as sqlite file"
(https://github.com/bitcoin/bitcoin/issues/24628)

💬 prusnak commented on issue "rpc: dumptxoutset as sqlite file":
(https://github.com/bitcoin/bitcoin/issues/24628#issuecomment-2666086329)
> I think this is resolved with the merged of [#27432](https://github.com/bitcoin/bitcoin/pull/27432)?

Yes

💬 Sjors commented on pull request "mining: drop unused -nFees and sigops from CBlockTemplate":
(https://github.com/bitcoin/bitcoin/pull/31897#discussion_r1960007754)
Changed from -1 to 0, because if someone ignores the repeated instruction to not use this value, at least they won't incorrectly sum up `vTxFees`.

💬 rkrux commented on pull request "descriptor: check whitespace in keys within fragments":
(https://github.com/bitcoin/bitcoin/pull/31603#issuecomment-2666108177)
One way to ensure code structure correctness and get rid of the brittleness noticed here (https://github.com/bitcoin/bitcoin/pull/31603#discussion_r1959636120) is to incorporate the following change. In a cursory attempt, I have this that has minimal diff that passes the tests. LMK how does this sound.

```cpp
void CheckUnparsable(const std::string& prv, const std::string& pub, const std::string& expected_error, bool check_prv_error_only = false)
{
FlatSigningProvider keys_priv, keys_pu
...

🤔 hodlinator reviewed a pull request: "refactor: CLI cleanups"
(https://github.com/bitcoin/bitcoin/pull/31887#pullrequestreview-2624027023)
Code reviewed 3252f3bab02605129a568112f650ef3cb29703b7

ACK 9184ec9e8ea6efd4d0d3625a06f223156f940243

Advise dropping 3rd commit, the others seem good.

💬 hodlinator commented on pull request "refactor: CLI cleanups":
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1960004078)
Weird to change inheritance from `class -> class` to `struct -> class`.

💬 hodlinator commented on pull request "refactor: CLI cleanups":
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1960014260)
Makes more sense to use `class`es for these as they use `virtual` functions. Not worth changing just to be able to remove `public` IMO.

⚠️ fanquake opened an issue: "cmake: (release) version handling is broken"
(https://github.com/bitcoin/bitcoin/issues/31898)
Create a test tag: `v99.99`
Checkout `v99.99` and perform a guix build:`./contrib/guix/guix-build`.
Take the dist-archive tarball: `/bitcoin/guix-build-99.99/output/dist-archive/bitcoin-99.99.tar.gz` and perform a build:
```bash
tar xf bitcoin-99.99.tar.gz
cd bitcoin-99.99
cmake -B build
cmake --build build
# ./build/src/bitcoind --version
Bitcoin Core daemon version v28.99.0-g43e287b3ff5f0d223b0911b6ef90054ce5eb69d2
```

👍 darosior approved a pull request: "descriptor: check whitespace in keys within fragments"
(https://github.com/bitcoin/bitcoin/pull/31603#pullrequestreview-2623967849)
ACK 35d5adbcd58e04991bb4651d2dd97af6e186d1f9

💬 darosior commented on pull request "descriptor: check whitespace in keys within fragments":
(https://github.com/bitcoin/bitcoin/pull/31603#discussion_r1959975429)
*(comment about the commit, not this line)*

nit: in commit e91a9cf9d2ca50ee31ed36a975d2e01faa666c92 you state:
> Due to Base58, pubkeys with whitespace at the beginning
end/or at the end are successfully parsed.

But pubkeys are never base58 encoded.

💬 darosior commented on pull request "descriptor: check whitespace in keys within fragments":
(https://github.com/bitcoin/bitcoin/pull/31603#discussion_r1960036391)
Interestingly this line is correct only because we check `IsHex` before `ParseHex` below, as otherwise we would need to check for any space character across the string since `ParseHex` would skip those even if they happen in the middle.

Checking for a space character at any position would also improve the error message for instance when parsing `pk(03 a34b...)`. But no need to retouch.

💬 darosior commented on pull request "descriptor: check whitespace in keys within fragments":
(https://github.com/bitcoin/bitcoin/pull/31603#issuecomment-2666139402)
> is to incorporate the following change.

Improving the descriptor unit test utilities can be done in a followup. It would be nice to get the user-facing fix in first.