SubGeniusFinance/Offerings-to-Cthulhu

Watch

Fork 0

build: UPnP → libnatpmp migration — supersedes #19 #37

New issue

Closed

opened 2026-06-15 23:48:56 +00:00 by dobbscoin · 0 comments

dobbscoin commented

2026-06-15 23:48:56 +00:00

Goal

Replace miniupnpc with libnatpmp as OFF's port-mapping library. Removes the cross-fleet portability surface that has been a recurring source of pain (libminiupnpc so-major churn across distros, configure auto-detect overriding depends/ static link) while preserving auto-port-forwarding for users behind home-router NAT.

This issue supersedes #19. The strip-only plan in #19 would have left end-users with no auto-port-forwarding at all — fine for VPS-hosted nodes, regression for home/desktop users (and especially for the mobile / extension client surface tracked in #36).

Flagged by @9019x on 2026-06-14 as a precondition for retiring the existing cross-fleet portability headache.

Why libnatpmp specifically

	libminiupnpc	libnatpmp
Library size	~30K LoC	~3K LoC
Protocol	SSDP discovery + SOAP requests over HTTP	Single UDP request/response on port 5351
so-major churn	Multiple bumps over the years (`.so.16/17/18` across distros) — root cause of the cross-fleet portability headache	API stable since ~2010; very few so-major versions in the wild
Static-link via depends	Works but `configure` auto-detects host versions and overrides	Clean — small static link, no host interaction risk
Upstream Bitcoin Core	Removed from depends in `bitcoin/bitcoin#23956` (2021)	Added in `bitcoin/bitcoin#19686` (2021, v0.21.0)

Bitcoin Core upstream did the same swap in 2021 for the same reason. There's a clean reference implementation to port from.

Current state

Auditing the tree:

Build path	Current UPnP posture
Linux native Qt build	already `--without-miniupnpc` per the build recipe
Linux daemon-only CI (`linux-build-depends.yml:100`)	already `--without-miniupnpc`
Windows Qt CI (`windows-build-depends.yml`)	still bundles miniupnpc 2.2.2 from `depends/`

So the strip half of the work is small (Windows-CI cleanup + depends/ recipe deletion). The add half is the new scope: wire libnatpmp into the same MapPort / ThreadMapPort interface the daemon already uses.

Scope

Single bundled PR landing post-Codex (after block 1,050,667). Two layers, same shape as #19 but with libnatpmp added.

Layer 1 — `depends/` + CI

Change	File(s)	Size
Add `--without-miniupnpc --with-natpmp` to the Windows CI configure invocation	`.github/workflows/windows-build-depends.yml`	+1 line edit
Replace `upnp_packages=miniupnpc` with `nat_packages=natpmp`	`depends/packages/packages.mk:19`	-1 / +1
Delete `depends/packages/miniupnpc.mk`	(whole file)	-27 lines
Add `depends/packages/natpmp.mk`	(new file)	+~25 lines (mirror the miniupnpc.mk shape)

Layer 2 — source rewrite

Change	File(s)	Size
Swap miniupnpc includes for libnatpmp	`src/net.cpp:24-27`	-4 / +1
Rewrite `ThreadMapPort` body — drop SSDP discovery and SOAP plumbing; use libnatpmp's `sendpublicaddressrequest` / `readnatpmpresponseorretry` / `sendnewportmappingrequest` pattern	`src/net.cpp:953-1031`	-100 / +60
`MapPort` wrapper interface unchanged; body simplified	`src/net.cpp:1038-1060`	-5 / +5
`-upnp` arg remains as deprecated alias for `-natpmp` (one release of mercy; see "Backward compat" below)	`src/init.cpp:236`	+5
`configure.ac`: swap `--with-miniupnpc` for `--with-natpmp`, update detection block	`configure.ac:53-55, 398-403, 590-614`	rewrite ~30 LoC
Relabel UI checkbox `Use UPnP` → `Map ports via NAT-PMP`	`src/qt/optionsmodel.cpp`, `src/qt/forms/optionsdialog.ui`	~20 LoC adj

Net change: roughly +~150 / -230 LoC.

Backward compat for the `-upnp` flag

Existing operators with upnp=1 in Offerings.conf should not be silently broken. Two options were considered; this issue commits to (a):

(a) Default — deprecated alias for one release. -upnp=1 is accepted, internally translated to -natpmp=1, and emits a one-time WARN log on startup pointing to the new flag. Removal of the alias is its own follow-up issue, no earlier than the release after this one.
(b) Hard rename. Rejected — silently breaks home-operator configs.

