💬 jamesob commented on pull request "PoC: fuzz chainstate and block managers":
(https://github.com/bitcoin/bitcoin/pull/29158#discussion_r1446611692)
This type comes up so often in this file that it might be worth an alias.
(https://github.com/bitcoin/bitcoin/pull/29158#discussion_r1446611692)
This type comes up so often in this file that it might be worth an alias.
💬 sipa commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#issuecomment-1885174610)
Since the merge of #29058 I think the first commit here should be dropped.
(https://github.com/bitcoin/bitcoin/pull/24748#issuecomment-1885174610)
Since the merge of #29058 I think the first commit here should be dropped.
📝 dergoegge opened a pull request: "fuzz: Improve fuzzing stability for ellswift_roundtrip harness"
(https://github.com/bitcoin/bitcoin/pull/29219)
See #29018
(https://github.com/bitcoin/bitcoin/pull/29219)
See #29018
💬 sipa commented on pull request "fuzz: Improve fuzzing stability for ellswift_roundtrip harness":
(https://github.com/bitcoin/bitcoin/pull/29219#issuecomment-1885216408)
utACK 154fcce55c84c251fad8d280eafb3c0a5284fcd4
(https://github.com/bitcoin/bitcoin/pull/29219#issuecomment-1885216408)
utACK 154fcce55c84c251fad8d280eafb3c0a5284fcd4
💬 glozow commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1447653071)
True, it's helpful but not strictly necessary here. I think I'll just take this out of this PR and move it to #27018.
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1447653071)
True, it's helpful but not strictly necessary here. I think I'll just take this out of this PR and move it to #27018.
🚀 glozow merged a pull request: "test: assumeutxo: spend coin from snapshot chainstate after loading"
(https://github.com/bitcoin/bitcoin/pull/29215)
(https://github.com/bitcoin/bitcoin/pull/29215)
💬 glozow commented on pull request "net: create I2P sessions using both ECIES-X25519 and ElGamal encryption":
(https://github.com/bitcoin/bitcoin/pull/29200#issuecomment-1885243098)
Backported in #29209
(https://github.com/bitcoin/bitcoin/pull/29200#issuecomment-1885243098)
Backported in #29209
💬 fanquake commented on pull request "build: LLD based macOS toolchain":
(https://github.com/bitcoin/bitcoin/pull/21778#issuecomment-1885249692)
Would still like to see them PR'd separately, but I cherry-picked the two relevant commits, dropped `native_cmake`, and have dropped anything libtool related here.
(https://github.com/bitcoin/bitcoin/pull/21778#issuecomment-1885249692)
Would still like to see them PR'd separately, but I cherry-picked the two relevant commits, dropped `native_cmake`, and have dropped anything libtool related here.
💬 fanquake commented on pull request "build: LLD based macOS toolchain":
(https://github.com/bitcoin/bitcoin/pull/21778#issuecomment-1885270125)
The remaining issue here is with `make -C depends CC= CXX=` usage. If we override, we lose the c(xx) flags which are currently embedded into darwin_C(XX), which means things don't compile. This is why this PR works fine, if you just invoke `make -C depends HOST=arm64-apple-darwin`, with `clang` installed, but if you `make -C depends HOST=arm64-apple-darwin CC=clang-17 CXX=clang++-17`, like we want to do in the CI, things don't quite work because we loose flags, like `--target`, and clang things
...
(https://github.com/bitcoin/bitcoin/pull/21778#issuecomment-1885270125)
The remaining issue here is with `make -C depends CC= CXX=` usage. If we override, we lose the c(xx) flags which are currently embedded into darwin_C(XX), which means things don't compile. This is why this PR works fine, if you just invoke `make -C depends HOST=arm64-apple-darwin`, with `clang` installed, but if you `make -C depends HOST=arm64-apple-darwin CC=clang-17 CXX=clang++-17`, like we want to do in the CI, things don't quite work because we loose flags, like `--target`, and clang things
...
🤔 ryanofsky reviewed a pull request: "Support JSON-RPC 2.0 when requested by client"
(https://github.com/bitcoin/bitcoin/pull/27101#pullrequestreview-1813562575)
Code review 756da6d1e67fba65a992dc0090ee8c0cfa26abe3. Everything looks pretty good, and most of my suggestions can be ignored, but it seems like this is currently detecting notifications incorrectly and not sending responses in cases when they should be sent, when the request id field is present but null.
(https://github.com/bitcoin/bitcoin/pull/27101#pullrequestreview-1813562575)
Code review 756da6d1e67fba65a992dc0090ee8c0cfa26abe3. Everything looks pretty good, and most of my suggestions can be ignored, but it seems like this is currently detecting notifications incorrectly and not sending responses in cases when they should be sent, when the request id field is present but null.
💬 ryanofsky commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1447586115)
In commit "test: cover JSONRPC 2.0 requests, batches, and notifications" (d61e469179dae4e94fd00f47c2eeb8533d19e13c)
It's not great to make this a static method and use a global variable, because now tests are no longer independent of each other and can potentially interfere with each other. Also using deep copy is unnecessary here because only the top level dictionary is modified. Also the name of the class "Format" is not very descriptive. Would suggest cleaning all this up:
```diff
---
...
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1447586115)
In commit "test: cover JSONRPC 2.0 requests, batches, and notifications" (d61e469179dae4e94fd00f47c2eeb8533d19e13c)
It's not great to make this a static method and use a global variable, because now tests are no longer independent of each other and can potentially interfere with each other. Also using deep copy is unnecessary here because only the top level dictionary is modified. Also the name of the class "Format" is not very descriptive. Would suggest cleaning all this up:
```diff
---
...
💬 ryanofsky commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1447648863)
In commit "doc: add comments and release-notes for JSON-RPC 2.0" (756da6d1e67fba65a992dc0090ee8c0cfa26abe3)
Think this should also mention that responses contain "jsonrpc": "2.0" field. Or this could be an additional row in the table.
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1447648863)
In commit "doc: add comments and release-notes for JSON-RPC 2.0" (756da6d1e67fba65a992dc0090ee8c0cfa26abe3)
Think this should also mention that responses contain "jsonrpc": "2.0" field. Or this could be an additional row in the table.
💬 ryanofsky commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1447616571)
In commit "test: cover more HTTP error codes in interface_rpc.py" (29ebb0d7774ee2d10aa274086e8ff4b2395f7047)
Setting params to None seems like a bug because it means a `"params": null` JSON field will be sent, when previously authproxy would send an empty params object. Would suggest changing this to `params={}` to avoid a change in behavior.
Also, I'm not sure if it is a bug for our server to accept null params. The spec says "If present, parameters for the rpc call MUST be provided as a
...
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1447616571)
In commit "test: cover more HTTP error codes in interface_rpc.py" (29ebb0d7774ee2d10aa274086e8ff4b2395f7047)
Setting params to None seems like a bug because it means a `"params": null` JSON field will be sent, when previously authproxy would send an empty params object. Would suggest changing this to `params={}` to avoid a change in behavior.
Also, I'm not sure if it is a bug for our server to accept null params. The spec says "If present, parameters for the rpc call MUST be provided as a
...
💬 ryanofsky commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1447662808)
In commit "rpc: JSON-RPC 2.0 should not respond to "notifications"" (bc2d55898b315d1ee80a70377d65f79c829246ed)
This does not seem correct. According to https://www.jsonrpc.org/specification#notification a notification is a 'Request object without an "id" member', not a request object with a `null` id member. It also explicitly says requests that have "Null as a value for the id member" are treated as "Responses with an unknown id" not notifications.
Probably a reasonable way to fix this wo
...
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1447662808)
In commit "rpc: JSON-RPC 2.0 should not respond to "notifications"" (bc2d55898b315d1ee80a70377d65f79c829246ed)
This does not seem correct. According to https://www.jsonrpc.org/specification#notification a notification is a 'Request object without an "id" member', not a request object with a `null` id member. It also explicitly says requests that have "Null as a value for the id member" are treated as "Responses with an unknown id" not notifications.
Probably a reasonable way to fix this wo
...
💬 ryanofsky commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1447645890)
In commit "doc: add comments and release-notes for JSON-RPC 2.0" (756da6d1e67fba65a992dc0090ee8c0cfa26abe3)
Would drop "Protocols can be mixed in a batch request." It seems ok, but not a good thing to encourage, and unless it's required by the specs, I don't think we should commit to this behavior by adding it to our documentation.
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1447645890)
In commit "doc: add comments and release-notes for JSON-RPC 2.0" (756da6d1e67fba65a992dc0090ee8c0cfa26abe3)
Would drop "Protocols can be mixed in a batch request." It seems ok, but not a good thing to encourage, and unless it's required by the specs, I don't think we should commit to this behavior by adding it to our documentation.
💬 ryanofsky commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1447684268)
In commit "test: handle 204 NO_CONTENT correctly in authproxy" (174a995edf594dccf745d2593c281d3e4a1ff7f7)
It would be good to have a comment that explains why this block of code is needed, because it seems like an impossible case to hit through normal usage of authproxy. Maybe: "# Check for no-content HTTP status code, which can be returned when an RPC client requests a JSON-RPC 2.0 "notification" with no response. Currently this is only possible if clients call the _request() method directly
...
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1447684268)
In commit "test: handle 204 NO_CONTENT correctly in authproxy" (174a995edf594dccf745d2593c281d3e4a1ff7f7)
It would be good to have a comment that explains why this block of code is needed, because it seems like an impossible case to hit through normal usage of authproxy. Maybe: "# Check for no-content HTTP status code, which can be returned when an RPC client requests a JSON-RPC 2.0 "notification" with no response. Currently this is only possible if clients call the _request() method directly
...
💬 ryanofsky commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1447679340)
In commit "doc: add comments and release-notes for JSON-RPC 2.0" (756da6d1e67fba65a992dc0090ee8c0cfa26abe3)
Maybe replace "errors like if..." with "errors in cases like the endpoint not being found (404) or the RPC request not being formatted correctly (500)." Grammar sounds a little off the way it is written now.
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1447679340)
In commit "doc: add comments and release-notes for JSON-RPC 2.0" (756da6d1e67fba65a992dc0090ee8c0cfa26abe3)
Maybe replace "errors like if..." with "errors in cases like the endpoint not being found (404) or the RPC request not being formatted correctly (500)." Grammar sounds a little off the way it is written now.
👍 ryanofsky approved a pull request: "wallet: Add `createwalletdescriptor` and `gethdkeys` RPCs for adding new automatically generated descriptors"
(https://github.com/bitcoin/bitcoin/pull/29130#pullrequestreview-1813747756)
Code review ACK b55b46f07ae60bbd51ed9abb2f8c5b3249af60f2. Looks great! Just suggested changes since last review
(https://github.com/bitcoin/bitcoin/pull/29130#pullrequestreview-1813747756)
Code review ACK b55b46f07ae60bbd51ed9abb2f8c5b3249af60f2. Looks great! Just suggested changes since last review
👍 brunoerg approved a pull request: "fuzz: Improve fuzzing stability for ellswift_roundtrip harness"
(https://github.com/bitcoin/bitcoin/pull/29219#pullrequestreview-1813766726)
crACK 154fcce55c84c251fad8d280eafb3c0a5284fcd4
(https://github.com/bitcoin/bitcoin/pull/29219#pullrequestreview-1813766726)
crACK 154fcce55c84c251fad8d280eafb3c0a5284fcd4
💬 jamesob commented on pull request "PoC: fuzz chainstate and block managers":
(https://github.com/bitcoin/bitcoin/pull/29158#issuecomment-1885310514)
Pushed three additional commits to my branch that may make a dent in the CI issues:
- make `fs::path` hashable: https://github.com/jamesob/bitcoin/commit/8f5fdf8ad3d5102e4afaae415e004d4cf6c667cc
- avoid use of `std::filesystem::path` where possible: https://github.com/jamesob/bitcoin/commit/4ef785749ff1c481b5681739653c5b153637e44e
- update linter for mockable filesystem ops: https://github.com/jamesob/bitcoin/commit/face876d55711299c2679aad59b24ee04aa892f0
(https://github.com/bitcoin/bitcoin/pull/29158#issuecomment-1885310514)
Pushed three additional commits to my branch that may make a dent in the CI issues:
- make `fs::path` hashable: https://github.com/jamesob/bitcoin/commit/8f5fdf8ad3d5102e4afaae415e004d4cf6c667cc
- avoid use of `std::filesystem::path` where possible: https://github.com/jamesob/bitcoin/commit/4ef785749ff1c481b5681739653c5b153637e44e
- update linter for mockable filesystem ops: https://github.com/jamesob/bitcoin/commit/face876d55711299c2679aad59b24ee04aa892f0