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 diagnostic for ApplicationEngine #2613

Merged
merged 7 commits into from
Nov 10, 2021
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
35 changes: 35 additions & 0 deletions src/neo/IO/Caching/Tree.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
// Copyright (C) 2015-2021 The Neo Project.
//
// The neo is free software distributed under the MIT software license,
// see the accompanying file LICENSE in the main directory of the
// project or http://www.opensource.org/licenses/mit-license.php
// for more details.
//
// Redistribution and use in source and binary forms with or without
// modifications are permitted.

using System;
using System.Collections.Generic;

namespace Neo.IO.Caching
{
public class Tree<T>
{
public TreeNode<T> Root { get; private set; }

public TreeNode<T> AddRoot(T item)
{
if (Root is not null)
throw new InvalidOperationException();
Root = new TreeNode<T>(item, null);
return Root;
}

public IEnumerable<T> GetItems()
{
if (Root is null) yield break;
foreach (T item in Root.GetItems())
yield return item;
}
}
}
43 changes: 43 additions & 0 deletions src/neo/IO/Caching/TreeNode.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
// Copyright (C) 2015-2021 The Neo Project.
//
// The neo is free software distributed under the MIT software license,
// see the accompanying file LICENSE in the main directory of the
// project or http://www.opensource.org/licenses/mit-license.php
// for more details.
//
// Redistribution and use in source and binary forms with or without
// modifications are permitted.

using System.Collections.Generic;

namespace Neo.IO.Caching
{
public class TreeNode<T>
{
public T Item { get; }
public TreeNode<T> Parent { get; }

private readonly List<TreeNode<T>> children = new();

internal TreeNode(T item, TreeNode<T> parent)
{
Item = item;
Parent = parent;
}

public TreeNode<T> AddChild(T item)
{
TreeNode<T> child = new(item, this);
children.Add(child);
return child;
}

internal IEnumerable<T> GetItems()
{
yield return Item;
foreach (var child in children)
foreach (T item in child.GetItems())
yield return item;
}
}
}
3 changes: 2 additions & 1 deletion src/neo/Plugins/IApplicationEngineProvider.cs
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,8 @@ public interface IApplicationEngineProvider
/// <param name="persistingBlock">The block being persisted. It should be <see langword="null"/> if the <paramref name="trigger"/> is <see cref="TriggerType.Verification"/>.</param>
/// <param name="settings">The <see cref="ProtocolSettings"/> used by the engine.</param>
/// <param name="gas">The maximum gas used in this execution. The execution will fail when the gas is exhausted.</param>
/// <param name="diagnostic">The diagnostic to be used by the <see cref="ApplicationEngine"/>.</param>
Copy link
Contributor

Choose a reason for hiding this comment

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

Diagnostic parameter should have null default value to avoid code breakage

Copy link
Member Author

Choose a reason for hiding this comment

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

This is an interface. I think it is meaningless to add the default parameter. Because all classes that implement this interface should be updated.

Copy link
Contributor

Choose a reason for hiding this comment

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

Unless there's a null default value, we also have to update code that calls into classes that implement this interface.

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 classes are not to be called directly. We need to use ApplicationEngine.Create() instead.

/// <returns>The engine instance created.</returns>
ApplicationEngine Create(TriggerType trigger, IVerifiable container, DataCache snapshot, Block persistingBlock, ProtocolSettings settings, long gas);
ApplicationEngine Create(TriggerType trigger, IVerifiable container, DataCache snapshot, Block persistingBlock, ProtocolSettings settings, long gas, Diagnostic diagnostic);
erikzhang marked this conversation as resolved.
Show resolved Hide resolved
}
}
34 changes: 27 additions & 7 deletions src/neo/SmartContract/ApplicationEngine.cs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
// modifications are permitted.

using Neo.IO;
using Neo.IO.Caching;
using Neo.IO.Json;
using Neo.Network.P2P.Payloads;
using Neo.Persistence;
Expand Down Expand Up @@ -50,6 +51,7 @@ public partial class ApplicationEngine : ExecutionEngine

private static IApplicationEngineProvider applicationEngineProvider;
private static Dictionary<uint, InteropDescriptor> services;
private TreeNode<UInt160> currentNodeOfInvocationTree = null;
private readonly long gas_amount;
private List<NotifyEventArgs> notifications;
private List<IDisposable> disposables;
Expand All @@ -64,6 +66,11 @@ public partial class ApplicationEngine : ExecutionEngine
/// </summary>
public static IReadOnlyDictionary<uint, InteropDescriptor> Services => services;

