💬 Sjors commented on pull request "Stratum v2 Transport":
(https://github.com/bitcoin/bitcoin/pull/30315#issuecomment-2197155437)
I dropped the refactoring commits e18887d3c45d4c0b2d414392ec33dbc36c0d8df7, cf67a72aea67b8ecf2c24200f69c68d1e7f4de4b and 912b800581ba6c21b0ed43832c6307f98e7a3af1 which move Transport and its prerequisites to `libbitcoin_common`. See https://github.com/Sjors/bitcoin/pull/47 for a more ambitious attempt to introduce a whole new `libbitcoin_net`, but became real big and distracting real fast :-)

🤔 vasild reviewed a pull request: "net: Replace libnatpmp with built-in PCP+NATPMP implementation"
(https://github.com/bitcoin/bitcoin/pull/30043#pullrequestreview-2141814858)
Approach ACK 23916f2a7717b463799207b4b81b5795f2e9d7e3

💬 vasild commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1655068312)
nit: maybe worth logging also `num_tries`, e.g. "retrying 1/5", "retrying 2/5", ...

💬 vasild commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1654827053)
style: make more doxygen friendly:

```diff
//! Try to open a port using RFC 6886 NAT-PMP. IPv4 only.
//!
-//! * gateway: Destination address for PCP requests (usually the default gateway).
-//! * port: Internal port, and desired external port.
-//! * lifetime: Requested lifetime in seconds for mapping. The server may assign as shorter or longer lifetime. A lifetime of 0 deletes the mapping.
-//! * num_tries: Number of tries in case of no response.
+//! @param[in] gateway Destination
...

💬 vasild commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1655069402)
nit: maybe worth logging `ntry` and `num_tries`, e.g. "timeout 1/5", "timeout 2/5", ...

💬 vasild commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1654859721)
"RFC6887 section 1.1" should be "RFC6886 section 1.1"?

https://www.rfc-editor.org/rfc/rfc6886#section-1.1

💬 vasild commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1655104852)
From [RFC 6886 3.2](https://www.rfc-editor.org/rfc/rfc6886#section-3.2):

> Upon receiving a response packet, the client MUST check the source IP address, and silently discard the packet if the address is not the address of the gateway to which the request was sent.

Using `connect(2)` on UDP socket nicely ensures that we only receive packets from that address (`dest_addr`), so that we don't have to check explicitly. Maybe expand the comment to show this purpose too and to avoid this `Connec
...

💬 vasild commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1655169229)
From https://www.rfc-editor.org/rfc/rfc6886#section-3.1:

> ... client sends its request packet to port 5351 of its
configured gateway address, and waits 250 ms for a response. If no
NAT-PMP response is received from the gateway after 250 ms, the
client retransmits its request and waits 500 ms. The client SHOULD
repeat this process with the interval between attempts doubling each
time. If, after sending its ninth attempt (and then waiting for 64
seconds), the client
...

💬 vasild commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1657164986)
I wonder why the difference with `NATPMPRequestPortMap()` where we `Connect()` (without `Bind()`) and retrieve the local address from the socket.

My understanding is that both NAT-PMP and PCP servers will reject mapping requests for one address coming from another address.

