SubGeniusFinance/Offerings-to-Cthulhu

Watch

Fork 0

net: capture peer addrMe votes via AddLocal(LOCAL_PEER) instead of SeenLocal #7

New issue

Closed

opened 2026-06-03 20:21:30 +00:00 by dobbscoin · 1 comment

dobbscoin commented

2026-06-03 20:21:30 +00:00

Problem

At src/main.cpp:3895-3898, every inbound peer's addrMe vote — i.e., "this is the address I see you coming from" — is fed to SeenLocal(addrMe):

if (pfrom->fInbound && addrMe.IsRoutable())
{
    pfrom->addrLocal = addrMe;
    SeenLocal(addrMe);
}

But SeenLocal at src/net.cpp:271-283 is only an increment:

bool SeenLocal(const CService& addr)
{
    {
        LOCK(cs_mapLocalHost);
        if (mapLocalHost.count(addr) == 0)
            return false;       // <-- silently discards
        mapLocalHost[addr].nScore++;
    }
    AdvertizeLocal();
    return true;
}

If no static / LOCAL_BIND / LOCAL_IF / LOCAL_UPNP / LOCAL_HTTP entry already exists for that address, the peer's vote is silently thrown away. Peer agreement — which is the single most reliable signal of what our actual external address is — never gets a chance to seed mapLocalHost.

The change

Three lines.

