feat(network): Self-healing NAT state by githubawn · Pull Request #3010 · TheSuperHackers/GeneralsGameCode

githubawn · 2026-07-23T22:06:47Z

What this code did:

The ButtonFirewallRefresh button in OptionsMenu.cpp and FirewallNeedToRefresh in FirewallHelper.cpp / OptionPreferences.cpp:

Saved/read FirewallNeedToRefresh and LastFirewallIP boolean flags to/from Options.ini.
Required players to manually click a "Refresh NAT" button in the Options GUI when changing network interfaces or encountering P2P negotiation failures.
Contaminated GlobalData (TheWritableGlobalData->m_firewallBehavior) with disk-persisted firewall state across process restarts.

How the new code works:

Automated In-Engine RAM Lifecycle: NAT state classification (m_behavior) is managed 100% in memory within FirewallHelperClass.
Transparent Background Probing: Non-blocking STUN probing runs in the background during online lobby entry (WOLWelcomeMenu), caching the classified result in RAM for the remainder of the game session.
Self-Healing Re-Detection: If a peer connection times out (NAT.cpp) or the user selects a new IP address in Options (OptionPreferences.cpp), TheFirewallHelper->flagNeedToRefresh(TRUE) resets m_behavior = FIREWALL_TYPE_UNKNOWN in RAM to seamlessly trigger a fresh background probe.
Clean GUI Deprecation: Hides OptionsMenu.wnd:ButtonFirewallRefresh via winHide(TRUE) in C++ without breaking custom or legacy .wnd layout files.

Background & Reason for Removal:

Obsolete Manual Workaround: Manual NAT refresh buttons are a legacy 2003 workaround; modern network stacks handle NAT re-detection automatically in-engine.
Elimination of Disk I/O: Completely removes FirewallNeedToRefresh and LastFirewallIP from Options.ini, preventing stale or corrupted flags from persisting across game crashes or process restarts.
Architectural Decoupling: Fully encapsulates STUN probing state inside FirewallHelperClass in memory, removing direct mutations to TheWritableGlobalData->m_firewallBehavior.

greptile-apps · 2026-07-23T22:09:34Z

Greptile Summary

Self-healing NAT detection is moved from persisted global/preferences state into the session-owned firewall helper.

Restarts asynchronous detection after selected-IP changes and failed NAT negotiation.
Retains completed classifications in memory and exposes shared accessors to matchmaking and traversal code.
Moves helper ownership to GameSpy setup and teardown and removes completion-time deletion.
Removes the manual refresh action and hides its legacy options control in supported GUI builds.

Confidence Score: 4/5

The PR is not yet safe to merge because an IP-triggered refresh can advertise the invalidated previous interface's NAT behavior and allocation delta.

The replacement probe is asynchronous, but its accessors fall back to values captured from the interface whose change initiated the refresh, allowing matchmaking and traversal to act on incorrect NAT data and time out.

Files Needing Attention: Core/GameEngine/Source/GameNetwork/FirewallHelper.cpp

Important Files Changed

Filename	Overview
Core/GameEngine/Source/GameNetwork/FirewallHelper.cpp	Introduces the in-memory detection lifecycle and fallback accessors, but the fallback exposes an invalidated previous-interface classification during refresh.
Core/GameEngine/Source/Common/OptionPreferences.cpp	Routes LAN and online IP changes through a shared setter that restarts NAT detection when the effective value changes.
Core/GameEngine/Source/GameNetwork/NAT.cpp	Preserves helper ownership after negotiation, restarts detection after failure, and consumes the helper-managed port-allocation delta.
Core/GameEngine/Source/GameNetwork/GameSpy/PeerDefs.cpp	Aligns firewall-helper ownership with GameSpy setup and teardown.
Generals/Code/GameEngine/Source/GameClient/GUI/GUICallbacks/Menus/WOLGameSetupMenu.cpp	Publishes the best-known in-memory NAT classification when constructing staging-room state.
GeneralsMD/Code/GameEngine/Source/GameClient/GUI/GUICallbacks/Menus/WOLGameSetupMenu.cpp	Mirrors the staging-room NAT publication change for Zero Hour.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[GameSpy setup] --> B[Create FirewallHelper]
    B --> C[Asynchronous NAT probe]
    C --> D[Completed in-memory classification]
    E[Selected IP changes or NAT fails] --> F[Restart probe]
    D --> F
    F --> C
    D --> G[Matchmaking and NAT traversal]
    F -. previous-interface fallback .-> G
    H[GameSpy teardown] --> I[Delete FirewallHelper]

Prompt To Fix All With AI

Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
Core/GameEngine/Source/GameNetwork/FirewallHelper.cpp:80-83
**Refresh publishes stale NAT state**

