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

💬 instagibbs commented on pull request "policy: ephemeral dust followups":
(https://github.com/bitcoin/bitcoin/pull/31279#issuecomment-2474116685)
ready for review, I think

💬 achow101 commented on pull request "test: Rework wallet_migration.py to use previous releases":
(https://github.com/bitcoin/bitcoin/pull/31248#discussion_r1840779076)
It returned RPC originally, and I don't think that adding a rename to the diff would be helpful.

💬 achow101 commented on pull request "test: Rework wallet_migration.py to use previous releases":
(https://github.com/bitcoin/bitcoin/pull/31248#discussion_r1840781994)
No, the master node migrates a copy of the wallet. The old node will have the original copy which remains unchanged regardless.

📝 maflcko opened a pull request: "ci: Skip broken Wine64 tests by default"
(https://github.com/bitcoin/bitcoin/pull/31284)
I don't think the unit tests run in Wine after the Windows cross-compilation have ever shown a true positive since the MSVC task was added. However, they are a source of frequent false-positives.

Thus, disable them by default for now. Anyone can still enable them by setting `RUN_UNIT_TESTS=true`.

Fixes #31071

A follow-up could run them on real Windows, but this comes with its own downsides, see https://github.com/bitcoin/bitcoin/pull/31176.

Conceptually there are many other nightly t
...

💬 achow101 commented on pull request "test: Rework wallet_migration.py to use previous releases":
(https://github.com/bitcoin/bitcoin/pull/31248#discussion_r1840786362)
`notloaded` is specifically tested because legacy wallets won't be able to be loaded.

💬 achow101 commented on pull request "test: Rework wallet_migration.py to use previous releases":
(https://github.com/bitcoin/bitcoin/pull/31248#discussion_r1840787871)
Yes, no test relies on the time.

💬 achow101 commented on pull request "test: Rework wallet_migration.py to use previous releases":
(https://github.com/bitcoin/bitcoin/pull/31248#discussion_r1840789029)
Will do if I need to retouch.

💬 achow101 commented on pull request "test: Rework wallet_migration.py to use previous releases":
(https://github.com/bitcoin/bitcoin/pull/31248#discussion_r1840789191)
Will do if I need to retouch.