Extend `CalculateNetworkFee` with contract verification #3385

shargon · 2024-07-02T08:42:31Z

Description

Close #2805

Type of change

Optimization (the change is only an optimization)
Style (the change is only a code style for better maintenance or standard purpose)
Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

How Has This Been Tested?

We don't have a unit test system that allow us to deploy a dummy contract for verify methods... I think that it should be tested locally

Test Configuration:

Checklist:

My code follows the style guidelines of this project
I have performed a self-review of my code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
Any dependent changes have been merged and published in downstream modules

src/Neo/Wallets/Helper.cs

AnnaShaleva · 2024-07-04T15:43:20Z

src/Neo/Wallets/Helper.cs

+                                    script.Emit(OpCode.NEWMAP);
+                                    break;
+                                default:
+                                    throw new ArgumentException("The verify method requires parameters that need to be passed via the witness' invocation script.");


It differs slightly from NeoGo behaviour, we need to discuss it: NeoGo doesn't return error on unknown type; instead it tries to do the best and emit as much parameters as possible, skipping the others. Then "verify" method is processed in any case, and an error is returned only if "verify" doesn't return boolean. I think that this approach is good because it tries to do as much as possible without extra user's data.

Yes you need to support contract as signer with verify function with custom parameters.

The only thing that is missing is Map and InteropInterface currently. For maps some zero value can be added (empty map), but for interop interfaces it's harder to say, maybe NULL.

The only thing that is missing is Map and InteropInterface currently. For maps some zero value can be added (empty map), but for interop interfaces it's harder to say, maybe NULL.

#3385 (comment)

OK, but then throwing an error is not the best way to handle this.

@roman-khimov done

src/Neo/Wallets/Helper.cs

AnnaShaleva · 2024-07-04T15:51:10Z

src/Neo/Wallets/Helper.cs

+                else
                {
-                    int size_inv = 66 * m;
-                    size += IO.Helper.GetVarSize(size_inv) + size_inv + witnessScript.GetVarSize();
-                    networkFee += exec_fee_factor * MultiSignatureContractCost(m, n);
+                    // Regular signature verification.
+                    if (IsSignatureContract(witnessScript))
+                    {
+                        size += 67 + witnessScript.GetVarSize();
+                        networkFee += exec_fee_factor * SignatureContractCost();
+                    }
+                    else if (IsMultiSigContract(witnessScript, out int m, out int n))
+                    {
+                        int size_inv = 66 * m;
+                        size += IO.Helper.GetVarSize(size_inv) + size_inv + witnessScript.GetVarSize();
+                        networkFee += exec_fee_factor * MultiSignatureContractCost(m, n);
+                    }
                }
-                // We can support more contract types in the future.