Why this isn't standalone-mergeable pre-Codex

Same logic as #19. Not consensus-changing, not network-protocol-changing, no urgency. Risk budget around the Codex inscription window should stay protected for things that actually matter to chain health. Land the bundled PR post-block-1,050,667.

Side-effects

Win64 release binary loses the miniupnpc footprint and gains the smaller libnatpmp footprint. Net: ~120 KB smaller stripped.
depends/ cache shrinks marginally; Win64 CI ~1-2 min faster on cold cache.
Home-router users behind NAT-PMP-capable gateways get clean auto-port-forwarding.
UPnP-only routers no longer auto-map; users get a one-line release-notes pointer to manual port forwarding (or to upgrading their gateway).
Cross-fleet binary portability surface goes away. libnatpmp's stable API removes the so-major drift problem.

Reversibility

Trivial — git revert the PR. Both layers recoverable from git history.

Trade-offs / what this isn't

Not consensus-changing, not network-protocol-changing.
Not a Restoration blocker. PR #14 already removed the operational threat by defaulting -upnp off.
Not standalone-mergeable pre-Codex — see above.
Not a "kill auto-port-forwarding" change — this issue exists precisely to preserve that capability for end-users while retiring the troublesome library underneath it.

References

Upstream Bitcoin Core:
- bitcoin/bitcoin#19686 (libnatpmp added, 2021, v0.21.0)
- bitcoin/bitcoin#23956 (miniupnpc removed from depends, 2021)
depends/packages/packages.mk:19 — current upnp_packages=miniupnpc line
depends/packages/miniupnpc.mk — recipe to delete
.github/workflows/windows-build-depends.yml — needs --without-miniupnpc --with-natpmp
.github/workflows/linux-build-depends.yml:100 — already --without-miniupnpc
src/net.cpp:24-27, 953-1060 — port-mapping code surface
src/init.cpp:236 — -upnp arg help text
configure.ac:53-55, 398-403, 590-614 — UPnP detection block
Issue #19 — original strip-only scope (superseded by this issue)
Issue #36 — Electrum-server / mobile-extension wallet enablement (downstream beneficiary of preserving auto-port-forwarding)

Community chat: https://23skidoo.info/discord

