Limit SYSCALLs in Verification #856

erikzhang · 2019-06-24T13:14:56Z

I disabled the state-related SYSCALLs from being called in Verification. Now it is possible to avoid the re-verifications of transactions. But we still need to solve a few problems:

Transaction.ValidUntilBlock is state dependent.
Policies may be changed every block.
Check fees. (This can be solved easily.)

codecov-io · 2019-06-24T13:37:54Z

Codecov Report

Merging #856 into master will increase coverage by 0.12%.
The diff coverage is 83.8%.

@@            Coverage Diff             @@
##           master     #856      +/-   ##
==========================================
+ Coverage   44.93%   45.06%   +0.12%     
==========================================
  Files         177      178       +1     
  Lines       12577    12605      +28     
==========================================
+ Hits         5652     5680      +28     
  Misses       6925     6925

Impacted Files	Coverage Δ
neo/SmartContract/Native/Tokens/NeoToken.cs	`47.41% <ø> (ø)`	⬆️
...eo/SmartContract/Native/ContractMethodAttribute.cs	`100% <ø> (ø)`	⬆️
neo/SmartContract/Native/Tokens/Nep5Token.cs	`98.2% <ø> (ø)`	⬆️
neo/SmartContract/Native/PolicyContract.cs	`100% <ø> (ø)`	⬆️
neo/Network/P2P/Payloads/Transaction.cs	`72.97% <0%> (-7.23%)`	⬇️
neo/SmartContract/InteropService.NEO.cs	`25.73% <100%> (-0.07%)`	⬇️
neo/Ledger/MemoryPool.cs	`78.78% <100%> (+1.49%)`	⬆️
neo/SmartContract/Native/NativeContract.cs	`84.04% <50%> (-1.23%)`	⬇️
neo/SmartContract/InteropDescriptor.cs	`95.83% <95.83%> (ø)`
neo/SmartContract/InteropService.cs	`20.5% <96%> (-0.48%)`	⬇️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 950b5ff...6361bf3. Read the comment docs.

vncoelho

Beautiful job, @erikzhang, Yoda Master.
You also improved code readability with this new template for InteropDescriptor.

I like this design, let's discuss the needs, possibilities and visions, then, extend it now.

Maybe we need to allow part of the header for time-lock accounts as well.
Contract is payable Neo_Contract_IsPayable might be included in the transaction as well: transaction_IsPayable, it would be nice to have a pre-limit of the amount of GAS (even other tokens) that will spend during Application. This would assist a faster re-verification.

neo/SmartContract/Helper.cs

igormcoelho · 2019-06-24T14:21:15Z

@erikzhang congratulations for this amazing step!

Transaction.ValidUntilBlock is state dependent.

I thought a lot about it in the past month, and I think that this is not state-related. We can define two things: ledger structure (blockchain itself, including height); and blockchain state itself (result of operations over this chain).
So, requiring access to height is not a state operation per-se, because it only depends on unprocessed headers, and "material" stuff. The real problem is "imaterial stuff" like storage, notifications, and things that require processing, and are not directly persisted.
Item 1 is not a problem, although I agree that a tx which enter mempool, may eventually become invalid. That's why this field is of upmost importance on tx (not a simple verification script), because it allows us to remove tx over time, regardless of their state. We can use this field for prunning, as long as we don't allow tx to have high expiration values.

Policies may be changed every block.

This is fine, because of item 1. Tx will have limited life.

Check fees. (This can be solved easily.)

In fact, this would be the most problematic case, because like you said before, a NEP-5 balance is a state operation. So, this can be "easily" solved, as long as only balances for GAS and NEO are controlled. If we wanted to control every type of asset, on verification, we would need crazy ideas like the Storage.Add we were discussing. But since we won't need to do that, that's fine... just beware that we will likely have Payable contract operations inside smart contracts, right? So, I think the safest way to do this, is only allow GAS and NEO to be paid to other contracts (the other NEP-5 are regular dynamic/static invoke operations), to the limit which is transferred to the contract in the operation itself.

Example:

Tx A
verification:
Sending 100 GAS from user B to contract C
application:
invokes send 60 GAS from contract C to contract D

