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

💬 ryanofsky commented on pull request "tinyformat: Add compile-time checking for literal format strings":
(https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1840738226)
re: https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1840709479

I'm pretty sure they must be accidental. These cases came from https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1820504400, and I just pasted them without noticing the single quotes. Can remove if the PR is updated again.

👍 pablomartin4btc approved a pull request: "bench: add support for custom data directory"
(https://github.com/bitcoin/bitcoin/pull/31000#pullrequestreview-2433754801)
re-ACK fa66e0887ca1a1445d8b18ba1fadb12b2d911048

Since my last review: removal of `g_rng_temp_path`, renaming and moving definition of `SetupCommonTestArgs` to `src/test/util/setup_common.h` and using the timestamp for the test dir path instead of a random id.

💬 maflcko commented on pull request "tinyformat: Add compile-time checking for literal format strings":
(https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1840743103)
The `'` are used to denote the begin and the end of the string, which would otherwise not be possible, because trailing spaces can normally not be seen when printing. They are not needed in this test and they are a leftover when I tested this against tinyformat at runtime.

💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1840747719)
pushed a lightly modified version that also reports the child txid on failure, rather than guessing it's the ultimate tx in the package. thanks! https://github.com/bitcoin/bitcoin/pull/31279

👋 instagibbs's pull request is ready for review: "policy: ephemeral dust followups"
(https://github.com/bitcoin/bitcoin/pull/31279)

💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1840754607)
> 11:24 <@dergoegge> I've suggested `LIMITED_WHILE(provider.remaining_bytes() > 0, iter_limit)` in the past as well
11:24 <@dergoegge> I think the goal is to let the fuzzer "choose" the number of iterations and both the above and consumebool achieve that
11:24 <@dergoegge> Iirc there were concerns around using remaining_bytes as really large inputs might cause timeouts
11:24 <@dergoegge> which is something we've seen with consumebool as well if the iteration limit is too high

I'm going to
...