Skip to content
This repository has been archived by the owner on Nov 22, 2023. It is now read-only.

Commit

Permalink
Clone struct when picking item from array or map.
Browse files Browse the repository at this point in the history
  • Loading branch information
@erikzhang
erikzhang committed Aug 20, 2018
1 parent 25723de commit ab74c36
Showing 1 changed file with 5 additions and 6 deletions.
11 changes: 5 additions & 6 deletions src/neo-vm/ExecutionEngine.cs
View file
Expand Up @@ -732,6 +732,7 @@ private void ExecuteOp(OpCode opcode, ExecutionContext context)
State |= VMState.FAULT;
return;
}
StackItem item;
switch (context.EvaluationStack.Pop())
{
case VMArray array:
Expand All @@ -741,14 +742,10 @@ private void ExecuteOp(OpCode opcode, ExecutionContext context)
State |= VMState.FAULT;
return;
}
context.EvaluationStack.Push(array[index]);
item = array[index];
break;
case Map map:
if (map.TryGetValue(key, out StackItem value))
{
context.EvaluationStack.Push(value);
}
else
if (!map.TryGetValue(key, out item))
{
State |= VMState.FAULT;
return;
Expand All @@ -758,6 +755,8 @@ private void ExecuteOp(OpCode opcode, ExecutionContext context)
State |= VMState.FAULT;
return;
}
if (item is Struct s) item = s.Clone();
CurrentContext.EvaluationStack.Push(item);
}
break;
case OpCode.SETITEM:
Expand Down

4 comments on commit ab74c36

@dicarlo2
Copy link

@dicarlo2 dicarlo2 commented on ab74c36 Aug 29, 2018 •
edited

Choose a reason for hiding this comment

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

@erikzhang
This might actually break existing contracts depending on how their compiler is implemented. E.g. I'm almost certain it breaks the ICO template contract that most ICOs use and v1 switcheo DEX - I have test cases that test contract avm copied directly from the mainnet, and they started failing only after making this change. (Tests are here, though likely hard to grok https://github.com/neo-one-suite/neo-one/blob/master/packages/neo-one-node-vm/src/__tests__/execute.test.ts. At a high level, they test the concierge ICO and switcheo v1 DEX contracts).

@erikzhang
Copy link
Member Author

Choose a reason for hiding this comment

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

Looking at the results of your test, do we have to roll back this commit? @dicarlo2

@dicarlo2
Copy link

Choose a reason for hiding this comment

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

@erikzhang definitely a good thing it was rolled back - I went ahead and pushed the change to neo-one and our nodes are returning different results, most likely because of this change.

@dicarlo2
Copy link

Choose a reason for hiding this comment

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

@erikzhang also, sorry for the delayed reply, it's been a hectic couple of weeks.

Please sign in to comment.