💬 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
...

💬 ryanofsky commented on pull request "logging: Use LogFatal instead LogError for fatal errors":
(https://github.com/bitcoin/bitcoin/pull/30347#discussion_r1658862362)
re: https://github.com/bitcoin/bitcoin/pull/30347#discussion_r1657166039

I agree with you that the logging header should have better documentation, but I also think this section of developer notes is useful and provides good background information.

I actually already made some pretty significant improvements to documentation and organization of the logging header in #29256. If you would be interested in reviewing that PR (even just the documentation changes) I would really appreciate it. T
...

💬 ryanofsky commented on pull request "logging: Use LogFatal instead LogError for fatal errors":
(https://github.com/bitcoin/bitcoin/pull/30347#discussion_r1658862683)
re: https://github.com/bitcoin/bitcoin/pull/30347#discussion_r1657169459

Agree this does create a small maintenence burden. FWIW, #29256 consolidates and [simplifies the macro definitions](https://github.com/ryanofsky/bitcoin/blob/681b21f7ca07f0e1411354baa01649e7b1f30e8c/src/logging.h#L327-L332) so the burden can get a littler smaller.

Overall though, I think the macros provide convenience and efficiency. I think we can support having both convenience and variable logging levels, and not
...

💬 ryanofsky commented on pull request "logging: Use LogFatal instead LogError for fatal errors":
(https://github.com/bitcoin/bitcoin/pull/30347#discussion_r1658862564)
re: https://github.com/bitcoin/bitcoin/pull/30347#discussion_r1657237193

> I don't think it makes sense for us to have 7 levels of logging -- even 5 is a lot. Renaming Warning to Critical and Error to Fatal for the reasons given seems fine, but the old names shouldn't be kept around in that case.

I think the reality is we already have 7 levels of logging, but we are using 5 names to describe them. This PR is a small fix to make the names used in the code match the intent of the code.

Fo
...

💬 mzumsande commented on pull request "Assumeutxo: bugfix on loadtxoutset with a divergent chain + tests":
(https://github.com/bitcoin/bitcoin/pull/29996#issuecomment-2197228936)
> Rearranging the order of the commits to make it easier to test the bugfix: the first test ([53f714d](https://github.com/bitcoin/bitcoin/commit/53f714d8bfd2331651445bcadb773a10f4d30264)) can be run without the bugfix and is expected to fail, but it should succeed after applying the fix ([65343ec](https://github.com/bitcoin/bitcoin/commit/65343ec49a6b73c4197dfc38e1c2f433b0a3838a)).

I'd suggest to revert that: The CI should to pass on each commit, we even have a designated "test each commit" j
...

💬 sipa commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#issuecomment-2197235201)
If there are further nits, I will address them in a follow-up.

Ready for more (re)review.

💬 TheCharlatan commented on pull request "RFC: Instanced logs":
(https://github.com/bitcoin/bitcoin/pull/30338#issuecomment-2197248760)
Re https://github.com/bitcoin/bitcoin/pull/30338#issuecomment-2194940918

> I don't understand where this is coming from. Libraries can export globals, eg stdin, so I don't see why you're claiming it's necessary for the kernel when it's not even necessary for libc.

Being forced to eliminate all globals doesn't seem to be the claim here? As long as the globals don't carry over annoying state from previous usage, require annoying initialization, or aren't mostly safe from user misuse, their u
...