## Goal Replace miniupnpc with libnatpmp as OFF's port-mapping library. Removes the cross-fleet portability surface that has been a recurring source of pain (`libminiupnpc` so-major churn across distros, `configure` auto-detect overriding `depends/` static link) while preserving auto-port-forwarding for users behind home-router NAT. This issue **supersedes #19**. The strip-only plan in #19 would have left end-users with no auto-port-forwarding at all — fine for VPS-hosted nodes, regression for home/desktop users (and especially for the mobile / extension client surface tracked in #36). Flagged by @9019x on 2026-06-14 as a precondition for retiring the existing cross-fleet portability headache. ## Why libnatpmp specifically | | libminiupnpc | libnatpmp | |---|---|---| | Library size | ~30K LoC | ~3K LoC | | Protocol | SSDP discovery + SOAP requests over HTTP | Single UDP request/response on port 5351 | | so-major churn | Multiple bumps over the years (`.so.16/17/18` across distros) — root cause of the cross-fleet portability headache | API stable since ~2010; very few so-major versions in the wild | | Static-link via depends | Works but `configure` auto-detects host versions and overrides | Clean — small static link, no host interaction risk | | Upstream Bitcoin Core | Removed from depends in `bitcoin/bitcoin#23956` (2021) | Added in `bitcoin/bitcoin#19686` (2021, v0.21.0) | Bitcoin Core upstream did the same swap in 2021 for the same reason. There's a clean reference implementation to port from. ## Current state Auditing the tree: | Build path | Current UPnP posture | |---|---| | Linux native Qt build | already `--without-miniupnpc` per the build recipe | | Linux daemon-only CI (`linux-build-depends.yml:100`) | already `--without-miniupnpc` | | Windows Qt CI (`windows-build-depends.yml`) | **still bundles miniupnpc 2.2.2 from `depends/`** | So the strip half of the work is small (Windows-CI cleanup + `depends/` recipe deletion). The add half is the new scope: wire libnatpmp into the same `MapPort` / `ThreadMapPort` interface the daemon already uses. ## Scope Single bundled PR landing post-Codex (after block 1,050,667). Two layers, same shape as #19 but with libnatpmp added. ### Layer 1 — `depends/` + CI | Change | File(s) | Size | |---|---|---| | Add `--without-miniupnpc --with-natpmp` to the Windows CI configure invocation | `.github/workflows/windows-build-depends.yml` | +1 line edit | | Replace `upnp_packages=miniupnpc` with `nat_packages=natpmp` | `depends/packages/packages.mk:19` | -1 / +1 | | Delete `depends/packages/miniupnpc.mk` | (whole file) | -27 lines | | Add `depends/packages/natpmp.mk` | (new file) | +~25 lines (mirror the miniupnpc.mk shape) | ### Layer 2 — source rewrite | Change | File(s) | Size | |---|---|---| | Swap miniupnpc includes for libnatpmp | `src/net.cpp:24-27` | -4 / +1 | | Rewrite `ThreadMapPort` body — drop SSDP discovery and SOAP plumbing; use libnatpmp's `sendpublicaddressrequest` / `readnatpmpresponseorretry` / `sendnewportmappingrequest` pattern | `src/net.cpp:953-1031` | -100 / +60 | | `MapPort` wrapper interface unchanged; body simplified | `src/net.cpp:1038-1060` | -5 / +5 | | `-upnp` arg remains as deprecated alias for `-natpmp` (one release of mercy; see "Backward compat" below) | `src/init.cpp:236` | +5 | | `configure.ac`: swap `--with-miniupnpc` for `--with-natpmp`, update detection block | `configure.ac:53-55, 398-403, 590-614` | rewrite ~30 LoC | | Relabel UI checkbox `Use UPnP` → `Map ports via NAT-PMP` | `src/qt/optionsmodel.cpp`, `src/qt/forms/optionsdialog.ui` | ~20 LoC adj | Net change: roughly `+~150 / -230` LoC. ## Backward compat for the `-upnp` flag Existing operators with `upnp=1` in `Offerings.conf` should not be silently broken. Two options were considered; this issue commits to (a): - **(a) Default — deprecated alias for one release.** `-upnp=1` is accepted, internally translated to `-natpmp=1`, and emits a one-time WARN log on startup pointing to the new flag. Removal of the alias is its own follow-up issue, no earlier than the release after this one. - (b) Hard rename. Rejected — silently breaks home-operator configs. ## Why this isn't standalone-mergeable pre-Codex Same logic as #19. Not consensus-changing, not network-protocol-changing, no urgency. Risk budget around the Codex inscription window should stay protected for things that actually matter to chain health. Land the bundled PR post-block-1,050,667. ## Side-effects - Win64 release binary loses the miniupnpc footprint and gains the smaller libnatpmp footprint. Net: ~120 KB smaller stripped. - `depends/` cache shrinks marginally; Win64 CI ~1-2 min faster on cold cache. - Home-router users behind NAT-PMP-capable gateways get clean auto-port-forwarding. - UPnP-only routers no longer auto-map; users get a one-line release-notes pointer to manual port forwarding (or to upgrading their gateway). - Cross-fleet binary portability surface goes away. libnatpmp's stable API removes the so-major drift problem. ## Reversibility Trivial — `git revert` the PR. Both layers recoverable from git history. ## Trade-offs / what this isn't - **Not consensus-changing**, **not network-protocol-changing**. - **Not a Restoration blocker.** PR #14 already removed the operational threat by defaulting `-upnp` off. - **Not standalone-mergeable pre-Codex** — see above. - Not a "kill auto-port-forwarding" change — this issue exists precisely to preserve that capability for end-users while retiring the troublesome library underneath it. ## References - Upstream Bitcoin Core: - `bitcoin/bitcoin#19686` (libnatpmp added, 2021, v0.21.0) - `bitcoin/bitcoin#23956` (miniupnpc removed from depends, 2021) - `depends/packages/packages.mk:19` — current `upnp_packages=miniupnpc` line - `depends/packages/miniupnpc.mk` — recipe to delete - `.github/workflows/windows-build-depends.yml` — needs `--without-miniupnpc --with-natpmp` - `.github/workflows/linux-build-depends.yml:100` — already `--without-miniupnpc` - `src/net.cpp:24-27, 953-1060` — port-mapping code surface - `src/init.cpp:236` — `-upnp` arg help text - `configure.ac:53-55, 398-403, 590-614` — UPnP detection block - Issue #19 — original strip-only scope (superseded by this issue) - Issue #36 — Electrum-server / mobile-extension wallet enablement (downstream beneficiary of preserving auto-port-forwarding) Community chat: https://23skidoo.info/discord

dobbscoin closed this issue

2026-08-11 13:19:43 +00:00

No labels

enhancement

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

SubGeniusFinance/Offerings-to-Cthulhu#37

No description provided.

Rows
Columns