Add a LOCAL_PEER score tier in src/net.h, slotted between LOCAL_BIND and LOCAL_MANUAL (mirroring upstream bitcoin/bitcoin#7028):

enum LocalServiceFlags {
    LOCAL_NONE,
    LOCAL_IF,
    LOCAL_BIND,
    LOCAL_PEER,    // new
    LOCAL_UPNP,
    LOCAL_HTTP,
    LOCAL_MANUAL,
    LOCAL_MAX
};

Swap SeenLocal → AddLocal at src/main.cpp:3898:

-            SeenLocal(addrMe);
+            AddLocal(addrMe, LOCAL_PEER);

That's it. Existing AddLocal (src/net.cpp:216-243) already does the right thing: refuses non-routable / limited, creates if absent, increments score if present, calls AdvertizeLocal().

Why this is strictly an improvement

No code is removed. The HTTP path, UPnP, LOCAL_IF, LOCAL_BIND all keep doing what they do.
Peer agreement now seeds the map. Multiple inbound peers reporting the same addrMe accumulates LOCAL_PEER score and competes naturally with other sources.
Score tier is the lowest among meaningful tiers. A static LOCAL_BIND or -externalip= config still wins; peer votes are a fallback, not an override.
One-malicious-peer-pollution is bounded. A hostile inbound peer can plant one LOCAL_PEER entry, but honest peers' competing votes create their own entries with their own score, and LOCAL_BIND / LOCAL_MANUAL always beat LOCAL_PEER.

Files touched

src/net.h — add LOCAL_PEER enum value
src/main.cpp — one-line swap at line 3898

Trade-offs / risks

Cluster impact: ~zero. All of vps1 / vps3 / chaos run behind known public IPs and either let the OS report or configure -externalip= explicitly. The cluster's external-IP discovery is already handled outside this code path. So this change is a hygiene fix benefiting third-party operators, not us.
Behavior change in -debug=net logs: an extra AddLocal(%s, 3) line per first-time inbound peer at startup. Cosmetic.
Subtle: doesn't help outbound-only NAT'd nodes. The pfrom->fInbound guard at line 3895 means only inbound peers feed addrMe. A node behind NAT with no port-forwarding still learns nothing. Accepted — such a node is unreachable anyway, no useful address to advertise.

Relation to issue #5

Issue #5 proposed the same swap plus deleting the HTTP path (GetMyExternalIP / GetMyExternalIP2 / ThreadGetMyExternalIP). Splitting that into two issues:

This issue (#7) — capture the peer-vote signal. Strict improvement, no deletion, trivial review.
Issue #5 — now scoped to only the HTTP retirement, predicated on this issue being merged first.

Land this first, sit on the deletion in #5 until we've actually seen peer-vote scores accumulate on a real node in -debug=net logs.

References

Upstream PR: bitcoin/bitcoin#7028 — "Net: Remove getmyexternalip" (Pieter Wuille, 2016)
Issue #5 (now scoped to deletion-only): https://github.com/SubGeniusFinance/Offerings-to-Cthulhu/issues/5
PR #4 (the interim HTTP fix that made #5 / this issue worth pursuing): https://github.com/SubGeniusFinance/Offerings-to-Cthulhu/pull/4

## Problem At `src/main.cpp:3895-3898`, every inbound peer's `addrMe` vote — i.e., "this is the address I see you coming from" — is fed to `SeenLocal(addrMe)`: ```cpp if (pfrom->fInbound && addrMe.IsRoutable()) { pfrom->addrLocal = addrMe; SeenLocal(addrMe); } ``` But `SeenLocal` at `src/net.cpp:271-283` is **only an increment**: ```cpp bool SeenLocal(const CService& addr) { { LOCK(cs_mapLocalHost); if (mapLocalHost.count(addr) == 0) return false; // <-- silently discards mapLocalHost[addr].nScore++; } AdvertizeLocal(); return true; } ``` If no static / `LOCAL_BIND` / `LOCAL_IF` / `LOCAL_UPNP` / `LOCAL_HTTP` entry already exists for that address, **the peer's vote is silently thrown away.** Peer agreement — which is the single most reliable signal of what our actual external address is — never gets a chance to seed `mapLocalHost`. ## The change Three lines. 1. **Add a `LOCAL_PEER` score tier** in `src/net.h`, slotted between `LOCAL_BIND` and `LOCAL_MANUAL` (mirroring upstream [bitcoin/bitcoin#7028](https://github.com/bitcoin/bitcoin/pull/7028)): ```cpp enum LocalServiceFlags { LOCAL_NONE, LOCAL_IF, LOCAL_BIND, LOCAL_PEER, // new LOCAL_UPNP, LOCAL_HTTP, LOCAL_MANUAL, LOCAL_MAX }; ``` 2. **Swap `SeenLocal` → `AddLocal`** at `src/main.cpp:3898`: ```cpp - SeenLocal(addrMe); + AddLocal(addrMe, LOCAL_PEER); ``` That's it. Existing `AddLocal` (`src/net.cpp:216-243`) already does the right thing: refuses non-routable / limited, creates if absent, increments score if present, calls `AdvertizeLocal()`. ## Why this is strictly an improvement - **No code is removed.** The HTTP path, UPnP, `LOCAL_IF`, `LOCAL_BIND` all keep doing what they do. - **Peer agreement now seeds the map.** Multiple inbound peers reporting the same `addrMe` accumulates `LOCAL_PEER` score and competes naturally with other sources. - **Score tier is the lowest among meaningful tiers.** A static `LOCAL_BIND` or `-externalip=` config still wins; peer votes are a *fallback*, not an override. - **One-malicious-peer-pollution is bounded.** A hostile inbound peer can plant one `LOCAL_PEER` entry, but honest peers' competing votes create their own entries with their own score, and `LOCAL_BIND` / `LOCAL_MANUAL` always beat `LOCAL_PEER`. ## Files touched - `src/net.h` — add `LOCAL_PEER` enum value - `src/main.cpp` — one-line swap at line 3898 ## Trade-offs / risks - **Cluster impact: ~zero.** All of vps1 / vps3 / chaos run behind known public IPs and either let the OS report or configure `-externalip=` explicitly. The cluster's external-IP discovery is already handled outside this code path. So this change is a hygiene fix benefiting third-party operators, not us. - **Behavior change in `-debug=net` logs:** an extra `AddLocal(%s, 3)` line per first-time inbound peer at startup. Cosmetic. - **Subtle: doesn't help outbound-only NAT'd nodes.** The `pfrom->fInbound` guard at line 3895 means only inbound peers feed `addrMe`. A node behind NAT with no port-forwarding still learns nothing. Accepted — such a node is unreachable anyway, no useful address to advertise. ## Relation to issue #5 Issue #5 proposed the same swap **plus deleting the HTTP path** (`GetMyExternalIP` / `GetMyExternalIP2` / `ThreadGetMyExternalIP`). Splitting that into two issues: - **This issue (#7)** — capture the peer-vote signal. Strict improvement, no deletion, trivial review. - **Issue #5** — now scoped to *only* the HTTP retirement, predicated on this issue being merged first. Land this first, sit on the deletion in #5 until we've actually seen peer-vote scores accumulate on a real node in `-debug=net` logs. ## References - Upstream PR: [bitcoin/bitcoin#7028](https://github.com/bitcoin/bitcoin/pull/7028) — "Net: Remove getmyexternalip" (Pieter Wuille, 2016) - Issue #5 (now scoped to deletion-only): https://github.com/SubGeniusFinance/Offerings-to-Cthulhu/issues/5 - PR #4 (the interim HTTP fix that made #5 / this issue worth pursuing): https://github.com/SubGeniusFinance/Offerings-to-Cthulhu/pull/4

dobbscoin commented

2026-06-04 12:13:01 +00:00

Closing as substantively obviated by PR #9 (merge commit 24196aa), with an architectural caveat worth recording.

PR #9 ported gmaxwell's 2014 design (bitcoin/bitcoin#5161), not Wuille's 2016 design (bitcoin/bitcoin#7028) that this issue cites. The difference:

This issue's proposal (bitcoin#7028, 2016)	PR #9 reality (bitcoin#5161, 2014)
Peer votes → global `mapLocalHost` with new `LOCAL_PEER` tier	Peer's `addrMe` stored per-CNode in `pfrom->addrLocal`
Influences `GetLocalAddress()` for all peers	Used only when advertising back to the same peer that told us, via `IsPeerAddrLocalGood()`
Adds `LOCAL_PEER` enum value	Doesn't touch the enum (removes `LOCAL_HTTP`)

For the OFF cluster (which uses -externalip= everywhere) the two designs are functionally equivalent. The one residual benefit of layering this issue's LOCAL_PEER aggregation on top of PR #9 is for a fresh outbound-only NAT'd node that wants to learn its IP from one peer and advertise it to a different peer. The gmaxwell-2014 approach in PR #9 only ever advertises a peer-told IP back to the same peer who told us.

Closing because:

The original concern in this issue ("peer agreement never gets a chance to seed mapLocalHost") is moot — peer agreement no longer needs to seed mapLocalHost, because the per-peer addrLocal is consulted directly in AdvertizeLocal().
The marginal benefit of the global-aggregation path doesn't move the needle for the cluster.
If anyone (including future-us) wants this later, the change is now strictly additive on top of PR #9 — re-open then.

Thanks @9019x for the broader fix that made this unnecessary.

Closing as substantively obviated by PR #9 (merge commit `24196aa`), with an architectural caveat worth recording. PR #9 ported gmaxwell's **2014** design ([bitcoin/bitcoin#5161](https://github.com/bitcoin/bitcoin/pull/5161)), not Wuille's **2016** design ([bitcoin/bitcoin#7028](https://github.com/bitcoin/bitcoin/pull/7028)) that this issue cites. The difference: | This issue's proposal (bitcoin#7028, 2016) | PR #9 reality (bitcoin#5161, 2014) | |---|---| | Peer votes → global `mapLocalHost` with new `LOCAL_PEER` tier | Peer's `addrMe` stored per-CNode in `pfrom->addrLocal` | | Influences `GetLocalAddress()` for **all** peers | Used **only** when advertising back to the same peer that told us, via `IsPeerAddrLocalGood()` | | Adds `LOCAL_PEER` enum value | Doesn't touch the enum (removes `LOCAL_HTTP`) | For the OFF cluster (which uses `-externalip=` everywhere) the two designs are functionally equivalent. The one residual benefit of layering this issue's `LOCAL_PEER` aggregation **on top of** PR #9 is for a fresh outbound-only NAT'd node that wants to learn its IP from one peer and advertise it to a different peer. The gmaxwell-2014 approach in PR #9 only ever advertises a peer-told IP back to the same peer who told us. Closing because: 1. The original concern in this issue ("peer agreement never gets a chance to seed `mapLocalHost`") is moot — peer agreement no longer **needs** to seed `mapLocalHost`, because the per-peer `addrLocal` is consulted directly in `AdvertizeLocal()`. 2. The marginal benefit of the global-aggregation path doesn't move the needle for the cluster. 3. If anyone (including future-us) wants this later, the change is now strictly additive on top of PR #9 — re-open then. Thanks @9019x for the broader fix that made this unnecessary.

dobbscoin closed this issue

2026-06-04 12:13:08 +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#7

No description provided.

Rows
Columns