💬 achow101 commented on pull request "wallet: Add `createwalletdescriptor` and `gethdkeys` RPCs for adding new automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1446760656)
Done

💬 achow101 commented on pull request "wallet: Add `createwalletdescriptor` and `gethdkeys` RPCs for adding new automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1446760708)
Done

💬 achow101 commented on pull request "wallet: Add `createwalletdescriptor` and `gethdkeys` RPCs for adding new automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1446760770)
Done

💬 achow101 commented on pull request "wallet: Add `createwalletdescriptor` and `gethdkeys` RPCs for adding new automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1446760887)
Done

💬 achow101 commented on pull request "wallet: Add `createwalletdescriptor` and `gethdkeys` RPCs for adding new automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1446761034)
Updated the test to check this.

💬 achow101 commented on pull request "wallet: Add `createwalletdescriptor` and `gethdkeys` RPCs for adding new automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1446761065)
Done

💬 achow101 commented on pull request "wallet: Add `createwalletdescriptor` and `gethdkeys` RPCs for adding new automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1446761386)
I think that would require a much more invasive refactor as it requires accessing much lower level data. Perhaps for a followup.

📝 theStack opened a pull request: "test: assumeutxo: spend coin from snapshot chainstate after loading"
(https://github.com/bitcoin/bitcoin/pull/29215)
This PR extends the AssumeUTXO functional test by submitting a spending transaction for an UTXO that is only available in a the snapshot chainstate (after loading via `loadtxoutset`), i.e. it hasn't been seen in a block before. With that we can verify that snapshot coins are visible to the mempool.

Note that we unfortunately can't use MiniWallet here, as the only available UTXO to spend from the snapshot chainstate is at height 200, where a P2PKH created from the test framework's deterministi
...

💬 kevkevinpal commented on pull request "mempool / rpc: followup to getprioritisedtransactions and delete a mapDeltas entry when delta==0":
(https://github.com/bitcoin/bitcoin/pull/28885#discussion_r1446828496)
not sure why I didn't think of just using the value returned from `create_self_transfer` fixed in [24b490f](https://github.com/bitcoin/bitcoin/pull/28885/commits/24b490f292149e6151dcddaf11ac0d5bf16dde44)

🤔 ryanofsky reviewed a pull request: "Support JSON-RPC 2.0 when requested by client"
(https://github.com/bitcoin/bitcoin/pull/27101#pullrequestreview-1812307340)
Reviewed 756da6d1e67fba65a992dc0090ee8c0cfa26abe3 and this looks very good. So far I reviewed all the c++ code, but not the python or documentation yet.

💬 ryanofsky commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1446841611)
In commit "rpc: identify JSON-RPC 2.0 requests" (9e1c6fa84eef606d5504ebcbbd2a2ca7a4f748dc)

The spec says this field 'MUST be exactly "2.0"' (https://www.jsonrpc.org/specification#request_object), so maybe it should be an error if the field is set and contains any other value. This way if there is ever a JSON-RPC 2.1 or 3.0 protocol, clients can easily check for support without getting back a strange JSON 1.1 response. FWIW, chatgpt suggestd the following response:

```json
{
"jsonrpc"
...

💬 ryanofsky commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1446812606)
In commit "rpc: refactor single/batch requests" (27afe01560b1d930c3b072a180cab95c857eff24)

This ret variable seems unnecessary. Can this just say `reply = UniValue{UniValue::VARR}` and use the `reply` variable instead of `ret` below?

The name is also confusing because `ret` normally refers to a return value, which this is not what this is.

💬 ryanofsky commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1446816704)
In commit "rpc: refactor single/batch requests" (27afe01560b1d930c3b072a180cab95c857eff24)

Not new behavior but the result object, which is potentially large, is copied for no reason here. It would be nice in a separate commit to change `JSONRPCReplyObj` to accept a plain `UniValue` parameter instead of `const UniValue&` so `std::move(result)` could be passed here, and the result wouldn't be copied unnecessarily.

💬 ryanofsky commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1446844876)
In commit "rpc: make error and result mutually exclusive in JSON-RPC 2.0 replies" (c9196a2e5b747753e0d5ee59e1bcb987943eedbf)

I think these two lines should be switched to always return the result field before the error field. This would make responses look more consistent and avoid changing previous behavior.

💬 ryanofsky commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1446851259)
In commit "rpc: JSON-RPC 2.0 should not respond to "notifications"" (bc2d55898b315d1ee80a70377d65f79c829246ed)

Would be good to `std::move(response)` here to avoid a copy

💬 ryanofsky commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1446847073)
In commit "rpc: make error and result mutually exclusive in JSON-RPC 2.0 replies" (c9196a2e5b747753e0d5ee59e1bcb987943eedbf)

Again suggestion for an separate earlier commit, but it would be good if JSONRPCReplyObj took a `UniValue error` parameter instead of `const UniValue& error` so the error object could be moved instead of copied.

💬 Sjors commented on pull request "net: additional disconnect logging":
(https://github.com/bitcoin/bitcoin/pull/28521#issuecomment-1884376082)
@sr-gi I could, but I prefer to punt that until we have helper function that can be tested in just one place: https://github.com/bitcoin/bitcoin/pull/28521#issuecomment-1835692974

💬 S3RK commented on pull request "wallet, rpc: `FundTransaction` refactor":
(https://github.com/bitcoin/bitcoin/pull/28560#discussion_r1447033851)
in 0b0276bb00d3511172d662a94ffd7b55adbbf2d1

I don't see a reason to extract this function at this point (could be done later in the PR).
There are two disparate cases covered (`bool` and `vector<string>`). And each case is only used in one and only one place, further commits also don't introduce further usage of this code path as they pass `vector<int>`.

Wouldn't it be better if we keep this logic at the call site?
Benefits of my proposal:
* we don't need second parameter for this fun
...

💬 maflcko commented on pull request "test: assumeutxo: spend coin from snapshot chainstate after loading":
(https://github.com/bitcoin/bitcoin/pull/29215#issuecomment-1884411245)
lgtm ACK 931575418e082af37b88b1819125b0d0f0fabbd5

💬 naumenkogs commented on pull request "p2p: Allow whitelisting outgoing connections":
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1447050977)
What does `\sa` mean?