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

Max Memory Size #513

Closed
wants to merge 9 commits into from
Closed

Max Memory Size #513

wants to merge 9 commits into from

Conversation

shargon
Copy link
Member

@shargon shargon commented Oct 18, 2023

Alternative to #512

@shargon shargon requested a review from Ashuaidehao October 18, 2023 07:36
@shargon shargon mentioned this pull request Oct 18, 2023
@shargon
Copy link
Member Author

shargon commented Oct 23, 2023

@Liaojinghui what do you think about this option?

@Jim8y
Copy link
Contributor

Jim8y commented Oct 23, 2023

@Liaojinghui what do you think about this option?

I think it is elegent to have this. If we have it at first place, then we might not have those DOS attacks at all. But, i am not sure how many overhead this will introduce.

@Jim8y
Copy link
Contributor

Jim8y commented Nov 10, 2023

@neo-project/core should we finish this or ignore this?

@shargon
Copy link
Member Author

shargon commented Nov 10, 2023

I think that with ReferenceCounter is easy to know the memory used, so for me it worth. We can combine #202 (comment)

@Jim8y
Copy link
Contributor

Jim8y commented Nov 10, 2023

@roman-khimov
Copy link
Contributor

I don't think it'll be a big improvement after #514, it also affects performance and adds some unintuitive logic (from the VM user POV, it's hard to think about overall memory consumption, whiile it's easy to remember that you can only have MaxStackSize items of MaxItemSize). So I'd say it's not needed now.

@AnnaShaleva
Copy link
Member

Agree with Roman, we're going to drown in the VM constraints if introducing more of them. Existing constraints should be enough to restrict the overall memory consumption, and we can reduce MaxStackSize or MaxMaxItemSize in future if needed.

@Jim8y
Copy link
Contributor

Jim8y commented Nov 12, 2023

So @shargon, we will not merge this pr then.

@shargon shargon closed this Nov 12, 2023
@shargon shargon deleted the max-memory branch November 12, 2023 14:46
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants