Skip to content

vm: properly manage gasConsumed overflow - #4306

Merged
roman-khimov merged 4 commits into
masterfrom
gasconsumed-verflow
Jun 11, 2026
Merged

vm: properly manage gasConsumed overflow#4306
roman-khimov merged 4 commits into
masterfrom
gasconsumed-verflow

Conversation

@AnnaShaleva

@AnnaShaleva AnnaShaleva commented Jun 9, 2026

Copy link
Copy Markdown
Member

Close #4299.

@AnnaShaleva
AnnaShaleva requested a review from roman-khimov as a code owner June 9, 2026 11:41
@AnnaShaleva

AnnaShaleva commented Jun 9, 2026

Copy link
Copy Markdown
Member Author

@roman-khimov, it's my best attempt if we don't want to convert vm.gasConsumed to *big.Int, there will be differences in GasConsumed for FAULT-ed transactions. If it's not acceptable, either we need to use *big.Int as in C# node, or you may suggest a better solution.

@AnnaShaleva
AnnaShaleva force-pushed the gasconsumed-verflow branch from afe316c to 832a543 Compare June 9, 2026 11:44
@roman-khimov

roman-khimov commented Jun 9, 2026

Copy link
Copy Markdown
Member

big.Int is not an option for sure. uint256.Int can be considered.

roman-khimov
roman-khimov reviewed Jun 9, 2026
View reviewed changes
Comment thread pkg/core/native/oracle.go
Comment thread pkg/core/native/oracle.go
Comment thread pkg/vm/vm.go
Comment thread pkg/vm/vm.go Outdated
Comment thread pkg/vm/vm.go Outdated
Comment thread pkg/vm/vm.go Outdated
return v.AddPicoGas(gas * ExecFeeFactorMultiplier)
picoGas := gas * ExecFeeFactorMultiplier
if picoGas < 0 { // пу-пу-пу...
// To match C# behaviour, we still need to add this GAS to vm.gasConsumed (C# VM use *big.Int for gasConsumed calculations).

@roman-khimov roman-khimov Jun 9, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Try uint256.Int with benchmarks we have, maybe it's OK to use it here.

@AnnaShaleva
AnnaShaleva force-pushed the gasconsumed-verflow branch 2 times, most recently from 6c259a9 to f908ec3 Compare June 11, 2026 16:09
AnnaShaleva
AnnaShaleva commented Jun 11, 2026
View reviewed changes

@AnnaShaleva AnnaShaleva left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@roman-khimov Which exactly benchmarks do you want to check? Also, I'm afraid this change may affect the coefficients of #4087.

Comment thread pkg/vm/vm.go
Comment on lines +101 to 104
gasConsumed *uint256.Int
// gasLimit is the maximum amount of gas that can be consumed by the
// execution in picoGAS units.
gasLimit int64

@AnnaShaleva AnnaShaleva Jun 11, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@roman-khimov C# node uses bigint got gasLimit as far. I think we're OK with gasLimit being int64. Neither RPC nor real tx fees won't let gas limit become larger than maxInt64 / ExecFeeFactorMultiplier (or even maxInt64 / ExecFeeFactorMultiplier / 1000 after #4043).

@AnnaShaleva
AnnaShaleva requested a review from roman-khimov June 11, 2026 16:13
@codecov

codecov Bot commented Jun 11, 2026 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 75.00000% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.49%. Comparing base (6682850) to head (a848893).

Files with missing lines Patch % Lines
pkg/core/native/oracle.go 33.33% 1 Missing and 3 partials ⚠️
pkg/vm/vm.go 84.00% 2 Missing and 2 partials ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #4306      +/-   ##
==========================================
- Coverage   83.51%   83.49%   -0.03%     
==========================================
  Files         359      359              
  Lines       44210    44228      +18     
==========================================
+ Hits        36921    36926       +5     
- Misses       5472     5478       +6     
- Partials     1817     1824       +7

☔ 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.

@roman-khimov

roman-khimov commented Jun 11, 2026

Copy link
Copy Markdown
Member

Which exactly benchmarks do you want to check

Anything running some scripts (BenchScript*), I don't expect anything noticeable here.

roman-khimov
roman-khimov approved these changes Jun 11, 2026
View reviewed changes

@roman-khimov roman-khimov left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks much cleaner, if there are no regressions performance-wise I'd prefer this.

@AnnaShaleva
native: check Oracle response gas against int64 prior to processing
345501f 
`request` arguments unwrapping should happen before method processing:
https://github.com/neo-project/neo/blob/a09eeaecc458d3a604e2f586cc02047996bedfe5/src/Neo/SmartContract/Native/OracleContract.cs#L222

Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>
@AnnaShaleva
native: reuse (*big.Int).Int64() result
66c8c9c 
No functional changes.

Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>
@AnnaShaleva
vm: use uint256.Int instead of int64 for gasConsumed
ae63953 
Avoid possible int64 overflows, ref. #4299.

Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>
@AnnaShaleva
vm: properly manage gasConsumed overflow
a848893 
Close #4299. Comparing to master:
```
goos: linux
goarch: amd64
pkg: github.com/nspcc-dev/neo-go/pkg/vm
cpu: AMD Ryzen 7 PRO 7840U w/ Radeon 780M Graphics
                   │   old.txt   │           new.txt           │
                   │   sec/op    │   sec/op     vs base        │
ScriptFibonacci-16   214.0µ ± 1%   206.6µ ± 1%  -3.45% (n=100)

                   │   old.txt    │             new.txt             │
                   │     B/op     │     B/op      vs base           │
ScriptFibonacci-16   114.0Ki ± 0%   114.0Ki ± 0%  ~ (p=1.000 n=100)

                   │   old.txt   │             new.txt              │
                   │  allocs/op  │  allocs/op   vs base             │
ScriptFibonacci-16   3.170k ± 0%   3.170k ± 0%  ~ (p=1.000 n=100) ¹
¹ all samples are equal
```

Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>
@AnnaShaleva
AnnaShaleva force-pushed the gasconsumed-verflow branch from f908ec3 to a848893 Compare June 11, 2026 16:53
@AnnaShaleva

AnnaShaleva commented Jun 11, 2026

Copy link
Copy Markdown
Member Author

Last commit vs master:

goos: linux
goarch: amd64
pkg: github.com/nspcc-dev/neo-go/pkg/vm
cpu: AMD Ryzen 7 PRO 7840U w/ Radeon 780M Graphics  
                   │   old.txt   │           new.txt           │
                   │   sec/op    │   sec/op     vs base        │
ScriptFibonacci-16   214.0µ ± 1%   206.6µ ± 1%  -3.45% (n=100)

                   │   old.txt    │             new.txt             │
                   │     B/op     │     B/op      vs base           │
ScriptFibonacci-16   114.0Ki ± 0%   114.0Ki ± 0%  ~ (p=1.000 n=100)

                   │   old.txt   │             new.txt              │
                   │  allocs/op  │  allocs/op   vs base             │
ScriptFibonacci-16   3.170k ± 0%   3.170k ± 0%  ~ (p=1.000 n=100) ¹
¹ all samples are equal

@AnnaShaleva
AnnaShaleva requested a review from roman-khimov June 11, 2026 16:54
@roman-khimov
roman-khimov merged commit f9bb3bf into master Jun 11, 2026
32 of 34 checks passed
@roman-khimov
roman-khimov deleted the gasconsumed-verflow branch June 11, 2026 19:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@roman-khimov roman-khimov Awaiting requested review from roman-khimov roman-khimov is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Port GasConsumed overflow fix

2 participants

@AnnaShaleva @roman-khimov