💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1658390684)
nit:
```suggestion
* write without clearing the cache (CCoinsViewCache::Sync), we must also
```

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1658394351)
nit: redundant comment

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1658417894)
nit:
```suggestion
// Check that calling `ClearFlags` with 0 flags has no effect
```

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1658399777)
Probably not a realistic scenario (may require multithreaded access because of the `m_flags` guard, not sure) , but `m_prev` will never be `null` when we get here, right?

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1658743023)
I find it quite confusing that `AddFlags` modifies the linked list as well (vs `GetFlags` only returning the flags, of course). Could make sense to split it to multiple methods or add the side-effect to the name

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1658391340)
Nit:
```suggestion
* the parent cache for batch writing. This is a performance optimization
```

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1658765626)
Since iteration could misbehave severely and this would pass (e.g. if `Next()` would return `null` we wouldn't even enter the loop), it could make more sense to iterate over the target nodes instead, something like:
```C++
auto node{head.second.Next()};
for (const auto& expected : nodes) {
BOOST_CHECK_EQUAL(&expected, node);
node = node->second.Next();
}
BOOST_CHECK_EQUAL(nullptr, node);
```

(as an added benefit, this has a single `Next()` call)

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1658694832)
TODO: not yet sure why iteration is tied to mutation here

💬 sr-gi commented on pull request "net_processing: make any misbehavior trigger immediate discouragement":
(https://github.com/bitcoin/bitcoin/pull/29575#discussion_r1658897389)
I took a look at this to see if it would make sense to remove the comment, but looks like this is still consistent with the original approach, even after splitting the logic in two for txs and blocks:

https://github.com/bitcoin/bitcoin/commit/ef54b486d5333dfc85c56e6b933c81735196a25d

To me, it seems that, at the very least, this comment would need to be moved one case down, given `BlockValidationResult::BLOCK_MISSING_PREV` triggers misbehavior, so the comment doesn't apply there.

💬 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)) {
```