/// <summary>
/// The diagnostic used by the engine. This property can be <see langword="null"/>.
/// </summary>
public Diagnostic Diagnostic { get; }

private List<IDisposable> Disposables => disposables ??= new List<IDisposable>();

/// <summary>
Expand Down Expand Up @@ -135,14 +142,16 @@ public partial class ApplicationEngine : ExecutionEngine
/// <param name="persistingBlock">The block being persisted. It should be <see langword="null"/> if the <paramref name="trigger"/> is <see cref="TriggerType.Verification"/>.</param>
/// <param name="settings">The <see cref="Neo.ProtocolSettings"/> used by the engine.</param>
/// <param name="gas">The maximum gas used in this execution. The execution will fail when the gas is exhausted.</param>
protected unsafe ApplicationEngine(TriggerType trigger, IVerifiable container, DataCache snapshot, Block persistingBlock, ProtocolSettings settings, long gas)
/// <param name="diagnostic">The diagnostic to be used by the <see cref="ApplicationEngine"/>.</param>
protected unsafe ApplicationEngine(TriggerType trigger, IVerifiable container, DataCache snapshot, Block persistingBlock, ProtocolSettings settings, long gas, Diagnostic diagnostic)
{
this.Trigger = trigger;
this.ScriptContainer = container;
this.Snapshot = snapshot;
this.PersistingBlock = persistingBlock;
this.ProtocolSettings = settings;
this.gas_amount = gas;
this.Diagnostic = diagnostic;
this.exec_fee_factor = snapshot is null || persistingBlock?.Index == 0 ? PolicyContract.DefaultExecFeeFactor : NativeContract.Policy.GetExecFeeFactor(Snapshot);
this.StoragePrice = snapshot is null || persistingBlock?.Index == 0 ? PolicyContract.DefaultStoragePrice : NativeContract.Policy.GetStoragePrice(Snapshot);
this.nonceData = container is Transaction tx ? tx.Hash.ToArray()[..16] : new byte[16];
Expand Down Expand Up @@ -250,6 +259,8 @@ internal ContractTask<T> CallFromNativeContract<T>(UInt160 callingScriptHash, UI
protected override void ContextUnloaded(ExecutionContext context)
{
base.ContextUnloaded(context);
if (Diagnostic is not null)
currentNodeOfInvocationTree = currentNodeOfInvocationTree.Parent;
Copy link
Contributor

Choose a reason for hiding this comment

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

What if the parent of currentNodeOfInvocationTree is null? should we also remove the root of Diagnostic?

Copy link
Member Author

Choose a reason for hiding this comment

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

It doesn't matter.

if (!contractTasks.Remove(context, out var awaiter)) return;
if (UncaughtException is not null)
throw new VMUnhandledException(UncaughtException);
Expand All @@ -265,21 +276,29 @@ protected override void ContextUnloaded(ExecutionContext context)
/// <param name="persistingBlock">The block being persisted. It should be <see langword="null"/> if the <paramref name="trigger"/> is <see cref="TriggerType.Verification"/>.</param>
/// <param name="settings">The <see cref="Neo.ProtocolSettings"/> used by the engine.</param>
/// <param name="gas">The maximum gas used in this execution. The execution will fail when the gas is exhausted.</param>
/// <param name="diagnostic">The diagnostic to be used by the <see cref="ApplicationEngine"/>.</param>
/// <returns>The engine instance created.</returns>
public static ApplicationEngine Create(TriggerType trigger, IVerifiable container, DataCache snapshot, Block persistingBlock = null, ProtocolSettings settings = null, long gas = TestModeGas)
public static ApplicationEngine Create(TriggerType trigger, IVerifiable container, DataCache snapshot, Block persistingBlock = null, ProtocolSettings settings = null, long gas = TestModeGas, Diagnostic diagnostic = null)
Copy link
Contributor

Choose a reason for hiding this comment

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

Diagnostic parameter should have null default value to avoid code breakage

Copy link
Member Author

Choose a reason for hiding this comment

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

It does have a null default value.

{
return applicationEngineProvider?.Create(trigger, container, snapshot, persistingBlock, settings, gas)
?? new ApplicationEngine(trigger, container, snapshot, persistingBlock, settings, gas);
return applicationEngineProvider?.Create(trigger, container, snapshot, persistingBlock, settings, gas, diagnostic)
?? new ApplicationEngine(trigger, container, snapshot, persistingBlock, settings, gas, diagnostic);
}

protected override void LoadContext(ExecutionContext context)
{
// Set default execution context state

var state = context.GetState<ExecutionContextState>();
state.ScriptHash ??= ((byte[])context.Script).ToScriptHash();
invocationCounter.TryAdd(state.ScriptHash, 1);

if (Diagnostic is not null)
Copy link
Contributor

Choose a reason for hiding this comment

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

There is an interesting problem related to #2130 (comment), suppose we have a deployed contract with onNEP17Payment, we transfer some NEO to it, add a number of blocks and then transfer 1 NEO more. This triggers GAS distribution and onNEP17Payment call from GAS contract (even before onNEP17Payment call from NEO). The contract can ask GetCallingScriptHash() and get GAS hash in return, because this invocation is made from the GAS contract. But this will never be reflected in the invocation tree, there we'll have

  • entry
    • NEO
      • contract
      • contract

instead of

  • entry
    • NEO
      • GAS
        • contract
      • contract

{
if (currentNodeOfInvocationTree is null)
currentNodeOfInvocationTree = Diagnostic.InvocationTree.AddRoot(state.ScriptHash);
else
currentNodeOfInvocationTree = currentNodeOfInvocationTree.AddChild(state.ScriptHash);
}

base.LoadContext(context);
}

Expand Down Expand Up @@ -531,11 +550,12 @@ internal static void ResetApplicationEngineProvider()
/// <param name="settings">The <see cref="Neo.ProtocolSettings"/> used by the engine.</param>
/// <param name="offset">The initial position of the instruction pointer.</param>
/// <param name="gas">The maximum gas used in this execution. The execution will fail when the gas is exhausted.</param>
/// <param name="diagnostic">The diagnostic to be used by the <see cref="ApplicationEngine"/>.</param>
/// <returns>The engine instance created.</returns>
public static ApplicationEngine Run(byte[] script, DataCache snapshot, IVerifiable container = null, Block persistingBlock = null, ProtocolSettings settings = null, int offset = 0, long gas = TestModeGas)
public static ApplicationEngine Run(byte[] script, DataCache snapshot, IVerifiable container = null, Block persistingBlock = null, ProtocolSettings settings = null, int offset = 0, long gas = TestModeGas, Diagnostic diagnostic = null)
{
persistingBlock ??= CreateDummyBlock(snapshot, settings ?? ProtocolSettings.Default);
ApplicationEngine engine = Create(TriggerType.Application, container, snapshot, persistingBlock, settings, gas);
ApplicationEngine engine = Create(TriggerType.Application, container, snapshot, persistingBlock, settings, gas, diagnostic);
engine.LoadScript(script, initialPosition: offset);
engine.Execute();
return engine;
Expand Down
19 changes: 19 additions & 0 deletions src/neo/SmartContract/Diagnostic.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
// Copyright (C) 2015-2021 The Neo Project.
//
// The neo is free software distributed under the MIT software license,
// see the accompanying file LICENSE in the main directory of the
// project or http://www.opensource.org/licenses/mit-license.php
// for more details.
//
// Redistribution and use in source and binary forms with or without
// modifications are permitted.

using Neo.IO.Caching;

namespace Neo.SmartContract
{
public class Diagnostic
Copy link
Contributor

Choose a reason for hiding this comment

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

Diagnostic seems like a poor name for this class. It sounds to me as if it is more general purpose than simply tracking the tree of contract invocations. Could we call the class InvocationTree and call the property Contracts?

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 add more functions to Diagnostic later.

{
public Tree<UInt160> InvocationTree { get; }
erikzhang marked this conversation as resolved.
Show resolved Hide resolved
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -63,16 +63,16 @@ public void TestCanResetAppEngineProviderTwice()

class TestProvider : IApplicationEngineProvider
{
public ApplicationEngine Create(TriggerType trigger, IVerifiable container, DataCache snapshot, Block persistingBlock, ProtocolSettings settings, long gas)
public ApplicationEngine Create(TriggerType trigger, IVerifiable container, DataCache snapshot, Block persistingBlock, ProtocolSettings settings, long gas, Diagnostic diagnostic)
{
return new TestEngine(trigger, container, snapshot, persistingBlock, settings, gas);
return new TestEngine(trigger, container, snapshot, persistingBlock, settings, gas, diagnostic);
}
}

class TestEngine : ApplicationEngine
{
public TestEngine(TriggerType trigger, IVerifiable container, DataCache snapshot, Block persistingBlock, ProtocolSettings settings, long gas)
: base(trigger, container, snapshot, persistingBlock, settings, gas)
public TestEngine(TriggerType trigger, IVerifiable container, DataCache snapshot, Block persistingBlock, ProtocolSettings settings, long gas, Diagnostic diagnostic)
: base(trigger, container, snapshot, persistingBlock, settings, gas, diagnostic)
{
}
}
Expand Down