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

Would suggest std::move(*maybe_result) here as well to avoid copying

💬 ryanofsky commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1427089580)
In commit "MOVEONLY: define class before functions in request.h" (bcf90ae6b7fd4685755c3f2e1e736053e613e0f6)

Not important, but I think the developer guide suggests braces around multiline if statements because it can prevent subtle bugs from sneaking in. Generally only older code in bitcoin core omits the braces on multiline if statements.

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

Would suggest `reply = std::move(*maybe_result);` to avoid copying the result object

💬 ryanofsky commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1429389378)
In commit "test: cover more HTTP error codes in interface_rpc.py" (b41f09cbc09a6f5d75636eb2f7ba8bc6d8802942)

Is this new line just expanding test coverage and not testing new behavior? Not critical, but in general it helps reviewing if test coverage is expanded first, before commits making code changes. Then it is a lot easier to review the code changes, because you can see how they affect tests. And it is easier to identify gaps in test coverage. An initial commit expanding test coverage bef
...

💬 ryanofsky commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1429401422)
In commit "test: handle 204 NO_CONTENT correctly in authproxy" (c85c9998dc98d15a7e42a0d2eb142cb516126243)

The logic in this function seems too permissive:
- If an empty "application/json" response is returned, now it returns None instead of treating the missing JSON as an error
- It does not treat responses that have NO_CONTENT status code but do have content as errors. It will even parse json and return from NO_CONTENT responses and ignore the content_type value.

The code is also more c
...

💬 ryanofsky commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1430137654)
In commit "test: enable JSON-RPC 2.0 calls in tests and verify expected behavior" (65c2f4448dc556e7a922ebac4abed7ddfc8d7745)

This test code is broken because the requests do not have unique ids. So when the responses come back, the later responses override earlier ones in the `result_by_id` array, the existing test cases without a version fields will not be checked, and any result they return will be treated as valid.

I also think copying and pasting existing test cases for version 1.1 and
...

👍 kristapsk approved a pull request: "RPC/Wallet: Add "use_txids" to output of getaddressinfo"
(https://github.com/bitcoin/bitcoin/pull/22693#pullrequestreview-1787134005)
ACK cd4e5ddaf7f9bd50690952eb82228c3b328c9221

💬 fanquake commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1430288696)
Yea, I think that's better, as we don't really want to copy-paste these big blocks of curl output into the middle of our our release notes.

💬 vasild commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#issuecomment-1860824375)
I agree with what you say, except the below:

> It is re-implementing existing pieces of code

Which?

> in a CPU-wasteful manner.

Isn't that just a few CPU instructions? (comparing the tip age to the current time). I agree it can be done less often than 45 seconds for the purpose of avoiding limited peers.

> a node completely isolated, lagging behind, with no relevant connections vs a node that is lagging behind but it is connected to the network and syncing up the chain

Yes, the
...

🚀 glozow merged a pull request: "wallet, mempool: propagete `checkChainLimits` error message to wallet"
(https://github.com/bitcoin/bitcoin/pull/28863)

💬 glozow commented on pull request "Update doc/policy/README.md":
(https://github.com/bitcoin/bitcoin/pull/29095#issuecomment-1860843499)
Agree with not keeping 2 separate lists. Maybe just say "see node relay options" in `-help`.

💬 brunoerg commented on pull request "Improve display address handling for external signer":
(https://github.com/bitcoin/bitcoin/pull/24313#discussion_r1430336113)
Do you intend to address this TODO?

💬 brunoerg commented on pull request "Fix spelling errors":
(https://github.com/bitcoin/bitcoin/pull/29107#issuecomment-1860889562)
You could include this one (not detected by linter):
https://github.com/bitcoin/bitcoin/blob/dd391944dc2d4e7dda9439647e603b41ba751b78/src/wallet/fees.cpp#L52

💬 ryanofsky commented on pull request "init: Error if ignored bitcoin.conf file is found":
(https://github.com/bitcoin/bitcoin/pull/27302#issuecomment-1860891611)
Thanks @AdamISZ, this is great feedback. In your case it sounds like you have a `bitcoin.conf` file in the datadir, but you want it to be ignored, so I hope the error message was clear enough to explain why that configuration is ambiguous and how the ambiguity could be resolved by specifying `allowignoredconf=1`. I did want the error message to be self contained and tell you how to fix it without needing to refer to release notes. So if this wasn't the case, it sounds like some improvement could
...

💬 glozow commented on pull request "RPC: add new `listmempooltransactions`":
(https://github.com/bitcoin/bitcoin/pull/29016#issuecomment-1860896797)
> My initial attempt at this modified `getrawmempool` to
>
> * take a `start_sequence` to allow for filtering, and
>
> * adding the txdata (inputs outpoints + outputs (amount + scriptPubKeys)).

This seems like the best option imo. If we want to return the transaction hex (iiuc that should suffice if you are looking to get the tx data itself) we could add another level of verbosity to `getrawmempool` / `getmempoolentry`.

> That was pretty ugly however, given that the current d
...

💬 ns-xvrn commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1860904280)
Concept ACK

a bare minimum default value change of an optional parameter makes lot of sense to increase the chances of keeping chainstate dataset clean

💬 glozow commented on pull request "Fix spelling errors":
(https://github.com/bitcoin/bitcoin/pull/29107#issuecomment-1860904444)
> You could include this one (not detected by linter):

iff is an abbreviation of "if and only if"

💬 glozow commented on pull request "Fix spelling errors":
(https://github.com/bitcoin/bitcoin/pull/29107#discussion_r1430351051)
this is done in #28948

💬 1440000bytes commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1860915662)
Concept NACK

> These kind of parameters should default to false.
> Node operators should be active users and decide which feature they want to enable for their node.

Disagree. All transactions should be relayed by default if they follow consensus rules. Only transactions affecting security of p2p network with DoS vectors should be non-standard. Users can always set it to `false` if they dont want to see bare multisig transactions in mempool.

Changing defaults in core to stop relaying s
...

💬 brunoerg commented on pull request "Fix spelling errors":
(https://github.com/bitcoin/bitcoin/pull/29107#issuecomment-1860915941)
> iff is an abbreviation of "if and only if"

didn't know it, thanks! :)