probably it should allow sending back automatically to user B the 40 GAS difference, which was "unused" (this depends on logic). This type of thing guarantees that we only re-manage funds available over verification, so tx will never fail when out of GAS funds inside blocks.

neo/SmartContract/InteropService.NEO.cs

igormcoelho

Very clean implementation, I just think we need to decide upon having or not a deployed contract as witness.
A solution would be creating a new trigger type, only for Contract operations, and attaching this on Transaction Script. I think it's important to have Contract State as final Erik, even if storage is not final.

erikzhang · 2019-06-25T03:04:52Z

The only problem to be solved now is the change in policy. My solution is to clear mempool when policy changed.

igormcoelho · 2019-06-25T03:08:11Z

Why policy change is so problematic.. pricing changes?

vncoelho · 2019-06-25T03:08:12Z

Nice commits, Erik, how about the GAS spent during Application on in payable contracts? We need to sum them as well during reverify.

erikzhang · 2019-06-25T03:53:43Z

Why policy change is so problematic.. pricing changes?

Yes.

erikzhang · 2019-06-25T03:53:58Z

Nice commits, Erik, how about the GAS spent during Application on in payable contracts? We need to sum them as well during reverify.

Don't understand.

vncoelho · 2019-06-25T14:31:05Z

Don't understand.

Let's use the following scenario, an account with 10 GAS:

emits 10 different txs (tx_1, ..., tx_10) with 1 GAS each;
all of them in the mempool;
tx_1 of them enters in the current blocks and 9 still on the mempool;
tx_1 was a payable tx and its applications transferred 7 GAS to someone else;
2 out of the remaining 9 txs should not be dropped from mempool during re-verify

Notes:
This is why we have been also mentioning about:

A limit in which payable txs are willing to pay. With that we could use this limit during re-verify

igormcoelho · 2019-06-25T14:31:47Z

I guess Vitor is worried about the payable operations which we will be able to do during invoke now... I think they shouldn't be purely based on Application, otherwise their effects may become unpredictable in the future, so as user balances. I asset transfer is done purely on application, and storage is delayed, it will be hard to verify future tx, to be sure the funds will actually exist at that time (even to pay network fees, system fees, etc). So I think that payable stuff inside contracts should be pre-paid during verification, with the possibility of "cashing back" the difference or unused gas/neo.

erikzhang · 2019-06-26T08:16:36Z

2 out of the remaining 9 txs should not be dropped from mempool during re-verify

It is correctly handled currently.

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

Lines 131 to 135 in 94c4579

    
           BigInteger balance = NativeContract.GAS.BalanceOf(snapshot, Sender); 
        
           BigInteger fee = SystemFee + NetworkFee; 
        
           if (balance < fee) return false; 
        
           fee += mempool.Where(p => p != this && p.Sender.Equals(Sender)).Select(p => (BigInteger)(p.SystemFee + p.NetworkFee)).Sum(); 
        
           if (balance < fee) return false;

igormcoelho

Good job

neo/Network/P2P/Payloads/Transaction.cs

neo/SmartContract/InteropDescriptor.cs

neo/SmartContract/InteropService.NEO.cs

neo/SmartContract/InteropService.cs

neo/SmartContract/Native/NativeContract.cs

igormcoelho · 2019-06-28T11:12:55Z

Erik,looks like it works, logic is fine. Native nep5 is supposed to be sent on application trigger or only system trigger?

Just to clarify how transfers would work, and make sure we are on the same page (nep5 mint operation):

Sender = myhash
NetworkFee = gas from Sender
SystemFee = gas from Sender
WITNESSES= mywitness
Script (nep5 mint)
push myhash
push icohash
push 10 (neo)
Call NativeNeo
push "mint"
push myhash (in array)
Call icoContract

Then, IcoContract would get notifications, including from NativeNeo, and know how much tokens to send, right?

My only concern is when contracts start transfering native tokens internally, we need to make sure this wont break balances. But we can discuss this later, we have options.

igormcoelho

Still missing the policy change part I guess.

erikzhang · 2019-06-30T13:46:55Z

Still missing the policy change part I guess.

Yes. I will mark it as ready for review when it is completed.

igormcoelho · 2019-06-30T13:54:08Z

