Dynamic gas data trie loads #4804

BeniaminDrasovean · 2022-12-13T15:33:01Z

Reasoning behind the pull request

The new feature of dynamic gas cost computation based on trie loads needs to be activated at a certain epoch by all nodes.

Proposed changes

Add a new flag DynamicGasCostForDataTrieStorageLoadEnableEpoch that will enable at the specified epoch the computation of gas cost for data trie loads based on the depth of the trie.

Testing procedure

Create a smart contract with multiple leaves in the dataTrie. When a leaf is accessed, check that the gas cost is computed dynamically based on the depth of the leaf.

Pre-requisites

Based on the Contributing Guidelines the PR author and the reviewers must check the following requirements are met:

was the PR targeted to the correct branch?
if this is a larger feature that probably needs more than one PR, is there a feat branch created?
if this is a feat branch merging, do all satellite projects have a proper tag inside go.mod?

…-trie-loads # Conflicts: # cmd/node/config/enableEpochs.toml # cmd/node/config/gasSchedules/gasScheduleV7.toml # common/enablers/enableEpochsHandler.go # common/enablers/epochFlags.go # config/epochConfig.go # go.mod # go.sum

codecov-commenter · 2022-12-14T09:06:26Z

Codecov Report

Patch coverage: 50.00% and project coverage change: -0.02% ⚠️

Comparison is base (02d6610) 80.10% compared to head (8a46f6e) 80.09%.
Report is 1 commits behind head on rc/v1.6.0.

❗ Current head 8a46f6e differs from pull request most recent head 12a225a. Consider uploading reports for the commit 12a225a to get more accurate results

Additional details and impacted files

@@              Coverage Diff              @@
##           rc/v1.6.0    #4804      +/-   ##
=============================================
- Coverage      80.10%   80.09%   -0.02%     
=============================================
  Files            708      706       -2     
  Lines          93408    93359      -49     
=============================================
- Hits           74821    74772      -49     
+ Misses         13275    13271       -4     
- Partials        5312     5316       +4

Files Changed	Coverage Δ
common/enablers/epochFlags.go	`98.70% <33.33%> (-0.65%)`	⬇️
common/enablers/enableEpochsHandler.go	`100.00% <100.00%> (ø)`

... and 9 files with indirect coverage changes

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

BeniaminDrasovean · 2022-12-14T09:14:45Z

cmd/node/config/gasSchedules/gasScheduleV7.toml

+# The current values for the coefficients were computed based on benchmarking
+[DynamicStorageLoad]
+    A = 687
+    SignOfA = 0


Looking for better naming...

…-trie-loads # Conflicts: # cmd/node/config/enableEpochs.toml # common/enablers/enableEpochsHandler.go # common/enablers/epochFlags.go # common/interface.go # config/epochConfig.go # go.mod # go.sum # sharding/mock/enableEpochsHandlerMock.go # testscommon/enableEpochsHandlerStub.go

…-trie-loads # Conflicts: # go.mod # go.sum # integrationTests/vm/txsFee/multiShard/asyncCall_test.go # integrationTests/vm/txsFee/multiShard/asyncESDT_test.go # integrationTests/vm/txsFee/multiShard/relayedTxScCalls_test.go # integrationTests/vm/txsFee/multiShard/scCalls_test.go # integrationTests/vm/txsFee/relayedScCalls_test.go # integrationTests/vm/txsFee/scCalls_test.go # integrationTests/vm/wasm/utils.go

iulianpascalau · 2023-01-19T09:16:47Z

cmd/node/config/enableEpochs.toml

@@ -236,6 +236,9 @@
    # WipeSingleNFTLiquidityDecreaseEnableEpoch represents the epoch when the system account liquidity is decreased for wipeSingleNFT as well
    WipeSingleNFTLiquidityDecreaseEnableEpoch = 5

+    # DynamicGasCostForDataTrieStorageLoadEnableEpoch represents the epoch when dynamic gas cost for data trie storage load will be enabled
+    DynamicGasCostForDataTrieStorageLoadEnableEpoch = 6


