Skip to content

Fix: Serialize non deserializable object#1978

Closed
shargon wants to merge 5 commits intoneo-project:masterfrom
shargon:fix-serialization
Closed

Fix: Serialize non deserializable object#1978
shargon wants to merge 5 commits intoneo-project:masterfrom
shargon:fix-serialization

Conversation

@shargon
Copy link
Copy Markdown
Member

@shargon shargon commented Sep 29, 2020

  • Modified GasRecord to use enumerators, because otherwise we can overflow the limits, whatever it is

@shargon shargon requested a review from erikzhang September 29, 2020 14:44
}

private static void Serialize(StackItem item, BinaryWriter writer, uint maxSize)
private static void Serialize(StackItem item, BinaryWriter writer, uint maxSize, uint maxArraySize, uint maxItemSize)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why add maxArraySize and maxItemSize? If the item exceeds the limit, it cannot be pushed onto the stack.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This method it's shared with https://github.com/neo-project/neo/pull/1978/files#diff-03213bc090127a83f7aa1132e8e516e0R25 and it will be managed by the native contract not by neo-vm, so we can have different limmits

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Then we should limit in native contracts.

Copy link
Copy Markdown
Member Author

@shargon shargon Oct 1, 2020

Choose a reason for hiding this comment

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

Please, take a look to #1983

{
BigInteger bi => bi.ToByteArrayStandard(),
IInteroperable interoperable => BinarySerializer.Serialize(interoperable.ToStackItem(null), 4096),
IInteroperable interoperable => BinarySerializer.Serialize(interoperable.ToStackItem(null), 4096, 16, 34),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe 16 and 34 can be defined as some static fields instead of hard code.

Copy link
Copy Markdown
Contributor

@Qiao-Jin Qiao-Jin Oct 14, 2020

Choose a reason for hiding this comment

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

It seems that they have been modified to ExecutionEngineLimits.Default.MaxStackSize, ExecutionEngineLimits.Default.MaxItemSize correspondingly in #1988.
Besides other limits are also changed in #1988.
Maybe we can put the logic of this PR into #1988?

@shargon shargon closed this Oct 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants