ApplicationEngine: fix `IsHardforkEnabled` behaviour by AnnaShaleva · Pull Request #4764 · neo-project/neo

AnnaShaleva · 2026-10-08T10:50:35Z

Description

ApplicationEngine.IsHardforkEnabled should always respect persisting block index. Returning true based on the fact that hardfork is just present in the list of configured hardforks irrespective from the current block index is misleading and will affect methods that don't set persisting block (like VerifyWitness):

neo/src/Neo/SmartContract/Helper.cs

Line 333 in 99618f4

    
           using (ApplicationEngine engine = ApplicationEngine.Create(TriggerType.Verification, verifiable, snapshot.CloneCache(), null, settings, datoshi))

This problem will also lead to improper dynamic price calculation introduced by #4760 as described in #4760 (comment).

Draft because states check is in progress. This PR is required for #4760 and related to #4726.

Type of change

Optimization (the change is only an optimization)
Style (the change is only a code style for better maintenance or standard purpose)
Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

How Has This Been Tested?

Checklist:

My code follows the style guidelines of this project
I have performed a self-review of my code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
Any dependent changes have been merged and published in downstream modules

codecov · 2026-10-08T10:54:16Z

Codecov Report

❌ Patch coverage is 50.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 83.90%. Comparing base (a472162) to head (53c0d43).

Files with missing lines	Patch %	Lines
src/Neo/SmartContract/ApplicationEngine.cs	50.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##           master-n3    #4764      +/-   ##
=============================================
- Coverage      83.93%   83.90%   -0.04%     
=============================================
  Files            240      240              
  Lines          17129    17128       -1     
  Branches        2453     2453              
=============================================
- Hits           14377    14371       -6     
- Misses          1967     1968       +1     
- Partials         785      789       +4

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

ApplicationEngine.IsHardforkEnabled should always respect persisting block index. Returning `true` based on the fact that hardfork is just _present_ in the list of configured hardforks irrespective from the current block index is misleading and will affect methods that don't set persisting block (like VerifyWitness): https://github.com/neo-project/neo/blob/99618f483a7e8bf783ad50af8a049752abd5ce4e/src/Neo/SmartContract/Helper.cs#L333 This problem will also lead to improper dynamic price calculation introduced by #4760 as described in #4760 (comment). Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>

For those callers who don't set persisting block explicitly on ApplicationEngine construction, we use `NativeContract.Ledger.CurrentIndex` instead of persisitng block index. However, Ledger's `CurrentIndex` returns the index of last _persisted_ block most of the time (see the note in nspcc-dev/neo-go#3889 (comment)). Whereas we need the _persisting_ block index. Hence, the suggestion is to use an increment of `NativeContract.Ledger.CurrentIndex` as persisting block index. Related to #4764. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>

AnnaShaleva · 2026-10-09T12:04:39Z

Mainnet states match up to 13909277 (checked in combination with #4765):

anna@kiwi:~/Documents/GitProjects/nspcc-dev/neo-go/scripts$ go run ./compare-states/compare-states.go http://localhost:10332 http://seed1.neo.org:10332
RPC A:
	user-agent: /Neo:3.10.2/
	magic: 860833102
	nonce: 2879318736
RPC B:
	user-agent: /Neo:3.10.1/
	magic: 860833102
	nonce: 608774773
at 0: 58a5157b7e99eeabf631291f1747ec8eb12ab89461cda888492b17a301de81e8 vs 58a5157b7e99eeabf631291f1747ec8eb12ab89461cda888492b17a301de81e8
at 13909277: 9282a86973a03e5739151124e4ae26fc4b97ff22b298e947ad50e3c54a567e7a vs 9282a86973a03e5739151124e4ae26fc4b97ff22b298e947ad50e3c54a567e7a

AnnaShaleva · 2026-10-09T12:06:14Z

@neo-project/core ready for review & merge.

Testnet states check is in progress, but I don't expect this change to affect the states in any way because we always set PersistingBlock during blocks/transactions processing.

Wi1l-B0t · 2026-10-09T12:51:20Z

It's compatibility with previous behaviour?

AnnaShaleva · 2026-10-09T13:50:04Z

It's compatibility with previous behaviour?

States are compatible, no fork is required.

cschuchardt88 · 2026-10-09T15:18:58Z

-
-            return ProtocolSettings.IsHardforkEnabled(hardfork, PersistingBlock.Index);
+            var persistingIndex = PersistingBlock?.Index ?? (SnapshotCache is null ? 0 : NativeContract.Ledger.CurrentIndex(SnapshotCache) + 1 /* Ledger.CurrentIndex returns already persisted block, hence use +1 to define _persisting_ block index */);
+            return ProtocolSettings.IsHardforkEnabled(hardfork, persistingIndex);


[suggestion] The checklist claims tests were added, but the PR only touches ApplicationEngine.cs (2 insertions, 5 deletions). Codecov reports 50% patch coverage with the SnapshotCache is null branch untested. Existing UT_ApplicationEngine_Residual.IsHardforkEnabled_MatchesProtocolSettings builds an engine via TestEngineRunner.Create (no persistingBlock) and compares against ProtocolSettings.IsHardforkEnabled(hf, Ledger.CurrentIndex(_snapshot)) under TestProtocolSettings.Default, whose omitted hardforks are filled at height 0 — so ContainsKey, CurrentIndex, and CurrentIndex + 1 all return true and the test cannot prove this fix. The motivating case from neo#4760 (verification engine, Huyao listed at a future height, ledger still below it) is not covered here.

Suggestion: Add unit tests that construct ApplicationEngine.Create(TriggerType.Verification, …, persistingBlock: null) with a snapshot whose Ledger.CurrentIndex is known, and protocol settings that place a hardfork at (a) a future height (CurrentIndex + 2 → false), (b) CurrentIndex + 1 → true, (c) CurrentIndex → true, plus a control with an explicit PersistingBlock.Index that must win over the snapshot height. That is the matrix #4760’s review asked for and the only way to lock the +1 rule.

cschuchardt88 · 2026-10-09T15:18:58Z

-                return ProtocolSettings.Hardforks.ContainsKey(hardfork);
-
-            return ProtocolSettings.IsHardforkEnabled(hardfork, PersistingBlock.Index);
+            var persistingIndex = PersistingBlock?.Index ?? (SnapshotCache is null ? 0 : NativeContract.Ledger.CurrentIndex(SnapshotCache) + 1 /* Ledger.CurrentIndex returns already persisted block, hence use +1 to define _persisting_ block index */);


[suggestion] This helper now uses CurrentIndex + 1 when PersistingBlock is null, but three sibling height calculations still use CurrentIndex with no +1: the constructor’s persistingIndex (opcode / Faun / Huyao gating, line 262), ApplicationEngine.Create jump-table selection (line 781), and NativeContract.GetAllowedMethods (line 41). After this lands, a verification engine at CurrentIndex == hfHeight - 1 will report the hardfork enabled here (and therefore in OnSysCall, crypto, NativeContract.Invoke’s ActiveIn/DeprecatedIn checks, etc.) while still selecting the pre-fork jump table, opcode-price path, and native-method cache. neo-go uses persistingBlockHeight() for both IsHardforkEnabled and opcode pricing (initVM), so #4760 syscall prices gated on this helper will disagree with C# opcode prices for that one block. Persist is unaffected (PersistingBlock.Index is used in all four places).

Suggestion: Cache one persisting index on the engine — PersistingBlock?.Index ?? (snapshot is null ? 0 : Ledger.CurrentIndex(snapshot) + 1) — and use it from the constructor, Create, GetAllowedMethods, and this method. At minimum, document that verification still has a 1-block split until those call sites are updated; do not let #4760 assume this helper and constructor height are the same.

cschuchardt88 · 2026-10-09T15:18:58Z

-                return ProtocolSettings.Hardforks.ContainsKey(hardfork);
-
-            return ProtocolSettings.IsHardforkEnabled(hardfork, PersistingBlock.Index);
+            var persistingIndex = PersistingBlock?.Index ?? (SnapshotCache is null ? 0 : NativeContract.Ledger.CurrentIndex(SnapshotCache) + 1 /* Ledger.CurrentIndex returns already persisted block, hence use +1 to define _persisting_ block index */);


[nit] The +1 rationale is worth keeping, but it is buried in an inline comment on a packed ternary, which makes the new rule easy to miss and the line hard to read.

Suggestion: Split the fallback out and put the why-comment on its own line, e.g. compute Ledger.CurrentIndex(SnapshotCache) + 1 in a following statement (or a small local) with the comment above it.

superboyiii · 2026-10-10T03:43:59Z

Maybe add some unit tests. Coverage is down. The code is OK and compatible.

) For those callers who don't set persisting block explicitly on ApplicationEngine construction, we use `NativeContract.Ledger.CurrentIndex` instead of persisitng block index. However, Ledger's `CurrentIndex` returns the index of last _persisted_ block most of the time (see the note in nspcc-dev/neo-go#3889 (comment)). Whereas we need the _persisting_ block index. Hence, the suggestion is to use an increment of `NativeContract.Ledger.CurrentIndex` as persisting block index. Related to #4764. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> Co-authored-by: Alvaro <amjarag@gmail.com>

AnnaShaleva · 2026-10-13T14:29:19Z

Just for the record, testnet states match up to (checked in combination with #4765):

anna@kiwi:~/Documents/GitProjects/nspcc-dev/neo-go/scripts$ go run compare-states/compare-states.go http://localhost:10332 http://seed1t5.neo.org:20332
RPC A:
	user-agent: /Neo:3.10.2/
	magic: 894710606
	nonce: 267723738
	block count: 20158582
RPC B:
	user-agent: /Neo:3.10.1/
	magic: 894710606
	nonce: 1034786658
	block count: 20158582
at 0: 62fd8ff9b0543aea352257db5b00bbb01d1bc0d2cc665e1f24cf5de0d16ebc7b vs 62fd8ff9b0543aea352257db5b00bbb01d1bc0d2cc665e1f24cf5de0d16ebc7b
at 20158581: cfa7de9d85a323057a7e0a454e5d804c135f0c1ab184bfc356148b0b89c091fd vs cfa7de9d85a323057a7e0a454e5d804c135f0c1ab184bfc356148b0b89c091fd

Maybe add some unit tests.

Will do in a separate PR.

…o-project#4765) For those callers who don't set persisting block explicitly on ApplicationEngine construction, we use `NativeContract.Ledger.CurrentIndex` instead of persisitng block index. However, Ledger's `CurrentIndex` returns the index of last _persisted_ block most of the time (see the note in nspcc-dev/neo-go#3889 (comment)). Whereas we need the _persisting_ block index. Hence, the suggestion is to use an increment of `NativeContract.Ledger.CurrentIndex` as persisting block index. Related to neo-project#4764. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> Co-authored-by: Alvaro <amjarag@gmail.com>

ApplicationEngine.IsHardforkEnabled should always respect persisting block index. Returning `true` based on the fact that hardfork is just _present_ in the list of configured hardforks irrespective from the current block index is misleading and will affect methods that don't set persisting block (like VerifyWitness): https://github.com/neo-project/neo/blob/99618f483a7e8bf783ad50af8a049752abd5ce4e/src/Neo/SmartContract/Helper.cs#L333 This problem will also lead to improper dynamic price calculation introduced by neo-project#4760 as described in neo-project#4760 (comment). Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> Co-authored-by: Alvaro <amjarag@gmail.com> Co-authored-by: Shargon <shargon@gmail.com>

AnnaShaleva added the Bug Used to tag confirmed bugs label Oct 8, 2026

AnnaShaleva requested a review from shargon October 8, 2026 10:50

github-actions Bot added the N3 label Oct 8, 2026

AnnaShaleva requested review from Jim8y, Wi1l-B0t, ajara87, cschuchardt88, erikzhang, roman-khimov, superboyiii and vncoelho October 8, 2026 10:51

AnnaShaleva changed the title ~~ApplicationEngine: fix IsHardforkEnabled behaviour~~ ApplicationEngine: fix IsHardforkEnabled behaviour Oct 8, 2026

AnnaShaleva mentioned this pull request Oct 8, 2026

SmartContract: reprice static System.Runtime.* interops since Huyao #4760

Merged

6 of 10 tasks

AnnaShaleva force-pushed the fix-IsHardforkEnabled branch from f1bc773 to 49461b3 Compare October 8, 2026 10:56

AnnaShaleva mentioned this pull request Oct 8, 2026

Native: move hardforks management to native Policy #4726

Open

AnnaShaleva force-pushed the fix-IsHardforkEnabled branch from 49461b3 to 0b61521 Compare October 8, 2026 11:13

AnnaShaleva mentioned this pull request Oct 8, 2026

ApplicationEngine: fix persisting index definition in constructor #4765

Merged

10 of 19 tasks

AnnaShaleva marked this pull request as ready for review October 9, 2026 12:03

cschuchardt88 reviewed Oct 9, 2026

View reviewed changes

Merge branch 'master-n3' into fix-IsHardforkEnabled

20fc541

Merge branch 'master-n3' into fix-IsHardforkEnabled

53c0d43

shargon approved these changes Oct 11, 2026

View reviewed changes

ajara87 approved these changes Oct 11, 2026

View reviewed changes

shargon merged commit f7a212d into master-n3 Oct 12, 2026

shargon deleted the fix-IsHardforkEnabled branch October 12, 2026 03:16

Navigation Menu

Repository navigation

ApplicationEngine: fix `IsHardforkEnabled` behaviour - #4764

ApplicationEngine: fix `IsHardforkEnabled` behaviour#4764
shargon merged 3 commits into
master-n3from
fix-IsHardforkEnabled

AnnaShaleva commented Oct 8, 2026 •

edited

Loading

Uh oh!

codecov Bot commented Oct 8, 2026 •

edited

Loading

Uh oh!

AnnaShaleva commented Oct 9, 2026 •

edited

Loading

Uh oh!

AnnaShaleva commented Oct 9, 2026 •

edited

Loading

Uh oh!

Wi1l-B0t commented Oct 9, 2026

Uh oh!

AnnaShaleva commented Oct 9, 2026

Uh oh!

cschuchardt88 Oct 9, 2026

Uh oh!

cschuchardt88 Oct 9, 2026

Uh oh!

cschuchardt88 Oct 9, 2026

Uh oh!

superboyiii commented Oct 10, 2026 •

edited

Loading

Uh oh!

AnnaShaleva commented Oct 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Conversation

AnnaShaleva commented Oct 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of change

How Has This Been Tested?

Checklist:

Uh oh!

codecov Bot commented Oct 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

AnnaShaleva commented Oct 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AnnaShaleva commented Oct 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Wi1l-B0t commented Oct 9, 2026

Uh oh!

AnnaShaleva commented Oct 9, 2026

Uh oh!

cschuchardt88 Oct 9, 2026

Choose a reason for hiding this comment

Uh oh!

cschuchardt88 Oct 9, 2026

Choose a reason for hiding this comment

Uh oh!

cschuchardt88 Oct 9, 2026

Choose a reason for hiding this comment

Uh oh!

superboyiii commented Oct 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AnnaShaleva commented Oct 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

AnnaShaleva commented Oct 8, 2026 •

edited

Loading

codecov Bot commented Oct 8, 2026 •

edited

Loading

AnnaShaleva commented Oct 9, 2026 •

edited

Loading

AnnaShaleva commented Oct 9, 2026 •

edited

Loading

superboyiii commented Oct 10, 2026 •

edited

Loading