💬 theuni commented on pull request "build: always set `-g -O2` in `CORE_CXXFLAGS`":
(https://github.com/bitcoin/bitcoin/pull/29205#discussion_r1447482492)
Looks good, thanks. Looking at 7b00595d335915dc2bf856e3569115996381a402 it's clear that this is what was intended.
(https://github.com/bitcoin/bitcoin/pull/29205#discussion_r1447482492)
Looks good, thanks. Looking at 7b00595d335915dc2bf856e3569115996381a402 it's clear that this is what was intended.
💬 theuni commented on pull request "build: LLD based macOS toolchain":
(https://github.com/bitcoin/bitcoin/pull/21778#issuecomment-1885015028)
Re libtool: As far as I can see, it's only used for miniupnpc/libnatpnp?
If that's the case, I don't think it's worth the trouble of having users work around it.
I'd suggest we either:
- patch out `libtool` in the Makefiles and use `ar`, then drop our `libtool` machinery entirely.
- Switch to their cmake builds where I assume `libtool` is unused.
I have a [branch with commits to do the latter](https://github.com/theuni/bitcoin/commits/depends-cmake/) as part of a larger CMake switch,
...
(https://github.com/bitcoin/bitcoin/pull/21778#issuecomment-1885015028)
Re libtool: As far as I can see, it's only used for miniupnpc/libnatpnp?
If that's the case, I don't think it's worth the trouble of having users work around it.
I'd suggest we either:
- patch out `libtool` in the Makefiles and use `ar`, then drop our `libtool` machinery entirely.
- Switch to their cmake builds where I assume `libtool` is unused.
I have a [branch with commits to do the latter](https://github.com/theuni/bitcoin/commits/depends-cmake/) as part of a larger CMake switch,
...
💬 fanquake commented on pull request "build: LLD based macOS toolchain":
(https://github.com/bitcoin/bitcoin/pull/21778#issuecomment-1885034377)
> Edit: If there's interest in the above I can PR it separately.
Sure, I can rebase on top of that PR, if you want to open it.
(https://github.com/bitcoin/bitcoin/pull/21778#issuecomment-1885034377)
> Edit: If there's interest in the above I can PR it separately.
Sure, I can rebase on top of that PR, if you want to open it.
💬 sipa commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1447390077)
In "refactor: Add a pre-mutexed version of IsPeerRegistered":
I'd prefer to swap the names (= have an external `IsPeerRegistered`, and an internal `IsPeerRegisteredInternal`), as external callers ought to only care about the external one. Also is it possible to make the internal one `private`?
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1447390077)
In "refactor: Add a pre-mutexed version of IsPeerRegistered":
I'd prefer to swap the names (= have an external `IsPeerRegistered`, and an internal `IsPeerRegisteredInternal`), as external callers ought to only care about the external one. Also is it possible to make the internal one `private`?
💬 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.