💬 sipa commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1447509119)
In commit "p2p: Add transactions to reconciliation set"
This value could be cached inside `TxReconciliationTracker::Impl` I think, and updated on registering/deregister, to avoid recomputating it every time?
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1447509119)
In commit "p2p: Add transactions to reconciliation set"
This value could be cached inside `TxReconciliationTracker::Impl` I think, and updated on registering/deregister, to avoid recomputating it every time?
💬 sipa commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1447401962)
If this set may contain up to 3000 (=`MAX_SET_SIZE`) elements, it may be worth using an `std::unordered_set` instead (with something like `SaltedTxidHasher` as hasher).
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1447401962)
If this set may contain up to 3000 (=`MAX_SET_SIZE`) elements, it may be worth using an `std::unordered_set` instead (with something like `SaltedTxidHasher` as hasher).
💬 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
...