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

fix verify api #564

Merged
merged 12 commits into from
Mar 17, 2021
16 changes: 15 additions & 1 deletion src/RpcServer/RpcServer.SmartContract.cs
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 ?? string.Empty),
VerificationScript = Convert.FromBase64String(x.Verification ?? string.Empty)
}).ToArray()
};

// Validate format

Expand Down
35 changes: 24 additions & 11 deletions src/RpcServer/RpcServer.Wallet.cs
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,22 +347,36 @@ 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

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

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.


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

invocationScript = sb.ToArray();
tx.Witnesses ??= new Witness[] { new() { InvocationScript = invocationScript } };
engine.LoadScript(new Script(invocationScript), configureState: p => p.CallFlags = CallFlags.None);
}
JObject json = new JObject();
json["script"] = Convert.ToBase64String(contract.Script);

json["script"] = Convert.ToBase64String(invocationScript);
json["state"] = engine.Execute();
json["gasconsumed"] = engine.GasConsumed.ToString();
json["exception"] = GetExceptionMessage(engine.FaultException);
Expand Down