🚀 glozow merged a pull request: "test: Don't rely on incentive incompatible replacement in mempool_accept_v3.py"
(https://github.com/bitcoin/bitcoin/pull/29986)

💬 willcl-ark commented on issue ""Migrate Wallet" is unclear to translators":
(https://github.com/bitcoin/bitcoin/issues/29979#issuecomment-2084785331)
Some other suggestions, none I'm particularly favourable towards vs "migration" from an English standpoint tbh:

- transition
- convertion
- transformation

Of these IMO only "transition" is likely to have any chance at being _more_ clear (than "migration" apparently is) when translating into other languages.

🤔 glozow reviewed a pull request: "doc: removed help text saying that peers may not connect automatically"
(https://github.com/bitcoin/bitcoin/pull/29994#pullrequestreview-2030743114)
lgtm 95897ff181c0757e445f0e066a2a590a0a0120d2, cc @vasild

🤔 glozow reviewed a pull request: "test: add MiniWallet tagging support to avoid UTXO mixing, use in `fill_mempool`"
(https://github.com/bitcoin/bitcoin/pull/29939#pullrequestreview-2030752389)
This solution seems really elegant! I don't think the tag is overkill and I can imagine wanting more than 2 wallets so don't think a bool is better.

💬 glozow commented on pull request "test: add MiniWallet tagging support to avoid UTXO mixing, use in `fill_mempool`":
(https://github.com/bitcoin/bitcoin/pull/29939#discussion_r1584443662)
Also I was imagining that, with the ephemeral miniwallet, we can just delete the `miniwallet` param

💬 glozow commented on pull request "test: add MiniWallet tagging support to avoid UTXO mixing, use in `fill_mempool`":
(https://github.com/bitcoin/bitcoin/pull/29939#discussion_r1584441713)
901ffa9e6e57f7c0e0db967fba9c715139bbcf0b shouldn't this be sent from the ephemeral miniwallet?

💬 glozow commented on pull request "test: add MiniWallet tagging support to avoid UTXO mixing, use in `fill_mempool`":
(https://github.com/bitcoin/bitcoin/pull/29939#discussion_r1584437933)
901ffa9e6e57f7c0e0db967fba9c715139bbcf0b nit: did you mean ephemeral? (also in commit message)

💬 dergoegge commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1584428294)
https://github.com/bitcoin/bitcoin/pull/28558 made PeerManager own a FastRandomContext, so we could (should?) use `m_rng` here instead (otherwise `PeerManager::Options::deterministic_rng` still only applies to some of the randomness).

Since this PR kind of makes individual "make this component deterministic" options redudant, we could consider reverting #28558 (not necessarily in this PR)?

I was thinking that in the long run we could break the dependencies between components and the specif
...

👍 dergoegge approved a pull request: "Simplify network-adjusted time warning logic"
(https://github.com/bitcoin/bitcoin/pull/29623#pullrequestreview-2030922611)
utACK c6be144c4b774a03a8bcab5a165768cf81e9b06b

🤔 stickies-v reviewed a pull request: "net: Modernize logging in UPnP and nat-pmp code"
(https://github.com/bitcoin/bitcoin/pull/29978#pullrequestreview-2030936139)
Concept ACK, but it's probably better to use the new macros introduced in #28318? Currently, the conditional macros (which this PR would need) don't take a category parameter, but if you think that's important perhaps it'd be good to chime in on the conversation on #29256.

🤔 stickies-v reviewed a pull request: "refactor: convert string formatting to F-strings"
(https://github.com/bitcoin/bitcoin/pull/29969#pullrequestreview-2030972260)
Concept NACK. Thank you for looking into this, and I support increased f-string usage, but imo this should be done organically whenever these lines are touched for other reasons (or with a scripted-diff across the code base, and enforcing it with linter, should there be support to do that). In its current form, this is not worth the review time imo.

💬 laanwj commented on pull request "net: Modernize logging in UPnP and nat-pmp code":
(https://github.com/bitcoin/bitcoin/pull/29978#issuecomment-2085013529)
i dunno, i prefer being explicit and not using macros myself TBH, what would macros add? The way it's done now, it's clear where the messages come from and what their level is. i'm fine with changing if you really insist.

💬 laanwj commented on pull request "logging: Simplify API for level based logging":
(https://github.com/bitcoin/bitcoin/pull/28318#issuecomment-2085021744)
> https://github.com/bitcoin/bitcoin/commit/068de7abc5bda9fd9928d7145265ae3a3a851de0 The idea behind the severity level logging is to ultimately have only one macro like Log(category, level) with one consistent, simple interface for everyone -- both developers updating logging in the code and people who read the logs -- and remove all the other macros when we're ready, simplifying the logging macros, tests, and benchmarks. This commit not only would add several more macros, but also with differe
...

💬 iw4p commented on pull request "refactor: convert string formatting to F-strings":
(https://github.com/bitcoin/bitcoin/pull/29969#issuecomment-2085022352)
Hi,
Thank you.
I touched this file to optimize it like using built-in python request library instead of curl and decreasing the request numbers, and one more PR. That's why I decided to go for converting strings to f-string.

📝 fanquake opened a pull request: "depends: fix Qt to not use `-q` with `ar`"
(https://github.com/bitcoin/bitcoin/pull/30004)
BusyBox `ar` doesn't implement `q`, and GNU `ar` treats `qs` like `r`, so replace the Qt usage of `ar cqs` with `ar rc`, which should work properly everywhere. Played around with making this work a different way, but couldn't seem to make it work properly. Open to other working suggestions (our other `AR` substitution still works correctly, it's just this single qmake bootstrap build invocation that is an issue.

✅ laanwj closed a pull request: "net: Modernize logging in UPnP and nat-pmp code"
(https://github.com/bitcoin/bitcoin/pull/29978)

💬 laanwj commented on pull request "net: Modernize logging in UPnP and nat-pmp code":
(https://github.com/bitcoin/bitcoin/pull/29978#issuecomment-2085024610)
Honestly, i don't feel like repeating this argument. i'll do everyone a favor and close this.

✅ glozow closed a pull request: "refactor: convert string formatting to F-strings"
(https://github.com/bitcoin/bitcoin/pull/29969)

💬 glozow commented on pull request "refactor: convert string formatting to F-strings":
(https://github.com/bitcoin/bitcoin/pull/29969#issuecomment-2085038645)
Thanks for your interest in contributing! CONTRIBUTING.md and developer-notes.md contain good resources for beginners. As we have 300+ pulls open right now, closing this test refactor to focus review attention on the others (see https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#refactoring)

💬 stickies-v commented on pull request "net: Modernize logging in UPnP and nat-pmp code":
(https://github.com/bitcoin/bitcoin/pull/29978#issuecomment-2085041263)
> i prefer being explicit and not using macros myself TBH

`LogPrintLevel` is a macro too?

> The way it's done now, it's clear where the messages come from and what their level is

What #29256 proposes is that you'd be able to do exactly that with the shorthand `LogWarning(BCLog::NET, "natpmp: initnatpmp() failed with %d error.\n", r_init);`, hence why I thought you'd be interested in chiming in on that pull. Apologies for derailing this PR, that was not my intent.