💬 fjahr commented on pull request "rpc: Optimize serialization and enhance metadata of dumptxoutset output":
(https://github.com/bitcoin/bitcoin/pull/29612#issuecomment-2129225055)
Added release notes and dddressed post-merge comments in #30167

💬 pablomartin4btc commented on pull request "Bugfix on TransactionsView - Disable if privacy mode is set during wallet selection":
(https://github.com/bitcoin-core/gui/pull/815#discussion_r1613302049)
Sorry, that was because `setWalletActionsEnabled` was not working properly, and `setWalletActionsEnabled` was not working because I misread your suggestion and I was still calling the function `enableHistoryAction` incorrectly instead of just using directly `historyAction->setEnabled`. It fixed now in 260d6eb. Thanks!

💬 willcl-ark commented on pull request "ci: add markdown link check job":
(https://github.com/bitcoin/bitcoin/pull/30034#discussion_r1613305686)
Removed in 4b7d9842691046b01f0c08d69f924ddb62ccc4c6

💬 pablomartin4btc commented on pull request "Bugfix on TransactionsView - Disable if privacy mode is set during wallet selection":
(https://github.com/bitcoin-core/gui/pull/815#issuecomment-2129275595)
Updates:
- Incorporated @luke-jr's [suggestion](https://github.com/bitcoin-core/gui/pull/815#issuecomment-2127782169), simplifying the fix.

👍 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