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

💬 ismaelsadeeq commented on pull request "RPC: Add maxfeerate and maxburnamount args to submitpackage":
(https://github.com/bitcoin/bitcoin/pull/28950#discussion_r1447190456)
>Assuming you meant C's feerate is 100 sat/vbyte

Yes C fee rated is 100s/vb, edited thanks.

>This is prior to any relay, of course restricting feerates is incentive compatible once published on the open network

Do you mean incentive incompatible here? because in the given example if C is rejected A will be added to the mempool with mining score of 5s/vb which is lower than whats intended.

⚠️ ismaelsadeeq opened an issue: "Enable `maxfeerate` and `maxburnamount` as startup config options."
(https://github.com/bitcoin/bitcoin/issues/29217)
### Please describe the feature you'd like to see added.

`maxfeerate` is a sanity check optional parameter used by clients to ensure transactions submitted to the mempool and relayed to the network by a node with`sendrawtransaction`, `testmempoolaccept` and now possibly `submitpackage` #28950 does not exceed the `maxfeerate` value.
Node will reject transactions whose fee rate is higher than `maxfeerate`.

`maxburnamount` is also a sanity check optional parameter used by `sendrawtransaction
...

💬 ismaelsadeeq commented on pull request "RPC: Add maxfeerate and maxburnamount args to submitpackage":
(https://github.com/bitcoin/bitcoin/pull/28950#discussion_r1447202424)
Done in https://github.com/bitcoin/bitcoin/issues/29217