Erik, do you think we can pass two Script on a tx, first limited to a "system trigger", and second an application trigger?
first could be used for contract create, migrate, native transfers, and would run on block proposal. This ensures balance is always correct and updated, and "sender" could be removed too, as network fee could be a regular transfer to consensus address.

neo/Ledger/MemoryPool.cs

igormcoelho

Suggestion to put _policyChanged in local scope (while keep _feePerByte in global one)

neo/Ledger/MemoryPool.cs

neo/Network/P2P/Payloads/Transaction.cs

vncoelho

This is a nice PR, @erikzhang, as I already said. It provides a good refactor.

There are just some minor details I asked above, but maybe we can handle them in another PR if you want to proceed.

igormcoelho

A big evolution already, lets keep moving.

This optimization of clean or reverify the txs can be done later.

igormcoelho · 2019-07-05T15:17:45Z

neo/Ledger/MemoryPool.cs

-                    _unsortedTransactions.Clear();
-                    _sortedTransactions.Clear();
+                    foreach (PoolItem item in _unverifiedSortedTransactions.Reverse())
+                        _system.Blockchain.Tell(item.Tx, ActorRefs.NoSender);


Any chance that, in the time you retell, and then you clear already retelled tx? 🤔

vncoelho · 2019-07-05T18:50:42Z

@erikzhang, it has been a great PR and discussions.
Sorry for not taking dedicated time to help with fine tunes of this implementation.

Let's move to the next steps and later we double check everything.

* Limit SYSCALLs in Verification * Allow deployed contract as verification script * Simplify re-verification * Clear mempool when policy is changed * put _policyChanged in local scope * Reverify transactions when policy changes

Limit SYSCALLs in Verification

7de60e1

erikzhang added this to the NEO 3.0 milestone Jun 24, 2019

erikzhang requested review from igormcoelho, shargon and vncoelho June 24, 2019 13:14

vncoelho reviewed Jun 24, 2019

View reviewed changes

Merge branch 'master' into 3.0/SVM-3

a9b3ee7

shargon reviewed Jun 24, 2019

View reviewed changes

neo/SmartContract/Helper.cs Outdated Show resolved Hide resolved

igormcoelho reviewed Jun 24, 2019

View reviewed changes

neo/SmartContract/InteropService.NEO.cs Show resolved Hide resolved

igormcoelho reviewed Jun 24, 2019

View reviewed changes

erikzhang added 2 commits June 25, 2019 10:13

Allow deployed contract as verification script

f253fae

Simplify re-verification

94c4579

igormcoelho previously approved these changes Jun 28, 2019

View reviewed changes

igormcoelho self-requested a review June 28, 2019 23:28

igormcoelho reviewed Jun 28, 2019

View reviewed changes

shargon and others added 2 commits June 29, 2019 11:27

Merge branch 'master' into 3.0/SVM-3

1b78cd6

Merge branch 'master' into 3.0/SVM-3

c097453

igormcoelho reviewed Jul 1, 2019

View reviewed changes

neo/Ledger/MemoryPool.cs Show resolved Hide resolved

igormcoelho reviewed Jul 1, 2019

View reviewed changes

neo/Ledger/MemoryPool.cs Outdated Show resolved Hide resolved

igormcoelho reviewed Jul 1, 2019

View reviewed changes

neo/Ledger/MemoryPool.cs Outdated Show resolved Hide resolved

igormcoelho reviewed Jul 1, 2019

View reviewed changes

put _policyChanged in local scope

e80bb72

erikzhang dismissed shargon’s stale review via e80bb72 July 1, 2019 20:29

vncoelho reviewed Jul 1, 2019

View reviewed changes

neo/Ledger/MemoryPool.cs Show resolved Hide resolved

vncoelho previously requested changes Jul 1, 2019

View reviewed changes

neo/Ledger/MemoryPool.cs Outdated Show resolved Hide resolved

vncoelho reviewed Jul 1, 2019

View reviewed changes

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

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

vncoelho reviewed Jul 1, 2019

View reviewed changes

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

vncoelho reviewed Jul 1, 2019

View reviewed changes

igormcoelho previously approved these changes Jul 1, 2019

