💬 ChrisMartl commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1860650558)
Concept ACK
Database is a very important resource for the "Bitcoin"-System and its usage must be reserved exclusively for Sats endogenous monetary usage.
Since the Bitcoin's systemic flaw (https://github.com/bitcoin/bitcoin/issues/29089#issue-2043281653) is still unaddressed / unsolved, efforts to preserve the capability of already allocated storage devices resources in the network shall be considered and encouraged.
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1860650558)
Concept ACK
Database is a very important resource for the "Bitcoin"-System and its usage must be reserved exclusively for Sats endogenous monetary usage.
Since the Bitcoin's systemic flaw (https://github.com/bitcoin/bitcoin/issues/29089#issue-2043281653) is still unaddressed / unsolved, efforts to preserve the capability of already allocated storage devices resources in the network shall be considered and encouraged.
💬 ismaelsadeeq commented on pull request "wallet, mempool: propagete `checkChainLimits` error message to wallet":
(https://github.com/bitcoin/bitcoin/pull/28863#discussion_r1430217862)
Thanks Would create a tiny follow-up to fix this, so as not to invalidate ACK's.
(https://github.com/bitcoin/bitcoin/pull/28863#discussion_r1430217862)
Thanks Would create a tiny follow-up to fix this, so as not to invalidate ACK's.
💬 brunoerg commented on pull request "Improve display address handling for external signer":
(https://github.com/bitcoin/bitcoin/pull/24313#issuecomment-1860680045)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/24313#issuecomment-1860680045)
Concept ACK
💬 vasild commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1430238465)
The problem this PR aims to resolve is that if we are too much behind (48h), then limited peers may not be able to give us the blocks we need. To resolve this problem, I think that it is not necessary to change the "extra block relay only connections" logic. IMO that better be done in a separate PR with its own justification.
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1430238465)
The problem this PR aims to resolve is that if we are too much behind (48h), then limited peers may not be able to give us the blocks we need. To resolve this problem, I think that it is not necessary to change the "extra block relay only connections" logic. IMO that better be done in a separate PR with its own justification.
💬 maflcko commented on pull request "doc: Clarify C++20 comments":
(https://github.com/bitcoin/bitcoin/pull/29042#issuecomment-1860707320)
Included https://github.com/bitcoin/bitcoin/pull/29108
(https://github.com/bitcoin/bitcoin/pull/29042#issuecomment-1860707320)
Included https://github.com/bitcoin/bitcoin/pull/29108
💬 ismaelsadeeq commented on pull request "Policy: Report reason inputs are non standard":
(https://github.com/bitcoin/bitcoin/pull/29060#discussion_r1430246659)
Fixed thanks
(https://github.com/bitcoin/bitcoin/pull/29060#discussion_r1430246659)
Fixed thanks
💬 ismaelsadeeq commented on pull request "Policy: Report reason inputs are non standard":
(https://github.com/bitcoin/bitcoin/pull/29060#discussion_r1430246916)
fixed
(https://github.com/bitcoin/bitcoin/pull/29060#discussion_r1430246916)
fixed
💬 ismaelsadeeq commented on pull request "Policy: Report reason inputs are non standard":
(https://github.com/bitcoin/bitcoin/pull/29060#issuecomment-1860731819)
Thanks for reviewing @luke-jr
Force pushed from https://github.com/bitcoin/bitcoin/commit/e032462c44dba9b6961c1e18bd597d63c6afac02 to https://github.com/bitcoin/bitcoin/commit/e032462c44dba9b6961c1e18bd597d63c6afac02.
[Compare diff](https://github.com/bitcoin/bitcoin/compare/e032462c44dba9b6961c1e18bd597d63c6afac02..814571bf3ebb4e2640bbb6401c5822c038bb5287)
The changes in latest push are:
- Rebased on master to fix CI
- Updated the non-standardness reason to address @luke-jr comments
...
(https://github.com/bitcoin/bitcoin/pull/29060#issuecomment-1860731819)
Thanks for reviewing @luke-jr
Force pushed from https://github.com/bitcoin/bitcoin/commit/e032462c44dba9b6961c1e18bd597d63c6afac02 to https://github.com/bitcoin/bitcoin/commit/e032462c44dba9b6961c1e18bd597d63c6afac02.
[Compare diff](https://github.com/bitcoin/bitcoin/compare/e032462c44dba9b6961c1e18bd597d63c6afac02..814571bf3ebb4e2640bbb6401c5822c038bb5287)
The changes in latest push are:
- Rebased on master to fix CI
- Updated the non-standardness reason to address @luke-jr comments
...
💬 fanquake commented on issue "ci: feature_proxy failing in MSVC job":
(https://github.com/bitcoin/bitcoin/issues/29090#issuecomment-1860741967)
Similar failure, different test: https://github.com/bitcoin/bitcoin/actions/runs/7249043694/job/19746211856#step:27:521.
(https://github.com/bitcoin/bitcoin/issues/29090#issuecomment-1860741967)
Similar failure, different test: https://github.com/bitcoin/bitcoin/actions/runs/7249043694/job/19746211856#step:27:521.
⚠️ fanquake opened an issue: "ci: failure in `wallet_basic.py --descriptors`"
(https://github.com/bitcoin/bitcoin/issues/29110)
https://cirrus-ci.com/task/5898005006516224?logs=ci#L2515:
```bash
_20231218_134952/wallet_basic_259/node0/regtest/wallets/default_wallet/wallet.dat] SQLite Statement: ROLLBACK TRANSACTION
node0 2023-12-18T13:53:29.193915Z [scheduler] [wallet/sqlite.cpp:400] [Close] SQLiteBatch: Batch closed unexpectedly without the transaction being explicitly committed or aborted
node0 2023-12-18T13:53:29.194038Z [scheduler] [wallet/sqlite.cpp:53] [TraceSqlCallback] [/ci_container_base/ci/scratch/test_
...
(https://github.com/bitcoin/bitcoin/issues/29110)
https://cirrus-ci.com/task/5898005006516224?logs=ci#L2515:
```bash
_20231218_134952/wallet_basic_259/node0/regtest/wallets/default_wallet/wallet.dat] SQLite Statement: ROLLBACK TRANSACTION
node0 2023-12-18T13:53:29.193915Z [scheduler] [wallet/sqlite.cpp:400] [Close] SQLiteBatch: Batch closed unexpectedly without the transaction being explicitly committed or aborted
node0 2023-12-18T13:53:29.194038Z [scheduler] [wallet/sqlite.cpp:53] [TraceSqlCallback] [/ci_container_base/ci/scratch/test_
...
👍 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
...