SmartContract/Native: implement adjustable opcode pricing by roman-khimov · Pull Request #2045 · neo-project/neo

roman-khimov · 2020-11-03T17:56:48Z

This defines a new Policy contract setting, BaseExecFee which contains the price
of basic VM execution unit --- NOP instruction. It then redefines all opcode
prices as coefficients applied to that base fee.

The following invariants hold for this:

transaction fees are expressed in GAS
BaseExecFee is expressed in GAS
opcode price is a coefficient now
syscall price is expressed in GAS
ApplicationEngine.AddGas() also accepts GAS values, so it's a caller's
responsibility to calculate that in whatever fashion he likes.

Caveats:

while coefficients are based on table from Redefine opcode prices and make them adjustable #1875 (which is built on
instruction execution time relations for two complete VM implementations),
some modifications were applied:
- it's impossible for SYSCALL to have non-0 cost now (tests just hang)
- all slot operations have the same price
block and consensus payloads are adjusted to use BaseExecFee, but
ECDsaVerifyPrice is still a constant
it's not really tested other than unit tests

In general, this allows to define any execution cost in NOPs and then use
BaseExecFee to get the price. The same principle could be applied to storage
pricing based on FeePerByte (StoragePrice could just be 100 * FeePerByte),
but that's a bit different topic.

Closes #1875.

This defines a new Policy contract setting, BaseExecFee which contains a price of basic VM execution unit --- NOP instruction. It then redefines all opcode prices as coefficients applied to that base fee. The following invariants hold for this: * transaction fees are expressed in GAS * BaseExecFee is expressed in GAS * opcode price is a coefficient now * syscall price is expressed in GAS * ApplicationEngine.AddGas() also accepts GAS values, so it's a caller's responsibility to calculate that in whatever fashion he likes. Caveats: * while coefficients are based on table from neo-project#1875 (which is built on instruction execution time relations for two complete VM implementations), some modifications were applied: - it's impossible for SYSCALL to have non-0 cost now (tests just hang) - all slot operations have the same price * block and consensus payloads are adjusted to use BaseExecFee, but probably ECDsaVerifyPrice is still a constant * it's not really tested other than unit tests In general, this allows to define any execution cost in NOPs and then use BaseExecFee to get the price. The same principle could be applied to storage pricing based on FeePerByte (StoragePrice could just be `100 * FeePerByte`), but that's a bit different topic. Closes neo-project#1875.

src/neo/Network/P2P/Payloads/BlockBase.cs

ECDsaVerifyPrice is still a constant and it dominates here.

src/neo/SmartContract/ApplicationEngine.OpCodePrices.cs

Concentrate on adjustability for now.

tests/neo.UnitTests/Network/P2P/Payloads/UT_Transaction.cs

src/neo/SmartContract/ApplicationEngine.cs

src/neo/SmartContract/Native/PolicyContract.cs

Qiao-Jin · 2020-11-06T08:38:14Z

src/neo/Ledger/MemoryPool.cs

@@ -62,6 +62,7 @@ public class MemoryPool : IReadOnlyCollection<Transaction>



Not only price of opcode, but also price of interop services should be included.

Qiao-Jin · 2020-11-06T08:41:19Z

tests/neo.UnitTests/Network/P2P/Payloads/UT_Transaction.cs

@@ -256,7 +256,6 @@ public void FeeIsSignatureContractDetailed()
                        verificationGas += engine.GasConsumed;
                    }


Should add UT for cases where fee ratio is changed.

Qiao-Jin · 2020-11-06T08:45:05Z

src/neo/Ledger/MemoryPool.cs

@@ -62,6 +62,7 @@ public class MemoryPool : IReadOnlyCollection<Transaction>

        private int _maxTxPerBlock;


Should taken into account various hard coded verification prices.

vncoelho

As it is right now, it looks a good and clean PR, @roman-khimov, that is a good feature and will make maintenance simpler as well as increase protection of the network.

It's needed for BaseExecFee.

src/neo/Ledger/Blockchain.cs

Use currentShapshot we already have.

src/neo/Network/P2P/Payloads/Transaction.cs

