-
Notifications
You must be signed in to change notification settings - Fork 145
Fix ReferenceCounter
corner cases
#406
Fix ReferenceCounter
corner cases
#406
Conversation
Hi, can you post the script that reproduces the error, let me study it? |
@erikzhang Yes. This script will hit both error cases:
Let us know how else we can help. |
The scripts are too long and contain multiple methods. Can you provide a specific method that will cause the error? Can you provide its script and source code? |
This is the source code that produces that script: I'm not sure what you mean by "multiple methods"? I realize it's a lot of bytecode for such a small amount of source code. We've never had the change to optimize the compiler. |
I don't think so. The script you provided contains 2842 instructions. |
Well, this script can be thrown into
And NeoGo:
(I've used public RC1 RPC servers). So looks like there is something interesting going on here, but we're not yet sure what exactly that is. |
@spencercorwin could you share the source code of the contract? the script is too long to take an analysis. |
The source code for that script is just this:
It is a simplified version of this unit test: https://github.com/neo-one-suite/neo-one/blob/master/packages/neo-one-smart-contract-compiler/src/__tests__/compile/builtins/console/log.test.ts#L128 I know the script is too long to analyze but I'm not sure how we can make it smaller right now. Our compiler unfortunately produces a lot of boilerplate bytecode for even small amounts of source code. I can try to see if there's an easy way to remove some of the excess bytecode today. @roman-khimov The error you are seeing from the C# neo-vm is the same error we are getting. Here is the stack trace for that error: |
I can't imagine how to produce such a lot bytes for a single |
The patch fix the issue, but not the main problem, I tried to find it without luck with this script... It's a map, with and array, with and struct inside... |
Yeah I honestly don't know how/why our compile produces so much bytecode. I just tried looking into it, but it may take a while for me to understand what is going on. Unfortunately I didn't write the compiler, I've only maintained it and made updates to it, so I don't have a thorough understanding yet. Luckily, this issue only comes up in two of our unit tests, so it doesn't happen often.
This sounds correct. The compiler creates, duplicates, and modifies a lot of compound types in order to create the stack representation of this JavaScript object and to turn it into a VM notification that can be converted to a human-readable format. |
I create a CSharp smartcontract for reproducing this error. Here it is:
The main cause is that the VM cannot trace MapWrapper item's referenceCounter correctly. After "Iterator.Create", the map item and its subitem were removed in referenceCounter, then the iterator "reborn" its subitems, and will throw exception in the next zeroreffer check processs. Attached a quick fix code for MapWrapper, need a better way to resolve the problem completely:
|
I prefer to remove |
@erikzhang it's good to me |
It looks good to me, too.
|
We're working on our TypeScript smart contract compiler and we seem to have hit two corner case errors. It's quite difficult to try to trace/log exactly what's going on with all the CompoundType items our compiler creates and how they are managed in the
ReferenceCounter
. But this change fixes those corner cases.Two two corner cases are:
subitem
is being removed fromcounter
before it gets here.entry.ObjectReferences
is null. Not sure how this happens.Let us know if these changes are unacceptable. We're not sure if these corner cases are something we need to fix in our compiler or not. We've been looking at this problem for a bit and aren't quite sure what to do.
@ndhuutai