When the selected IP changes after an earlier probe completed, this fallback publishes the previous interface's NAT behavior while replacement detection is still running. The matching delta fallback is then consumed by NAT port prediction, causing peers to use the wrong traversal strategy or probe the wrong port until connection timeout.

_{Reviews (8): Last reviewed commit: "refactor(network): Put Refresh NAT butto..." | Re-trigger Greptile}

### What this code did: The `ButtonFirewallRefresh` button in `OptionsMenu.cpp` and `FirewallNeedToRefresh` in `FirewallHelper.cpp` / `OptionPreferences.cpp`: 1. Saved/read `FirewallNeedToRefresh` and `LastFirewallIP` boolean flags to/from `Options.ini`. 2. Required players to manually click a "Refresh NAT" button in the Options GUI when changing network interfaces or encountering P2P negotiation failures. 3. Contaminated `GlobalData` (`TheWritableGlobalData->m_firewallBehavior`) with disk-persisted firewall state across process restarts. ### How the new code works: 1. **Automated In-Engine RAM Lifecycle**: NAT state classification (`m_behavior`) is managed 100% in memory within `FirewallHelperClass`. 2. **Transparent Background Probing**: Non-blocking STUN probing runs in the background during online lobby entry (`WOLWelcomeMenu`), caching the classified result in RAM for the remainder of the game session. 3. **Self-Healing Re-Detection**: If a peer connection times out (`NAT.cpp`) or the user selects a new IP address in Options (`OptionPreferences.cpp`), `TheFirewallHelper->flagNeedToRefresh(TRUE)` resets `m_behavior = FIREWALL_TYPE_UNKNOWN` in RAM to seamlessly trigger a fresh background probe. 4. **Clean GUI Deprecation**: Hides `OptionsMenu.wnd:ButtonFirewallRefresh` via `winHide(TRUE)` in C++ without breaking custom or legacy `.wnd` layout files. ### Background & Reason for Removal: - **Obsolete Manual Workaround**: Manual NAT refresh buttons are a legacy 2003 workaround; modern network stacks handle NAT re-detection automatically in-engine. - **Elimination of Disk I/O**: Completely removes `FirewallNeedToRefresh` and `LastFirewallIP` from `Options.ini`, preventing stale or corrupted flags from persisting across game crashes or process restarts. - **Architectural Decoupling**: Fully encapsulates STUN probing state inside `FirewallHelperClass` in memory, removing direct mutations to `TheWritableGlobalData->m_firewallBehavior`.

…e-detection

…d matchmaking

placed nat detection earlier in the online Code some more cleanup

xezon

How can this change be tested? Did you test it? Does it work?

xezon · 2026-07-26T08:00:33Z

 	checkSendDelay				 = TheWindowManager->winGetWindowFromId( nullptr, checkSendDelayID);
-	buttonFirewallRefreshID	= TheNameKeyGenerator->nameToKey( "OptionsMenu.wnd:ButtonFirewallRefresh" );
-	buttonFirewallRefresh		= TheWindowManager->winGetWindowFromId( nullptr, buttonFirewallRefreshID);
+	// TheSuperHackers @info 25/07/2026 Refresh button has been hidden, we have migrated this to self-healing


Please avoid using "we", "i". Ideally comments are person-less.

xezon · 2026-07-26T08:03:00Z


+	if (TheFirewallHelper != nullptr)
+	{
+		TheFirewallHelper->behaviorDetectionUpdate();


Why does the WOL Login need a firewall helper update? Isn't the firewall helper needed for peer to peer connections?

This is to move the firewall NAT detection as early as possible to heighten the chance it is known once you join a match.

xezon · 2026-07-26T08:09:56Z

+	(*this)[key] = IP;
+
+	if (TheFirewallHelper != nullptr)
+		TheFirewallHelper->flagNeedToRefresh(TRUE);


It looks like this class is not intended to set state elsewhere in the program. Can this be implemented differently to keep the focus of this class for writing options?

xezon · 2026-07-26T08:13:26Z


+		Bool isBehaviorDetectionComplete() {return(m_currentState == DETECTIONSTATE_DONE);}
+		FirewallBehaviorType getLastFirewallBehavior() {return(m_lastBehavior);}
+		Short getLastSourcePortAllocationDelta() {return((Short)m_lastSourcePortAllocationDelta);}


Can all be const

xezon · 2026-07-26T08:16:17Z


 	TheGameSpyConfig = GameSpyConfigInterface::create(configBuffer);

+	if (TheFirewallHelper == nullptr)


Can this condition ever be false at this point? If not, can it be replaced with an assert?

xezon · 2026-07-26T08:20:22Z

+	return helper;
+}
+
+FirewallHelperClass::FirewallBehaviorType getBestKnownFirewallBehavior()


All it does is interact with FirewallHelperClass. Can this be member of FirewallHelperClass?

xezon · 2026-07-26T08:20:35Z

