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 state is moved from persisted global preferences into a session-owned firewall helper.

Restarts background firewall detection after interface changes or failed peer negotiation.
Keeps the latest completed classification available while replacement probing runs.
Updates lobby and quick-match publication paths and removes the legacy manual refresh workflow.

Confidence Score: 4/5

The PR is not yet safe to merge because matchmaking can still publish UNKNOWN before the initial NAT probe completes, causing traversal failures for players behind NAT.

The new previous-result fallback fixes refresh-time publication only after a classification already exists; on the first probe, setup and quick-match paths remain reachable while both current and previous behavior are UNKNOWN.

Files Needing Attention: Core/GameEngine/Source/GameNetwork/FirewallHelper.cpp and the Generals/GeneralsMD game-setup and quick-match menu integrations

Important Files Changed

Filename	Overview
Core/GameEngine/Source/GameNetwork/FirewallHelper.cpp	Introduces session-owned detection and previous-result fallback, but the initial-probe fallback still exposes UNKNOWN to active matchmaking paths.
Core/GameEngine/Source/GameNetwork/NAT.cpp	Restarts classification after negotiation failures and retains the helper, while continuing to interpret UNKNOWN as the non-mangling path.
Generals/Code/GameEngine/Source/GameClient/GUI/GUICallbacks/Menus/WOLGameSetupMenu.cpp	Publishes the helper's best-known NAT classification during staging-room initialization, including UNKNOWN before the first probe completes.
GeneralsMD/Code/GameEngine/Source/GameClient/GUI/GUICallbacks/Menus/WOLGameSetupMenu.cpp	Mirrors the staging-room publication behavior for Zero Hour, including the initial UNKNOWN window.
Generals/Code/GameEngine/Source/GameClient/GUI/GUICallbacks/Menus/WOLQuickMatchMenu.cpp	Sources quick-match NAT metadata from the session helper, but can submit UNKNOWN during the initial probe.
GeneralsMD/Code/GameEngine/Source/GameClient/GUI/GUICallbacks/Menus/WOLQuickMatchMenu.cpp	Mirrors the quick-match helper integration and its initial UNKNOWN publication path.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[Enter online lobby] --> B[Create session FirewallHelper]
  B --> C[Run background NAT probe]
  C --> D{Probe complete?}
  D -->|Yes| E[Publish current classification]
  D -->|No, previous result exists| F[Publish previous classification]
  D -->|No previous result| G[Publish UNKNOWN]
  G --> H[NAT treats peer as non-mangling]
  H --> I[Peer negotiation may time out]
  J[IP change or connection failure] --> K[Restart probe]
  K --> C

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:86
**Initial probe still publishes UNKNOWN**

When a player enters game setup or starts quick match before the session's first firewall probe completes, both the current and previous classifications are UNKNOWN, so this fallback publishes UNKNOWN. NAT traversal treats that value as no port mangling and sends the raw source port, causing peer negotiation to time out for players behind NAT.

_{Reviews (7): Last reviewed commit: "update after greptile feedback" | 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

greptile-apps · 2026-07-25T23:34:48Z

+		}
+	}
+
+	return FirewallHelperClass::FIREWALL_TYPE_UNKNOWN;


Initial probe still publishes UNKNOWN

When a player enters game setup or starts quick match before the session's first firewall probe completes, both the current and previous classifications are UNKNOWN, so this fallback publishes UNKNOWN. NAT traversal treats that value as no port mangling and sends the raw source port, causing peer negotiation to time out for players behind NAT.

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: 86 Comment: **Initial probe still publishes UNKNOWN** When a player enters game setup or starts quick match before the session's first firewall probe completes, both the current and previous classifications are UNKNOWN, so this fallback publishes UNKNOWN. NAT traversal treats that value as no port mangling and sends the raw source port, causing peer negotiation to time out for players behind NAT. **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.

xezon

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

xezon · 2026-07-26T07:59:33Z


-	{ "FirewallBehavior",						INI::parseInt,				nullptr,			offsetof( GlobalData, m_firewallBehavior ) },
 	{ "FirewallPortOverride",				INI::parseInt,				nullptr,			offsetof( GlobalData, m_firewallPortOverride ) },
-	{	"FirewallPortAllocationDelta",INI::parseInt,				nullptr,			offsetof( GlobalData, m_firewallPortAllocationDelta) },


Maybe we can deprecate them instead of deleting? Otherwise there may be confusion why some GlobalData.ini settings do not exist in code.

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:00:52Z

+	// TheSuperHackers @info 25/07/2026 Refresh button has been hidden, we have migrated this to self-healing
+	GameWindow *buttonFirewallRefresh = TheWindowManager->winGetWindowFromId(nullptr, NAMEKEY("OptionsMenu.wnd:ButtonFirewallRefresh"));
+	if (buttonFirewallRefresh)
+		buttonFirewallRefresh->winHide(TRUE);


Best put behind ENABLE_GUI_HACKS define

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?

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

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


It looks like this class is not intended the 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.

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

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


		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!

greptile-apps Bot Jul 25, 2026

Choose a reason for hiding this comment

Uh oh!

xezon left a comment

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

greptile-apps Bot commented Jul 23, 2026 •

edited

Loading