💬 maflcko commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2129331327)
For testing, something like https://github.com/bitcoin/bitcoin/pull/30164 could be used

👍 willcl-ark approved a pull request: "[27.x] Backports and rc1"
(https://github.com/bitcoin/bitcoin/pull/30092#pullrequestreview-2076707734)
ACK cb6def3855427b613357696ba16a431c7964dbcc

Reviewed that all backports were unmodified, and that associated PR #'s match.

Lightly reviewed release notes and manpage and they look good to me.

💬 maflcko commented on issue "prune shall not delete blocks it did not download":
(https://github.com/bitcoin/bitcoin/issues/30163#issuecomment-2129349133)
> > Using the same blocksdir for two different nodes is not supported. Nodes may download blocks in a different order and save them to different locations in the blocksfiles. This will lead to an error at some point, latest when one of the nodes can't find a block where it believes to be one.
> > Currently, I don't think what you are trying to achieve is possible without copying blocks.
>
> Hm, my experience differs: using this regularly and never ran into issues. Since the external HDD cont
...

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