It's a nice time to support custom verification contracts, it's a part of the issue. So we need to perform witness verification by executing scripts and record GAS consumed not only for contract-based witnesses, but also for unknown witness types. For example, Koblitz-based verification scripts (#3209) could be perfectly handled by running this custom verification script (with user-defined invocation script).

So e.g. in NeoGo we removed this part with signature/multisignature scripts fee calculation and use unified approach with witness script invocation.

Currently should work as the same, this optimization (if it's faster) can come in a different PR

It's not faster, but it allows to remove some code and reduce cognitive overhead for developers.

Oh, and BTW, running non-contract scripts (let's put default sig/multisig aside for a moment) is an important part of the deal. Koblitz scripts are the best check for this --- if you can handle them without any specific code you're good.

src/Neo/Wallets/Helper.cs

cschuchardt88 · 2024-07-04T17:31:36Z

src/Neo/Wallets/Helper.cs

+                                    script.Emit(OpCode.NEWMAP);
+                                    break;
+                                default:
+                                    throw new ArgumentException("The verify method requires parameters that need to be passed via the witness' invocation script.");


Yes you need to support contract as signer with verify function with custom parameters.

accidentally picked Changed Requested

shargon · 2024-07-09T07:54:30Z

src/Neo/Wallets/Helper.cs


                    // Check verify cost
                    using ApplicationEngine engine = ApplicationEngine.Create(TriggerType.Verification, tx, snapshot.CreateSnapshot(), settings: settings, gas: maxExecutionCost);
                    engine.LoadContract(contract, md, CallFlags.ReadOnly);
                    if (invocationScript != null) engine.LoadScript(invocationScript, configureState: p => p.CallFlags = CallFlags.None);
-                    if (engine.Execute() == VMState.FAULT) throw new ArgumentException($"Smart contract {contract.Hash} verification fault.");
-                    if (!engine.ResultStack.Pop().GetBoolean()) throw new ArgumentException($"Smart contract {contract.Hash} returns false.");
+                    _ = engine.Execute(); // https://github.com/neo-project/neo/issues/2805


Suggested change

_ = engine.Execute(); // https://github.com/neo-project/neo/issues/2805

if (engine.Execute() == VMState.FAULT) throw new ArgumentException($"Smart contract {contract.Hash} verification fault.");

_ = engine.ResultStack.Pop().GetBoolean(); // The result must be convertible to boolean

Yes, that's right, but here we should add one more check that there's no extra items on stack.

Exactly like a standard verification code.

but here we should add one more check that there's no extra items on stack.

FYI. That's a good point for users. But it's not the case the real verification process doing.
I'm OK with both versions. It's not a bad thing to have a strict check before submitting to the blockchain.

neo/src/Neo/SmartContract/Helper.cs

Lines 361 to 363 in 3351533

if (engine.Execute() == VMState.FAULT) return false;

if (!engine.ResultStack.Peek().GetBoolean()) return false;

fee = engine.FeeConsumed;

If we can reuse the VerifyWitness from neo/src/Neo/SmartContract/Helper.cs, it would be much better. If not, ignore me and let's keep going on.

shargon · 2024-07-23T07:14:58Z

@neo-project/core anything else?

src/Neo/Wallets/Helper.cs

roman-khimov

It mostly looks OK, but more tests are needed to ensure it works correctly and the old code works is not broken somehow.

src/Neo/Wallets/Helper.cs

roman-khimov · 2024-07-30T06:25:13Z

src/Neo/Wallets/Helper.cs

+                                    script.Emit(OpCode.NEWMAP);
+                                    break;
+                                default:
+                                    throw new ArgumentException("The verify method requires parameters that need to be passed via the witness' invocation script.");


The only thing that is missing is Map and InteropInterface currently. For maps some zero value can be added (empty map), but for interop interfaces it's harder to say, maybe NULL.

src/Neo/Wallets/Helper.cs

shargon · 2024-08-09T16:18:32Z

merge?

#3385 (comment)

Non-standard verification scripts are still not supported.

You mean without a contract?

roman-khimov · 2024-08-09T16:27:51Z

Yes. Like in #3209.

shargon · 2024-08-09T17:06:49Z

Yes. Like in #3209.

It's done in neo-go? i can't find it

AnnaShaleva · 2024-08-09T17:11:20Z

It's done in neo-go? i can't find it

Take a look at https://github.com/nspcc-dev/neo-go/blob/79e78980c4679e8929ceb509ed56359c5b017279/pkg/services/rpcsrv/server.go#L1001, we take signer's witness, infer invocation script (if needed) and don't change verification script. And hence, verification script is executed by s.chain.VerifyWitness "as is" without any changes even if it's not a contract/sig/multisig script.

Jim8y · 2024-09-05T10:33:22Z

It's done in neo-go? i can't find it

Take a look at https://github.com/nspcc-dev/neo-go/blob/79e78980c4679e8929ceb509ed56359c5b017279/pkg/services/rpcsrv/server.go#L1001, we take signer's witness, infer invocation script (if needed) and don't change verification script. And hence, verification script is executed by s.chain.VerifyWitness "as is" without any changes even if it's not a contract/sig/multisig script.

Any following work?

shargon · 2024-09-16T08:17:16Z

We may merge #3385 as it is now and move non-standard verification scripts support to another issue. I think it's acceptable way since #3385 contains everything that's required except non-standard verification scripts handling.

https://github.com/neo-project/neo/pull/3434/files#r1725538241

cschuchardt88 · 2024-09-16T08:42:34Z

Anyway you can do this #3481 (comment)

shargon · 2024-09-16T10:00:53Z

Anyway you can do this #3481 (comment)

in a different pr please, this is for a different thing

cschuchardt88 · 2024-10-13T04:56:13Z

@shargon you have conflict

roman-khimov

I'd really like to see some tests added along with it, but code-wise it's correct for the purpose and we have #3539 for extension. Let's get this in and move on.

shargon added 3 commits July 2, 2024 10:40

Calculate fee

099f7be

change values

3a332fa

remove using

ad3d477

shargon marked this pull request as ready for review July 2, 2024 08:47

shargon changed the title ~~Calculate fee~~ Extend CalculateNetworkFee with contract verification Jul 2, 2024

fix using

7b6ccb9

shargon added the Waiting for Review label Jul 2, 2024

cschuchardt88 reviewed Jul 2, 2024

View reviewed changes

src/Neo/Wallets/Helper.cs Outdated Show resolved Hide resolved

Remove comments

78ddbb1

dusmart reviewed Jul 4, 2024

View reviewed changes

src/Neo/Wallets/Helper.cs Outdated Show resolved Hide resolved

Merge branch 'master' into core-calculate-fee

eccec43

AnnaShaleva reviewed Jul 4, 2024

View reviewed changes

cschuchardt88 previously requested changes Jul 4, 2024

View reviewed changes

Remove map

8a8af47

Merge branch 'master' into core-calculate-fee

20935fd

shargon commented Jul 9, 2024

View reviewed changes

shargon and others added 6 commits July 11, 2024 10:43

Anna's feedback

58cab0d

ensure only one result

e2cc49f

Merge branch 'master' into core-calculate-fee

1d071a5

Merge branch 'master' into core-calculate-fee

dabcd58

Merge branch 'master' into core-calculate-fee

45588a9

Merge branch 'master' into core-calculate-fee

373e1d0

cschuchardt88 previously approved these changes Jul 23, 2024

View reviewed changes

src/Neo/Wallets/Helper.cs Show resolved Hide resolved

cschuchardt88 and others added 2 commits July 23, 2024 04:33

Merge branch 'master' into core-calculate-fee

e4fa517

Merge branch 'master' into core-calculate-fee

56f39ed

Jim8y previously approved these changes Jul 30, 2024

View reviewed changes

roman-khimov reviewed Jul 30, 2024

View reviewed changes

shargon commented Jul 30, 2024

View reviewed changes

src/Neo/Wallets/Helper.cs Outdated Show resolved Hide resolved

Merge branch 'master' into core-calculate-fee

149f910

Merge branch 'master' into core-calculate-fee

590e83d

Merge branch 'master' into core-calculate-fee

ea69990

cschuchardt88 previously approved these changes Oct 13, 2024

View reviewed changes

Merge branch 'master' into core-calculate-fee

e7154ef

shargon dismissed stale reviews from cschuchardt88 and Jim8y via e7154ef October 14, 2024 08:52

roman-khimov mentioned this pull request Oct 17, 2024

Support calculating fees for non-standard verification scripts #3539

Open

roman-khimov previously approved these changes Oct 17, 2024

View reviewed changes

shargon and others added 2 commits October 17, 2024 23:05

Merge branch 'master' into core-calculate-fee

fca78e8

Merge branch 'master' into core-calculate-fee

f4234d1

AnnaShaleva previously approved these changes Oct 24, 2024

View reviewed changes

Fix compilation

256e1e2

shargon dismissed stale reviews from AnnaShaleva and roman-khimov via 256e1e2 October 24, 2024 17:48

AnnaShaleva approved these changes Oct 25, 2024

View reviewed changes

Jim8y approved these changes Oct 25, 2024

View reviewed changes

Jim8y added the Ready to Merge label Oct 25, 2024

Merge branch 'master' into core-calculate-fee

d682870

NGDAdmin merged commit 2f4717a into master Nov 5, 2024
6 checks passed

NGDAdmin deleted the core-calculate-fee branch November 5, 2024 02:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend `CalculateNetworkFee` with contract verification #3385

Extend `CalculateNetworkFee` with contract verification #3385

shargon commented Jul 2, 2024 •

edited

Loading

AnnaShaleva Jul 4, 2024

cschuchardt88 Jul 4, 2024 •

edited

Loading

roman-khimov Jul 30, 2024

shargon Jul 30, 2024

roman-khimov Jul 30, 2024

shargon Jul 31, 2024

AnnaShaleva Jul 4, 2024

shargon Jul 11, 2024

roman-khimov Jul 30, 2024

roman-khimov Jul 30, 2024

cschuchardt88 Jul 4, 2024 •

edited

Loading

shargon Jul 9, 2024

AnnaShaleva Jul 10, 2024

AnnaShaleva Jul 10, 2024

shargon Jul 11, 2024

dusmart Jul 11, 2024

dusmart Jul 11, 2024 •

edited

Loading

shargon commented Jul 23, 2024

roman-khimov left a comment

roman-khimov Jul 30, 2024

shargon commented Aug 9, 2024

roman-khimov commented Aug 9, 2024

shargon commented Aug 9, 2024

AnnaShaleva commented Aug 9, 2024 •

edited

Loading

Jim8y commented Sep 5, 2024

shargon commented Sep 16, 2024

cschuchardt88 commented Sep 16, 2024

shargon commented Sep 16, 2024

cschuchardt88 commented Oct 13, 2024

roman-khimov left a comment

	_ = engine.Execute(); // https://github.com/neo-project/neo/issues/2805
	if (engine.Execute() == VMState.FAULT) throw new ArgumentException($"Smart contract {contract.Hash} verification fault.");
	_ = engine.ResultStack.Pop().GetBoolean(); // The result must be convertible to boolean

	if (engine.Execute() == VMState.FAULT) return false;
	if (!engine.ResultStack.Peek().GetBoolean()) return false;
	fee = engine.FeeConsumed;

Extend CalculateNetworkFee with contract verification #3385

Extend CalculateNetworkFee with contract verification #3385

Conversation

shargon commented Jul 2, 2024 • edited Loading

Description

Type of change

How Has This Been Tested?

Checklist:

Choose a reason for hiding this comment

cschuchardt88 Jul 4, 2024 • edited Loading

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

cschuchardt88 Jul 4, 2024 • edited Loading

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

dusmart Jul 11, 2024 • edited Loading

Choose a reason for hiding this comment

shargon commented Jul 23, 2024

roman-khimov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shargon commented Aug 9, 2024

roman-khimov commented Aug 9, 2024

shargon commented Aug 9, 2024

AnnaShaleva commented Aug 9, 2024 • edited Loading

Jim8y commented Sep 5, 2024

shargon commented Sep 16, 2024

cschuchardt88 commented Sep 16, 2024

shargon commented Sep 16, 2024

cschuchardt88 commented Oct 13, 2024

roman-khimov left a comment

Choose a reason for hiding this comment

Extend `CalculateNetworkFee` with contract verification #3385

Extend `CalculateNetworkFee` with contract verification #3385

shargon commented Jul 2, 2024 •

edited

Loading

cschuchardt88 Jul 4, 2024 •

edited

Loading

cschuchardt88 Jul 4, 2024 •

edited

Loading

dusmart Jul 11, 2024 •

edited

Loading

AnnaShaleva commented Aug 9, 2024 •

edited

Loading