💬 0xB10C commented on pull request "rpc: introduce getversion RPC":
(https://github.com/bitcoin/bitcoin/pull/30112#issuecomment-2131226810)
> Concept NACK, consumers should just check if features are available

The [current recommendation](https://github.com/bitcoin/bitcoin/blob/42d5a1ff25a8045b6f4c09fa1fb71736dbccc034/doc/JSON-RPC-interface.md?plain=1#L69-L70) is already to use the `version` field from the `getnetworkinfo` RPC call. This PR doesn't really add anything new that wasn't already exposed and recommend for RPC clients to use.

---

Throwing this in for even more confusion about this new RPC and the software vs RPC
...

💬 king-11 commented on issue ""Rolling forward" at startup can take a long time, and is not interruptible":
(https://github.com/bitcoin/bitcoin/issues/11600#issuecomment-2131241170)
I have been encountering this myself the `bitcoind` runs inside of a docker container and when its asked to stop and doesn't respond to `SIGTERM` the docker after a timeout uses `SIGKILL` resulting in exit code of `137` being unclear stop. Can we attach listener for all the signals somewhere?

💬 epiccurious commented on pull request "doc: add guidance for RPC to developer notes":
(https://github.com/bitcoin/bitcoin/pull/30142#discussion_r1614633882)
nit: this sentence might be better structured starting with a verb:

```
Implement associated `-deprecatedrpc=` option to retain previous RPC behavior when modifying RPC interface JSON structure, including (but not limited to) the following:
```

💬 epiccurious commented on pull request "doc: add guidance for RPC to developer notes":
(https://github.com/bitcoin/bitcoin/pull/30142#discussion_r1614634416)
nit: this sentence might be better structured starting with a verb:

```
Prefer to choose RPC JSON data types that are flexible to expansion without change of data type.
```

💬 epiccurious commented on pull request "doc: add guidance for RPC to developer notes":
(https://github.com/bitcoin/bitcoin/pull/30142#issuecomment-2131241674)
ACK 61853a25bbbdb6d5dfb2f1570e41f541a4b7de78.

⚠️ sipa opened an issue: "Enable `importprivkey`, `addmultisigaddress` in descriptor wallets"
(https://github.com/bitcoin/bitcoin/issues/30175)
### Please describe the feature you'd like to see added.

Descriptor wallets currently do not support the "legacy" import commands `importprivkey`, `importpubkey`, `importaddress`, `addmultisigaddress`, `importmulti`, and `importwallet`. This was done because the semantics of these RPC cannot be replicated in descriptor wallets, as they just fundamentally work differently (being explicit about what to watch rather than "whatever matches the bag of keys and scripts we have").

I want to suggest
...

✅ kevkevinpal closed a pull request: "test: Added test to ensure log and failure happen when work is less than active chainstate"
(https://github.com/bitcoin/bitcoin/pull/30105)

💬 kevkevinpal commented on pull request "test: Added test to ensure log and failure happen when work is less than active chainstate":
(https://github.com/bitcoin/bitcoin/pull/30105#issuecomment-2131273947)
> Just realised this is similar to this PR: #29428

Oh nice I didnt see this, closing this now

💬 kevkevinpal commented on pull request "test: Assumeutxo: snapshots with less work should not be loaded":
(https://github.com/bitcoin/bitcoin/pull/29428#issuecomment-2131274346)
utACK [df6dc2a](https://github.com/bitcoin/bitcoin/pull/29428/commits/df6dc2aaaeffc664006b86ee8c8797dc484ec40e)

💬 kevkevinpal commented on pull request "bench: enable wallet creation benchmarks on all platforms":
(https://github.com/bitcoin/bitcoin/pull/30122#issuecomment-2131286969)
utACK [7c8abf3](https://github.com/bitcoin/bitcoin/pull/30122/commits/7c8abf3c2001152423da06d25f9f4906611685ea)

🤔 tdb3 reviewed a pull request: "doc: update mention of generating bitcoin.conf"
(https://github.com/bitcoin/bitcoin/pull/30154#pullrequestreview-2079033825)
ACK for 9013e2b97e8f50d2be63ce740c42d0b0e0b9b7f2
Good find. Makes sense to point the user to conf file generation (helping to ensure the conf reflects the implementation). Followed the link (seemed to work fine).

💬 hebasto commented on issue "make cov fails with lcov-2":
(https://github.com/bitcoin/bitcoin/issues/28468#issuecomment-2131301980)
FWIW, the https://github.com/hebasto/bitcoin/pull/191 supports both LCOV versions for CMake.

💬 furszy commented on issue "Enable `importprivkey`, `addmultisigaddress` in descriptor wallets":
(https://github.com/bitcoin/bitcoin/issues/30175#issuecomment-2131302391)
I think `importaddress` and `addmultisigaddress` intents are clear enough and they map well to descriptors. Both specify the address type. Forcing users to learn how to craft a descriptor when they merely want to watch an address seems overwhelming to me. -> #27034 maps `importaddress` to `addr()` and `raw()` descriptors. And #28307 fixes and extends `createmultisig`/`addmultisigaddress`.

Not sure about `importprivkey` as it would clash with the new `void(KEY)` descriptor (#29136). The intent
...

👍 tdb3 approved a pull request: "cli: restrict multiple exclusive argument usage in bitcoin-cli"
(https://github.com/bitcoin/bitcoin/pull/30148#pullrequestreview-2079066296)
ACK for 628d2d4173660c1ad35b2d84524f71b92593d7cd

Thanks for picking this up. Built and ran all functional tests (all passed). Manually executed `bitcoin-cli` with one and multiple instances of the argument group covered by this PR. The error was seen (as expected) if more than one argument of the group was specified.

Left a few notes.

💬 tdb3 commented on pull request "cli: restrict multiple exclusive argument usage in bitcoin-cli":
(https://github.com/bitcoin/bitcoin/pull/30148#discussion_r1614742727)
The implicit conversions from bool to int seem safe (at least at first glance, https://en.cppreference.com/w/cpp/language/implicit_conversion), but I didn't get a change to dig into C++ standard docs to check. It is a tiny bit unintuitive for a reader, but this doesn't seem too cryptic and seems cleaner than the separate `if` statements and temp variable used in 27815.

💬 tdb3 commented on pull request "cli: restrict multiple exclusive argument usage in bitcoin-cli":
(https://github.com/bitcoin/bitcoin/pull/30148#discussion_r1614742124)
What's the rationale for using `IsArgSet()` for `getinfo` but `GetBoolArg()` for the others? Maybe I'm overlooking something simple.

Tried the following locally and didn't see any issues with it:
```diff
diff --git a/src/bitcoin-cli.cpp b/src/bitcoin-cli.cpp
index 4002424af0..b682544fa2 100644
--- a/src/bitcoin-cli.cpp
+++ b/src/bitcoin-cli.cpp
@@ -1175,10 +1175,11 @@ static int CommandLineRPC(int argc, char *argv[])
fputc('\n', stdout);
}
}
+
...

💬 maflcko commented on pull request "test: switch from curl to urllib for HTTP requests":
(https://github.com/bitcoin/bitcoin/pull/29970#discussion_r1614761341)
Was this addressed?

💬 maflcko commented on pull request "doc, rpc: Release notes and follow-ups for #29612":
(https://github.com/bitcoin/bitcoin/pull/30167#discussion_r1614770544)
I agree that the assert should be safe here, but I think consistency-wise it makes sense to use exceptions (even for internal logic errors) over asserts/assumes in contexts where exceptions are already used and where it is safe to use them. RPC and serialization are two such areas.

Thanks for fixing.

👍 maflcko approved a pull request: "doc, rpc: Release notes and follow-ups for #29612"
(https://github.com/bitcoin/bitcoin/pull/30167#pullrequestreview-2079083069)
utACK efc1b5be8a4696c0db19ba18316b2d4ed09e10f2

👍 AngusP approved a pull request: "test: switch from curl to urllib for HTTP requests"
(https://github.com/bitcoin/bitcoin/pull/29970#pullrequestreview-2079184785)
ACK 588cad38f7813d88f022d0304aa20a0b96d184ed