💬 mzumsande commented on pull request "p2p: skip netgroup diversity of new connections for tor/i2p/cjdns":
(https://github.com/bitcoin/bitcoin/pull/27374#discussion_r1156247998)
I agree, see below.
(https://github.com/bitcoin/bitcoin/pull/27374#discussion_r1156247998)
I agree, see below.
👍 aureleoules approved a pull request: "test: refactor: replace unnecessary `BytesIO` uses"
(https://github.com/bitcoin/bitcoin/pull/27389)
ACK f842ed9a40c0db656b86f85e84dd4978865cc0a0 - It seems that these are the only instances that can be changed and it simplifies test code.
(https://github.com/bitcoin/bitcoin/pull/27389)
ACK f842ed9a40c0db656b86f85e84dd4978865cc0a0 - It seems that these are the only instances that can be changed and it simplifies test code.
💬 martinus commented on pull request "util: Use steady clock instead of system clock to measure durations":
(https://github.com/bitcoin/bitcoin/pull/27405#discussion_r1156279841)
I think this should be `20s`. I guess in this case it might be safer to write
```cpp
std::chrono::milliseconds g_socks5_recv_timeout = 20s;
```
because just
```cpp
std::chrono::milliseconds g_socks5_recv_timeout = 20;
```
would give you a compile error
(https://github.com/bitcoin/bitcoin/pull/27405#discussion_r1156279841)
I think this should be `20s`. I guess in this case it might be safer to write
```cpp
std::chrono::milliseconds g_socks5_recv_timeout = 20s;
```
because just
```cpp
std::chrono::milliseconds g_socks5_recv_timeout = 20;
```
would give you a compile error
💬 sdaftuar commented on pull request "p2p: skip netgroup diversity of new connections for tor/i2p/cjdns":
(https://github.com/bitcoin/bitcoin/pull/27374#issuecomment-1494762525)
> however, the OP of that post also states:
>> This is still somewhat incomplete protection because our outbound
peers could be down but not timed out or might all be on 'local'
networks (although the requirement for multiple netgroups helps).
@amitiuttarwar Thanks for finding this, I hadn't thought about the issue of wanting to distinguish between local and non-local IPV4 networks for the purposes of detecting whether a node's network connectivity is down.
This means my suggestion of
...
(https://github.com/bitcoin/bitcoin/pull/27374#issuecomment-1494762525)
> however, the OP of that post also states:
>> This is still somewhat incomplete protection because our outbound
peers could be down but not timed out or might all be on 'local'
networks (although the requirement for multiple netgroups helps).
@amitiuttarwar Thanks for finding this, I hadn't thought about the issue of wanting to distinguish between local and non-local IPV4 networks for the purposes of detecting whether a node's network connectivity is down.
This means my suggestion of
...
👋 pinheadmz's pull request is ready for review: "net: support unix domain sockets for -proxy and -onion"
(https://github.com/bitcoin/bitcoin/pull/27375)
(https://github.com/bitcoin/bitcoin/pull/27375)
💬 theuni commented on pull request "depends: add `NO_HARDEN=` option":
(https://github.com/bitcoin/bitcoin/pull/27406#discussion_r1156323737)
Nit: spelling
(https://github.com/bitcoin/bitcoin/pull/27406#discussion_r1156323737)
Nit: spelling
💬 TheBlueMatt commented on pull request "contrib: allow multi-sig binary verification v2":
(https://github.com/bitcoin/bitcoin/pull/27358#issuecomment-1494803900)
> Downloading to a clean dir and using a wildcard should still work for tooling I think. @TheBlueMatt ?
Not sure exactly what you mean here - part of the goal is to have tooling to verify all the binaries sitting in a folder? If you can "download" all of them in one go from `file:///` I guess that works too?
(https://github.com/bitcoin/bitcoin/pull/27358#issuecomment-1494803900)
> Downloading to a clean dir and using a wildcard should still work for tooling I think. @TheBlueMatt ?
Not sure exactly what you mean here - part of the goal is to have tooling to verify all the binaries sitting in a folder? If you can "download" all of them in one go from `file:///` I guess that works too?
📝 mzumsande opened a pull request: "p2p: Restrict self-advertisements with privacy networks to avoid fingerprinting"
(https://github.com/bitcoin/bitcoin/pull/27411)
The current logic for self-advertisements works such that we detect as many local addresses as we can, and then, using the scoring matrix from `CNetAddr::GetReachabilityFrom()`, self-advertise with the address that fits best to our peer.
It is in general not hard for our peers to distinguish our self-advertisements from other addrs we send them, because we self-advertise every ~24h and because the first addr we send over a connection is likely our self-advertisement.
`GetReachabilityFrom()`
...
(https://github.com/bitcoin/bitcoin/pull/27411)
The current logic for self-advertisements works such that we detect as many local addresses as we can, and then, using the scoring matrix from `CNetAddr::GetReachabilityFrom()`, self-advertise with the address that fits best to our peer.
It is in general not hard for our peers to distinguish our self-advertisements from other addrs we send them, because we self-advertise every ~24h and because the first addr we send over a connection is likely our self-advertisement.
`GetReachabilityFrom()`
...
📝 mzumsande converted_to_draft a pull request: "p2p: Restrict self-advertisements with privacy networks to avoid fingerprinting"
(https://github.com/bitcoin/bitcoin/pull/27411)
The current logic for self-advertisements works such that we detect as many local addresses as we can, and then, using the scoring matrix from `CNetAddr::GetReachabilityFrom()`, self-advertise with the address that fits best to our peer.
It is in general not hard for our peers to distinguish our self-advertisements from other addrs we send them, because we self-advertise every ~24h and because the first addr we send over a connection is likely our self-advertisement.
`GetReachabilityFrom()`
...
(https://github.com/bitcoin/bitcoin/pull/27411)
The current logic for self-advertisements works such that we detect as many local addresses as we can, and then, using the scoring matrix from `CNetAddr::GetReachabilityFrom()`, self-advertise with the address that fits best to our peer.
It is in general not hard for our peers to distinguish our self-advertisements from other addrs we send them, because we self-advertise every ~24h and because the first addr we send over a connection is likely our self-advertisement.
`GetReachabilityFrom()`
...
💬 mzumsande commented on pull request "p2p: Don't require services from `ADDR_FETCH` peers":
(https://github.com/bitcoin/bitcoin/pull/26343#issuecomment-1494908602)
closing, there doesn't seem to be much interest and even though I still think it could make sense, it's certainly not critical.
(https://github.com/bitcoin/bitcoin/pull/26343#issuecomment-1494908602)
closing, there doesn't seem to be much interest and even though I still think it could make sense, it's certainly not critical.
✅ mzumsande closed a pull request: "p2p: Don't require services from `ADDR_FETCH` peers"
(https://github.com/bitcoin/bitcoin/pull/26343)
(https://github.com/bitcoin/bitcoin/pull/26343)
💬 furszy commented on pull request "wallet, gui: bugfix, getAvailableBalance skips selected coins":
(https://github.com/bitcoin/bitcoin/pull/26699#discussion_r1156408310)
Was remembering why I left it unhandled:
And it was because this cannot happen. `FetchSelectedInputs` fails only if any of the selected coins are not solvable or invalid. And we set the selected coins via the coin control dialog, which is loaded from the wallet's available UTXOs, which are always valid.
(https://github.com/bitcoin/bitcoin/pull/26699#discussion_r1156408310)
Was remembering why I left it unhandled:
And it was because this cannot happen. `FetchSelectedInputs` fails only if any of the selected coins are not solvable or invalid. And we set the selected coins via the coin control dialog, which is loaded from the wallet's available UTXOs, which are always valid.
💬 furszy commented on pull request "wallet, gui: bugfix, getAvailableBalance skips selected coins":
(https://github.com/bitcoin/bitcoin/pull/26699#discussion_r1156411543)
Sure. Doesn't hurt to add a comment.
(https://github.com/bitcoin/bitcoin/pull/26699#discussion_r1156411543)
Sure. Doesn't hurt to add a comment.
💬 furszy commented on pull request "wallet, gui: bugfix, getAvailableBalance skips selected coins":
(https://github.com/bitcoin/bitcoin/pull/26699#discussion_r1156423239)
np about creating a gui issue for this.
still, I'm not fan of the link to this particular line because this isn't the only place that has to be modified to introduce the feature. Plus, code changes over time, so the issue could easily get behind, which would mean more maintenance burden for us.
(https://github.com/bitcoin/bitcoin/pull/26699#discussion_r1156423239)
np about creating a gui issue for this.
still, I'm not fan of the link to this particular line because this isn't the only place that has to be modified to introduce the feature. Plus, code changes over time, so the issue could easily get behind, which would mean more maintenance burden for us.
💬 furszy commented on pull request "wallet, gui: bugfix, getAvailableBalance skips selected coins":
(https://github.com/bitcoin/bitcoin/pull/26699#discussion_r1156424730)
pushed
(https://github.com/bitcoin/bitcoin/pull/26699#discussion_r1156424730)
pushed
💬 furszy commented on pull request "wallet, gui: bugfix, getAvailableBalance skips selected coins":
(https://github.com/bitcoin/bitcoin/pull/26699#discussion_r1156424958)
pushed
(https://github.com/bitcoin/bitcoin/pull/26699#discussion_r1156424958)
pushed
💬 furszy commented on pull request "wallet, gui: bugfix, getAvailableBalance skips selected coins":
(https://github.com/bitcoin/bitcoin/pull/26699#issuecomment-1494936715)
Feedback tackled. Thanks Sjors.
Only a comment change, [diff](https://github.com/bitcoin/bitcoin/compare/86b43c78ecd5a68b5569b1c2a7b2696b1f70ef21..68eed5df8656bed1be6526b014e58d3123102b03).
(https://github.com/bitcoin/bitcoin/pull/26699#issuecomment-1494936715)
Feedback tackled. Thanks Sjors.
Only a comment change, [diff](https://github.com/bitcoin/bitcoin/compare/86b43c78ecd5a68b5569b1c2a7b2696b1f70ef21..68eed5df8656bed1be6526b014e58d3123102b03).
📝 brunoerg opened a pull request: "logging, net: add ASN from peers on logs "
(https://github.com/bitcoin/bitcoin/pull/27412)
When using `-asmap`, you can check the ASN assigned to the peers only with the RPC command `getpeerinfo` (check `mapped_as` field), however, it's not possible to check it in logs (e.g. see in logs the ASN of the peers when a new outbound peer has been connected). This PR adds a new flag `-logasmap`, when set will include the peers' ASN in debug output. The apporach is so similar to `-logips`.
Obs: Open this primarily to chase some Concept ACK, I've been using this on my node to facilitate to
...
(https://github.com/bitcoin/bitcoin/pull/27412)
When using `-asmap`, you can check the ASN assigned to the peers only with the RPC command `getpeerinfo` (check `mapped_as` field), however, it's not possible to check it in logs (e.g. see in logs the ASN of the peers when a new outbound peer has been connected). This PR adds a new flag `-logasmap`, when set will include the peers' ASN in debug output. The apporach is so similar to `-logips`.
Obs: Open this primarily to chase some Concept ACK, I've been using this on my node to facilitate to
...
💬 furszy commented on pull request "reduce cs_main scope, guard block index 'nFile' under a local mutex":
(https://github.com/bitcoin/bitcoin/pull/27006#discussion_r1156452551)
> Are you sure these are protected by cs_main, is it?
The members' write process, not the read. If the read would be protected by `cs_main`, this PR wouldn't make any sense :p.
The two places are: the `CBlockIndex` creation time and `CBlockIndex` pruning time.
Still, those `cs_main` locks can also be removed in a follow-up PR. I haven't removed them here because they require further changes.
Not sure if I'm understanding your concern here. You are thinking on a race in-between `GetFile
...
(https://github.com/bitcoin/bitcoin/pull/27006#discussion_r1156452551)
> Are you sure these are protected by cs_main, is it?
The members' write process, not the read. If the read would be protected by `cs_main`, this PR wouldn't make any sense :p.
The two places are: the `CBlockIndex` creation time and `CBlockIndex` pruning time.
Still, those `cs_main` locks can also be removed in a follow-up PR. I haven't removed them here because they require further changes.
Not sure if I'm understanding your concern here. You are thinking on a race in-between `GetFile
...
💬 sipsorcery commented on pull request "Fixes compile errors in MSVC build #27332":
(https://github.com/bitcoin/bitcoin/pull/27335#issuecomment-1494973574)
tACK 28436965b6422bd92d23033a5ddbd60a6c183cd7.
The version pinning is nice. It was badly missed feature when we first started relying on vcpkg. We pinned the version of vcpkg instead but individual package pinning should be more flexible.
(https://github.com/bitcoin/bitcoin/pull/27335#issuecomment-1494973574)
tACK 28436965b6422bd92d23033a5ddbd60a6c183cd7.
The version pinning is nice. It was badly missed feature when we first started relying on vcpkg. We pinned the version of vcpkg instead but individual package pinning should be more flexible.