💬 sipa commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1447521744)
In commit "Cache fanout candidates to optimize txreconciliation"
Since the cache entry `std::set<NodeId>` don't change until being replaced entirely, I believe it would be more efficient to use a `std::vector<NodeId>`, with the nodeids in sorted order. You can then use `std::binary_search` to query in that vector.
It'd be more efficient as `std::vector` has much better memory locality than `std::set`, and fewer indirections.
If you do this, can also just reuse the `best_peers` vector, a
...
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1447521744)
In commit "Cache fanout candidates to optimize txreconciliation"
Since the cache entry `std::set<NodeId>` don't change until being replaced entirely, I believe it would be more efficient to use a `std::vector<NodeId>`, with the nodeids in sorted order. You can then use `std::binary_search` to query in that vector.
It'd be more efficient as `std::vector` has much better memory locality than `std::set`, and fewer indirections.
If you do this, can also just reuse the `best_peers` vector, a
...
💬 sipa commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1447502258)
In commit "p2p: Add transactions to reconciliation sets"
It doesn't actually matter whether we pick the lowest-`hash_key` entries or highest-`hash_key` entries to fanout to, I think? If so, can just use `std::sort(best_peers.begin(), best_peers.end());`, and drop the `cmp_by_key`.
Also, since you only care about the top `targets_size` results, I believe using `std::partial_sort` *may* be faster (but benchmark it).
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1447502258)
In commit "p2p: Add transactions to reconciliation sets"
It doesn't actually matter whether we pick the lowest-`hash_key` entries or highest-`hash_key` entries to fanout to, I think? If so, can just use `std::sort(best_peers.begin(), best_peers.end());`, and drop the `cmp_by_key`.
Also, since you only care about the top `targets_size` results, I believe using `std::partial_sort` *may* be faster (but benchmark it).
💬 sipa commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1447531912)
In commit "Cache fanout candidates to optimize txreconciliation"
Use `std::move` around `new_fanout_candidates` (or `best_peers` if you take my earlier suggestion to use a vector), to avoid a copy. If lookup the `peer_id` in it before moving, so you can return that value.
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1447531912)
In commit "Cache fanout candidates to optimize txreconciliation"
Use `std::move` around `new_fanout_candidates` (or `best_peers` if you take my earlier suggestion to use a vector), to avoid a copy. If lookup the `peer_id` in it before moving, so you can return that value.
💬 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
---
...