👍 ryanofsky approved a pull request: "Support JSON-RPC 2.0 when requested by client"
(https://github.com/bitcoin/bitcoin/pull/27101#pullrequestreview-1782332723)
Code review ACK 5d5e8cbca785db851747c5143dc1d651a1ea11e6. This is definitely an improvement overall, but I think it leaves httprpc.cpp and rpc/server.cpp organization and the functional tests in a messier state, and I left suggestions for cleanup.
I was also a confused by parts of the PR description. I'm not sure what "different behavior for single and batched RPC requests" and "same behavior for single and batch requests" refers to, and think these parts could be omitted or clarified. Also "
...
(https://github.com/bitcoin/bitcoin/pull/27101#pullrequestreview-1782332723)
Code review ACK 5d5e8cbca785db851747c5143dc1d651a1ea11e6. This is definitely an improvement overall, but I think it leaves httprpc.cpp and rpc/server.cpp organization and the functional tests in a messier state, and I left suggestions for cleanup.
I was also a confused by parts of the PR description. I'm not sure what "different behavior for single and batched RPC requests" and "same behavior for single and batch requests" refers to, and think these parts could be omitted or clarified. Also "
...
💬 ryanofsky commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1427031318)
In commit "rpc: refactor single/batch requests" (fcc8cd08dbed9a44bab277ad267306dd2a9f5199)
Would be clearer if jreq were a const reference, since it is not modified
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1427031318)
In commit "rpc: refactor single/batch requests" (fcc8cd08dbed9a44bab277ad267306dd2a9f5199)
Would be clearer if jreq were a const reference, since it is not modified
💬 ryanofsky commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1427085941)
In commit "MOVEONLY: define class before functions in request.h" (bcf90ae6b7fd4685755c3f2e1e736053e613e0f6)
It seems ok to move these functions, but for future reference, there should be no need to do this because you can forward declare the class with `class JSONRPCRequest;` at the top of the file. I do think it would be slightly preferable to do that instead of moving code, but it's not important.
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1427085941)
In commit "MOVEONLY: define class before functions in request.h" (bcf90ae6b7fd4685755c3f2e1e736053e613e0f6)
It seems ok to move these functions, but for future reference, there should be no need to do this because you can forward declare the class with `class JSONRPCRequest;` at the top of the file. I do think it would be slightly preferable to do that instead of moving code, but it's not important.
💬 ryanofsky commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1427171338)
In commit "rpc: make error and result mutually exclusive in JSON-RPC 2.0 replies" (55dae852d88cdc6a4ad7e4a5cd3ba1988af9872c)
This comment is confusing because result and error _values_ are mutually exclusive in both cases.
You could clarify the comment saying "error and result _keys_ are mutually exclusive, to clarify that the comment is referring to keys not values. But I think the way this is written with duplicated logic is unnecessarily confusing anyway. Would suggest just writing the
...
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1427171338)
In commit "rpc: make error and result mutually exclusive in JSON-RPC 2.0 replies" (55dae852d88cdc6a4ad7e4a5cd3ba1988af9872c)
This comment is confusing because result and error _values_ are mutually exclusive in both cases.
You could clarify the comment saying "error and result _keys_ are mutually exclusive, to clarify that the comment is referring to keys not values. But I think the way this is written with duplicated logic is unnecessarily confusing anyway. Would suggest just writing the
...
💬 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
(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.
(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
(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
...
(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
...
(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
...
(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
(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.
(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
...
(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)
(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`.
(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?
(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
(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
...
(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
...
(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
(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