🤔 Sjors reviewed a pull request: "net: Replace libnatpmp with built-in PCP+NATPMP implementation"
(https://github.com/bitcoin/bitcoin/pull/30043#pullrequestreview-2076503238)
`pcp.{h,cpp}` (mostly) look good to me as well, but I haven't tested the NAT-PMP fallback.

Next on my review list is the remaining code that drops the old Nat-PMP dependency, but that's probably fine.

💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1613380035)
It seems like the gateway can keep us waiting forever by sending an invalid response at least once per second. Should be give up at some point?

💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1613266348)
```cpp
// We only attempt renewal once.
```

The spec recommend trying again with ever shorter intervals, but no less than 4 seconds, as the deadline approached. But it seems fine to not bother.

If renewal fails we fall out of the loop, `ProcessPCP` returns. At that point we fall back to UPNP (if enabled) and/or try again 5 minutes later.

Renewal for NAT-PMP works the same it seems, see towards the end of https://datatracker.ietf.org/doc/html/rfc6886#section-3.3

💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1613246935)
typo: portmap

⚠️ maflcko opened an issue: "descriptor: Tapscript-specific Miniscript key serialization / parsing leads to fuzz timeouts"
(https://github.com/bitcoin/bitcoin/issues/30168)
This started happening after 8571b89a7fce50229242ef3c6d9f807949f716a3 / db283a6b6f1419291bcd15d74d51c8598aefe06a . So far there are two different timeout flamegraphs for the `descriptor_parse` fuzz target.

* https://github.com/bitcoin/bitcoin/issues/28812#issuecomment-2128761184
* https://github.com/bitcoin/bitcoin/issues/28812#issuecomment-2098816463

💬 Sjors commented on pull request "[PoC] ci: Add FreeBSD GitHub Actions job":
(https://github.com/bitcoin/bitcoin/pull/30164#issuecomment-2129384678)
Which FreeBSD version is this? The oldest maintained release is 13.2 I believe, though we'll have to install clang 15 on it.

👍 brunoerg approved a pull request: "fuzz: More accurate coverage reports"
(https://github.com/bitcoin/bitcoin/pull/30156#pullrequestreview-2076772413)
nice, utACK 949abebea0059edd929b653b4b475a5880fc0a3e

📝 maflcko opened a pull request: "fuzz: Fix wallet_bdb_parser stdlib error matching"
(https://github.com/bitcoin/bitcoin/pull/30169)
The stdlib error string is an implementation detail and can not be relied upon.

Ref: `libc++abi: terminating due to uncaught exception of type std::runtime_error: AutoFile::read: end of file: unspecified iostream_category error`

📝 maflcko opened a pull request: "refactor: Use type-safe time in txorphanage"
(https://github.com/bitcoin/bitcoin/pull/30170)
This allows to remove manual conversions like multiplication by `60`, and uses a type-safe type instead of a raw `int64_t`.

💬 maflcko commented on pull request "[PoC] ci: Add FreeBSD GitHub Actions job":
(https://github.com/bitcoin/bitcoin/pull/30164#issuecomment-2129420150)
@Sjors According to https://github.com/vmactions/freebsd-vm?tab=readme-ov-file#under-the-hood you can use `with_release` to pick one.

💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2129421039)
The first two commits of https://github.com/bitcoin/bitcoin/pull/26812 would make it possible to test and fuzz how this code interacts with a router.

💬 apulsifer commented on issue "LevelDB read failure: Corruption: block checksum mismatch":
(https://github.com/bitcoin/bitcoin/issues/30159#issuecomment-2129447054)
Took about 20 minutes. They all match.

Fri May 24 12:12:33 UTC 2024
{
"height": 844917,
"bestblock": "000000000000000000017dd5f59b73629f6f88797e90017a9df39c5e435296bf",
"txouts": 181984254,
"bogosize": 13994337434,
"muhash": "b97a3fb13a61e8c889668d064f7ae5408a78ee7e4c6a4fdebedb30ecb2f23378",
"total_amount": 19702649.24256659,
"transactions": 125720653,
"disk_size": 12220356091
}
Fri May 24 12:39:58 UTC 2024

💬 TheCharlatan commented on pull request "indexes: Don't wipe indexes again when continuing a prior reindex":
(https://github.com/bitcoin/bitcoin/pull/30132#issuecomment-2129452748)
Thanks for the reviews @maflcko,

891784ce73a1d911d33b125b66f3856fe6cda56b -> eeea0818c1a20adc5225b98b185953d386c033e0 ([preserveIndexOnRestart_3](https://github.com/TheCharlatan/bitcoin/tree/preserveIndexOnRestart_3) -> [preserveIndexOnRestart_4](https://github.com/TheCharlatan/bitcoin/tree/preserveIndexOnRestart_4), [compare](https://github.com/TheCharlatan/bitcoin/compare/preserveIndexOnRestart_3..preserveIndexOnRestart_4))

* Addressed @maflcko's [comment](https://github.com/bitcoin/bitc
...

✅ zefir-k closed an issue: "prune shall not delete blocks it did not download"
(https://github.com/bitcoin/bitcoin/issues/30163)

💬 TheCharlatan commented on pull request "indexes: Don't wipe indexes again when continuing a prior reindex":
(https://github.com/bitcoin/bitcoin/pull/30132#discussion_r1613414830)
Yeah, I was thinking this might be used for some more regresssion tests, but as it is right now, it is useless. Will remove.

💬 fanquake commented on pull request "[PoC] ci: Add FreeBSD GitHub Actions job":
(https://github.com/bitcoin/bitcoin/pull/30164#issuecomment-2129492145)
> I believe, though we'll have to install clang 15 on it.

Then we should pick something newer, or which won't be a blocker to using a newer Clang. Because soon our minimum Clang will be 16.

🤔 vasild reviewed a pull request: "init: fixes file descriptor accounting"
(https://github.com/bitcoin/bitcoin/pull/30065#pullrequestreview-2076548666)
Approach ACK 55f16f56f4

Might as well squash the last commit 55f16f56f4 `init: Remove FreeBSD workaround to #2695` into some of the previous ones that touch that line.

💬 vasild commented on pull request "init: fixes file descriptor accounting":
(https://github.com/bitcoin/bitcoin/pull/30065#discussion_r1613282956)
```suggestion
// If we are using select instead of poll, our actual limit may be even smaller
```

💬 vasild commented on pull request "init: fixes file descriptor accounting":
(https://github.com/bitcoin/bitcoin/pull/30065#discussion_r1613272500)
In the commit message of a44da681f5 `init: fixes fd accounting regarding poll/select`:

> ... file descriptions limits ...

s/descriptions/descriptors/

💬 vasild commented on pull request "init: fixes file descriptor accounting":
(https://github.com/bitcoin/bitcoin/pull/30065#discussion_r1613459411)
snake_case for variables. I think `available` or `available_fds` is a good name:

```suggestion
available_fds = RaiseFileDescriptorLimit(nMaxConnections + nMinRequiredFds);
```