💬 0xB10C commented on pull request "tracing: network connection tracepoints":
(https://github.com/bitcoin/bitcoin/pull/25832#discussion_r1840483719)
I think this can be handled on the user side. The bpftrace tool cuts of strings after 64 chars by default and bcc and libbpf both have [bpf_probe_read_user_str()](https://docs.ebpf.io/linux/helper-function/bpf_probe_read_user_str/), which would replace the last char with `\0` on overflow.

I haven't had any problems with this using the tracepoints in Python and Rust. I did a quick check with a C program, and using `bpf_probe_read_user_str()` seems fine: https://github.com/0xB10C/libbpf-bootst
...

💬 polespinasa commented on pull request "wallet, rpc: Settxfeerate":
(https://github.com/bitcoin/bitcoin/pull/31278#issuecomment-2473845095)
@fjahr @ismaelsadeeq ``txfeerate`` is now deprecated, but I kept it hidden. IMO it makes sense to have a deprecated rpc call hidden as only users who were using it before should know about it and may need info, they can continue using ``help settxfee``

💬 andrewtoth commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1840503929)
I suppose I can `addnode` my trusted node for now, but I will be leaking other outbound connections.
I'm not sure how to handle my case unless we don't consider the ephemeral broadcast connections as true "connections" in the sense of `connect` option.

💬 andrewtoth commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1840504627)
I suppose I can `addnode` my trusted node for now, but I will be leaking other outbound connections.
I'm not sure how to handle my case unless we don't consider the ephemeral broadcast connections as true "connections" in the sense of `connect` option.

💬 m3dwards commented on pull request "ci: skip Github CI on branch pushes for forks":
(https://github.com/bitcoin/bitcoin/pull/30487#issuecomment-2473926492)
ACK 8610bcef9d030013f9e36cffe0c58dd2cfe85d66

💬 l0rinc commented on pull request "optimization: change XOR obfuscation key from `std::vector<std::byte>{8}` to `uint64_t`":
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2473947449)
> I understand you were burned by endianness but I disagree that it's worth sacrificing readability where endianness is a non-issue.

Thanks @hodlinator for the suggestions, I tried them all, but in the end decided that I value consistency more than coming up with a separate solution for each test case. These are ugly, I agree, but at least they're testing the setup we're using in prod.
I did however change the hard-coded 8 values to `sizeof xor_key` (for memcpy) or `sizeof(uint64_t)` for vec
...

💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads ~17% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#issuecomment-2473948407)
For later blocks where cache misses are much more common, this change has an even bigger impact.
This benchmark report shows a 40% speedup measuring from blocks 840k to 850k. Also, compare flamegraphs of master and this branch, where the latter has 15 worker threads fetching coins from disk.
https://bitcoin-dev-tools.github.io/benchcoin/results/pr-19/11798124132/index.html

🤔 instagibbs reviewed a pull request: "Improve parallel script validation error debug logging"
(https://github.com/bitcoin/bitcoin/pull/31112#pullrequestreview-2430699423)
first pass, looks good but I want to spend more time making sure queue and control flow changes are correct

💬 instagibbs commented on pull request "Improve parallel script validation error debug logging":
(https://github.com/bitcoin/bitcoin/pull/31112#discussion_r1838775296)
note: this is removed because it's causing a 2nd error that may mask the first's error being reported. Seems like a mistake in the original test code? Agree with its removal.

💬 instagibbs commented on pull request "Improve parallel script validation error debug logging":
(https://github.com/bitcoin/bitcoin/pull/31112#discussion_r1840601923)
asserting on logging is subpar I know, but we could do tighter assertions now if we want to check we're formatting properly:
```
diff --git a/test/functional/feature_cltv.py b/test/functional/feature_cltv.py
index 557fcc7cea..220a6f8b25 100755
--- a/test/functional/feature_cltv.py
+++ b/test/functional/feature_cltv.py
@@ -172,11 +172,13 @@ class BIP65Test(BitcoinTestFramework):
# Now we verify that a block with this transaction is also invalid.
block.vtx[1] = sp
...

💬 instagibbs commented on pull request "Improve parallel script validation error debug logging":
(https://github.com/bitcoin/bitcoin/pull/31112#discussion_r1838787662)
maybe unnecessary if the logging includes the actual block with witness commitment?

```Suggestion
const auto debug_str = strprintf("input %i of %s (wtxid %s), spending %s:%i", nIn, ptxTo->GetHash().ToString(), ptxTo->GetWitnessHash().ToString(), ptxTo->vin[nIn].prevout.hash.ToString(), ptxTo->vin[nIn].prevout.n);
```

💬 instagibbs commented on pull request "Improve parallel script validation error debug logging":
(https://github.com/bitcoin/bitcoin/pull/31112#discussion_r1838774485)
nit suggestion
`s/results2/policy_only_result/` ?

💬 instagibbs commented on pull request "Improve parallel script validation error debug logging":
(https://github.com/bitcoin/bitcoin/pull/31112#discussion_r1838777337)
73417066867ea7920647054eb0752e0dd3100de7 seems to remove the last usages of `-par=1`. I think it behooves us to keep coverage somewhere since IIUC par=1 means different control flow.

💬 Sjors commented on pull request "ci: skip Github CI on branch pushes for forks":
(https://github.com/bitcoin/bitcoin/pull/30487#issuecomment-2473986387)
Just repeating my "note to maintainers" from the description: like with Cirrus, `SKIP_BRANCH_PUSH=true` needs to be set for the GUI repo to maintain existing behaviour.

💬 maflcko commented on pull request "tinyformat: Add compile-time checking for literal format strings":
(https://github.com/bitcoin/bitcoin/pull/31174#issuecomment-2473989052)
re-ACK fe39acf88ff552bfc4a502c99774375b91824bb1 🕐

<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: re-ACK fe39acf88ff552bfc4a5
...

💬 maflcko commented on pull request "test: Introduce ensure_for helper":
(https://github.com/bitcoin/bitcoin/pull/30893#issuecomment-2473989953)
re-ACK 111465d72dd35e42361fc2a089036f652417ed37 🍋

<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: re-ACK 111465d72dd35e42361f
...

💬 l0rinc commented on pull request "tinyformat: Add compile-time checking for literal format strings":
(https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1840648404)
it could be more readable and a lot shorter if we extract the flags as a variable and delete the comment stating the same - what do you think?

💬 maflcko commented on pull request "bench: add support for custom data directory":
(https://github.com/bitcoin/bitcoin/pull/31000#issuecomment-2473998823)
re-ACK fa66e0887ca1a1445d8b18ba1fadb12b2d911048

Only change is removing unused `g_rng_temp_path`

💬 l0rinc commented on pull request "tinyformat: Add compile-time checking for literal format strings":
(https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1840651076)
@hodlinator, @maflcko, what do you think?
I can ack without this as well, but I'd prefer reducing duplication.

💬 maflcko commented on pull request "tinyformat: Add compile-time checking for literal format strings":
(https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1840684127)
My preference would be to leave style-only nits to a follow-up, especially given that they will be temporary anyway until C++23. This pull request is basically ready for two weeks now, with more than 50 comments. Unless there are any real issues or bugs with the code, and a foce-push needs to happen anyway, I don't really see the value of asking reviewers to go through more comments and code changes, some of which don't even compile.