src/neo/SmartContract/Native/PolicyContract.cs

src/neo/SmartContract/ApplicationEngine.cs

Tommo-L · 2020-11-12T08:30:48Z

Conflicts

…rices Resolve opcode price conflict (neo-project#2020).

roman-khimov · 2020-11-12T10:40:01Z

Fixed.

superboyiii · 2020-11-20T02:02:16Z

Let's move on.

roman-khimov · 2020-11-20T08:35:02Z

I guess it depends on #2054 now which depends on #2052.

MemoryPool contents is always valid (verified) against some snapshot. This snapshot is only changed when new block is added. Between blocks we only have one valid chain state that can be read by multiple threads without any issues, thus we can execute concurrently not only state-independent, but also state-dependent parts of transaction verification. To simplify execution flow (minimize single-threaded Blockchain message handling and eliminate duplicate DB accesses for ContainsTransaction) TransactionRouter is an independent entity now, though of course we can also attach the same actor functionality to MemoryPool itself. TransactionVerificationContext balance checking is moved to MemoryPool from Transaction with this, Transaction shouldn't care (it can check overall GAS balance though). This is directly related to neo-project#2045 work (it solves state access problem there) and in some sense is an alternative to neo-project#2054 (makes fee calculation easier, though IsStandardContract() trick to optimize out these contracts during reverification is still relevant and can be added here). At this stage it's just a prototype, some additional optimizations and simplifications are possible of course, but this prototype shows the direction and the main question for now is whether this direction is interesting for us.

erikzhang · 2020-12-09T10:47:40Z

@Qiao-Jin

roman-khimov · 2020-12-09T12:03:48Z

src/neo/SmartContract/ApplicationEngine.OpCodePrices.cs

+            [OpCode.PUSHINT8] = 1 << 0,
+            [OpCode.PUSHINT16] = 1 << 0,
+            [OpCode.PUSHINT32] = 1 << 0,
+            [OpCode.PUSHINT64] = 1 << 0,


These PUSHes (as well as PUSHM1..PUSH16) do cost more than NOP, I'd suggest 1<<2 here based on data we have (and it's very similar for two implementations with real costs around 4-5 NOPs).

roman-khimov · 2020-12-09T12:07:04Z

src/neo/SmartContract/ApplicationEngine.OpCodePrices.cs

+            [OpCode.JMPIF_L] = 1 << 1,
+            [OpCode.JMPIFNOT] = 1 << 1,
+            [OpCode.JMPIFNOT_L] = 1 << 1,
+            [OpCode.JMPEQ] = 1 << 1,


JMPs with embedded comparisons also reliably use more execution time than simple JMPs, not a lot, but enough to justify 1<<2 vs 1<<1.

Should be good for JMPEQ, JMPEQ_L, JMPNE and JMPNE_L

roman-khimov · 2020-12-09T12:08:39Z

src/neo/SmartContract/ApplicationEngine.OpCodePrices.cs

-            [OpCode.ISNULL] = 60,
-            [OpCode.ISTYPE] = 60,
-            [OpCode.CONVERT] = 80000,
+            [OpCode.DEPTH] = 1 << 1,


It's similar to PUSHINT, so if we're to change them, it needs to be changed also.

roman-khimov · 2020-12-09T12:10:17Z

src/neo/SmartContract/ApplicationEngine.OpCodePrices.cs

+            [OpCode.DEPTH] = 1 << 1,
+            [OpCode.DROP] = 1 << 1,
+            [OpCode.NIP] = 1 << 1,
+            [OpCode.XDROP] = 1 << 4,


I'd suggest 1<<6 here, there are edge cases for it.

I think 10 to 20 times is OK

roman-khimov · 2020-12-09T12:15:43Z

src/neo/SmartContract/ApplicationEngine.OpCodePrices.cs

+            [OpCode.ROLL] = 1 << 4,
+            [OpCode.REVERSE3] = 1 << 1,
+            [OpCode.REVERSE4] = 1 << 1,
+            [OpCode.REVERSEN] = 1 << 4,


Suggesting at least 1 << 5 here, N can be quite big.

roman-khimov · 2020-12-09T12:15:59Z

