🤔 sipa reviewed a pull request: "p2p: Fill reconciliation sets (Erlay)"
(https://github.com/bitcoin/bitcoin/pull/28765#pullrequestreview-1780420519)
Not a full review; mostly performance improvement suggestions as I saw there was a suggestion to cache things. Before considering that, it may be worth benchmarking if that's still relevant with these review comments addressed.
(https://github.com/bitcoin/bitcoin/pull/28765#pullrequestreview-1780420519)
Not a full review; mostly performance improvement suggestions as I saw there was a suggestion to cache things. Before considering that, it may be worth benchmarking if that's still relevant with these review comments addressed.
💬 sipa commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1425854190)
Constructing the full set of fanout candidates any time you want to consider relaying anything to any node has O(n^2 log n) scaling (you'll call `GetFanoutTargets` O(n) times, and each does O(n log n) work going over all nodes), I believe.
That may or may not matter, but I'd consider at least one of these variants:
* Cache the `GetFanoutTargets` result per transaction as discussed elsewhere in this PR, making it O(n log n) only.
* Instead of computing an `std::set<NodeId>` in `GetFanoutTarg
...
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1425854190)
Constructing the full set of fanout candidates any time you want to consider relaying anything to any node has O(n^2 log n) scaling (you'll call `GetFanoutTargets` O(n) times, and each does O(n log n) work going over all nodes), I believe.
That may or may not matter, but I'd consider at least one of these variants:
* Cache the `GetFanoutTargets` result per transaction as discussed elsewhere in this PR, making it O(n log n) only.
* Instead of computing an `std::set<NodeId>` in `GetFanoutTarg
...
💬 sipa commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1425857575)
Nit: `const size_t targets_size = integral_part + add_extra`.
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1425857575)
Nit: `const size_t targets_size = integral_part + add_extra`.
💬 sipa commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1425842632)
The `deterministic_randomizer_with_wtxid` variable could be `const CSipHasher&` here (to make it clear that this object is only ever used as a starting point for (independent) hashes).
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1425842632)
The `deterministic_randomizer_with_wtxid` variable could be `const CSipHasher&` here (to make it clear that this object is only ever used as a starting point for (independent) hashes).
💬 sipa commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1425845551)
I believe it would be significantly faster to make this an `std::vector<std::pair<uint64_t, NodeId>>`, `.reserve()` it once, fill it up like in the loop below, and then `std::sort` it in-place using `cmp_by_key`. This has far better memory locality, less allocation overhead, and ought to have the same asymptotic complexity (O(n log n)).
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1425845551)
I believe it would be significantly faster to make this an `std::vector<std::pair<uint64_t, NodeId>>`, `.reserve()` it once, fill it up like in the loop below, and then `std::sort` it in-place using `cmp_by_key`. This has far better memory locality, less allocation overhead, and ought to have the same asymptotic complexity (O(n log n)).
💬 sipa commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1425862263)
Pass `indexed_state` by reference. This is creating a copy of the entire `TxReconciliationState` for every element in `m_states`.
`[](const auto& indexed_state) { ... }` works.
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1425862263)
Pass `indexed_state` by reference. This is creating a copy of the entire `TxReconciliationState` for every element in `m_states`.
`[](const auto& indexed_state) { ... }` works.
💬 sipa commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1425866501)
`best_peers.emplace(hash_key, indexed_state.first);` (avoids a temporary `std::pair` object in the caller).
(it'd become `best_peers.emplace_back(hash_key indexed_state.first);` if `best_peers` is converted to a vector as suggested above.
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1425866501)
`best_peers.emplace(hash_key, indexed_state.first);` (avoids a temporary `std::pair` object in the caller).
(it'd become `best_peers.emplace_back(hash_key indexed_state.first);` if `best_peers` is converted to a vector as suggested above.
💬 sipa commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1425865073)
An alternative which may be slightly faster (unsure, depends on how fast `std::modf` is):
`const size_t targets_size = ((deterministic_randomizer_with_wtxid.Finalize() & 0xFFFFFFFF) + uint64_t(limit * 0x100000000)) >> 32;`
It has lower precision, but I suspect 32 bits is more than sufficient here.
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1425865073)
An alternative which may be slightly faster (unsure, depends on how fast `std::modf` is):
`const size_t targets_size = ((deterministic_randomizer_with_wtxid.Finalize() & 0xFFFFFFFF) + uint64_t(limit * 0x100000000)) >> 32;`
It has lower precision, but I suspect 32 bits is more than sufficient here.
💬 sipa commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1425856345)
I believe the appropriate type in this case is `CSipHasher&& deterministic_randomizer`. This avoids a copy, allows the local variable inside `ShouldFanoutTo` to be modified, and the caller is constructing a fresh `CSipHasher` anyway.
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1425856345)
I believe the appropriate type in this case is `CSipHasher&& deterministic_randomizer`. This avoids a copy, allows the local variable inside `ShouldFanoutTo` to be modified, and the caller is constructing a fresh `CSipHasher` anyway.
💬 sipa commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1425870404)
Use `const auto& [cur_peer_id, cur_peer] : m_peer_map` here. You're making a copy of the `PeerRef` object for every iteration here (which isn't the end of the world, it's just a shared pointer, but copying those does involve an atomic memory operation).
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1425870404)
Use `const auto& [cur_peer_id, cur_peer] : m_peer_map` here. You're making a copy of the `PeerRef` object for every iteration here (which isn't the end of the world, it's just a shared pointer, but copying those does involve an atomic memory operation).
🤔 amitiuttarwar reviewed a pull request: "p2p: attempt to fill full outbound connection slots with peers that support tx relay"
(https://github.com/bitcoin/bitcoin/pull/28538#pullrequestreview-1780458476)
ACK d636e38d79a4c3950da91090b1f787163f11e24d
(https://github.com/bitcoin/bitcoin/pull/28538#pullrequestreview-1780458476)
ACK d636e38d79a4c3950da91090b1f787163f11e24d
💬 amitiuttarwar commented on pull request "p2p: attempt to fill full outbound connection slots with peers that support tx relay":
(https://github.com/bitcoin/bitcoin/pull/28538#discussion_r1425865265)
hm, I think this comment can be a bit misleading for people who don't readily have the full context. the setup tests peers who set the version relay bool to false, indicating they don't want tx messages. if they are running bitcoind, that means they are running in blocksonly mode, but from the POV of the tests we don't actually know that. I think it could be helpful to clarify that in the comment here, and then can continue to use the var naming of "blocksonly" in the test.
(https://github.com/bitcoin/bitcoin/pull/28538#discussion_r1425865265)
hm, I think this comment can be a bit misleading for people who don't readily have the full context. the setup tests peers who set the version relay bool to false, indicating they don't want tx messages. if they are running bitcoind, that means they are running in blocksonly mode, but from the POV of the tests we don't actually know that. I think it could be helpful to clarify that in the comment here, and then can continue to use the var naming of "blocksonly" in the test.
💬 amitiuttarwar commented on pull request "p2p: attempt to fill full outbound connection slots with peers that support tx relay":
(https://github.com/bitcoin/bitcoin/pull/28538#discussion_r1425878915)
> I added a refactor commit renaming ForEachNode() to ForEachFullyConnectedNode() and removing the duplicate checks / comments.
I like it, I find it clear
(https://github.com/bitcoin/bitcoin/pull/28538#discussion_r1425878915)
> I added a refactor commit renaming ForEachNode() to ForEachFullyConnectedNode() and removing the duplicate checks / comments.
I like it, I find it clear
💬 aureleoules commented on pull request "bench: wallet, fix change position out of range error":
(https://github.com/bitcoin/bitcoin/pull/29065#issuecomment-1854701221)
> Maybe it is a bug in your g++?
Right maybe, I just tested with `g++ 13.2.0` and it runs fine now. No worries then.
(https://github.com/bitcoin/bitcoin/pull/29065#issuecomment-1854701221)
> Maybe it is a bug in your g++?
Right maybe, I just tested with `g++ 13.2.0` and it runs fine now. No worries then.
💬 maflcko commented on pull request "fuzz: set `m_fallback_fee` and `m_fee_mode` in `wallet_fees` target":
(https://github.com/bitcoin/bitcoin/pull/29076#issuecomment-1854708178)
Could squash this easy change into one commit?
(https://github.com/bitcoin/bitcoin/pull/29076#issuecomment-1854708178)
Could squash this easy change into one commit?
💬 brunoerg commented on pull request "fuzz: set `m_fallback_fee` and `m_fee_mode` in `wallet_fees` target":
(https://github.com/bitcoin/bitcoin/pull/29076#issuecomment-1854718328)
> Could squash this easy change into one commit?
Sure, done!
(https://github.com/bitcoin/bitcoin/pull/29076#issuecomment-1854718328)
> Could squash this easy change into one commit?
Sure, done!
💬 shovas commented on issue "Use of a wallet shouldn't be blocked in prune mode ("wallet loading failed... beyond pruned data")":
(https://github.com/bitcoin/bitcoin/issues/27188#issuecomment-1854796784)
> Would be good to know why your node is aborting on a bad block. Maybe you have a storage issue? Could you share the logs? (here or in a new issue is fine, just tag me when you do it).
So far I have been using USB storage (128GB SanDisk Extreme Pro USB and a 1TB ADATA external ssd) whereas I have now successfully downloaded the complete blockchain to internal nvme fine, something that failed many times when trying to use a usb device as storage. Two machines, both Windows 10.
I am running
...
(https://github.com/bitcoin/bitcoin/issues/27188#issuecomment-1854796784)
> Would be good to know why your node is aborting on a bad block. Maybe you have a storage issue? Could you share the logs? (here or in a new issue is fine, just tag me when you do it).
So far I have been using USB storage (128GB SanDisk Extreme Pro USB and a 1TB ADATA external ssd) whereas I have now successfully downloaded the complete blockchain to internal nvme fine, something that failed many times when trying to use a usb device as storage. Two machines, both Windows 10.
I am running
...
💬 kashifs commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1425998172)
Perhaps. That would make it consistent with const CAmount& selection_target. It's my understanding that the documentation should exactly match the function definition. Is there another way to look at this?
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1425998172)
Perhaps. That would make it consistent with const CAmount& selection_target. It's my understanding that the documentation should exactly match the function definition. Is there another way to look at this?
💬 murchandamus commented on pull request "fuzz: coinselection, improve `min_viable_change`/`change_output_size`":
(https://github.com/bitcoin/bitcoin/pull/28372#issuecomment-1855053056)
Just kicked off 10×10h of fuzzing, will take a look tomorrow
(https://github.com/bitcoin/bitcoin/pull/28372#issuecomment-1855053056)
Just kicked off 10×10h of fuzzing, will take a look tomorrow
💬 maflcko commented on pull request "[WIP] C++20 std::views::reverse":
(https://github.com/bitcoin/bitcoin/pull/28687#issuecomment-1855414135)
Interesting that the guix macOS build passed, given that it is on clang-15, no?
(https://github.com/bitcoin/bitcoin/pull/28687#issuecomment-1855414135)
Interesting that the guix macOS build passed, given that it is on clang-15, no?