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

Jump to bottom

fix verify api #564

Merged
merged 12 commits into from
Mar 17, 2021
Merged

fix verify api #564

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
2d467f1
fix verify api
Ashuaidehao Mar 16, 2021
ca8d87f
Merge branch 'master' into Fix-verifyfee
superboyiii Mar 16, 2021
331e24a
refac
Ashuaidehao Mar 16, 2021
ec221df
Merge branch 'Fix-verifyfee' of https://github.com/Ashuaidehao/neo-mo…
Ashuaidehao Mar 16, 2021
647f56b
default signer & parameter
Ashuaidehao Mar 17, 2021
108b15e
refac
Ashuaidehao Mar 17, 2021
bf5034c
add Witness
Ashuaidehao Mar 17, 2021
b79c6de
format
Ashuaidehao Mar 17, 2021
9f89979
update
Ashuaidehao Mar 17, 2021
1d4b121
update
Ashuaidehao Mar 17, 2021
1043c8f
update
Ashuaidehao Mar 17, 2021
2069218
Merge branch 'master' into Fix-verifyfee
superboyiii Mar 17, 2021
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
16 changes: 15 additions & 1 deletion src/RpcServer/RpcServer.SmartContract.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,21 @@ private static Signers SignersFromJson(JArray _params, ProtocolSettings settings
Scopes = (WitnessScope)Enum.Parse(typeof(WitnessScope), u["scopes"]?.AsString()),
AllowedContracts = ((JArray)u["allowedcontracts"])?.Select(p => UInt160.Parse(p.AsString())).ToArray(),
AllowedGroups = ((JArray)u["allowedgroups"])?.Select(p => ECPoint.Parse(p.AsString(), ECCurve.Secp256r1)).ToArray()
}).ToArray());
}).ToArray())
{
Witnesses = _params
.Select(u => new
{
Invocation = u["invocation"]?.AsString(),
Verification = u["verification"]?.AsString()
})
.Where(x => x.Invocation != null && x.Verification != null)
.Select(x => new Witness()
{
InvocationScript = Convert.FromBase64String(x.Invocation),
VerificationScript = Convert.FromBase64String(x.Verification)
}).ToArray()
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
.Where(x => x.Invocation != null && x.Verification != null)
.Select(x => new Witness()
{
InvocationScript = Convert.FromBase64String(x.Invocation),
VerificationScript = Convert.FromBase64String(x.Verification)
}).ToArray()
.Where(x => x.Invocation != null || x.Verification != null)
.Select(x => new Witness()
{
InvocationScript = x.Invocation != null ?? Convert.FromBase64String(x.Invocation) : null,
VerificationScript = x.Verification != null ?? Convert.FromBase64String(x.Verification) : null
}).ToArray()

};

// Validate format

Expand Down
28 changes: 18 additions & 10 deletions src/RpcServer/RpcServer.Wallet.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -333,7 +333,6 @@ protected virtual JObject SendToAddress(JArray _params)
[RpcMethod]
protected virtual JObject InvokeContractVerify(JArray _params)
{
CheckWallet();
UInt160 script_hash = UInt160.Parse(_params[0].AsString());
ContractParameter[] args = _params.Count >= 2 ? ((JArray)_params[1]).Select(p => ContractParameter.FromJson(p)).ToArray() : new ContractParameter[0];
Signers signers = _params.Count >= 3 ? SignersFromJson((JArray)_params[2], system.Settings) : null;
Expand All @@ -348,20 +347,29 @@ private JObject GetVerificationResult(UInt160 scriptHash, ContractParameter[] ar
{
throw new RpcException(-100, "Unknown contract");
}
var methodName = "verify";
var md = contract.Manifest.Abi.GetMethod("verify", -1);
if (md is null)
throw new RpcException(-101, $"The smart contract {contract.Hash} haven't got verify method.");
if (md.ReturnType != ContractParameterType.Boolean)
throw new RpcException(-102, "The verify method doesn't return boolean value.");

Transaction tx = signers == null ? null : new Transaction
Transaction tx = new Transaction
{
Signers = signers.GetSigners(),
Attributes = Array.Empty<TransactionAttribute>()
Signers = signers == null ? new Signer[] { new() { Account = scriptHash } } : signers.GetSigners(),
Attributes = Array.Empty<TransactionAttribute>(),
Copy link
Member

Choose a reason for hiding this comment

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

Can we also fill the transaction script? I think that []byte{OpCode.RET} is enough.

This is needed for those verify methods that use System.Runtime.GetScriptContainer interop, because transaction can't be serialized without a script.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

Witnesses = signers?.Witnesses,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Witnesses = signers?.Witnesses,
Witnesses = signers == null ? new Witness() { InvocationScript = sb.ToArray() } : signers.Witnesses,

where sb is from

if (args.Length > 0)
            {
                using ScriptBuilder sb = new ScriptBuilder();
                for (int i = args.Length - 1; i >= 0; i--)
                    sb.EmitPush(args[i]);
             }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sb will be loaded into engine directly, is this InvocationScript necessary?

Copy link
Member

Choose a reason for hiding this comment

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

We know exactly the invocation script, why not to fill it in? By doing this we're getting closer to the real transaction witnesses.

Script = new[] { (byte)OpCode.RET }
Copy link
Member

@AnnaShaleva AnnaShaleva Mar 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

And the last issue: at the current moment, the transaction is completely missing witnesses. I would suggest to pass witnesses along with signers via RPC request parameters using the following structure:

{
    "jsonrpc": "2.0",
    "method": "invokecontractverify",
    "params": [
        "0x31f714e04766139c8f3705f5ce94518e1411ec07",
        [ ],
        [
            {
                "account": "NiRqSd5MtRZT5yUhgWd7oG11brkDG76Jim",
                "scopes": "CalledByEntry",
                "invocation": "DEDr2gA/8T/wxQvgOZVfCdkbj6uGrprkDgJvpOJCcbl+tvlKZkZytCZEWm6NoZhJyIlEI3VQSLtU3AHuJfShAT5L",
                "verification": "DCEDAS1H52IQrsc745qz0YbgpA/o2Gv6PU+r/aV7oTuI+WoLQZVEDXg=",
            },
            {
                "account": "NMA2FKN8up2cEwaJgtmAiDrZWB69ApnDfp",
                "scopes": "CalledByEntry",
            }
        ]
    ],
    "id": 1
}

And then parse the provided witnesses in GetSignersFromJson method (https://github.com/neo-project/neo-modules/blob/master/src/RpcServer/RpcServer.SmartContract.cs#L94).

And then fill the transaction like the following:

            Transaction tx = new Transaction
            {
                Witnesses = signers.Witnesses,
                ...
            }

Copy link
Member

@AnnaShaleva AnnaShaleva Mar 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

It will help us to get rid of the private key dependency (because if we parse witnesses, then we don't need private keys in the RPC node wallet anymore). And that's the answer to the #552 (comment) question.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok

};
ContractParametersContext context = new ContractParametersContext(snapshot, tx);
wallet.Sign(context);
tx.Witnesses = context.Completed ? context.GetWitnesses() : null;

using ApplicationEngine engine = ApplicationEngine.Create(TriggerType.Verification, tx, snapshot.CreateSnapshot(), settings: system.Settings);
engine.LoadScript(new ScriptBuilder().EmitDynamicCall(scriptHash, methodName, args).ToArray(), rvcount: 1);
engine.LoadContract(contract, md, CallFlags.ReadOnly);
Copy link
Member

Choose a reason for hiding this comment

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

And create an invocation script from the given args (it will push all arguments on the stack). And load the script right after the contract script, just like in https://github.com/neo-project/neo/blob/master/src/neo/SmartContract/Helper.cs#L246.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated it. Please test it like:

{
    "jsonrpc": "2.0",
    "method": "invokecontractverify",
    "params": [
        "0x31f714e04766139c8f3705f5ce94518e1411ec07",
        [
            {
                "type": "ByteArray",
                "value": "DED2HMp00JZ2zcJYjKgTp4Tl0GODm5r14PKN6ip8i2+8ox3JYqN2xNHxQzCgzENqSD52GhVF4eGx2i405zxM1KTH"
            }
        ],
        [
            {
                "account": "NMA2FKN8up2cEwaJgtmAiDrZWB69ApnDfp",
                "scopes": "CalledByEntry"
            }
        ]
    ],
    "id": 1
}

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 very convenient to construct an invocation script on the client. I would prefer to pass simple standard parameters to the server as we do in the InvokeFunction RPC method. Like the following:

{
    "jsonrpc": "2.0",
    "method": "invokecontractverify",
    "params": [
        "0x31f714e04766139c8f3705f5ce94518e1411ec07",
        [
            {
                "type": "Boolean",
                "value": "true"
            },
            {
                "type": "Integer",
                "value": 123
            }
        ],
        [
            {
                "account": "NMA2FKN8up2cEwaJgtmAiDrZWB69ApnDfp",
                "scopes": "CalledByEntry"
            }
        ]
    ],
    "id": 1
}

And then we can construct an invocation script on the server by performing the following:

using (ScriptBuilder sb = new ScriptBuilder())
{
        for (int i = args.Count - 1; i >= 0; i--)
                sb.EmitPush(args[i]);
}

Copy link
Member

Choose a reason for hiding this comment

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

Agree with @AnnaShaleva

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

if (args.Length > 0)
{
using ScriptBuilder sb = new ScriptBuilder();
for (int i = args.Length - 1; i >= 0; i--)
sb.EmitPush(args[i]);

engine.LoadScript(new Script(sb.ToArray()), configureState: p => p.CallFlags = CallFlags.None);
}
JObject json = new JObject();
json["script"] = Convert.ToBase64String(contract.Script);
Copy link
Member

Choose a reason for hiding this comment

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

And let the resulting script be the InvocationScript (which is sb.ToArray()). Because 1) the user is aware of the contract being invoked and 2) the user should have the ability to check that all the parameters were converted as expected.

Copy link
Member

Choose a reason for hiding this comment

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

See nspcc-dev/neo-go#1825 (comment).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What script mean if sb is empty?

Copy link
Member

Choose a reason for hiding this comment

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

That no parameters were pushed on the stack before verify execution.

json["state"] = engine.Execute();
Expand Down