switch the value to 3? (what will come from rc/v1.4.0 will have all the other values set to 2)

iulianpascalau · 2023-01-19T09:17:38Z

cmd/node/config/gasSchedules/gasScheduleV1.toml

@@ -691,3 +691,16 @@
    MaxBuiltInCallsPerTx = 100
    MaxNumberOfTransfersPerTx = 100
    MaxNumberOfTrieReadsPerTx = 100
+
+# Quadratic, Linear and Constant are the coefficients for a quadratic func. Separate variables are used for the
+# sign of each coefficient, 0 meaning + and 1 meaning -


0 meaning positive and 1 meaning negative?
as it reads better. Valid on all the other gasmaps

iulianpascalau · 2023-01-19T09:28:06Z

config/epochConfig.go

@@ -91,6 +91,7 @@ type EnableEpochs struct {
 	RefactorPeersMiniBlocksEnableEpoch                uint32
 	MaxBlockchainHookCountersEnableEpoch              uint32
 	WipeSingleNFTLiquidityDecreaseEnableEpoch         uint32
+	DynamicGasCostForDataTrieStorageLoadEnableEpoch   uint32


also adapt tests in the tomlConfig_test.go ?

iulianpascalau · 2023-01-19T09:31:27Z

integrationTests/vm/txsFee/relayedScCalls_test.go

@@ -11,14 +11,17 @@ import (
 	"testing"

 	"github.com/multiversx/mx-chain-go/config"
+	"github.com/multiversx/mx-chain-go/integrationTests"


please add a semiintegration test here in which you test the transaction fee is computed correctly if the DynamicGasCostForDataTrieStorageLoad is activated. Suggestion to have 2 tests: one when using the SaveKeyValue builtin function and one using a contract that can add data (maybe using an internal array or something).

…-trie-loads # Conflicts: # cmd/node/config/gasSchedules/gasScheduleV1.toml # cmd/node/config/gasSchedules/gasScheduleV2.toml # cmd/node/config/gasSchedules/gasScheduleV3.toml # cmd/node/config/gasSchedules/gasScheduleV4.toml # cmd/node/config/gasSchedules/gasScheduleV5.toml # cmd/node/config/gasSchedules/gasScheduleV6.toml # cmd/node/config/gasSchedules/gasScheduleV7.toml # common/enablers/enableEpochsHandler.go # common/enablers/epochFlags.go # common/interface.go # config/epochConfig.go # sharding/mock/enableEpochsHandlerMock.go # testscommon/enableEpochsHandlerStub.go

…-trie-loads # Conflicts: # config/tomlConfig_test.go # go.mod # go.sum

iulianpascalau · 2023-02-28T06:47:54Z

go.sum

@@ -611,14 +611,14 @@ github.com/multiversx/mx-chain-storage-go v1.0.7 h1:UqLo/OLTD3IHiE/TB/SEdNRV1GG2
 github.com/multiversx/mx-chain-storage-go v1.0.7/go.mod h1:gtKoV32Cg2Uy8deHzF8Ud0qAl0zv92FvWgPSYIP0Zmg=
 github.com/multiversx/mx-chain-vm-common-go v1.3.34/go.mod h1:sZ2COLCxvf2GxAAJHGmGqWybObLtFuk2tZUyGqnMXE8=
 github.com/multiversx/mx-chain-vm-common-go v1.3.36/go.mod h1:sZ2COLCxvf2GxAAJHGmGqWybObLtFuk2tZUyGqnMXE8=
-github.com/multiversx/mx-chain-vm-common-go v1.3.37 h1:KeK6JCjeNUOHC5Z12/CTQIa8Z1at0dnnL9hY1LNrHS8=
-github.com/multiversx/mx-chain-vm-common-go v1.3.37/go.mod h1:sZ2COLCxvf2GxAAJHGmGqWybObLtFuk2tZUyGqnMXE8=
+github.com/multiversx/mx-chain-vm-common-go v1.3.38-0.20230227140728-5bd58c1e7990 h1:AYpgFg+jhi90nuxK8sRFyFPovSvQgbF4wdo025SrsFE=


proper release after initial tests

Will add a proper release right before merge

integrationTests/testInitializer.go

integrationTests/vm/txsFee/dynamicGasCost_test.go

ssd04 · 2023-02-28T14:51:18Z

config/tomlConfig_test.go

 			RuntimeMemStoreLimitEnableEpoch:             63,
+			DynamicGasCostForDataTrieStorageLoadEnableEpoch: 64,


go fmt here? plus remove line 690

…-trie-loads # Conflicts: # common/enablers/enableEpochsHandler.go # common/enablers/epochFlags.go # common/interface.go # config/epochConfig.go # config/tomlConfig_test.go # go.mod # go.sum # integrationTests/benchmarks/loadFromTrie_test.go # integrationTests/testInitializer.go # integrationTests/vm/txsFee/asyncCall_test.go # integrationTests/vm/wasm/utils.go # sharding/mock/enableEpochsHandlerMock.go # testscommon/enableEpochsHandlerMock/enableEpochsHandlerStub.go

…-trie-loads # Conflicts: # config/tomlConfig_test.go # go.sum # sharding/mock/enableEpochsHandlerMock.go

gabi-vuls

Normal allin test: master-0a0bcee937 -> dynamic-gas-data-trie-load-2a6908a5d3

--- Specific errors ---

block hash does not match 8743
wrong nonce in block 3105
miniblocks does not match 0
num miniblocks does not match 0
miniblock hash does not match 0
block bodies does not match 0
receipts hash missmatch 0

/------/

--- Statistics ---

Nr. of all ERRORS: 3
Nr. of all WARNS: 145
Nr. of new ERRORS: 0
Nr. of new WARNS: 5
Nr. of PANICS: 0

/------/

…-trie-loads # Conflicts: # go.mod # go.sum

iulianpascalau

just the proper release

gabi-vuls

Normal allin test: master-0a0bcee937 -> dynamic-gas-data-trie-load-12a225a566

--- Specific errors ---

block hash does not match 1080
wrong nonce in block 467
miniblocks does not match 0
num miniblocks does not match 0
miniblock hash does not match 0
block bodies does not match 0
receipts hash missmatch 0

/------/

--- Statistics ---

Nr. of all ERRORS: 0
Nr. of all WARNS: 133
Nr. of new ERRORS: 0
Nr. of new WARNS: 0
Nr. of PANICS: 0

/------/

--- ERRORS ---

/------/

--- WARNINGS ---

/------/

BeniaminDrasovean added 4 commits December 13, 2022 16:14

add dynamic gas cost for data trie load

59c6359

update EnableEpochHandler stubs

79072f0

fix failing test

d85888a

BeniaminDrasovean commented Dec 14, 2022

View reviewed changes

BeniaminDrasovean marked this pull request as ready for review December 22, 2022 12:04

BeniaminDrasovean added 5 commits January 9, 2023 12:04

add MinimumGasCost to dynamicStorageLoad in gasSchedule

20a5646

update go mod

770e23a

update all versions of the gasSchedule

aeede82

iulianpascalau self-requested a review January 19, 2023 09:14

iulianpascalau reviewed Jan 19, 2023

View reviewed changes

BeniaminDrasovean added impact:medium interested:sc interested:protocol size:medium type:improvement complexity:medium labels Jan 19, 2023

BeniaminDrasovean added 7 commits February 1, 2023 16:47

fix after merge

4ce0fcc

fix after review

82abe4b

Merge remote-tracking branch 'origin/rc/v1.6.0' into dynamic-gas-data…

2250981

…-trie-loads # Conflicts: # config/tomlConfig_test.go # go.mod # go.sum

update go mod and fix after merge

894f87e

fix linter issue

9f8d485

do not run test with race flag

ed89cf5

iulianpascalau reviewed Feb 28, 2023

View reviewed changes

BeniaminDrasovean added 2 commits February 28, 2023 10:23

fix after review

3788c7d

fix linter issue

6e04a25

ssd04 self-requested a review February 28, 2023 13:41

ssd04 reviewed Feb 28, 2023

View reviewed changes

BeniaminDrasovean and others added 3 commits March 1, 2023 11:25

fix after review

30f3dcf

Merge branch 'rc/v1.6.0' into dynamic-gas-data-trie-loads

ac26233

Merge branch 'rc/v1.6.0' into dynamic-gas-data-trie-loads

38f6051

BeniaminDrasovean changed the base branch from rc/v1.6.0 to feat/vm1.5 March 1, 2023 10:49

BeniaminDrasovean changed the base branch from feat/vm1.5 to rc/v1.6.0 March 9, 2023 14:41

BeniaminDrasovean marked this pull request as draft March 9, 2023 14:50

BeniaminDrasovean and others added 6 commits June 28, 2023 13:03

fix after merge

5c516f2

Merge branch 'rc/v1.6.0' into dynamic-gas-data-trie-loads

e1e2c83

Merge remote-tracking branch 'origin/rc/v1.6.0' into dynamic-gas-data…

4c76bac

…-trie-loads # Conflicts: # config/tomlConfig_test.go # go.sum # sharding/mock/enableEpochsHandlerMock.go

fix after merge

8b5e396

update go mod

2a6908a

gabi-vuls previously approved these changes Aug 17, 2023

View reviewed changes

BeniaminDrasovean added 2 commits August 18, 2023 11:21

Merge remote-tracking branch 'origin/rc/v1.6.0' into dynamic-gas-data…

9166ce4

…-trie-loads # Conflicts: # go.mod # go.sum

go mod update

0156022

BeniaminDrasovean dismissed gabi-vuls’s stale review via 0156022 August 18, 2023 08:43

BeniaminDrasovean marked this pull request as ready for review August 18, 2023 08:43

iulianpascalau reviewed Aug 18, 2023

View reviewed changes

update go mod with proper releases

12a225a

iulianpascalau approved these changes Aug 18, 2023

View reviewed changes

ssd04 approved these changes Aug 18, 2023

View reviewed changes

gabi-vuls approved these changes Aug 18, 2023

View reviewed changes

gabi-vuls merged commit f7045ec into rc/v1.6.0 Aug 18, 2023
6 checks passed

gabi-vuls deleted the dynamic-gas-data-trie-loads branch August 18, 2023 15:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dynamic gas data trie loads #4804

Dynamic gas data trie loads #4804

BeniaminDrasovean commented Dec 13, 2022 •

edited

codecov-commenter commented Dec 14, 2022 •

edited by codecov bot

BeniaminDrasovean Dec 14, 2022

iulianpascalau Jan 19, 2023

BeniaminDrasovean Feb 27, 2023

iulianpascalau Jan 19, 2023

BeniaminDrasovean Feb 27, 2023

iulianpascalau Jan 19, 2023

BeniaminDrasovean Feb 27, 2023

iulianpascalau Jan 19, 2023

BeniaminDrasovean Feb 27, 2023

iulianpascalau Feb 28, 2023

BeniaminDrasovean Feb 28, 2023

ssd04 Feb 28, 2023

BeniaminDrasovean Mar 1, 2023

gabi-vuls left a comment

iulianpascalau left a comment

gabi-vuls left a comment

		RuntimeMemStoreLimitEnableEpoch: 63,
		DynamicGasCostForDataTrieStorageLoadEnableEpoch: 64,

Dynamic gas data trie loads #4804

Dynamic gas data trie loads #4804

Conversation

BeniaminDrasovean commented Dec 13, 2022 • edited

Reasoning behind the pull request

Proposed changes

Testing procedure

Pre-requisites

codecov-commenter commented Dec 14, 2022 • edited by codecov bot

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabi-vuls left a comment

Choose a reason for hiding this comment

iulianpascalau left a comment

Choose a reason for hiding this comment

gabi-vuls left a comment

Choose a reason for hiding this comment

BeniaminDrasovean commented Dec 13, 2022 •

edited

codecov-commenter commented Dec 14, 2022 •

edited by codecov bot