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.

Sign up for GitHub

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

Extend CalculateNetworkFee with contract verification #3385

Merged
merged 30 commits into from
Nov 5, 2024
Merged

Extend CalculateNetworkFee with contract verification #3385

Changes from 6 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
099f7be
Calculate fee
shargon Jul 2, 2024
3a332fa
change values
shargon Jul 2, 2024
ad3d477
remove using
shargon Jul 2, 2024
7b6ccb9
fix using
shargon Jul 2, 2024
78ddbb1
Remove comments
shargon Jul 2, 2024
eccec43
Merge branch 'master' into core-calculate-fee
cschuchardt88 Jul 4, 2024
8a8af47
Remove map
shargon Jul 5, 2024
20935fd
Merge branch 'master' into core-calculate-fee
shargon Jul 9, 2024
58cab0d
Anna's feedback
shargon Jul 11, 2024
e2cc49f
ensure only one result
shargon Jul 11, 2024
1d071a5
Merge branch 'master' into core-calculate-fee
cschuchardt88 Jul 11, 2024
dabcd58
Merge branch 'master' into core-calculate-fee
Jim8y Jul 15, 2024
45588a9
Merge branch 'master' into core-calculate-fee
Jim8y Jul 22, 2024
373e1d0
Merge branch 'master' into core-calculate-fee
shargon Jul 23, 2024
e4fa517
Merge branch 'master' into core-calculate-fee
cschuchardt88 Jul 23, 2024
56f39ed
Merge branch 'master' into core-calculate-fee
Jim8y Jul 30, 2024
3812a45
Update src/Neo/Wallets/Helper.cs
shargon Jul 30, 2024
c0f03b1
Merge branch 'master' into core-calculate-fee
shargon Jul 30, 2024
15a563d
Merge branch 'master' into core-calculate-fee
shargon Jul 30, 2024
8ad1acf
Remove error
shargon Jul 31, 2024
6fa6ef9
Merge branch 'master' into core-calculate-fee
shargon Aug 1, 2024
2745757
Merge branch 'master' into core-calculate-fee
shargon Aug 6, 2024
149f910
Merge branch 'master' into core-calculate-fee
shargon Aug 9, 2024
590e83d
Merge branch 'master' into core-calculate-fee
Jim8y Sep 5, 2024
ea69990
Merge branch 'master' into core-calculate-fee
shargon Sep 16, 2024
e7154ef
Merge branch 'master' into core-calculate-fee
shargon Oct 14, 2024
fca78e8
Merge branch 'master' into core-calculate-fee
shargon Oct 17, 2024
f4234d1
Merge branch 'master' into core-calculate-fee
Jim8y Oct 24, 2024
256e1e2
Fix compilation
shargon Oct 24, 2024
d682870
Merge branch 'master' into core-calculate-fee
Jim8y Nov 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 58 additions & 15 deletions src/Neo/Wallets/Helper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
using Neo.SmartContract;
using Neo.SmartContract.Native;
using Neo.VM;
using Neo.VM.Types;
using System;
using static Neo.SmartContract.Helper;

Expand Down Expand Up @@ -117,6 +118,7 @@ public static long CalculateNetworkFee(this Transaction tx, DataCache snapshot,

if (witnessScript is null || witnessScript.Length == 0)
{
// Contract-based verification
var contract = NativeContract.ContractManagement.GetContract(snapshot, hash);
if (contract is null)
throw new ArgumentException($"The smart contract or address {hash} is not found");
Expand All @@ -126,35 +128,76 @@ public static long CalculateNetworkFee(this Transaction tx, DataCache snapshot,
if (md.ReturnType != ContractParameterType.Boolean)
throw new ArgumentException("The verify method doesn't return boolean value.");
if (md.Parameters.Length > 0 && invocationScript is null)
throw new ArgumentException("The verify method requires parameters that need to be passed via the witness' invocation script.");
{
var script = new ScriptBuilder();
foreach (var par in md.Parameters)
{
switch (par.Type)
{
case ContractParameterType.Any:
case ContractParameterType.Signature:
case ContractParameterType.String:
case ContractParameterType.ByteArray:
script.EmitPush(new byte[64]);
break;
case ContractParameterType.Boolean:
script.EmitPush(true);
break;
case ContractParameterType.Integer:
script.Emit(OpCode.PUSHINT256, new byte[Integer.MaxSize]);
break;
case ContractParameterType.Hash160:
script.EmitPush(new byte[UInt160.Length]);
break;
case ContractParameterType.Hash256:
script.EmitPush(new byte[UInt256.Length]);
break;
case ContractParameterType.PublicKey:
script.EmitPush(new byte[33]);
break;
case ContractParameterType.Array:
script.Emit(OpCode.PUSHINT256, new byte[Integer.MaxSize]);
script.Emit(OpCode.NEWARRAY);
shargon marked this conversation as resolved.
Show resolved Hide resolved
shargon marked this conversation as resolved.
Show resolved Hide resolved
break;
case ContractParameterType.Map:
script.Emit(OpCode.NEWMAP);
shargon marked this conversation as resolved.
Show resolved Hide resolved
break;
default:
throw new ArgumentException("The verify method requires parameters that need to be passed via the witness' invocation script.");
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

@cschuchardt88 cschuchardt88 Jul 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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)

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

@roman-khimov done

}
}
invocationScript = script.ToArray();
}

// Empty verification and non-empty invocation scripts
var invSize = invocationScript?.GetVarSize() ?? Array.Empty<byte>().GetVarSize();
size += Array.Empty<byte>().GetVarSize() + invSize;
var invSize = invocationScript?.GetVarSize() ?? System.Array.Empty<byte>().GetVarSize();
size += System.Array.Empty<byte>().GetVarSize() + invSize;

// 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
shargon marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Exactly like a standard verification code.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

Copy link

Choose a reason for hiding this comment

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

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;

Copy link

@dusmart dusmart Jul 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.


maxExecutionCost -= engine.FeeConsumed;
if (maxExecutionCost <= 0) throw new InvalidOperationException("Insufficient GAS.");
networkFee += engine.FeeConsumed;
}
else if (IsSignatureContract(witnessScript))
{
size += 67 + witnessScript.GetVarSize();
networkFee += exec_fee_factor * SignatureContractCost();
}
else if (IsMultiSigContract(witnessScript, out int m, out int n))
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.
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 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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

}
networkFee += size * NativeContract.Policy.GetFeePerByte(snapshot);
foreach (TransactionAttribute attr in tx.Attributes)
Expand Down
Loading