👍 maflcko approved a pull request: "doc, rpc: Release notes and follow-ups for #29612"
(https://github.com/bitcoin/bitcoin/pull/30167#pullrequestreview-2076606115)
lgtm, apart from the assert

💬 maflcko commented on pull request "doc, rpc: Release notes and follow-ups for #29612":
(https://github.com/bitcoin/bitcoin/pull/30167#discussion_r1613303760)
nit: Why not use the prams from chainman? Seems more consistent, because the chainstate is also used from chainman. (Same nit on all other places)

💬 maflcko commented on pull request "doc, rpc: Release notes and follow-ups for #29612":
(https://github.com/bitcoin/bitcoin/pull/30167#discussion_r1613308886)
Not sure about turning an internal logic error in serialization into a node crash. Seems best to remove this line, because `.value()` will already handle the internal logic error correctly by throwing an exception.

💬 maflcko commented on pull request "ci: add markdown link check job":
(https://github.com/bitcoin/bitcoin/pull/30034#issuecomment-2129283769)
re-utACK 4b7d9842691046b01f0c08d69f924ddb62ccc4c6

💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2129306509)
Do you know what's the minimum FreeBSD version it can be compiled on? Let's bump the version to that.

💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2129309096)
> Do you know what's the minimum FreeBSD version it can be compiled on? Let's bump the version bound to that.

I'll spin up some more recent VMs to test, probably next week though.

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