💬 maflcko commented on pull request "refactor: Fix remaining clang-tidy performance-inefficient-vector errors":
(https://github.com/bitcoin/bitcoin/pull/31305#issuecomment-2500182183)
review ACK 11f3bc229ccd4b20191855fb1df882cfa6145264 🎦

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK 11f3bc229ccd
...

💬 i-am-yuvi commented on issue "Prioritize processing of peers based on their CPU usage":
(https://github.com/bitcoin/bitcoin/issues/31033#issuecomment-2500190904)
> Good questions that I do not have an answer to.
>
> I guess this should start with planting some metrics - CPU time used by each peer in the last 1, 5 and 15 minutes and assessing what a "usual" situation looks like.
>
> Specific to CPU DoS via invalid transactions: for peers that send us such transactions, maybe multiply the CPU time by a factor e.g. 10x or 100x. Or the other way around - nullify CPU time spend on definitely-good-and-positive things like new blocks or new transactions t
...

💬 maflcko commented on pull request "ci: limit max stack size to 512 KiB":
(https://github.com/bitcoin/bitcoin/pull/31367#issuecomment-2500191918)
It looks like `DEBUG=1` has an effect on this?

💬 maflcko commented on pull request "ci: limit max stack size to 512 KiB":
(https://github.com/bitcoin/bitcoin/pull/31367#issuecomment-2500196258)
> tinyformat

Pretty sure this can be ignored, as in this codebase it is not relevant, see https://github.com/c42f/tinyformat/issues/70#issuecomment-579391305

💬 vasild commented on issue "Prioritize processing of peers based on their CPU usage":
(https://github.com/bitcoin/bitcoin/issues/31033#issuecomment-2500225499)
We already keep stats for each and every peer, displayed in the `getpeerinfo` RPC, e.g. `bitcoin-cli getpeerinfo`.

💬 hodlinator commented on pull request "test: Shut down framework cleanly on RPC connection failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1858229897)
I am satisfied with the PR in it's latest revision, but if you don't consider this a nit, but rather a blocker, let me know.

💬 evgeniytar commented on issue "Discover() will not run if listening on any address with an explicit bind=0.0.0.0":
(https://github.com/bitcoin/bitcoin/issues/31293#issuecomment-2500256851)
Applied patch provided by @vasild and test run. But seems like `Discover()` indeed doesn't see IPv4. I printed `self.nodes[0].getnetworkinfo()['localaddresses']` before first assertion and it shows only IPv6 addresses.

```python3
[{'address': '2a0d:3344:34da:6410::fc5', 'port': 31001, 'score': 1}, {'address': '2a0d:3344:34da:6410:80:af5f:cf76:f4b9', 'port': 31001, 'score': 1}, {'address': '2a0d:3344:34da:6410:8b8:ca87:43b2:565f', 'port': 31001, 'score': 1}]
```

Second node displays empt
...

💬 hebasto commented on pull request "refactor: Fix remaining clang-tidy performance-unnecessary-copy-initialization errors":
(https://github.com/bitcoin/bitcoin/pull/31364#discussion_r1858230932)
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?

👍 rkrux approved a pull request: "doc, test: more ephemeral dust follow-ups"
(https://github.com/bitcoin/bitcoin/pull/31371#pullrequestreview-2461044048)
tACK 160799d9135528dbdea40690f0bb0d56c6c4803a

I agree with this change, gets rid of duplication. Successful make and functional tests.

I noticed 3 call sites at the end where the returned `sweep_tx` is not used, though it seems redundant but I don't suppose it's a big deal, just noteworthy.

Left few minor suggestions.

💬 rkrux commented on pull request "doc, test: more ephemeral dust follow-ups":
(https://github.com/bitcoin/bitcoin/pull/31371#discussion_r1858216903)
```
fee_per_output=dust_tx_fee
```

Since technically it's the fee per output, let's call it `dust_tx_fee_per_output`? Otherwise reading the function signatures gives an impression that `dust_tx_fee` is for the whole tx.

💬 rkrux commented on pull request "doc, test: more ephemeral dust follow-ups":
(https://github.com/bitcoin/bitcoin/pull/31371#discussion_r1858219028)
```diff
- def create_ephemeral_dust_package(self, *, tx_version, dust_tx_fee=0, dust_value=0, num_dust_outputs=1, extra_sponsors=None):
+ def create_ephemeral_dust_package(self, *, tx_version=3, dust_tx_fee=0, dust_value=0, num_dust_outputs=1, extra_sponsors=None):
```

Because v3 is the most common in the file.

💬 rkrux commented on pull request "doc, test: more ephemeral dust follow-ups":
(https://github.com/bitcoin/bitcoin/pull/31371#discussion_r1858220783)
```diff
- def create_ephemeral_dust_package(self, *, tx_version, dust_tx_fee=0, dust_value=0, num_dust_outputs=1, extra_sponsors=None):
+ def create_ephemeral_dust_package(self, *, tx_version, dust_tx_fee=0, dust_value=0, num_dust_outputs=1, extra_sponsor_coins=None):
```

Nit for verbosity in function signature.

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