💬 vasild commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1657447061)
Correlating responses to requests is at odds with the description in [section 6](https://www.rfc-editor.org/rfc/rfc6887#section-6):

> ... the message flows can be viewed as two somewhat independent streams ... These two message flows are loosely correlated ... The PCP server can send unsolicited responses to clients ... This design goal helps explain why PCP request and response messages have no transaction ID ...

According to the last paragraph in [section 11.2](https://www.rfc-editor.org
...

💬 vasild commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1655128453)
Could use the constants instead of magic numbers:

```suggestion
if (response[NATPMP_HDR_VERSION_OFS] != NATPMP_VERSION || response[NATPMP_HDR_OP_OFS] != (NATPMP_RESPONSE | NATPMP_OP_MAP_TCP)) {
```

👍 TheCharlatan approved a pull request: "build: Bump clang minimum supported version to 16"
(https://github.com/bitcoin/bitcoin/pull/30263#pullrequestreview-2148321652)
Re-ACK fa8f53273c7e5965620d31a8c3fe5f223cb76888

💬 hebasto commented on pull request "build: Bump clang minimum supported version to 16":
(https://github.com/bitcoin/bitcoin/pull/30263#discussion_r1658924083)
I can confirm that the GHA [macos-14](https://github.com/actions/runner-images/blob/main/images/macos/macos-14-arm64-Readme.md) image with Xcode 16.0 (beta) (build 16A5171c) does not require this workaround.

💬 mzumsande commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1658924696)
Yes, I thought some more and don't find a great way to simplify the logic. I think it would be enough to remove / amend the comment, maybe mentioning instead that FRESH-but-not-DIRTY coins are not expected to occur but would be handled well if they did.

> replace the single FRESH-but-not-DIRTY check

Not sure if that's the "check" you mean, but replacing [this](https://github.com/bitcoin/bitcoin/blob/b27afb7fb774809c9c075d56e44657e0b607a00c/src/coins.cpp#L51-L55) with an assertion was esse
...

💬 TheCharlatan commented on pull request "kernel: De-globalize validation caches":
(https://github.com/bitcoin/bitcoin/pull/30141#issuecomment-2197197304)
475c57ba482abd8675d0d3ccde98452808be5536 -> 4a2be7eb19725be7ff11f40e1fcb764951bea0fe ([noGlobalScriptCache_8](https://github.com/TheCharlatan/bitcoin/tree/noGlobalScriptCache_8) -> [noGlobalScriptCache_9](https://github.com/TheCharlatan/bitcoin/tree/noGlobalScriptCache_9), [compare](https://github.com/TheCharlatan/bitcoin/compare/noGlobalScriptCache_8..noGlobalScriptCache_9))

* Addressed @theuni's [comment_1](https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1658797600) and [comment_2
...

💬 TheCharlatan commented on pull request "kernel: De-globalize validation caches":
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1658934242)
I did this now, but was not completely sure where to put the nonce generation.

📝 maflcko opened a pull request: " scripted-diff: Log parameter interaction not thrice "
(https://github.com/bitcoin/bitcoin/pull/30358)
Seems a bit overkill to log the words "parameter interaction" thrice, when at least once is enough. So do that.

Before:

```
2024-06-28T15:30:57Z [init.cpp:745] [InitParameterInteraction] InitParameterInteraction: parameter interaction: -connect or -maxconnections=0 set -> setting -dnsseed=0
```

After:

```
2024-06-28T15:47:27Z [init.cpp:745] [InitParameterInteraction] parameter interaction: -connect or -maxconnections=0 set -> setting -dnsseed=0

💬 vasild commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1658949236)
Also, it just occurred to me, `GetLocalAddresses()` operates regardless of `-bind=` options. If the machine has two IPv6 addresses but `-bind=` has been used and only one of them specified, then we shouldn't do mapping for the other one (where nobody is listening).

💬 theuni commented on pull request "kernel: De-globalize validation caches":
(https://github.com/bitcoin/bitcoin/pull/30141#issuecomment-2197223467)
> * Applied suggestion in @theuni's [comment_1](https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1657337310) and [comment_2](https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1657354399), making the nonce used to initialize the caches a constructor argument, as well as re-using the same nonce for both caches. I did not expose it as an argument to the chainstate manager, but I'm also not completely sure if it is generated in a better spot now.

Paging @sipa for a quick concept
...

🤔 ryanofsky reviewed a pull request: "logging: Use LogFatal instead LogError for fatal errors"
(https://github.com/bitcoin/bitcoin/pull/30347#pullrequestreview-2148219505)
Updated 475b70da309bbfd889968ce87d251f539f1d3351 -> b27dc672f6e575d739e037b6e46202e619bc386b ([`pr/logfat.1`](https://github.com/ryanofsky/bitcoin/commits/pr/logfat.1) -> [`pr/logfat.2`](https://github.com/ryanofsky/bitcoin/commits/pr/logfat.2), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/logfat.1..pr/logfat.2)) with suggested fixes and expanding it to cover `Logging::Warning` and `Logging::Error`.

---

re: https://github.com/bitcoin/bitcoin/pull/30347#issuecomment-2194685490
...