💬 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
(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?
(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)
(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
...
(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
(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.
(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.
(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.
(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
...
(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.
(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.
(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)
(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.
(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)
(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)
(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.
(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.
📝 laanwj opened a pull request: "[PoC, nomerge] IPv6 PCP pinhole test"
(https://github.com/bitcoin/bitcoin/pull/30005)
The overarching goal is to increase the number of connectable nodes that are not in the big datacenters.
**Context:** IPv6 doesn't have NAT. Computers behind a router tend to get a globally routable address. However, by default this address is usually completely firewalled for incoming connections, as a security measure. See issue #17012.
This is where "pinholing" comes in. By sending a request, a machine on the network can request a port to be opened. This is similar to requesting a port
...
(https://github.com/bitcoin/bitcoin/pull/30005)
The overarching goal is to increase the number of connectable nodes that are not in the big datacenters.
**Context:** IPv6 doesn't have NAT. Computers behind a router tend to get a globally routable address. However, by default this address is usually completely firewalled for incoming connections, as a security measure. See issue #17012.
This is where "pinholing" comes in. By sending a request, a machine on the network can request a port to be opened. This is similar to requesting a port
...
💬 laanwj commented on pull request "net: Modernize logging in UPnP and nat-pmp code":
(https://github.com/bitcoin/bitcoin/pull/29978#issuecomment-2085108270)
> What https://github.com/bitcoin/bitcoin/pull/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.
Thanks, i'll keep it in mind.
> Apologies for derailing this PR, that was not my intent.
No need for apologies ! you're right, i don't know what has been discussed
...
(https://github.com/bitcoin/bitcoin/pull/29978#issuecomment-2085108270)
> What https://github.com/bitcoin/bitcoin/pull/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.
Thanks, i'll keep it in mind.
> Apologies for derailing this PR, that was not my intent.
No need for apologies ! you're right, i don't know what has been discussed
...
👍 brunoerg approved a pull request: "Fuzz: extend CConnman tests"
(https://github.com/bitcoin/bitcoin/pull/28584#pullrequestreview-2031084551)
reACK 6d9083b249376d503621da7980ef7ae02e690e0b
(https://github.com/bitcoin/bitcoin/pull/28584#pullrequestreview-2031084551)
reACK 6d9083b249376d503621da7980ef7ae02e690e0b
💬 ajtowns commented on pull request "Improve new LogDebug/Trace/Info/Warning/Error Macros":
(https://github.com/bitcoin/bitcoin/pull/29256#issuecomment-2085153558)
> I think most of the criticism of this PR from AJ is saying that the changes here are unnecessary, not really that they are harmful,
I said "As I've already said, I think `m_log.SetCategory(NET); m_log.Debug("connected to foo")` is much worse than `LogDebug(NET, "connected to foo")`." Does that really not come across as me saying they're harmful? I really don't think I'm being **that** unclear. Anyway, I'll bow out, I'm already sorry for touching this.
(https://github.com/bitcoin/bitcoin/pull/29256#issuecomment-2085153558)
> I think most of the criticism of this PR from AJ is saying that the changes here are unnecessary, not really that they are harmful,
I said "As I've already said, I think `m_log.SetCategory(NET); m_log.Debug("connected to foo")` is much worse than `LogDebug(NET, "connected to foo")`." Does that really not come across as me saying they're harmful? I really don't think I'm being **that** unclear. Anyway, I'll bow out, I'm already sorry for touching this.