View reviewed changes

shargon previously approved these changes Jul 2, 2019

View reviewed changes

vncoelho previously approved these changes Jul 2, 2019

View reviewed changes

shargon and others added 2 commits July 3, 2019 14:28

Merge branch 'master' into 3.0/SVM-3

6361bf3

Reverify transactions when policy changes

d829748

erikzhang dismissed stale reviews from vncoelho, shargon, and igormcoelho via d829748 July 4, 2019 04:27

shargon approved these changes Jul 5, 2019

View reviewed changes

igormcoelho reviewed Jul 5, 2019

View reviewed changes

igormcoelho approved these changes Jul 5, 2019

View reviewed changes

vncoelho approved these changes Jul 5, 2019

View reviewed changes

vncoelho merged commit 1d71675 into master Jul 5, 2019

vncoelho deleted the 3.0/SVM-3 branch July 5, 2019 18:50

erikzhang removed this from the NEO 3.0 milestone Dec 6, 2019

Thacryba pushed a commit to simplitech/neo that referenced this pull request Feb 17, 2020

Update plugin.md (neo-project#856)

68f099d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Limit SYSCALLs in Verification #856

Limit SYSCALLs in Verification #856

erikzhang commented Jun 24, 2019

codecov-io commented Jun 24, 2019 •

edited

Loading

vncoelho left a comment •

edited

Loading

igormcoelho commented Jun 24, 2019 •

edited

Loading

igormcoelho left a comment •

edited

Loading

erikzhang commented Jun 25, 2019

igormcoelho commented Jun 25, 2019

vncoelho commented Jun 25, 2019

erikzhang commented Jun 25, 2019

erikzhang commented Jun 25, 2019

vncoelho commented Jun 25, 2019

igormcoelho commented Jun 25, 2019 •

edited

Loading

erikzhang commented Jun 26, 2019 •

edited

Loading

igormcoelho left a comment

igormcoelho commented Jun 28, 2019 •

edited

Loading

igormcoelho left a comment

erikzhang commented Jun 30, 2019

igormcoelho commented Jun 30, 2019 •

edited

Loading

igormcoelho left a comment

vncoelho left a comment

igormcoelho left a comment

igormcoelho Jul 5, 2019

vncoelho commented Jul 5, 2019

Limit SYSCALLs in Verification #856

Limit SYSCALLs in Verification #856

Conversation

erikzhang commented Jun 24, 2019

codecov-io commented Jun 24, 2019 • edited Loading

Codecov Report

vncoelho left a comment • edited Loading

Choose a reason for hiding this comment

igormcoelho commented Jun 24, 2019 • edited Loading

igormcoelho left a comment • edited Loading

Choose a reason for hiding this comment

erikzhang commented Jun 25, 2019

igormcoelho commented Jun 25, 2019

vncoelho commented Jun 25, 2019

erikzhang commented Jun 25, 2019

erikzhang commented Jun 25, 2019

vncoelho commented Jun 25, 2019

igormcoelho commented Jun 25, 2019 • edited Loading

erikzhang commented Jun 26, 2019 • edited Loading

igormcoelho left a comment

Choose a reason for hiding this comment

igormcoelho commented Jun 28, 2019 • edited Loading

igormcoelho left a comment

Choose a reason for hiding this comment

erikzhang commented Jun 30, 2019

igormcoelho commented Jun 30, 2019 • edited Loading

igormcoelho left a comment

Choose a reason for hiding this comment

vncoelho left a comment

Choose a reason for hiding this comment

igormcoelho left a comment

Choose a reason for hiding this comment

igormcoelho Jul 5, 2019

Choose a reason for hiding this comment

vncoelho commented Jul 5, 2019

codecov-io commented Jun 24, 2019 •

edited

Loading

vncoelho left a comment •

edited

Loading

igormcoelho commented Jun 24, 2019 •

edited

Loading

igormcoelho left a comment •

edited

Loading

igormcoelho commented Jun 25, 2019 •

edited

Loading

erikzhang commented Jun 26, 2019 •

edited

Loading

igormcoelho commented Jun 28, 2019 •

edited

Loading

igormcoelho commented Jun 30, 2019 •

edited

Loading