π¬ hebasto commented on pull request "tracepoints: Disables `-Wgnu-zero-variadic-macro-arguments` to compile without warnings":
(https://github.com/bitcoin/bitcoin/pull/27401#issuecomment-1495607071)
Maybe add a check to `configure.ac` whether ignoring of `-Wgnu-zero-variadic-macro-arguments` is [really](https://github.com/bitcoin/bitcoin/issues/26916#issuecomment-1400156218) required?
(https://github.com/bitcoin/bitcoin/pull/27401#issuecomment-1495607071)
Maybe add a check to `configure.ac` whether ignoring of `-Wgnu-zero-variadic-macro-arguments` is [really](https://github.com/bitcoin/bitcoin/issues/26916#issuecomment-1400156218) required?
π¬ fanquake commented on pull request "depends: add `NO_HARDEN=` option":
(https://github.com/bitcoin/bitcoin/pull/27406#discussion_r1156956396)
Thanks, fixed.
(https://github.com/bitcoin/bitcoin/pull/27406#discussion_r1156956396)
Thanks, fixed.
π¬ fanquake commented on pull request "depends: add `NO_HARDEN=` option":
(https://github.com/bitcoin/bitcoin/pull/27406#issuecomment-1495616682)
Pushed to fix the typo.
(https://github.com/bitcoin/bitcoin/pull/27406#issuecomment-1495616682)
Pushed to fix the typo.
π fanquake merged a pull request: "test: refactor: replace unnecessary `BytesIO` uses"
(https://github.com/bitcoin/bitcoin/pull/27389)
(https://github.com/bitcoin/bitcoin/pull/27389)
π hebasto approved a pull request: "depends: add `NO_HARDEN=` option"
(https://github.com/bitcoin/bitcoin/pull/27406)
re-ACK 436df1e826cae036caed3e983715a4ed4e441321
(https://github.com/bitcoin/bitcoin/pull/27406)
re-ACK 436df1e826cae036caed3e983715a4ed4e441321
π¬ fanquake commented on pull request "util: Use steady clock instead of system clock to measure durations":
(https://github.com/bitcoin/bitcoin/pull/27405#issuecomment-1495674216)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/27405#issuecomment-1495674216)
Concept ACK
π¬ Ayush170-Future commented on pull request "logging, net: add ASN from peers on logs":
(https://github.com/bitcoin/bitcoin/pull/27412#discussion_r1157016059)
Yeah, that does make more sense. I don't see any harm in automatically logging the asmap information.
(https://github.com/bitcoin/bitcoin/pull/27412#discussion_r1157016059)
Yeah, that does make more sense. I don't see any harm in automatically logging the asmap information.
π¬ Ayush170-Future commented on pull request "logging, net: add ASN from peers on logs":
(https://github.com/bitcoin/bitcoin/pull/27412#issuecomment-1495685701)
Concept ACK.
(https://github.com/bitcoin/bitcoin/pull/27412#issuecomment-1495685701)
Concept ACK.
π¬ dergoegge commented on pull request "p2p: Restrict self-advertisements with privacy networks to avoid fingerprinting":
(https://github.com/bitcoin/bitcoin/pull/27411#issuecomment-1495695976)
Concept ACK
Personally, I have given up a little bit on fighting fingerprinting (at least I've de-prioritized it) because at this point there are so many fingerprinting techniques that we should probably either give up or figure out a way to avoid them ~entirely by design.
> we could advertise our onion address to clearnet peers
But this is such an obvious fingerprint that we should address it.
(https://github.com/bitcoin/bitcoin/pull/27411#issuecomment-1495695976)
Concept ACK
Personally, I have given up a little bit on fighting fingerprinting (at least I've de-prioritized it) because at this point there are so many fingerprinting techniques that we should probably either give up or figure out a way to avoid them ~entirely by design.
> we could advertise our onion address to clearnet peers
But this is such an obvious fingerprint that we should address it.
π¬ vasild commented on pull request "p2p: Restrict self-advertisements with privacy networks to avoid fingerprinting":
(https://github.com/bitcoin/bitcoin/pull/27411#issuecomment-1495725877)
Or why not even expand this logic to all networks and delete `CNetAddr::GetReachabilityFrom()`? That is - advertise our X address only to peers from network X (for X being any of IPv4, IPv6, Tor, I2P, CJDNS).
(https://github.com/bitcoin/bitcoin/pull/27411#issuecomment-1495725877)
Or why not even expand this logic to all networks and delete `CNetAddr::GetReachabilityFrom()`? That is - advertise our X address only to peers from network X (for X being any of IPv4, IPv6, Tor, I2P, CJDNS).
π¬ MarcoFalke commented on pull request "util: Use steady clock instead of system clock to measure durations":
(https://github.com/bitcoin/bitcoin/pull/27405#discussion_r1157055154)
Thanks, fixed
(https://github.com/bitcoin/bitcoin/pull/27405#discussion_r1157055154)
Thanks, fixed
π MarcoFalke opened a pull request: "test: Remove windows workaround in authproxy"
(https://github.com/bitcoin/bitcoin/pull/27418)
I wonder if the windows issues have also been fixed by bumping the server timeout in commit 88134fcee998849d9569368d6cefbfa487dc82d1.
I guess the only way to find out and try.
Note that even with the workaround, the issue would still happen occasionally: https://github.com/bitcoin/bitcoin/issues/18623
(https://github.com/bitcoin/bitcoin/pull/27418)
I wonder if the windows issues have also been fixed by bumping the server timeout in commit 88134fcee998849d9569368d6cefbfa487dc82d1.
I guess the only way to find out and try.
Note that even with the workaround, the issue would still happen occasionally: https://github.com/bitcoin/bitcoin/issues/18623
π¬ furszy commented on pull request "wallet, gui: bugfix, getAvailableBalance skips selected coins":
(https://github.com/bitcoin/bitcoin/pull/26699#discussion_r1157195466)
Ok, sounds good. Will do it in the next push.
(https://github.com/bitcoin/bitcoin/pull/26699#discussion_r1157195466)
Ok, sounds good. Will do it in the next push.
π¬ fanquake commented on pull request "test: Remove windows workaround in authproxy":
(https://github.com/bitcoin/bitcoin/pull/27418#issuecomment-1495927906)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/27418#issuecomment-1495927906)
Concept ACK
π TheCharlatan opened a pull request: "refactor: Extract common/args from util/system"
(https://github.com/bitcoin/bitcoin/pull/27419)
This pull request is part of the `libbitcoinkernel` project https://github.com/bitcoin/bitcoin/issues/24303 https://github.com/bitcoin/bitcoin/projects/18 and more specifically its "Step 2: Decouple most non-consensus code from libbitcoinkernel". It is part of a series of patches splitting up the `util/system` files. Its preceding pull request is https://github.com/bitcoin/bitcoin/pull/27254.
The pull request contains an extraction of ArgsManager related functions from util/system into their
...
(https://github.com/bitcoin/bitcoin/pull/27419)
This pull request is part of the `libbitcoinkernel` project https://github.com/bitcoin/bitcoin/issues/24303 https://github.com/bitcoin/bitcoin/projects/18 and more specifically its "Step 2: Decouple most non-consensus code from libbitcoinkernel". It is part of a series of patches splitting up the `util/system` files. Its preceding pull request is https://github.com/bitcoin/bitcoin/pull/27254.
The pull request contains an extraction of ArgsManager related functions from util/system into their
...
π TheCharlatan's pull request is ready for review: "refactor: Extract common/args from util/system"
(https://github.com/bitcoin/bitcoin/pull/27419)
(https://github.com/bitcoin/bitcoin/pull/27419)
π¬ hebasto commented on pull request "ci: use clang-16 in tidy task":
(https://github.com/bitcoin/bitcoin/pull/27404#issuecomment-1495953487)
FWIW, on a local Ubuntu 23.04 installation, I have no issues with running the IWYU tool manually.
(https://github.com/bitcoin/bitcoin/pull/27404#issuecomment-1495953487)
FWIW, on a local Ubuntu 23.04 installation, I have no issues with running the IWYU tool manually.
π¬ furszy commented on pull request "wallet: coin selection, don't return results that exceed the max allowed weight":
(https://github.com/bitcoin/bitcoin/pull/26720#discussion_r1157298121)
> I haven't written a test for this, so all the following is not verified and just my guess based on the code understanding.
>
> This is not a regression in this particular PR, but this could lead to misleading errors in a situation when a proper solution exists, but our coin selection failed to find it and only found solution that exceeded weight. Let's imagine following scenario:
>
> * UTXO pool: two big coins 50BTC, a ton of 0.001BTC.
> * Target amount: 90BTC
> * Proper solution: just
...
(https://github.com/bitcoin/bitcoin/pull/26720#discussion_r1157298121)
> I haven't written a test for this, so all the following is not verified and just my guess based on the code understanding.
>
> This is not a regression in this particular PR, but this could lead to misleading errors in a situation when a proper solution exists, but our coin selection failed to find it and only found solution that exceeded weight. Let's imagine following scenario:
>
> * UTXO pool: two big coins 50BTC, a ton of 0.001BTC.
> * Target amount: 90BTC
> * Proper solution: just
...
π¬ dergoegge commented on pull request "reduce cs_main scope, guard block index 'nFile' under a local mutex":
(https://github.com/bitcoin/bitcoin/pull/27006#issuecomment-1496041020)
> It isnβt the main one actually, there are other areas that shouldnβt be locking the net messages reception / validation thread because of a disk read: e.g. βgetblockβ, βgetblocktxnβ and compact block net messages, the current indexes sync process (all of them), getblock/gettxoutproof RPC commands. They all are locking the node at every single block read.
This has been the case for 10+ years, if we're gonna change it then we should take the time to do it without adding to our technical debt.
...
(https://github.com/bitcoin/bitcoin/pull/27006#issuecomment-1496041020)
> It isnβt the main one actually, there are other areas that shouldnβt be locking the net messages reception / validation thread because of a disk read: e.g. βgetblockβ, βgetblocktxnβ and compact block net messages, the current indexes sync process (all of them), getblock/gettxoutproof RPC commands. They all are locking the node at every single block read.
This has been the case for 10+ years, if we're gonna change it then we should take the time to do it without adding to our technical debt.
...
β
MarcoFalke closed an issue: "Investiage RPC connection lifetime issues"
(https://github.com/bitcoin/bitcoin/issues/18623)
(https://github.com/bitcoin/bitcoin/issues/18623)