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

Enhance the limits #514

Merged
merged 3 commits into from
Oct 25, 2023
Merged

Enhance the limits #514

merged 3 commits into from
Oct 25, 2023

Conversation

shargon
Copy link
Member

@shargon shargon commented Oct 19, 2023 •
edited
Loading

As conversed with @roman-khimov

Alternative to fix #512

Max transaction script size is 64Kb, has no sense to have a bigger item, the longest one should be a nef file, and it should be limited to the transaction script size (now is not)

@shargon shargon requested a review from Jim8y October 19, 2023 08:55
@shargon
Copy link
Member Author

shargon commented Oct 19, 2023

Please @superboyiii could you check that there are no conflicts with any non FAULTED tx?

@roman-khimov
Copy link
Contributor

Just to reiterate some things from our conversation.

There is a limit for NEF script (which is the main part of it): https://github.com/neo-project/neo/blob/f5e257c11c514e47cdc1da6116e475a7324ba1ac/src/Neo/SmartContract/NefFile.cs#L77

But it's way off and deploying such a NEF wouldn't be trivial.

There is also a limit for manifest (somewhat more reasonable): https://github.com/neo-project/neo/blob/f5e257c11c514e47cdc1da6116e475a7324ba1ac/src/Neo/SmartContract/Manifest/ContractManifest.cs#L32

@Jim8y
Copy link
Contributor

Jim8y commented Oct 19, 2023

Can we put all these limitations into one file where we can easily check them? In another pr i mean.

Jim8y
Jim8y previously approved these changes Oct 19, 2023
View reviewed changes
AnnaShaleva
AnnaShaleva approved these changes Oct 19, 2023
View reviewed changes
Copy link
Member

@AnnaShaleva AnnaShaleva left a comment

Choose a reason for hiding this comment

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

Reasonable.

@AnnaShaleva AnnaShaleva mentioned this pull request Oct 19, 2023
Closed
@roman-khimov roman-khimov mentioned this pull request Oct 19, 2023
Closed
vncoelho
vncoelho reviewed Oct 20, 2023
View reviewed changes
Copy link
Member

@vncoelho vncoelho left a comment

Choose a reason for hiding this comment

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

I am not sure if transaction need to be turned to success for an attack to succeed.
Maybe the logic inside it can just change state if is check limits inside it and handle the exception properly.

This falls in that same kind of possible attacks.
@vang1ong7ang @dusmart

@Jim8y
Copy link
Contributor

Jim8y commented Oct 20, 2023 •
edited
Loading

I am not sure if transaction need to be turned to success for an attack to succeed. Maybe the logic inside it can just change state if is check limits inside it and handle the exception properly.

This falls in that same kind of possible attacks. @vang1ong7ang @dusmart

No way we can make any progress right now if we stuck in there. Though i uderstand your concern, we can not do anything about it before we have a solid version system. A desperate situation. This patch neo-project/neo-modules#835 should be able to alleviate the issue, but i dont like resync solutions and no idea when it can be merged.

@dusmart
Copy link

dusmart commented Oct 20, 2023 •
edited
Loading

Thanks for inviting.

Max transaction script size is 64Kb, has no sense to have a bigger item

Nope. By using dup and concat, one can easily build an explosive large item.

I misunderstood this PR's intention before. BTW. The name Reduce VM limits may mislead someone like me, I thought you will remove the limits (LOL). Enhance the limits is easy for me to get the point.

IIRC, the only way to make an item larger is using the cat opcode or Stdlib.JsonSerialize or something like this. If the valid largest object contains 65535 bytes. The invalid one can only be as large as 65535 * 2 bytes.

@vang1ong7ang
Copy link
Contributor

@vncoelho @Liaojinghui yes it will cause fork but it can be purposely ignored.

sometimes stack items are expanded during the execution -> make sense to have a bigger item than 64kb

@vang1ong7ang
Copy link
Contributor

neovm is like a well-equipped tesla electric car, and we keep it safe by limiting its battery capacity to 3000mAh

@shargon shargon changed the title Reduce VM limits Enhance the limits Oct 20, 2023
@shargon
Copy link
Member Author

shargon commented Oct 20, 2023

@superboyiii could you check if there are any conflict?

@shargon
Copy link
Member Author

shargon commented Oct 20, 2023 •
edited
Loading

I am not sure if transaction need to be turned to success for an attack to succeed.

No, is not required.

@vncoelho
Copy link
Member

Please @superboyiii could you check that there are no conflicts with any non FAULTED tx?

So, this check is not enough that you previously mentioned.

I think that check now is not enough to ensure safety of mainnet.
This is still imprudent.

We need to use the available versioning system or create a new one.

@shargon
Copy link
Member Author

shargon commented Oct 20, 2023

Please @superboyiii could you check that there are no conflicts with any non FAULTED tx?

So, this check is not enough that you previously mentioned.

I think that check now is not enough to ensure safety of mainnet. This is still imprudent.

We need to use the available versioning system or create a new one.

We reduce from 2Gb to 250 Mb the maximun of memory allowed per execution.

@superboyiii
Copy link
Member

Please @superboyiii could you check that there are no conflicts with any non FAULTED tx?

Sure

@dusmart
Copy link

dusmart commented Oct 23, 2023

We reduce from 2Gb to 250 Mb the maximun of memory allowed per execution.

IMO, this will also alleviate the harm introduced in neo-project/neo#2723.

@superboyiii
Copy link
Member

@shargon Due to the test result, no compatibility issue on mainnet. I will try testnet tomorrow.

@shargon shargon mentioned this pull request Oct 24, 2023
Merged
@superboyiii
Copy link
Member

superboyiii commented Oct 24, 2023 •
edited
Loading

@shargon No storage incompatible either on mainnet or testnet.

superboyiii
superboyiii approved these changes Oct 25, 2023
View reviewed changes
Copy link
Member

@superboyiii superboyiii left a comment

Choose a reason for hiding this comment

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

Here we go?

@Jim8y Jim8y merged commit 859417a into master Oct 25, 2023
2 checks passed
@Jim8y Jim8y deleted the reduce-limits branch October 25, 2023 13:54
AnnaShaleva added a commit to nspcc-dev/neo-go that referenced this pull request Nov 2, 2023
@AnnaShaleva
vm: reduce maximum stckitem size
b8a72c8 
To prevent possible DoS. Port the neo-project/neo-vm#514,
close #3170.

Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>
@AnnaShaleva AnnaShaleva mentioned this pull request Nov 2, 2023
Merged
@roman-khimov roman-khimov mentioned this pull request Nov 10, 2023
Closed
AnnaShaleva added a commit to nspcc-dev/neo-go that referenced this pull request Nov 13, 2023
@AnnaShaleva
vm: reduce maximum stckitem size
b0cdae4 
To prevent possible DoS. Port the neo-project/neo-vm#514,
close #3170.

Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@vncoelho vncoelho vncoelho left review comments

@vang1ong7ang vang1ong7ang vang1ong7ang left review comments

@Jim8y Jim8y Jim8y approved these changes

@AnnaShaleva AnnaShaleva AnnaShaleva approved these changes

@superboyiii superboyiii superboyiii approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@shargon @roman-khimov @Jim8y @dusmart @vang1ong7ang @vncoelho @superboyiii @AnnaShaleva