💬 maflcko commented on pull request "refactor: Fix remaining clang-tidy performance-unnecessary-copy-initialization errors":
(https://github.com/bitcoin/bitcoin/pull/31364#discussion_r1858243815)
> The `prev_spk` variable is passed to the `CTxOut` ctor, which takes a copy rather than a const reference.
>
> This raises a concern about the correctness of the change, doesn't it?

It is possible to create a copy from a const reference. Previously a copy was taken from a mutable reference.

The change here only affects the code in this scope, to avoid a second copy in this line.

Not sure what your question is, can you rephrase it?

💬 l0rinc commented on pull request "refactor: Fix remaining clang-tidy performance-unnecessary-copy-initialization errors":
(https://github.com/bitcoin/bitcoin/pull/31364#discussion_r1858253875)
@hebasto are you asking because [CScript is trivially copyable](https://github.com/bitcoin/bitcoin/blob/master/src/prevector.h#L38), but https://clang.llvm.org/extra/clang-tidy/checks/performance/unnecessary-copy-initialization.html claims
> Finds local variable declarations that are initialized using the copy constructor of a non-trivially-copyable type but it would suffice to obtain a const reference.
?

👍 hebasto approved a pull request: "refactor: Fix remaining clang-tidy performance-inefficient-vector errors"
(https://github.com/bitcoin/bitcoin/pull/31305#pullrequestreview-2461110080)
ACK 11f3bc229ccd4b20191855fb1df882cfa6145264, tested with clang 19.1.5 + clang-tidy.

💬 amir13l commented on pull request "refactor: Fix remaining clang-tidy performance-inefficient-vector errors":
(https://github.com/bitcoin/bitcoin/pull/31305#issuecomment-2500296811)
Amir1361

در تاریخ سه‌شنبه ۲۶ نوامبر ۲۰۲۴، ۱۴:۲۴ Hennadii Stepanov <
***@***.***> نوشت:

> ***@***.**** approved this pull request.
>
> ACK 11f3bc2
> <https://github.com/bitcoin/bitcoin/commit/11f3bc229ccd4b20191855fb1df882cfa6145264>,
> tested with clang 19.1.5 + clang-tidy.
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/bitcoin/bitcoin/pull/31305#pullrequestreview-2461110080>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/BN
...

💬 hebasto commented on pull request "refactor: Fix remaining clang-tidy performance-unnecessary-copy-initialization errors":
(https://github.com/bitcoin/bitcoin/pull/31364#discussion_r1858266544)
> The `prev_spk` variable is passed to the `CTxOut` ctor...

The ctor treats `CScript scriptPubKeyIn` parameter as `const`. Everything is OK.

Sorry for the noise.

👍 hebasto approved a pull request: "refactor: Fix remaining clang-tidy performance-unnecessary-copy-initialization errors"
(https://github.com/bitcoin/bitcoin/pull/31364#pullrequestreview-2461125224)
ACK 3305972f7bfd78181566e4297891c2dd7cae0f2b, tested with clang 19.1.5 + clang-tidy.

💬 l0rinc commented on pull request "refactor: Fix remaining clang-tidy performance-unnecessary-copy-initialization errors":
(https://github.com/bitcoin/bitcoin/pull/31364#discussion_r1858274475)
It does? `SetNull` can clear it internally, but it's copied in the constructor, that's why it's safe, isn't it?

💬 vasild commented on pull request "fuzz: set the output argument of FuzzedSock::Accept()":
(https://github.com/bitcoin/bitcoin/pull/31316#issuecomment-2500321236)
Hmm, to have it included in https://github.com/bitcoin/bitcoin/pull/28584 - yes, I should do that. But also to have it standalone here because this is fixing a bug in `master` and shouldn't be tied to the fate of #28584.

💬 amir13l commented on pull request "fuzz: set the output argument of FuzzedSock::Accept()":
(https://github.com/bitcoin/bitcoin/pull/31316#issuecomment-2500329263)
[Uploading google_terms_of_service_fa.pdf…]()

🤔 brunoerg reviewed a pull request: "Add coin-grinder example test"
(https://github.com/bitcoin/bitcoin/pull/31352#pullrequestreview-2461195198)
According to https://corecheck.dev/bitcoin/bitcoin/pulls/31352, this PR does not bring any new test coverage for coin selection. What are you exactly testing here that is not covered yet? If you simply are reproducing the example code, so I tend to concept NACK on adding this to the test, the example is only an example.

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

Consider `fanout=false` because the peer has too many parents (cut the tail from `fanout_with_ancestors`).
Now, we fail to add a transaction to the set (whatever reason, say set too full), and we're now going to flood a child while reconciling the parents again?

Perhaps `AddToSet` result could be handled more carefully instead (or have a todo/comment, this is a corner case of a corner case of course).

💬 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?