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

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1840691067)
Just an idea - if the trusted node is accepting inbound Tor connections, then run the private node with `-onlynet=onion -addnode=trusted.onion:8333 -privatebroadcast=1`. Benefit of connecting to the trusted node over Tor - you know that you actually connected to the trusted node (or somebody who owns the private key for the `trusted.onion` service). If you connect over clearnet to an IP address, then you have to use v2 (bip324) _and_ verify that the session ids match (`session_id` in `getpeerinf
...

💬 maflcko commented on pull request "tinyformat: Add compile-time checking for literal format strings":
(https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1840692781)
I think `while ('0' <= *it && *it <= '9') ++it;` is fine. It is pretty standard self-explanatory code. I don't think a one-line while loop needs to be de-duplicated. Also, I like that the diff is minimal as-is now.

💬 mzumsande commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1840705057)
This is a hack and I haven't tested it, but instead of using `-connect`, `-maxconnections=0` `-privatebrodcast` and then addnode your trusted peer might work for your use case?

💬 l0rinc commented on pull request "tinyformat: Add compile-time checking for literal format strings":
(https://github.com/bitcoin/bitcoin/pull/31174#issuecomment-2474058623)
Thanks for improving developer productivity with these small changes <3

ACK fe39acf88ff552bfc4a502c99774375b91824bb1

💬 l0rinc commented on pull request "tinyformat: Add compile-time checking for literal format strings":
(https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1840709479)
are the extra surrounding `'` deliberate? If so, what do they mean?