💬 TheCharlatan commented on pull request "refactor: Extract common/args from util/system":
(https://github.com/bitcoin/bitcoin/pull/27419#discussion_r1157735880)
No I cannot, because it doesn't make sense. Will revert.

💬 jonatack commented on pull request "Add warnings for non-active addresses in receive tab and address book":
(https://github.com/bitcoin-core/gui/pull/723#discussion_r1157712788)
Missing brackets here (we require brackets with conditionals over one line due to CVE-2014-1266, the Apple "goto fail" vulnerability, see also https://dwheeler.com/essays/apple-goto-fail).

💬 jonatack commented on pull request "Add warnings for non-active addresses in receive tab and address book":
(https://github.com/bitcoin-core/gui/pull/723#discussion_r1157714528)
nit, I prefer to read left-to-right rather than up-to-down, when these can all be on one line

💬 jonatack commented on pull request "Add warnings for non-active addresses in receive tab and address book":
(https://github.com/bitcoin-core/gui/pull/723#discussion_r1157715369)
Could set only the first enum value for smaller diffs/less churn in future changes.

💬 jonatack commented on pull request "Add warnings for non-active addresses in receive tab and address book":
(https://github.com/bitcoin-core/gui/pull/723#discussion_r1157718871)
(Idem brackets here and lines 118-119)

💬 jonatack commented on pull request "Add warnings for non-active addresses in receive tab and address book":
(https://github.com/bitcoin-core/gui/pull/723#discussion_r1157717210)
(Idem brackets.)

nit, looks like this warning is the same one as above at https://github.com/bitcoin-core/gui/pull/723/files#diff-74dd156c97c401ff337c0ed5399192d86ea2120f7baed1368d2a407a74e347cfR74. Maybe define it in one place if it makes sense to do so.

💬 jonatack commented on pull request "Add warnings for non-active addresses in receive tab and address book":
(https://github.com/bitcoin-core/gui/pull/723#discussion_r1157721419)
Why not just on one line?

💬 jonatack commented on pull request "Add warnings for non-active addresses in receive tab and address book":
(https://github.com/bitcoin-core/gui/pull/723#discussion_r1157716418)
(As above) same line or brackets here.

💬 jonatack commented on pull request "Add warnings for non-active addresses in receive tab and address book":
(https://github.com/bitcoin-core/gui/pull/723#discussion_r1157721554)
Why not just on one line?

💬 jonatack commented on pull request "Add warnings for non-active addresses in receive tab and address book":
(https://github.com/bitcoin-core/gui/pull/723#discussion_r1157722990)
(Same line or brackets)

💬 jonatack commented on pull request "Add warnings for non-active addresses in receive tab and address book":
(https://github.com/bitcoin-core/gui/pull/723#discussion_r1157719967)
Could set only the first enum value for smaller diffs/less churn in future changes.

👍 TheCharlatan approved a pull request: "build: remove ancient unused define"
(https://github.com/bitcoin/bitcoin/pull/27420)
ACK 9fbc5fcd28eeefd3ad1932a2d94e5558deeb16d7

💬 jamesob commented on pull request "logging, net: add ASN from peers on logs":
(https://github.com/bitcoin/bitcoin/pull/27412#issuecomment-1496579012)
Big concept ACK. Will test next week (post-honeymoon).

💬 jonatack commented on pull request "Add warnings for non-active addresses in receive tab and address book":
(https://github.com/bitcoin-core/gui/pull/723#discussion_r1157742775)
> In general, I think I'd start with the date column first, and then the warnings one.

I see that Jonas Schnelli used a date-first order as well in https://github.com/bitcoin/bitcoin/issues/3314#issuecomment-81567665.

💬 TheCharlatan commented on pull request "refactor: Extract common/args from util/system":
(https://github.com/bitcoin/bitcoin/pull/27419#issuecomment-1496581866)
Updated 93c56f4f203f3bf3a616863b4ec6443aa36b2f9d -> c02e451882bb1d220b944af3b444d4b529ac351a ([kernelChainName_0](https://github.com/TheCharlatan/bitcoin/commits/splitSystemArgs_0) -> [splitSystemArgs_1](https://github.com/TheCharlatan/bitcoin/commits/splitSystemArgs_1), [compare](https://github.com/TheCharlatan/bitcoin/compare/splitSystemArgs_0..splitSystemArgs_1)):
* Addressed @MarcoFalke's [comment](https://github.com/bitcoin/bitcoin/pull/27419#discussion_r1157536088) for cleaning up a needl
...

💬 TheCharlatan commented on pull request "compat: move (win) S_* defines into bdb":
(https://github.com/bitcoin/bitcoin/pull/26832#issuecomment-1496583438)
re-ACK 54e406118926c2a526455a60f67961520dfb65da

💬 theuni commented on pull request "contrib: allow multi-sig binary verification v2":
(https://github.com/bitcoin/bitcoin/pull/27358#discussion_r1157778909)
Not sure what you mean, I think this just intends to say `verify.py` ?

💬 theuni commented on pull request "contrib: allow multi-sig binary verification v2":
(https://github.com/bitcoin/bitcoin/pull/27358#discussion_r1157782969)
Looks like this has been implemented as `pub --cleanup`. A quick test shows it working as intended. Will update the doc.

💬 theuni commented on pull request "contrib: allow multi-sig binary verification v2":
(https://github.com/bitcoin/bitcoin/pull/27358#issuecomment-1496637445)
I believe I've addressed the comments here. I've chosen to add fresh comments on top rather than squashing to avoid mixing up my changes on top of @jamesob's/@achow101's.

The os/arch filters like `verify.py 22.0-osx` no longer work for me and I'm not quite sure why. Otherwise remote/local verification seem to work ok.

💬 mzumsande commented on pull request "p2p: Restrict self-advertisements with privacy networks to avoid fingerprinting":
(https://github.com/bitcoin/bitcoin/pull/27411#issuecomment-1496649369)
> Is it not possible to achieve the same with something simpler, like the following? (...)

We'd need one more `if` so that we don't stop advertising onion addresses to `127.0.0.1` peers (inbounds from tor).

But yes, I will try this alternative out.
Alternatively, I also thought of renaming `GetReachabilityFrom` to something like `GetAdvertisingScore` - since we don't use it anywhere else.

> Or why not even expand this logic to all networks and delete `CNetAddr::GetReachabilityFrom()`?
...