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

💬 naumenkogs commented on pull request "p2p: Allow whitelisting outgoing connections":
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1447076053)
But we don't do this much anyway?
I think the code could be easily modified for this change, you'd only have to pass addr direction to `AddWhitelistPermissionFlags. I won't insist though.

💬 naumenkogs commented on pull request "p2p: Allow whitelisting outgoing connections":
(https://github.com/bitcoin/bitcoin/pull/27114#issuecomment-1884445322)
ACK 6175a2ee09663f7cf20a048b33e537442dfeb81d

👍 BrandonOdiwuor approved a pull request: "test: wallet migration, add coverage for tx extra data"
(https://github.com/bitcoin/bitcoin/pull/29204#pullrequestreview-1812762957)
lgtm ACK 016cc807f77a9128d430a0df1edd133521628a33

💬 darosior commented on issue "fuzz: Fix timeouts":
(https://github.com/bitcoin/bitcoin/issues/28812#issuecomment-1884514851)
I think this is due to a missing `std::move` for `j:` which makes `BuildScript` copy `subs[0]`: https://github.com/bitcoin/bitcoin/blob/063a8b83875997068b3eb506b5f30f2691d18052/src/script/miniscript.h#L762C43-L762C43.

Reproducing locally now.

💬 TheCharlatan commented on pull request "kernel: Run sanity checks on context construction":
(https://github.com/bitcoin/bitcoin/pull/28228#issuecomment-1884549947)
Rebased 53f0be73cc816f39e0131f5809bf14b43fee094a -> 673bcf4d25572bc49ff25e06515afe6c2e776537 ([contextSanityChecks_5](https://github.com/TheCharlatan/bitcoin/tree/contextSanityChecks_5) -> [contextSanityChecks_6](https://github.com/TheCharlatan/bitcoin/tree/contextSanityChecks_6), [compare](https://github.com/TheCharlatan/bitcoin/compare/contextSanityChecks_5..contextSanityChecks_6))

- Fixed conflict with #28051

📝 Satoshin-Btc opened a pull request: "Create cmake-multi-platform.yml"
(https://github.com/bitcoin/bitcoin/pull/29216)
<!--
*** Please remove the following help text before submitting: ***

Pull requests without a rationale and clear improvement may be closed
immediately.

GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->

<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:

* Any test improvements or new tests that improv
...

✅ fanquake closed a pull request: "Create cmake-multi-platform.yml"
(https://github.com/bitcoin/bitcoin/pull/29216)

📝 fanquake locked a pull request: "Create cmake-multi-platform.yml"
(https://github.com/bitcoin/bitcoin/pull/29216)
<!--
*** Please remove the following help text before submitting: ***

Pull requests without a rationale and clear improvement may be closed
immediately.

GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->

<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:

* Any test improvements or new tests that improv
...