From ef7f5fc3980eba42573291db5e8e5446e2dc0406 Mon Sep 17 00:00:00 2001 From: Yang Chun Ung Date: Wed, 1 Jul 2026 17:02:50 +0900 Subject: [PATCH 1/3] fix: prevent OverflowException in Player.SetRuneSkills for high-stat casters Rune skills with a Caster stat reference computed their power as `(int)Math.Round(stat * SkillValue)`. Because a C# `decimal -> int` cast always performs an overflow check (regardless of checked/unchecked context), any character whose referenced stat is large enough that `stat * SkillValue` exceeds int.MaxValue threw System.OverflowException, which aborted every battle action that calls SetRuneSkills (HackAndSlash, Sweep, Raid, AdventureBoss, InfiniteTower) and blocked gameplay. Widen `power` to `long` and use NumberConversionHelper.SafeDecimalToInt64, matching the rest of the pipeline which is already long-based (SkillFactory.GetV1(long power), Skill.Power, SkillCustomField.BuffValue, and the AttackSkill/ArenaAttackSkill damage calc via SafeDecimalToInt64). Reproduced from the real failing tx 1ab27944f05a195c1d5a1906513bd15e7c2e97253ba077e9bd0272965abf556a (ATK 629,550,973 + rune 10003 lv200, SkillValue 4.47 -> (int)Math.Round(2,814,092,849) overflow). Added RuneSkillOverflowTest as a regression test. The obsolete Player.SetRuneV1 (used by StageSimulatorV3/RaidSimulatorV2 for legacy action versions) has the same cast but is intentionally left unchanged to preserve historical replay determinism. Co-Authored-By: Claude Opus 4.8 (1M context) --- .Lib9c.Tests/Model/RuneSkillOverflowTest.cs | 101 ++++++++++++++++++++ Lib9c/Model/Character/Player.cs | 8 +- 2 files changed, 106 insertions(+), 3 deletions(-) create mode 100644 .Lib9c.Tests/Model/RuneSkillOverflowTest.cs diff --git a/.Lib9c.Tests/Model/RuneSkillOverflowTest.cs b/.Lib9c.Tests/Model/RuneSkillOverflowTest.cs new file mode 100644 index 0000000000..3f8e408740 --- /dev/null +++ b/.Lib9c.Tests/Model/RuneSkillOverflowTest.cs @@ -0,0 +1,101 @@ +namespace Lib9c.Tests.Model +{ + using System; + using System.Collections.Generic; + using System.Linq; + using Lib9c.Tests.Action; + using Nekoyume.Helper; + using Nekoyume.Model; + using Nekoyume.Model.EnumType; + using Nekoyume.Model.Stat; + using Nekoyume.Model.State; + using Xunit; + + /// + /// Regression test for PLD-1390. A caster-stat-referencing rune skill + /// (e.g. rune 10003, ATK% Caster) used to throw an + /// inside when stat * SkillValue exceeded + /// , because the resulting power was cast to int. + /// The power is now computed as long, matching the rest of the skill/damage + /// pipeline (, Skill.Power, + /// SkillCustomField.BuffValue are all long). + /// Based on the real failing transaction + /// 1ab27944f05a195c1d5a1906513bd15e7c2e97253ba077e9bd0272965abf556a. + /// + public class RuneSkillOverflowTest + { + private readonly TableSheets _tableSheets; + private readonly AvatarState _avatarState; + + public RuneSkillOverflowTest() + { + _tableSheets = new TableSheets(TableSheetsImporter.ImportSheets()); + _avatarState = AvatarState.Create( + default, + default, + 0, + _tableSheets.GetAvatarSheets(), + default); + } + + [Fact] + public void SetRuneSkills_DoesNotOverflow_On_High_ATK() + { + // Values taken from the real failing tx. + const long atk = 629_550_973L; + const int runeId = 10003; + const int runeLevel = 200; + + // (int)Math.Round(629_550_973 * 4.47m) = 2_814_092_849 > int.MaxValue (used to throw). + const long expectedPower = 2_814_092_849L; + + var player = new Player( + _avatarState, + _tableSheets.CharacterSheet, + _tableSheets.CharacterLevelSheet, + _tableSheets.EquipmentItemSetEffectSheet); + + // Inflate the player's ATK to the on-chain value via a collection Add modifier. + var baseAtk = player.Stats.GetStatAsLong(StatType.ATK); + player.Stats.SetCollections(new[] + { + new StatModifier(StatType.ATK, StatModifier.OperationType.Add, atk - baseAtk), + }); + Assert.Equal(atk, player.Stats.GetStatAsLong(StatType.ATK)); + + // Sanity-check the sheet row matches the on-chain rune option. + Assert.True( + _tableSheets.RuneOptionSheet.TryGetOptionInfo(runeId, runeLevel, out var optionInfo)); + Assert.Equal(4.47m, optionInfo.SkillValue); + Assert.Equal(StatModifier.OperationType.Percentage, optionInfo.SkillValueType); + Assert.Equal(StatType.ATK, optionInfo.SkillStatType); + Assert.Equal(StatReferenceType.Caster, optionInfo.StatReferenceType); + + var runeState = new RuneState(runeId, runeLevel); + + // Must not throw, and the power must be the full long value (no int truncation). + player.SetRuneSkills( + new List { runeState }, + _tableSheets.RuneOptionSheet, + _tableSheets.SkillSheet); + + var runeSkill = player.RuneSkills.Single(); + Assert.Equal(expectedPower, runeSkill.Power); + Assert.True(runeSkill.CustomField.HasValue); + Assert.Equal(expectedPower, runeSkill.CustomField.Value.BuffValue); + } + + [Fact] + public void SafeDecimalToInt64_Preserves_Value_Where_Int_Cast_Overflows() + { + var rounded = Math.Round(629_550_973L * 4.47m); + Assert.Equal(2_814_092_849m, rounded); + + // The old code path: an (int) cast of this decimal still overflow-checks and throws. + Assert.Throws(() => (int)rounded); + + // The new code path keeps the full value (well within long range). + Assert.Equal(2_814_092_849L, NumberConversionHelper.SafeDecimalToInt64(rounded)); + } + } +} diff --git a/Lib9c/Model/Character/Player.cs b/Lib9c/Model/Character/Player.cs index 80a888f7c0..8561a66161 100644 --- a/Lib9c/Model/Character/Player.cs +++ b/Lib9c/Model/Character/Player.cs @@ -3,6 +3,7 @@ using System.Linq; using Nekoyume.Action; using Nekoyume.Battle; +using Nekoyume.Helper; using Nekoyume.Model.BattleStatus; using Nekoyume.Model.Buff; using Nekoyume.Model.Item; @@ -599,16 +600,17 @@ public void SetRuneSkills( continue; } - var power = 0; + long power = 0; if (optionInfo.SkillValueType == StatModifier.OperationType.Add) { - power = (int)optionInfo.SkillValue; + power = NumberConversionHelper.SafeDecimalToInt64(optionInfo.SkillValue); } else if (optionInfo.StatReferenceType == EnumType.StatReferenceType.Caster) { var value = Stats.GetStatAsLong(optionInfo.SkillStatType); - power = (int)Math.Round(value * optionInfo.SkillValue); + power = NumberConversionHelper.SafeDecimalToInt64( + Math.Round(value * optionInfo.SkillValue)); } var skill = SkillFactory.GetV1(skillRow, power, optionInfo.SkillChance); var customField = new SkillCustomField From 16593a7994356a6a5801cb1bce11501fc2679ee4 Mon Sep 17 00:00:00 2001 From: Yang Chun Ung Date: Wed, 1 Jul 2026 18:18:59 +0900 Subject: [PATCH 2/3] test: add XML doc comments to RuneSkillOverflowTest members Document the constructor and the two test methods so the PR does not increase the no-docs count enforced by the count-no-docs lint gate. Co-Authored-By: Claude Opus 4.8 (1M context) --- .Lib9c.Tests/Model/RuneSkillOverflowTest.cs | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/.Lib9c.Tests/Model/RuneSkillOverflowTest.cs b/.Lib9c.Tests/Model/RuneSkillOverflowTest.cs index 3f8e408740..327e25d154 100644 --- a/.Lib9c.Tests/Model/RuneSkillOverflowTest.cs +++ b/.Lib9c.Tests/Model/RuneSkillOverflowTest.cs @@ -27,6 +27,9 @@ public class RuneSkillOverflowTest private readonly TableSheets _tableSheets; private readonly AvatarState _avatarState; + /// + /// Initializes shared table sheets and a default avatar state for the tests. + /// public RuneSkillOverflowTest() { _tableSheets = new TableSheets(TableSheetsImporter.ImportSheets()); @@ -38,6 +41,11 @@ public RuneSkillOverflowTest() default); } + /// + /// Verifies that no longer throws + /// for a high-stat caster and that the resulting + /// rune skill power is kept as the full long value. + /// [Fact] public void SetRuneSkills_DoesNotOverflow_On_High_ATK() { @@ -85,6 +93,10 @@ public void SetRuneSkills_DoesNotOverflow_On_High_ATK() Assert.Equal(expectedPower, runeSkill.CustomField.Value.BuffValue); } + /// + /// Confirms that the conversion used by the fix keeps the full value where a raw + /// (int) cast of the same decimal would overflow. + /// [Fact] public void SafeDecimalToInt64_Preserves_Value_Where_Int_Cast_Overflows() { From 4d694e146618b6a3e9b33f68a80863d897f93dcf Mon Sep 17 00:00:00 2001 From: Yang Chun Ung Date: Thu, 2 Jul 2026 11:31:56 +0900 Subject: [PATCH 3/3] fix: align arena SetRuneSkills to long, matching the PvE fix ArenaCharacter.SetRuneSkills clamped rune skill power to int.MaxValue via SafeDecimalToInt32, while the PvE Player.SetRuneSkills fix widened it to long. For a high-stat caster this made PvE and arena compute different rune skill power (PvE full value, arena capped at ~2.1B). Widen the active arena SetRuneSkills to long/SafeDecimalToInt64 as well so both paths agree and match the long-based damage pipeline. The obsolete ArenaCharacter.SetRuneV1 is left unchanged for historical determinism. Add an arena regression test alongside the PvE one, and drop the per-member XML doc comments on the test (no longer needed now that the count-no-docs gate excludes test projects, #3295). Co-Authored-By: Claude Opus 4.8 (1M context) --- .Lib9c.Tests/Model/RuneSkillOverflowTest.cs | 91 ++++++++++++++------- Lib9c/Model/Character/ArenaCharacter.cs | 7 +- 2 files changed, 67 insertions(+), 31 deletions(-) diff --git a/.Lib9c.Tests/Model/RuneSkillOverflowTest.cs b/.Lib9c.Tests/Model/RuneSkillOverflowTest.cs index 327e25d154..c8b2da4cd6 100644 --- a/.Lib9c.Tests/Model/RuneSkillOverflowTest.cs +++ b/.Lib9c.Tests/Model/RuneSkillOverflowTest.cs @@ -4,9 +4,12 @@ namespace Lib9c.Tests.Model using System.Collections.Generic; using System.Linq; using Lib9c.Tests.Action; + using Nekoyume.Action; + using Nekoyume.Arena; using Nekoyume.Helper; using Nekoyume.Model; using Nekoyume.Model.EnumType; + using Nekoyume.Model.Rune; using Nekoyume.Model.Stat; using Nekoyume.Model.State; using Xunit; @@ -16,20 +19,28 @@ namespace Lib9c.Tests.Model /// (e.g. rune 10003, ATK% Caster) used to throw an /// inside when stat * SkillValue exceeded /// , because the resulting power was cast to int. - /// The power is now computed as long, matching the rest of the skill/damage - /// pipeline (, Skill.Power, + /// The arena counterpart () clamped the same + /// value to instead. Both now compute the power as + /// long, matching the rest of the skill/damage pipeline + /// (, Skill.Power, /// SkillCustomField.BuffValue are all long). /// Based on the real failing transaction /// 1ab27944f05a195c1d5a1906513bd15e7c2e97253ba077e9bd0272965abf556a. /// public class RuneSkillOverflowTest { + private const int RuneId = 10003; + private const int RuneLevel = 200; + + // Values taken from the real failing tx: ATK 629,550,973, rune 10003 lv200 + // (ATK% Caster, SkillValue 4.47). (int)Math.Round(629_550_973 * 4.47m) = + // 2,814,092,849 > int.MaxValue. + private const long Atk = 629_550_973L; + private const long ExpectedPower = 2_814_092_849L; + private readonly TableSheets _tableSheets; private readonly AvatarState _avatarState; - /// - /// Initializes shared table sheets and a default avatar state for the tests. - /// public RuneSkillOverflowTest() { _tableSheets = new TableSheets(TableSheetsImporter.ImportSheets()); @@ -41,22 +52,9 @@ public RuneSkillOverflowTest() default); } - /// - /// Verifies that no longer throws - /// for a high-stat caster and that the resulting - /// rune skill power is kept as the full long value. - /// [Fact] public void SetRuneSkills_DoesNotOverflow_On_High_ATK() { - // Values taken from the real failing tx. - const long atk = 629_550_973L; - const int runeId = 10003; - const int runeLevel = 200; - - // (int)Math.Round(629_550_973 * 4.47m) = 2_814_092_849 > int.MaxValue (used to throw). - const long expectedPower = 2_814_092_849L; - var player = new Player( _avatarState, _tableSheets.CharacterSheet, @@ -67,19 +65,19 @@ public void SetRuneSkills_DoesNotOverflow_On_High_ATK() var baseAtk = player.Stats.GetStatAsLong(StatType.ATK); player.Stats.SetCollections(new[] { - new StatModifier(StatType.ATK, StatModifier.OperationType.Add, atk - baseAtk), + new StatModifier(StatType.ATK, StatModifier.OperationType.Add, Atk - baseAtk), }); - Assert.Equal(atk, player.Stats.GetStatAsLong(StatType.ATK)); + Assert.Equal(Atk, player.Stats.GetStatAsLong(StatType.ATK)); // Sanity-check the sheet row matches the on-chain rune option. Assert.True( - _tableSheets.RuneOptionSheet.TryGetOptionInfo(runeId, runeLevel, out var optionInfo)); + _tableSheets.RuneOptionSheet.TryGetOptionInfo(RuneId, RuneLevel, out var optionInfo)); Assert.Equal(4.47m, optionInfo.SkillValue); Assert.Equal(StatModifier.OperationType.Percentage, optionInfo.SkillValueType); Assert.Equal(StatType.ATK, optionInfo.SkillStatType); Assert.Equal(StatReferenceType.Caster, optionInfo.StatReferenceType); - var runeState = new RuneState(runeId, runeLevel); + var runeState = new RuneState(RuneId, RuneLevel); // Must not throw, and the power must be the full long value (no int truncation). player.SetRuneSkills( @@ -88,15 +86,52 @@ public void SetRuneSkills_DoesNotOverflow_On_High_ATK() _tableSheets.SkillSheet); var runeSkill = player.RuneSkills.Single(); - Assert.Equal(expectedPower, runeSkill.Power); + Assert.Equal(ExpectedPower, runeSkill.Power); Assert.True(runeSkill.CustomField.HasValue); - Assert.Equal(expectedPower, runeSkill.CustomField.Value.BuffValue); + Assert.Equal(ExpectedPower, runeSkill.CustomField.Value.BuffValue); + } + + [Fact] + public void ArenaCharacter_SetRuneSkills_KeepsFullLong_On_High_ATK() + { + // The arena path used to clamp the power to int.MaxValue via SafeDecimalToInt32; + // it now keeps the full long value so PvE and arena stay consistent. + var allRuneState = new AllRuneState(); + allRuneState.AddRuneState(new RuneState(RuneId, RuneLevel)); + + // Rune 10003 is a Skill-type rune (rune_type 2) -> Skill slot (index 3). + var runeSlotState = new RuneSlotState(BattleType.Arena); + runeSlotState.UpdateSlot( + new List { new RuneSlotInfo(3, RuneId) }, + _tableSheets.RuneListSheet); + + var digest = new ArenaPlayerDigest( + _avatarState, + new List(), + new List(), + allRuneState, + runeSlotState); + + var simulator = new ArenaSimulator(new TestRandom()); + var character = new ArenaCharacter( + simulator, + digest, + _tableSheets.GetArenaSimulatorSheets(), + simulator.HpModifier, + new List + { + new StatModifier(StatType.ATK, StatModifier.OperationType.Add, Atk), + }); + + Assert.True( + _tableSheets.RuneOptionSheet.TryGetOptionInfo(RuneId, RuneLevel, out var optionInfo)); + var expectedPower = (long)Math.Round(character.ATK * optionInfo.SkillValue); + + var runeSkill = character._runeSkills.Single(); + Assert.Equal(expectedPower, runeSkill.Power); + Assert.True(runeSkill.Power > int.MaxValue); } - /// - /// Confirms that the conversion used by the fix keeps the full value where a raw - /// (int) cast of the same decimal would overflow. - /// [Fact] public void SafeDecimalToInt64_Preserves_Value_Where_Int_Cast_Overflows() { diff --git a/Lib9c/Model/Character/ArenaCharacter.cs b/Lib9c/Model/Character/ArenaCharacter.cs index 774afca83f..d769f6b69b 100644 --- a/Lib9c/Model/Character/ArenaCharacter.cs +++ b/Lib9c/Model/Character/ArenaCharacter.cs @@ -470,16 +470,17 @@ public void SetRuneSkills( continue; } - var power = 0; + long power = 0; if (optionInfo.SkillValueType == StatModifier.OperationType.Add) { - power = NumberConversionHelper.SafeDecimalToInt32(optionInfo.SkillValue); + power = NumberConversionHelper.SafeDecimalToInt64(optionInfo.SkillValue); } else if (optionInfo.StatReferenceType == EnumType.StatReferenceType.Caster) { var value = Stats.GetStatAsLong(optionInfo.SkillStatType); - power = NumberConversionHelper.SafeDecimalToInt32(Math.Round(value * optionInfo.SkillValue)); + power = NumberConversionHelper.SafeDecimalToInt64( + Math.Round(value * optionInfo.SkillValue)); } var skill = SkillFactory.GetForArena(skillRow, power, optionInfo.SkillChance, default, StatType.NONE); var customField = new SkillCustomField