Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master-n3 #4716 +/- ##
=============================================
- Coverage 81.83% 81.65% -0.18%
=============================================
Files 236 237 +1
Lines 16490 16502 +12
Branches 2402 2404 +2
=============================================
- Hits 13494 13475 -19
- Misses 2221 2248 +27
- Partials 775 779 +4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
shargon
left a comment
There was a problem hiding this comment.
Any other change that must be fixed? I think it's better to fix the syscalls, then we can set invariant at the beginning of neo-node
roman-khimov
left a comment
There was a problem hiding this comment.
And I agree with @shargon, maybe it's easier to set it once in node init, I doubt anyone really cares about locale of the node.
This can also allow us to drop many of the other fixes made earlier in specific contracts.
| public override VMState Execute() | ||
| { | ||
| var index = PersistingBlock?.Index ?? NativeContract.Ledger.CurrentIndex(SnapshotCache); | ||
| if (!ProtocolSettings.IsHardforkEnabled(Hardfork.HF_Huyao, index)) |
There was a problem hiding this comment.
To me it's not HF-dependent, previous behavior is just buggy and can lead to multiple valid state while there can only be a single canonical one (but we don't know which one, hi, #1526).
There was a problem hiding this comment.
@roman-khimov, I partially agree with you if we had more CN changes since N3 launches.
But, in reality, we almost had the same set of CNs.
But this PR is worth verifying state changes against different culture, let's say US, EU, CHINA, etc.....
Maybe the HF can be indeed removed after some investigation.
There was a problem hiding this comment.
if we had more CN changes since N3 launches
Don't see it's related. We can't have different states before or after Huyao (or any other HF). We don't know which one is valid for sure and don't have any consensus around that (#1526), but practically we know LANG=C or LC_ALL=en_US.UTF-8 are fine and likely to match the state data of seed nodes, so there is no behavior change at Huyao, it's just that finally some subset of incorrect behaviors is incorrect officially.
It's going from de-facto standard to more real standard, not "from height X the behavior is changed".
There was a problem hiding this comment.
As I said, I partially agree with you on this.
For now, until the HF is done, I would prefer to have HF.
Later it can be removed.
For example, an attacker could now calculate the culture of current CNs and prepare an attack that locks the sync of the network (if nodes resync).
With the HF and without drastic CNs changes until it, we are safer.
There was a problem hiding this comment.
By the way, other HF conditions should also be verified in the future and maybe removed in order to clean the codebase.
vncoelho
left a comment
There was a problem hiding this comment.
Good change in my opinion.
HF is needed
| } | ||
| finally | ||
| { | ||
| CultureInfo.CurrentCulture = previousCulture; |
There was a problem hiding this comment.
Why is CurrentCulture set to previousCulture?
It's necessary?
| var positiveKeyScript = BuildMissingMapKeyCatchScript(new BigInteger(5)); | ||
|
|
||
| Assert.AreEqual( | ||
| "Key \u22125 not found in Map.", |
There was a problem hiding this comment.
This expected string hardcodes U+2212 (MINUS SIGN) as what sv-SE produces for -5 before Huyao. That is ICU/CLDR NumberFormat.NegativeSign, not a Neo constant, and it is not guaranteed across OS/ICU versions (Windows ships app-local ICU; Linux/macOS use the system copy).
Build the expected pre-Huyao text from the culture under test so the assertion follows the platform rather than a particular minus glyph:
var sv = CultureInfo.GetCultureInfo("sv-SE");
var minus = sv.NumberFormat.NegativeSign;
Assert.AreEqual(
$"Key {minus}5 not found in Map.",
ExecuteCaughtMessage(negativeKeyScript, settings, HuyaoEnable - 1, "sv-SE"));If the point of the pre-Huyao case is to prove divergence from ASCII -, assert that explicitly (Assert.AreNotEqual("-", minus)) or Assert.Inconclusive when this runner's sv-SE already uses U+002D. Same pattern on the SETITEM expected string below (\u22125/[0, 0)).
| var script = BuildOutOfRangeSetItemCatchScript(BigInteger.MinusOne * 5); | ||
|
|
||
| Assert.AreEqual( | ||
| "The index of VMArray is out of range, \u22125/[0, 0).", |
There was a problem hiding this comment.
Same as the PICKITEM pre-Huyao assert: \u2212 is this machine's sv-SE minus, not a stable protocol byte sequence. Derive it from CultureInfo.GetCultureInfo("sv-SE").NumberFormat.NegativeSign (or share a helper with the test above) so Linux/system-ICU runners do not fail when CLDR data differs.
Description
Make contract execution culture-independent after
HF_Huyao.Some cultures render negative values with a non-ASCII minus sign. Because catchable exception messages are exposed to the VM as bytes, the same script can observe different values under different process cultures.
This change preserves historical behavior before Huyao and executes
ApplicationEnginewithInvariantCultureafter activation. The previous process culture is restored when execution finishes.Change Log
HF_Huyao-gated invariant-culture execution boundary.PICKITEMandSETITEMmessages.Type of change
How Has This Been Tested?
The following validation was completed:
Checklist: