Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add MaxVerificationGas #1745

Merged
merged 3 commits into from
Jul 3, 2020
Merged

Add MaxVerificationGas #1745

merged 3 commits into from
Jul 3, 2020

Conversation

erikzhang
Copy link
Member

No description provided.

@erikzhang erikzhang added this to the NEO 3.0 milestone Jul 2, 2020
@erikzhang erikzhang mentioned this pull request Jul 2, 2020
@@ -13,6 +13,8 @@ namespace Neo.SmartContract
{
public static class Helper
{
private const long MaxVerificationGas = 0_50000000;
Copy link
Member

Choose a reason for hiding this comment

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

ConsensusPayload has 0_02000000 maybe we should unify these values.

Copy link
Member Author

Choose a reason for hiding this comment

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

ConsensusPayload doesn't pay, and it only do the simple signature verification.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this value should be a part of the policy contract. We may see something on the mainnet that would suggest raising or lowering it, execution cost and GAS price could also change, so we're better be adaptive. And this value is a policy in its essence.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think it will be changed often.

Copy link
Member

Choose a reason for hiding this comment

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

It's not a bad idea

Copy link
Member

Choose a reason for hiding this comment

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

I think so as well,better to be in Policy.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think so. Even this value can be different for each node, it will not have much impact. Because it only effects verifications.

Copy link
Member

Choose a reason for hiding this comment

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

I see,Erik.
But are there bad aspects in moving it to Policy? In summary,I imagine that it has been good to have been moving parameters to the Native Policy.

Copy link
Member Author

Choose a reason for hiding this comment

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

We can't put everything into policy.

Copy link
Member

Choose a reason for hiding this comment

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

If we can run large scale applications perhaps we can include many things on the Blockchain Governance Smart Contract, Erik.
I understand that everything may be too much...aheuaheuaea But I still think it is a good practice to move parameters towards there.

@@ -130,6 +132,7 @@ public static UInt160 ToScriptHash(this ReadOnlySpan<byte> script)
internal static bool VerifyWitnesses(this IVerifiable verifiable, StoreView snapshot, long gas)
{
if (gas < 0) return false;
if (gas > MaxVerificationGas) gas = MaxVerificationGas;
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this small change already limits? Does it apply to all scripts or just verification?

Copy link
Member Author

Choose a reason for hiding this comment

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

Just verification.

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean, I don't see how this prevents execution... perhaps this PR is just to add this Max variable, and then it is used in near future to halt executions, right?

Copy link
Contributor

@igormcoelho igormcoelho left a comment

Choose a reason for hiding this comment

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

Time and Space are two faces of the same creature. It's not just Einstein speaking for physics, but it also applies to computing.
GAS is an abstraction of time, in fact, computing time. By limiting execution by GAS, you limit time, and as computing power evolves, this value will need adjustments in time to remain "consistent".
By limiting execution via a turing-incomplete no-jump policy, you don't care about time, as scripts will mostly occupy space. Then you care more about space, which doesn't improve too much in time. In fact, space is typically the most significant problem of a blockchain, from a long-lasting perspective.

That said, this solution will fix the problem, from a time perspective, but we will still require other fixes for the space perspective (limiting transaction size), that is why I strongly prefer a beautiful turing-incomplete safe execution mode.

Copy link
Contributor

@igormcoelho igormcoelho left a comment

Choose a reason for hiding this comment

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

If intention is to limit execution by gas, then this is the path to go. A MaxVerificationGas variable is desirable and necessary.
Although this value may change in the future, I agree that it doesn't affect much things, as persisted blocks will be kept persisted, and all verifications are assumed to be correct on the blockchain (no need to verify gas of accepted tx (IVerifiable entities).

@shargon shargon merged commit ad5816a into master Jul 3, 2020
@shargon shargon deleted the MaxVerificationGas branch July 3, 2020 06:10
ShawnYun pushed a commit to ShawnYun/neo that referenced this pull request Jul 6, 2020
Co-authored-by: Shargon <shargon@gmail.com>
shargon added a commit that referenced this pull request Jul 22, 2020
* Allow call from native contract (#1700)

* Fix MethodCallback (#1723)

* Fix #1714 (#1715)

* Add base64 SYSCALLs (#1717)

* Neo.VM.3.0.0-CI00230 (#1725)

* Allow to iterate buffer (#1726)

* Buffer iterator

* Rename

* Remove using

Co-authored-by: Erik Zhang <erik@neo.org>

* Update StorageContext.cs (#1728)

StorageContext needs to be public so it can be accessed by Neo Debugger

Co-authored-by: Erik Zhang <erik@neo.org>

* Fix return value check (#1730)

Co-authored-by: Shargon <shargon@gmail.com>

* Fix ContractEventDescriptor (#1733)

* fix enumerator (#1744)

Co-authored-by: Tommo-L <luchuan@neo.org>

* Change json fields to all lower case for consistency (#1736)

* Replace DataCache.Find by DataCache.Seek (#1740)

* replace DataCache.Find by DataCache.Seek

* fix order

* optimize

* Update ByteArrayComparer.cs

* Update ByteArrayComparer.cs

* Update DataCache.cs

* fix comments

* fix comments

* fix comments

* Update DataCache.cs

* Update DataCache.cs

* Reorder methods

Co-authored-by: Tommo-L <luchuan@neo.org>
Co-authored-by: erikzhang <erik@neo.org>
Co-authored-by: Shargon <shargon@gmail.com>

* Add MaxVerificationGas (#1745)

Co-authored-by: Shargon <shargon@gmail.com>

* Create KeyBuilder (#1748)

* Speed up the initialization of ApplicationEngine. (#1749)

* Change nef checksum to double SHA256 (#1751)

* Change to double SHA256

* Clean code

* Revert change

* Clean code

* Fix checksum

* Update NefFile.cs

* Fix compile

* Clean code

Clean code
Fix checksum
Update NefFile.cs
Fix compile

* Optimize

* Optimize

* Fix

Co-authored-by: erikzhang <erik@neo.org>

* Check witnesses on isStandard (#1754)

* Check witness on isStandard

* Add IsDeployed

* Add IsPayable

* Fix UT

* format

* Add coverage

* Remove IsPayable, IsDeployed

* Move NEP10 to manifest (#1729)

* NEP10 abi

* To lower case

* Move to Manifest

* Clean code

* Update ContractManifest.cs

* Update ContractManifest.cs

* Fix native contracts

Co-authored-by: Erik Zhang <erik@neo.org>

* Capture fault Exception (#1761)

* Capture faultException

* Update Wallet error

* Remove flag check

* Clean code

* Sender from signers (#1752)

* Sender from signers

* Remove co-

* Move signers outside attributes

* Fix UT and remove Signers class

* Fix UT

* Add FeeOnly scope

* Remove orderBy

* Remove _signersCache

* Fix Signers

* Fix WitnessScope

* Fix Sender

* Update TransactionAttributeType.cs

* Update Wallet.cs

* Fix Wallet

* Rename

* Update Wallet.cs

* Update Wallet.cs

* Partial UT fix

* More UT fixes

* Fix Sender's WitnessScope

* Fix Wallet

* Fix UT

* Explicit FeeOnly for DeployNativeContracts

* Same order as serialization

* Test FeeOnly

* dotnet format

* format

Co-authored-by: Erik Zhang <erik@neo.org>

* IApplicationEngineProvider (#1758)

* Remove the lock from SendersFeeMonitor and rename it to TransactionVerificationContext (#1756)

* Add EffectiveVoterTurnout (#1762)

* Remove AllowedTriggers from SYSCALLs (#1755)

* fix validatorscount (#1770)

Co-authored-by: Tommo-L <luchuan@neo.org>

* impl SeekInternal

Co-authored-by: Erik Zhang <erik@neo.org>
Co-authored-by: Shargon <shargon@gmail.com>
Co-authored-by: Harry Pierson <harrypierson@hotmail.com>
Co-authored-by: Luchuan <luchuan@ngd.neo.org>
Co-authored-by: Tommo-L <luchuan@neo.org>
Co-authored-by: joeqian <qianzhuo@ngd.neo.org>
shargon added a commit that referenced this pull request Jul 23, 2020
* Allow call from native contract (#1700)

* Fix MethodCallback (#1723)

* Fix #1714 (#1715)

* Add base64 SYSCALLs (#1717)

* Neo.VM.3.0.0-CI00230 (#1725)

* Allow to iterate buffer (#1726)

* Buffer iterator

* Rename

* Remove using

Co-authored-by: Erik Zhang <erik@neo.org>

* Update StorageContext.cs (#1728)

StorageContext needs to be public so it can be accessed by Neo Debugger

Co-authored-by: Erik Zhang <erik@neo.org>

* Fix return value check (#1730)

Co-authored-by: Shargon <shargon@gmail.com>

* Fix ContractEventDescriptor (#1733)

* fix enumerator (#1744)

Co-authored-by: Tommo-L <luchuan@neo.org>

* Change json fields to all lower case for consistency (#1736)

* Replace DataCache.Find by DataCache.Seek (#1740)

* replace DataCache.Find by DataCache.Seek

* fix order

* optimize

* Update ByteArrayComparer.cs

* Update ByteArrayComparer.cs

* Update DataCache.cs

* fix comments

* fix comments

* fix comments

* Update DataCache.cs

* Update DataCache.cs

* Reorder methods

Co-authored-by: Tommo-L <luchuan@neo.org>
Co-authored-by: erikzhang <erik@neo.org>
Co-authored-by: Shargon <shargon@gmail.com>

* Add MaxVerificationGas (#1745)

Co-authored-by: Shargon <shargon@gmail.com>

* Create KeyBuilder (#1748)

* Speed up the initialization of ApplicationEngine. (#1749)

* Change nef checksum to double SHA256 (#1751)

* Change to double SHA256

* Clean code

* Revert change

* Clean code

* Fix checksum

* Update NefFile.cs

* Fix compile

* Clean code

Clean code
Fix checksum
Update NefFile.cs
Fix compile

* Optimize

* Optimize

* Fix

Co-authored-by: erikzhang <erik@neo.org>

* Check witnesses on isStandard (#1754)

* Check witness on isStandard

* Add IsDeployed

* Add IsPayable

* Fix UT

* format

* Add coverage

* Remove IsPayable, IsDeployed

* Move NEP10 to manifest (#1729)

* NEP10 abi

* To lower case

* Move to Manifest

* Clean code

* Update ContractManifest.cs

* Update ContractManifest.cs

* Fix native contracts

Co-authored-by: Erik Zhang <erik@neo.org>

* Capture fault Exception (#1761)

* Capture faultException

* Update Wallet error

* Remove flag check

* Clean code

* Sender from signers (#1752)

* Sender from signers

* Remove co-

* Move signers outside attributes

* Fix UT and remove Signers class

* Fix UT

* Add FeeOnly scope

* Remove orderBy

* Remove _signersCache

* Fix Signers

* Fix WitnessScope

* Fix Sender

* Update TransactionAttributeType.cs

* Update Wallet.cs

* Fix Wallet

* Rename

* Update Wallet.cs

* Update Wallet.cs

* Partial UT fix

* More UT fixes

* Fix Sender's WitnessScope

* Fix Wallet

* Fix UT

* Explicit FeeOnly for DeployNativeContracts

* Same order as serialization

* Test FeeOnly

* dotnet format

* format

Co-authored-by: Erik Zhang <erik@neo.org>

* IApplicationEngineProvider (#1758)

* Remove the lock from SendersFeeMonitor and rename it to TransactionVerificationContext (#1756)

* Add EffectiveVoterTurnout (#1762)

* Remove AllowedTriggers from SYSCALLs (#1755)

* fix validatorscount (#1770)

Co-authored-by: Tommo-L <luchuan@neo.org>

* workflows: use checkout action v2 (#1775)

* Update git workflow

* Update .github/workflows/main.yml

Co-authored-by: Erik Zhang <erik@neo.org>

Co-authored-by: Erik Zhang <erik@neo.org>

* Add cache to native contract executions V2 (#1766)

* cache v2

* More optimizations

* Policy contract optimization

* Remove interporable variable

* Two more

* Add Set

* Optimizations

* Optimize

Co-authored-by: erikzhang <erik@neo.org>

* Optimize attributes (#1774)

* Add length before compression data (#1768)

* Add length before compression

* Optimize

* Fix UT

* Add UT

* Add UT

Co-authored-by: erikzhang <erik@neo.org>

* Fix VerifyWitnesses (#1776)

* Ensure non predictable peers (#1739)

* Plugins from List to array

* Move false to init

* Fix UT

* Refactor

* Add Exception Message For CreateContract (#1787)

* Add Exception Message When Create Contract

* Add Exception Message

* Code optimization

* add UT

Co-authored-by: Shargon <shargon@gmail.com>
Co-authored-by: Qiao Jin <43407364+Qiao-Jin@users.noreply.github.com>

Co-authored-by: Erik Zhang <erik@neo.org>
Co-authored-by: Shargon <shargon@gmail.com>
Co-authored-by: Harry Pierson <harrypierson@hotmail.com>
Co-authored-by: Luchuan <luchuan@ngd.neo.org>
Co-authored-by: Tommo-L <luchuan@neo.org>
Co-authored-by: joeqian <qianzhuo@ngd.neo.org>
Co-authored-by: cloud8little <34291844+cloud8little@users.noreply.github.com>
Co-authored-by: Qiao Jin <43407364+Qiao-Jin@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants