SubGeniusFinance/Offerings-to-Cthulhu

Watch

Fork 0

build: Phase 3 of UPnP hardening — drop miniupnpc from depends/ + finish Windows CI alignment [post-Codex] #19

New issue

Closed

opened 2026-06-06 23:58:26 +00:00 by dobbscoin · 1 comment

dobbscoin commented

2026-06-06 23:58:26 +00:00

Follow-up to #10. Phase 3 of the UPnP-hardening plan. Phase 1 (release-notes recommendation in v2.0.4/v2.0.5) and Phase 2 (runtime default flip via #14, squash 63626e76) are done; this is the cleanup pass.

Decision: post-Codex. Bundled with the aggressive source-strip below into a single PR landed after block 1,050,667. See § Scope decision.

Surprise finding during scoping

Auditing the tree showed Phase 3 is much smaller than the parent issue suggested. Linux is already there. Only the Windows Qt release still bundles miniupnpc:

Build path	Current UPnP posture
vps3 native Linux Qt	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 Phase 3 is really "finish the Windows side and remove the dependency tree."

Scope — single bundled PR, post-Codex

Two layers, landing together rather than as two separate PRs:

Layer 1 — build-system (`+1 / −28` LOC)

Change	File(s)	Size
Add `--without-miniupnpc` to the Windows CI configure invocation	`.github/workflows/windows-build-depends.yml`	+1 line
Drop `miniupnpc` from the depends package list	`depends/packages/packages.mk:19` (`upnp_packages=miniupnpc`)	-1 line
Delete the depends recipe	`depends/packages/miniupnpc.mk`	-27 lines (whole file)

Layer 2 — source strip (`~−200 LOC`)

Change	File(s)	Size
Remove `MapPort` / `ThreadMapPort` UPnP wrapper code	`src/net.cpp` (under `#ifdef USE_UPNP`)	~150-200 LOC delete
Remove the `-upnp` help-text + SoftSetBoolArg plumbing	`src/init.cpp`	~10 LOC delete
Remove `fUseUPnP` QSettings entry + optionsdialog UPnP checkbox	`src/qt/optionsmodel.cpp`, `src/qt/forms/optionsdialog.ui`	~20 LOC delete
Simplify UPnP detection block	`configure.ac:53-55, 398-403, 590-614`	~30 LOC simplification

Net change: ~+1 / −230 LOC across the combined PR.

Scope decision: why bundled, why post-Codex

The original draft of this issue split Layer 1 (minimum-viable, "arguably safe pre-Codex") from Layer 2 (aggressive, hard-freeze). On reflection that split is the wrong shape:

Layer 1 alone has near-zero operational benefit during the inscription window — slightly smaller binary, ~1-2 min faster CI on cold cache. Not worth burning pre-Codex risk budget.
Layer 1 has nonzero CI-break risk — the Windows mingw cross-compile path has been historically twitchy (cf. the -Wa,-mbig-obj saga, the TOUCHINPUT/Jammy pin). A red CI run on the wrong day adds noise to a window that should stay boring.
Bundling gives cleaner history — one Phase 3 PR removing UPnP support from the tree end-to-end, instead of two PRs across the Codex window's freeze cliff.
No urgency exists — PR #14 already shipped the operational threat fix (default off). Phase 3 is hygiene, not bleeding control.

So: land both layers together as a single PR after block 1,050,667 (Codex window close + safety margin).

Side-effects of the bundled change