+	return FirewallHelperClass::FIREWALL_TYPE_UNKNOWN;
+}
+
+Short getBestKnownSourcePortAllocationDelta()


All it does is interact with FirewallHelperClass. Can this be member of FirewallHelperClass?

xezon · 2026-07-26T08:25:58Z

-	m_lastBehavior = (FirewallBehaviorType) ConfigINI.Get_Int("MultiPlayer", "FirewallSettings", FIREWALL_UNKNOWN);
-	m_lastSourcePortAllocationDelta = ConfigINI.Get_Int("MultiPlayer", "FirewallDelta", 1);
-#endif //(0)
+	if (flag) {


This flag argument looks very useless. Can be removed. "Munkee" text above needs updating then.

greptile-apps · 2026-07-26T16:48:04Z

+		if (TheFirewallHelper->getLastFirewallBehavior() != FirewallHelperClass::FIREWALL_TYPE_UNKNOWN)
+		{
+			return TheFirewallHelper->getLastFirewallBehavior();
+		}


Refresh publishes stale NAT state

When the selected IP changes after an earlier probe completed, this fallback publishes the previous interface's NAT behavior while replacement detection is still running. The matching delta fallback is then consumed by NAT port prediction, causing peers to use the wrong traversal strategy or probe the wrong port until connection timeout.

Knowledge Base Used: GameNetwork: Multiplayer Networking

Prompt To Fix With AI

This is a comment left during a code review. Path: Core/GameEngine/Source/GameNetwork/FirewallHelper.cpp Line: 80-83 Comment: **Refresh publishes stale NAT state** When the selected IP changes after an earlier probe completed, this fallback publishes the previous interface's NAT behavior while replacement detection is still running. The matching delta fallback is then consumed by NAT port prediction, causing peers to use the wrong traversal strategy or probe the wrong port until connection timeout. **Knowledge Base Used:** [GameNetwork: Multiplayer Networking](https://app.greptile.com/thesuperhackers/-/custom-context/knowledge-base/thesuperhackers/generalsgamecode/-/docs/gamenetwork-multiplayer.md) How can I resolve this? If you propose a fix, please make it concise.

githubawn changed the title ~~feat(network): make NAT state self-healing~~ feat(network): Self-healing NAT state Jul 23, 2026

greptile-apps Bot reviewed Jul 23, 2026

View reviewed changes

Comment thread Core/GameEngine/Source/GameNetwork/FirewallHelper.cpp

githubawn force-pushed the refactor/remove-refresh-nat branch from c5806f1 to 79e5c8e Compare July 23, 2026 22:14

githubawn force-pushed the refactor/remove-refresh-nat branch from 79e5c8e to 8113c09 Compare July 23, 2026 22:18

greptile-apps Bot reviewed Jul 23, 2026

View reviewed changes

Comment thread Core/GameEngine/Source/GameNetwork/FirewallHelper.cpp

githubawn added 2 commits July 24, 2026 00:25

fix(network): Reset probe state and close spare sockets on firewall r…

0a4ada6

…e-detection

fix(network): Preserve FirewallHelper lifetime across online menus an…

fd146ae

…d matchmaking

Skyaero42 reviewed Jul 25, 2026

View reviewed changes

Updated after review feedback

6c50745

greptile-apps Bot reviewed Jul 25, 2026

View reviewed changes

Comment thread Generals/Code/GameEngine/Source/GameClient/GUI/GUICallbacks/Menus/WOLGameSetupMenu.cpp Outdated

update after greptile feedback

67eef47

placed nat detection earlier in the online Code some more cleanup

greptile-apps Bot reviewed Jul 25, 2026

View reviewed changes

Comment thread Core/GameEngine/Source/GameNetwork/FirewallHelper.cpp

xezon reviewed Jul 26, 2026

View reviewed changes

xezon added Enhancement Is new feature or request Minor Severity: Minor < Major < Critical < Blocker Network Anything related to network, servers Gen Relates to Generals ZH Relates to Zero Hour labels Jul 26, 2026

refactor(network): Put Refresh NAT button hiding behind ENABLE_GUI_HACKS

802dd30

greptile-apps Bot reviewed Jul 26, 2026

View reviewed changes

githubawn marked this pull request as draft July 26, 2026 17:08


		TheGameSpyConfig = GameSpyConfigInterface::create(configBuffer);

		if (TheFirewallHelper == nullptr)

Uh oh!

Conversation

githubawn commented Jul 23, 2026

What this code did:

How the new code works:

Background & Reason for Removal:

Uh oh!

greptile-apps Bot commented Jul 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Greptile Summary

Confidence Score: 4/5

Important Files Changed

Flowchart

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

xezon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xezon Jul 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Jul 26, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

greptile-apps Bot commented Jul 23, 2026 •

edited

Loading

xezon Jul 26, 2026 •

edited

Loading