Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[VM] Remove or Update Tarjan in the virtual machine for better performance. #3517

Open
Jim8y opened this issue Oct 8, 2024 · 7 comments
Open
Labels
Discussion Initial issue state - proposed but not yet accepted
Milestone

Comments

@Jim8y
Copy link
Contributor

Jim8y commented Oct 8, 2024

Summary or problem description
To precisely calculate and timely update the total number of References in the vm, we use Tarjan in the vm to refresh the reference relationship. Its used to build the strongly connected components in the stack items, but strongly connected components actually are not necessary for neo vm to calculate and update the references. What's worse, we need to rerun the expensive Tarjan everytime we update the compound to compound reference relationship in the vm, which incurs high overhead.

Do you have any solution you want to propose?
we should remove the Tarjan, and replace it with a light weight reference management algorithm, or at least we should upadte Tarjan to make it can be partially updated.

Where in the software does this update applies to?

  • VM
@Jim8y Jim8y added the Discussion Initial issue state - proposed but not yet accepted label Oct 8, 2024
@roman-khimov
Copy link
Contributor

Burn it into ashes. Nuke it from the orbit. Make it die bleeding with a hell lot of red lines. nspcc-dev/neo-go#2500 (comment)

Really, just drop it. Echidna is perfect HF to do this.

@AnnaShaleva
Copy link
Member

Vote up for removal, it will simplify things a lot, and I'm so glad to see this issue. From NeoGo side, we'll be able to solve our compatibility issue, because right now our VM counter implementation is incompatible with C#, we don't consider circular references.

@dusmart
Copy link

dusmart commented Oct 10, 2024

Vote up for removal.

  • Removal of the heavy GC could avoid a lot of DoS attacks.
  • Compilers should take care of the opcodes and try their best to avoid circular references.
  • If some program have to use the circular reference, they should break the circle before dropping by themselves.

@shargon
Copy link
Member

shargon commented Oct 10, 2024

Compilers should take care of the opcodes and try their best to avoid circular references.

An attacker can create his own script without a compiler

@roman-khimov
Copy link
Contributor

An attacker can create his own script without a compiler

It doesn't really matter. Regular contracts won't notice and attacker will just run out of 2048 items faster than before if he tries to play with circular references, so a simple reference counter prevents some attacks in fact.

@shargon
Copy link
Member

shargon commented Oct 10, 2024

An attacker can create his own script without a compiler

It doesn't really matter. Regular contracts won't notice and attacker will just run out of 2048 items faster than before if he tries to play with circular references, so a simple reference counter prevents some attacks in fact.

We should test the worst case, memory it's also involved.

@cschuchardt88
Copy link
Member

An attacker can create his own script without a compiler

It doesn't really matter. Regular contracts won't notice and attacker will just run out of 2048 items faster than before if he tries to play with circular references, so a simple reference counter prevents some attacks in fact.

We should test the worst case, memory it's also involved.

Memory ready is involved. I've been experimenting with rebuilding this reference counter. And it can be removed all together it's really unnecessary. The reason it's unnecessary is the logic of how it checks and copies the data All it's really doing is comparing same instances and reusing them

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Discussion Initial issue state - proposed but not yet accepted
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants