💬 sipa commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1447504347)
In commit "p2p: Add transactions to reconciliation set"
Would it make sense to have a helper function instead of `IsPeerRegistered`, that instead of returning a `bool` returns a `TxReconciliationState*` (`nullptr` if not found)? That would avoid locking/finding twice. I suspect it'd be usable in other places too.
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1447504347)
In commit "p2p: Add transactions to reconciliation set"
Would it make sense to have a helper function instead of `IsPeerRegistered`, that instead of returning a `bool` returns a `TxReconciliationState*` (`nullptr` if not found)? That would avoid locking/finding twice. I suspect it'd be usable in other places too.
💬 sipa commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1447514047)
In commit "p2p: Add transactions to reconciliation set"
You could pick a fixed seed and wtxid for which you assert that with say 35 peers 3 are picked, and another seed/wtxid for which you assert that with 35 peers 4 are picked. That would at least show that the fractional probability code isn't just rounding down.
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1447514047)
In commit "p2p: Add transactions to reconciliation set"
You could pick a fixed seed and wtxid for which you assert that with say 35 peers 3 are picked, and another seed/wtxid for which you assert that with 35 peers 4 are picked. That would at least show that the fractional probability code isn't just rounding down.
👍 dergoegge approved a pull request: "build: Pass sanitize flags to instrument `libsecp256k1` code"
(https://github.com/bitcoin/bitcoin/pull/28875#pullrequestreview-1813583546)
tACK a0419151541fd5698cb4ddb4754395a22ec749a6
It might be better to reverse the order of the commits or squash them to avoid bisect failures in between.
(https://github.com/bitcoin/bitcoin/pull/28875#pullrequestreview-1813583546)
tACK a0419151541fd5698cb4ddb4754395a22ec749a6
It might be better to reverse the order of the commits or squash them to avoid bisect failures in between.
🤔 jamesob reviewed a pull request: "PoC: fuzz chainstate and block managers"
(https://github.com/bitcoin/bitcoin/pull/29158#pullrequestreview-1812003162)
Concept ACK; midway through review and trying to resolve some of the CI issues.
(https://github.com/bitcoin/bitcoin/pull/29158#pullrequestreview-1812003162)
Concept ACK; midway through review and trying to resolve some of the CI issues.
💬 jamesob commented on pull request "PoC: fuzz chainstate and block managers":
(https://github.com/bitcoin/bitcoin/pull/29158#discussion_r1446610769)
When generating a valid block (here and below) do we want to do some kind of assertion that the block was actually considered valid? I could see the test code silently generating only invalid blocks and (afaik) there wouldn't be an indication of it here.
(https://github.com/bitcoin/bitcoin/pull/29158#discussion_r1446610769)
When generating a valid block (here and below) do we want to do some kind of assertion that the block was actually considered valid? I could see the test code silently generating only invalid blocks and (afaik) there wouldn't be an indication of it here.
💬 jamesob commented on pull request "PoC: fuzz chainstate and block managers":
(https://github.com/bitcoin/bitcoin/pull/29158#discussion_r1447615580)
https://github.com/bitcoin/bitcoin/pull/29158/commits/ea36af80beeeee0b9de793e52887ba3e164b803c
`fmemopen` is Linux-specific; here's a fixup commit that `ifdef`s it out and fixes some of the CI errors if you want it: https://github.com/jamesob/bitcoin/commit/26b9c9d48e3ec16e69d550266c7f27f4db9cb9f8
(https://github.com/bitcoin/bitcoin/pull/29158#discussion_r1447615580)
https://github.com/bitcoin/bitcoin/pull/29158/commits/ea36af80beeeee0b9de793e52887ba3e164b803c
`fmemopen` is Linux-specific; here's a fixup commit that `ifdef`s it out and fixes some of the CI errors if you want it: https://github.com/jamesob/bitcoin/commit/26b9c9d48e3ec16e69d550266c7f27f4db9cb9f8
💬 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
...