neo-project / neo

NEO Smart Economy
MIT License
3.47k stars 1.03k forks source link

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

Open Jim8y opened 1 week ago

Jim8y commented 1 week ago

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?

roman-khimov commented 1 week ago

Burn it into ashes. Nuke it from the orbit. Make it die bleeding with a hell lot of red lines. https://github.com/nspcc-dev/neo-go/issues/2500#issuecomment-1233975724

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

AnnaShaleva commented 1 week ago

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 commented 6 days ago

Vote up for removal.

shargon commented 6 days ago

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 commented 6 days ago

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 commented 6 days ago

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 commented 4 days ago

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