💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1858312000)
```
auto assign_key = [](const TxReconciliationState& peer_state, NodeId node_id, CSipHasher randomizer, std::vector<std::pair<uint64_t, NodeId>>& weighted_peers) {
uint64_t hash_key = randomizer.Write(node_id).Finalize();
weighted_peers.emplace_back(hash_key, node_id);
};

for (const auto& [node_id, op_peer_state]: m_states) {
const auto peer_state = std::get_if<TxReconciliationState>(&op_peer_state);
if (peer_state
...

💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1858243236)
```
// TODO: now rename sorted_inbounds to weighted_inbounds
std::vector<NodeId> fanout_targets(inbounds_target_size + outbounds_target_size);
auto collect_fanout_targets = [&fanout_targets](std::vector<std::pair<uint64_t, NodeId>> weighted_peers, const size_t target_size) {
const size_t result_size = std::min(target_size, weighted_peers.size());
if (result_size == 0) return;
std::nth_element(weighted_peers.begin(), weighted_peers
...

💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1857918168)
840a4e14f2d833717c87a54ec2707a92d5131ada

been thinking about the use of a delayed set here.

We apply the delay only on addition, but not on fanouting parents (or counting them), neither on removing stuff from the sets (say at receiving a transaction).

For me the choice is pretty arbitrary (and probably fine to save the code complexity for this rare corner case), but perhaps we would benefit from better documentation around it.

💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1857884471)
https://github.com/bitcoin/bitcoin/commit/0092f70314b8d59b41ff4bd2bc15a71f954668d4

consider defining this variable inside the loop rather than cleaning it after each iteration?

💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1857868518)
1dce9aedc59eba2b623c7785f59430a586fbd833

the variable `found` could be dropped

💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1857872449)
What's the plan? I still think `force_relay` is unnecessary here.

💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1858323811)
Better check if the size is not 1 already? It's currently safe, but could cause bugs in the future.

💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1857927041)
fc06786f32b2fc82c9571ebde67b4ff54f4ad825

this wrapper is not needed anymore. The Registered check already happens at the caller site. Just drop it and expose `GetFanoutTargets`?

💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1857898012)
This is slightly changed later in 5996d473434d364cff937ae89ffd95cd09c5d1a7. `AddToSetResult::Failed` still not handled though?

👋 hebasto's pull request is ready for review: "ci: Test cross-built Windows executables on Windows natively"
(https://github.com/bitcoin/bitcoin/pull/31176)

💬 hebasto commented on pull request "ci: Test cross-built Windows executables on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31176#issuecomment-2500496959)
Reworked. The GHA job now re-uses `ci/test/00_setup_env_win64.sh`.

Added caching for `depends/built` and Ccache.

💬 hebasto commented on pull request "ci: Test cross-built Windows executables on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31176#discussion_r1858340882)
The caching has been [added](https://github.com/bitcoin/bitcoin/pull/31176#issuecomment-2500496959).

💬 hebasto commented on pull request "ci: Test cross-built Windows executables on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31176#discussion_r1858341129)
[Reworked](https://github.com/bitcoin/bitcoin/pull/31176#issuecomment-2500496959).

💬 hebasto commented on pull request "ci: Test cross-built Windows executables on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31176#discussion_r1858341637)
[Reworked](https://github.com/bitcoin/bitcoin/pull/31176#issuecomment-2500496959).

💬 Sjors commented on pull request "guix: swap `moreutils` for just `sponge`":
(https://github.com/bitcoin/bitcoin/pull/31323#issuecomment-2500514151)
Getting the same guix hashes on `x86_64` as @TheCharlatan, still working on my amd64 guix system.

💬 TheCharlatan commented on pull request "kernel, refactor: Replace `goto` with RAII in `bitcoin-chainstate`":
(https://github.com/bitcoin/bitcoin/pull/31362#issuecomment-2500562746)
There is some context on both the usage of `goto` and tests in the original PR https://github.com/bitcoin/bitcoin/pull/24304#discussion_r804072128, and https://github.com/bitcoin/bitcoin/pull/24304#issuecomment-1042814237 and https://github.com/bitcoin/bitcoin/pull/24304#issuecomment-1042920837 respectively. If you look at aj's original suggested [solution](https://github.com/ajtowns/bitcoin/commit/5d7d8de713961a88a40c236346bbfad4ad2906f0#diff-04e685224f1ac5bfd91d47d8d7528a2e44f94fab5535d4b6b5af
...

💬 hebasto commented on pull request "refactor: Fix remaining clang-tidy performance-unnecessary-copy-initialization errors":
(https://github.com/bitcoin/bitcoin/pull/31364#discussion_r1858402086)
I think so.

💬 willcl-ark commented on pull request "dbwrapper: Bump LevelDB max file size to 128 MiB to avoid system slowdown from high disk cache flush rate":
(https://github.com/bitcoin/bitcoin/pull/30039#issuecomment-2500591909)
Just wanted to clarify that I'd love to see this get in sooner rather than later, as it can provide a valuable speedup and I'd be ready to ACK with a reduced (default) size selected, for the reasons I outlined above.

We can in tandem discuss making this value configurable over in #30059.

💬 Sjors commented on pull request "net, net_processing: additional and consistent disconnect logging":
(https://github.com/bitcoin/bitcoin/pull/28521#discussion_r1858444085)
Tempted to leave the PR as is for now as it's already quite large.

💬 Sjors commented on pull request "net, net_processing: additional and consistent disconnect logging":
(https://github.com/bitcoin/bitcoin/pull/28521#discussion_r1858444169)
Reverted this change (and its corresponding test).