src/neo/SmartContract/ApplicationEngine.OpCodePrices.cs

+            [OpCode.TUCK] = 1 << 1,
+            [OpCode.SWAP] = 1 << 1,
+            [OpCode.ROT] = 1 << 1,
+            [OpCode.ROLL] = 1 << 4,


It has the same problem as XDROP.

roman-khimov · 2020-12-09T15:47:40Z

Just to get things clear, we can argue about specific prices for a long time, but that won't bring us closer to preview4, so I'm OK with almost any values just to be done with this release. Still, we do have a lot of real performance data for these opcodes and we should take that into account, because if we don't do it now, some well-known disproportions will be hardcoded for eternity. The most important of them to me is PICKITEM which is a frequently used opcode that is actually way cheaper in execution than it's priced now (HASKEY and REMOVE are similar in their performance to PICKITEM, so all three should be priced the same).

erikzhang

We can merge it first and adjust the price value in the future if necessary.

erikzhang

I have updated some price values.

shargon

It could be merged for me

shargon · 2020-12-10T09:18:52Z

src/neo/SmartContract/Native/PolicyContract.cs

+        [ContractMethod(0_03000000, CallFlags.WriteStates)]
+        private bool SetExecFeeFactor(ApplicationEngine engine, uint value)
+        {
+            if (value == 0 || value > MaxExecFeeFactor) throw new ArgumentOutOfRangeException(nameof(value));


Maybe if we allow 0 (checking the magic), we can configure a free private network easier.

It is too dangerous. Maybe a minimum value should be added to protocol.json.

This certainly can be done later if needed.

…t#2045)

erikzhang reviewed Nov 5, 2020

View reviewed changes

src/neo/Network/P2P/Payloads/BlockBase.cs Outdated Show resolved Hide resolved

roman-khimov added 2 commits November 5, 2020 10:14

Payloads: use fixed verification limits for Block and ConsensusPayload

baf7324

ECDsaVerifyPrice is still a constant and it dominates here.

BlockBase: drop no longer needed using

5110db1

erikzhang reviewed Nov 6, 2020

View reviewed changes

src/neo/SmartContract/ApplicationEngine.OpCodePrices.cs Outdated Show resolved Hide resolved

OpCodePrices: restore old prices (/30)

f73d644

Concentrate on adjustability for now.

roman-khimov commented Nov 6, 2020

View reviewed changes

tests/neo.UnitTests/Network/P2P/Payloads/UT_Transaction.cs Show resolved Hide resolved

erikzhang reviewed Nov 6, 2020

View reviewed changes

src/neo/SmartContract/ApplicationEngine.cs Outdated Show resolved Hide resolved

src/neo/SmartContract/Native/PolicyContract.cs Outdated Show resolved Hide resolved

Qiao-Jin reviewed Nov 6, 2020

View reviewed changes

vncoelho reviewed Nov 6, 2020

View reviewed changes

roman-khimov added 4 commits November 6, 2020 21:29

PolicyContract: use lower MaxBaseExecFee

67737e4

Transaction: add snapshot parameter to VerifyStateIndependent()

696f615

It's needed for BaseExecFee.

PolicyContract: add setBaseExecFee/getBaseExecFee UT

eee3596

dotnet format

0371d8e

Qiao-Jin reviewed Nov 9, 2020

View reviewed changes

src/neo/Ledger/Blockchain.cs Outdated Show resolved Hide resolved

Blockchain: don't create snapshot for every incoming tx

2038bd0

Use currentShapshot we already have.

Tommo-L reviewed Nov 9, 2020

View reviewed changes

src/neo/Network/P2P/Payloads/Transaction.cs Outdated Show resolved Hide resolved

shargon reviewed Nov 10, 2020

View reviewed changes

src/neo/SmartContract/Native/PolicyContract.cs Show resolved Hide resolved

src/neo/SmartContract/ApplicationEngine.cs Outdated Show resolved Hide resolved

shargon mentioned this pull request Nov 11, 2020

Add Fee Ratio #2032

Closed

Merge remote-tracking branch 'origin/master' into adjustable-opcode-p…

b1e5ef5

…rices Resolve opcode price conflict (neo-project#2020).

Qiao-Jin mentioned this pull request Nov 13, 2020

divide all interop price by 30 #2063

Closed

superboyiii mentioned this pull request Nov 19, 2020

Preview4 Checklist #1936

Closed

44 tasks

Tommo-L mentioned this pull request Nov 25, 2020

Optimize VerifyWitness and tx.verify #2054

Merged

Policy: add StoragePrice policy value

717ccad

roman-khimov commented Dec 9, 2020

View reviewed changes

erikzhang added 6 commits December 10, 2020 09:34

Update PolicyContract.cs

365a802

Update ApplicationEngine.cs

55494eb

Rename

125a72a

Update MemoryPool.cs

7bd3df2

Update MemoryPool.cs

a5256b9

Rename

1dab2c6

erikzhang previously approved these changes Dec 10, 2020

View reviewed changes

erikzhang added 2 commits December 10, 2020 09:57

Merge branch 'master' into adjustable-opcode-prices

d3de42e

Modify the price of HASKEY

b4906e6

erikzhang dismissed their stale review via b4906e6 December 10, 2020 02:52

erikzhang added 3 commits December 10, 2020 10:57

Modify the price of NEWBUFFER

47a9571

Modify the price of PICKITEM

16c8ca7

Fix

4e7bc63

erikzhang approved these changes Dec 10, 2020

View reviewed changes

shargon approved these changes Dec 10, 2020

View reviewed changes

erikzhang merged commit 22c2477 into neo-project:master Dec 10, 2020

erikzhang added this to the NEO 3.0 milestone Dec 10, 2020

Qiao-Jin mentioned this pull request Dec 11, 2020

Fix transaction route #2116

Closed

roman-khimov mentioned this pull request Dec 13, 2020

Adjustable opcode pricing nspcc-dev/neo-go#1611

Closed

csmuller mentioned this pull request Dec 14, 2020

Adapt to adjustable opcode pricing neow3j/neow3j#312

Closed

danwbyrne mentioned this pull request Dec 18, 2020

Update to Neo3-preview4 neo-one-suite/neo-one#2267

Closed

45 tasks

danwbyrne mentioned this pull request Dec 29, 2020

Update NativeContract models for Preview4 changes neo-one-suite/neo-one#2273

Closed

17 tasks

ShawnYun pushed a commit to ShawnYun/neo that referenced this pull request Jan 8, 2021

SmartContract/Native: implement adjustable opcode pricing (neo-projec…

0b2f6d6

…t#2045)

cloud8little pushed a commit to cloud8little/neo that referenced this pull request Jan 24, 2021

SmartContract/Native: implement adjustable opcode pricing (neo-projec…

8b1191c

…t#2045)

ixje added a commit to CityOfZion/neo-mamba that referenced this pull request Feb 3, 2021

https://github.com/neo-project/neo/pull/2045

3c361b3

roman-khimov mentioned this pull request Jul 21, 2023

add price for ASSERTMSG and ABORTMSG opcodes #2877

Merged

		@@ -62,6 +62,7 @@ public class MemoryPool : IReadOnlyCollection<Transaction>

		@@ -256,7 +256,6 @@ public void FeeIsSignatureContractDetailed()
		verificationGas += engine.GasConsumed;
		}

		@@ -62,6 +62,7 @@ public class MemoryPool : IReadOnlyCollection<Transaction>

		private int _maxTxPerBlock;

Conversation

roman-khimov commented Nov 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vncoelho left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Tommo-L commented Nov 12, 2020

Uh oh!

roman-khimov commented Nov 12, 2020

Uh oh!

superboyiii commented Nov 20, 2020

Uh oh!

roman-khimov commented Nov 20, 2020

Uh oh!

erikzhang commented Dec 9, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

roman-khimov commented Dec 9, 2020

Uh oh!

erikzhang left a comment

Choose a reason for hiding this comment

Uh oh!

erikzhang left a comment

Choose a reason for hiding this comment

Uh oh!

shargon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

roman-khimov commented Nov 3, 2020 •

edited

Loading

vncoelho left a comment •

edited

Loading