Win64 release binary shrinks by the miniupnpc footprint (~150-200 KB stripped)
depends/ cache shrinks; Win64 CI ~1-2 min faster on cold cache
Offeringsd -upnp=1 on the resulting binary becomes a silent no-op (the arg isn't recognized — init.cpp no longer registers it)
Worth a v2.2.x (or whichever release this lands in) release-notes line so existing operators with upnp=1 in Offerings.conf know to remove it

Reversibility

Trivial — git revert the PR. The earlier "irreversible without re-adding the recipe" framing was overstated; even Layer 2's ~200 LOC of UPnP wrapper code is fully 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 (default off). Phase 3 is hardening hygiene.
Not a CVE-style emergency. UPnP compiled in but defaulted off is fine for the threat model.
Not standalone-mergeable pre-Codex — see scope decision above. Don't split this back into two PRs to try to land Layer 1 early.

References

depends/packages/packages.mk:19 — upnp_packages=miniupnpc
depends/packages/miniupnpc.mk — 27-line recipe to delete
.github/workflows/windows-build-depends.yml — needs --without-miniupnpc added
.github/workflows/linux-build-depends.yml:100 — pattern to copy (already has --without-miniupnpc)
configure.ac:53-55, 398-403, 590-614 — UPnP detection + macro definition logic
src/net.cpp, src/init.cpp, src/qt/optionsmodel.cpp — source-side #ifdef USE_UPNP guards to strip
Phase 2 = #14 (merged 63626e76)
Parent threat-model: #10
Modern upstream pattern: bitcoin/bitcoin#23956 (2021, removed miniupnpc from depends in Bitcoin Core)

The Slumbering Lord no longer carries unused trinkets in His pockets. Iä Iä.

Follow-up to #10. **Phase 3** of the UPnP-hardening plan. Phase 1 (release-notes recommendation in v2.0.4/v2.0.5) and Phase 2 (runtime default flip via #14, squash `63626e76`) are done; this is the cleanup pass. > **Decision: post-Codex.** Bundled with the aggressive source-strip below into a single PR landed after block 1,050,667. See § Scope decision. ## Surprise finding during scoping Auditing the tree showed Phase 3 is much smaller than the parent issue suggested. **Linux is already there.** Only the Windows Qt release still bundles miniupnpc: | Build path | Current UPnP posture | |---|---| | **vps3 native Linux Qt** | 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 Phase 3 is really "finish the Windows side and remove the dependency tree." ## Scope — single bundled PR, post-Codex Two layers, landing together rather than as two separate PRs: ### Layer 1 — build-system (`+1 / −28` LOC) | Change | File(s) | Size | |---|---|---| | Add `--without-miniupnpc` to the Windows CI configure invocation | `.github/workflows/windows-build-depends.yml` | +1 line | | Drop `miniupnpc` from the depends package list | `depends/packages/packages.mk:19` (`upnp_packages=miniupnpc`) | -1 line | | Delete the depends recipe | `depends/packages/miniupnpc.mk` | -27 lines (whole file) | ### Layer 2 — source strip (`~−200 LOC`) | Change | File(s) | Size | |---|---|---| | Remove `MapPort` / `ThreadMapPort` UPnP wrapper code | `src/net.cpp` (under `#ifdef USE_UPNP`) | ~150-200 LOC delete | | Remove the `-upnp` help-text + SoftSetBoolArg plumbing | `src/init.cpp` | ~10 LOC delete | | Remove `fUseUPnP` QSettings entry + optionsdialog UPnP checkbox | `src/qt/optionsmodel.cpp`, `src/qt/forms/optionsdialog.ui` | ~20 LOC delete | | Simplify UPnP detection block | `configure.ac:53-55, 398-403, 590-614` | ~30 LOC simplification | Net change: ~`+1 / −230` LOC across the combined PR. ## Scope decision: why bundled, why post-Codex The original draft of this issue split Layer 1 (minimum-viable, "arguably safe pre-Codex") from Layer 2 (aggressive, hard-freeze). On reflection that split is the wrong shape: 1. **Layer 1 alone has near-zero operational benefit** during the inscription window — slightly smaller binary, ~1-2 min faster CI on cold cache. Not worth burning pre-Codex risk budget. 2. **Layer 1 has nonzero CI-break risk** — the Windows mingw cross-compile path has been historically twitchy (cf. the `-Wa,-mbig-obj` saga, the TOUCHINPUT/Jammy pin). A red CI run on the wrong day adds noise to a window that should stay boring. 3. **Bundling gives cleaner history** — one Phase 3 PR removing UPnP support from the tree end-to-end, instead of two PRs across the Codex window's freeze cliff. 4. **No urgency exists** — PR #14 already shipped the operational threat fix (default off). Phase 3 is hygiene, not bleeding control. So: land both layers together as a single PR after **block 1,050,667** (Codex window close + safety margin). ## Side-effects of the bundled change - Win64 release binary shrinks by the miniupnpc footprint (~150-200 KB stripped) - depends/ cache shrinks; Win64 CI ~1-2 min faster on cold cache - `Offeringsd -upnp=1` on the resulting binary becomes a silent no-op (the arg isn't recognized — `init.cpp` no longer registers it) - Worth a v2.2.x (or whichever release this lands in) release-notes line so existing operators with `upnp=1` in `Offerings.conf` know to remove it ## Reversibility Trivial — `git revert` the PR. The earlier "irreversible without re-adding the recipe" framing was overstated; even Layer 2's ~200 LOC of UPnP wrapper code is fully 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 (default off). Phase 3 is hardening hygiene. - **Not a CVE-style emergency.** UPnP compiled in but defaulted off is fine for the threat model. - **Not standalone-mergeable pre-Codex** — see scope decision above. Don't split this back into two PRs to try to land Layer 1 early. ## References - `depends/packages/packages.mk:19` — `upnp_packages=miniupnpc` - `depends/packages/miniupnpc.mk` — 27-line recipe to delete - `.github/workflows/windows-build-depends.yml` — needs `--without-miniupnpc` added - `.github/workflows/linux-build-depends.yml:100` — pattern to copy (already has `--without-miniupnpc`) - `configure.ac:53-55, 398-403, 590-614` — UPnP detection + macro definition logic - `src/net.cpp`, `src/init.cpp`, `src/qt/optionsmodel.cpp` — source-side `#ifdef USE_UPNP` guards to strip - Phase 2 = #14 (merged `63626e76`) - Parent threat-model: #10 - Modern upstream pattern: `bitcoin/bitcoin#23956` (2021, removed miniupnpc from depends in Bitcoin Core) --- *The Slumbering Lord no longer carries unused trinkets in His pockets. Iä Iä.*

dobbscoin commented

2026-06-15 23:49:26 +00:00

Superseded by #37.

Scope widened from "strip miniupnpc, replace with nothing" to "strip miniupnpc, replace with libnatpmp." Reasoning: the strip-only plan would leave home/desktop users with no auto-port-forwarding at all, which is fine for VPS-hosted nodes but a real UX regression for the end-user surface that the Electrum / mobile-extension work in #36 targets.

The Layer 1 (depends/ + CI) and Layer 2 (source rewrite) breakdown carries over to #37 unchanged in shape; only the destination library differs. Post-Codex landing decision (post block 1,050,667) carries over too.

Closing as superseded (this exact scope) rather than completed.

Superseded by #37. Scope widened from "strip miniupnpc, replace with nothing" to "strip miniupnpc, replace with libnatpmp." Reasoning: the strip-only plan would leave home/desktop users with no auto-port-forwarding at all, which is fine for VPS-hosted nodes but a real UX regression for the end-user surface that the Electrum / mobile-extension work in #36 targets. The Layer 1 (depends/ + CI) and Layer 2 (source rewrite) breakdown carries over to #37 unchanged in shape; only the destination library differs. Post-Codex landing decision (post block 1,050,667) carries over too. Closing as superseded (this exact scope) rather than completed.

dobbscoin closed this issue

2026-06-15 23:49:27 +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#19

No description provided.

Rows
Columns