💬 maflcko commented on pull request "bench: wallet, fix change position out of range error":
(https://github.com/bitcoin/bitcoin/pull/29065#issuecomment-1854610666)
> This seems important to fix to me.
I tried on current master with `clang-13` and `valgrind` on Jammy, but it seems to pass.
Maybe it is a bug in your g++?
```
# git log -1 && valgrind ./src/bench/bench_bitcoin -filter=WalletCreateTxUsePresetInputsAndCoinSelection
commit 9f0f83d6509a214b827f5110c0f857b494ae854c (HEAD, origin/master, origin/HEAD)
Merge: 019ec8a 37c75c5
Author: Ava Chow <github@achow101.com>
Date: Wed Dec 13 12:38:11 2023 -0500
Merge bitcoin/bitcoin#29065
...
(https://github.com/bitcoin/bitcoin/pull/29065#issuecomment-1854610666)
> This seems important to fix to me.
I tried on current master with `clang-13` and `valgrind` on Jammy, but it seems to pass.
Maybe it is a bug in your g++?
```
# git log -1 && valgrind ./src/bench/bench_bitcoin -filter=WalletCreateTxUsePresetInputsAndCoinSelection
commit 9f0f83d6509a214b827f5110c0f857b494ae854c (HEAD, origin/master, origin/HEAD)
Merge: 019ec8a 37c75c5
Author: Ava Chow <github@achow101.com>
Date: Wed Dec 13 12:38:11 2023 -0500
Merge bitcoin/bitcoin#29065
...
💬 mzumsande commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#issuecomment-1854630701)
> What do you think?
To get an estimate about performance, I wrote a simple benchmark, see https://github.com/mzumsande/bitcoin/commit/22f98d6f54042b3e8ee41889ceb362f6355e4fa0. On my computer (neither particularly fast nor slow) I get `~1000op/s` per wtxid to call `ShouldFanoutTo()` for all 120 peers, so a batch of 3000 txs should take ~3s time in `ShouldFanoutTo`.
So I think that caching could make sense.
I wonder if there is there a good way to restrict this map to wtxids that are stil
...
(https://github.com/bitcoin/bitcoin/pull/28765#issuecomment-1854630701)
> What do you think?
To get an estimate about performance, I wrote a simple benchmark, see https://github.com/mzumsande/bitcoin/commit/22f98d6f54042b3e8ee41889ceb362f6355e4fa0. On my computer (neither particularly fast nor slow) I get `~1000op/s` per wtxid to call `ShouldFanoutTo()` for all 120 peers, so a batch of 3000 txs should take ~3s time in `ShouldFanoutTo`.
So I think that caching could make sense.
I wonder if there is there a good way to restrict this map to wtxids that are stil
...
👍 ryanofsky approved a pull request: "refactor: Remove pre-C++20 code, fs::path cleanup"
(https://github.com/bitcoin/bitcoin/pull/29040#pullrequestreview-1780346500)
Code review ACK 856c88776f8486446602476a1c9e133ac0cff510
I suggested a scripted diff followup, but all these changes seem good, and the followup could happen later. I am also a little disturbed that 856c88776f8486446602476a1c9e133ac0cff510 doesn't begin with `fa` but I think I can get over it.
(https://github.com/bitcoin/bitcoin/pull/29040#pullrequestreview-1780346500)
Code review ACK 856c88776f8486446602476a1c9e133ac0cff510
I suggested a scripted diff followup, but all these changes seem good, and the followup could happen later. I am also a little disturbed that 856c88776f8486446602476a1c9e133ac0cff510 doesn't begin with `fa` but I think I can get over it.
💬 ryanofsky commented on pull request "refactor: Remove pre-C++20 code, fs::path cleanup":
(https://github.com/bitcoin/bitcoin/pull/29040#discussion_r1425838080)
In commit "ArgsManager: return path by value from GetBlocksDirPath()" (856c88776f8486446602476a1c9e133ac0cff510)
This change looks ok, but it seems like the clang-18 warning is a false alarm. We know know m_cached_blocks_path will never change after it is written, and we always acquire the cs_args lock before reading m_cached_blocks_path so we can be sure that by the time we do read it we are necessarily seeing the latest value. I don't see how there could be a race condition here, because it
...
(https://github.com/bitcoin/bitcoin/pull/29040#discussion_r1425838080)
In commit "ArgsManager: return path by value from GetBlocksDirPath()" (856c88776f8486446602476a1c9e133ac0cff510)
This change looks ok, but it seems like the clang-18 warning is a false alarm. We know know m_cached_blocks_path will never change after it is written, and we always acquire the cs_args lock before reading m_cached_blocks_path so we can be sure that by the time we do read it we are necessarily seeing the latest value. I don't see how there could be a race condition here, because it
...
💬 ryanofsky commented on pull request "refactor: Remove pre-C++20 code, fs::path cleanup":
(https://github.com/bitcoin/bitcoin/pull/29040#discussion_r1425796516)
In commit "refactor: Remove pre-C++20 fs code" (fa3d9304e80c214c8b073f12a7f4b08c5a94af04)
re: https://github.com/bitcoin/bitcoin/pull/29040#discussion_r1422242907
> If a u8string was returned here, UniValue would have to be updated to take it as well, in which case the conversion would need to happen there.
In theory, no conversions are actually required, right? In theory UniValue could use u8string instead of string internally, and other parts of the codebase could too, so nothing woul
...
(https://github.com/bitcoin/bitcoin/pull/29040#discussion_r1425796516)
In commit "refactor: Remove pre-C++20 fs code" (fa3d9304e80c214c8b073f12a7f4b08c5a94af04)
re: https://github.com/bitcoin/bitcoin/pull/29040#discussion_r1422242907
> If a u8string was returned here, UniValue would have to be updated to take it as well, in which case the conversion would need to happen there.
In theory, no conversions are actually required, right? In theory UniValue could use u8string instead of string internally, and other parts of the codebase could too, so nothing woul
...
💬 ryanofsky commented on pull request "Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly":
(https://github.com/bitcoin/bitcoin/pull/28051#discussion_r1425860646)
> Nit in the last commit: Shouldn't new node code be in the node namespace? Otherwise it will have to be touched again when it is moved into the node namespace later?
This is true, but it is not different from other init.h methods. Logically init.h and init.cpp belong in node subdirectory and should use the node namespace, so I didn't want to make this one function stand out. This seemed like the most straightforward approach for now.
(https://github.com/bitcoin/bitcoin/pull/28051#discussion_r1425860646)
> Nit in the last commit: Shouldn't new node code be in the node namespace? Otherwise it will have to be touched again when it is moved into the node namespace later?
This is true, but it is not different from other init.h methods. Logically init.h and init.cpp belong in node subdirectory and should use the node namespace, so I didn't want to make this one function stand out. This seemed like the most straightforward approach for now.
🤔 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.