CityOfZion / neo-sharp

Neo-sharp is a new core and node implementation of NEO focused on modular design, best coding practices and testability.
MIT License
35 stars 24 forks source link

Initial attempt to persist InvocationTransaction #536

Closed osmirnov closed 5 years ago

osmirnov commented 5 years ago

@shargon I am trying to implement InvocationTransaction persistence. I attempted to hook up the VM. The execution always returns false as a result. Probably, because I need to register APIs. Please verify my brainstorm dump how it works :D #521

codecov[bot] commented 5 years ago

Codecov Report

Merging #536 into development will decrease coverage by 0.27%. The diff coverage is 0%.

Impacted file tree graph

@@               Coverage Diff               @@
##           development     #536      +/-   ##
===============================================
- Coverage        49.53%   49.26%   -0.28%     
===============================================
  Files              315      317       +2     
  Lines            13085    13157      +72     
===============================================
  Hits              6482     6482              
- Misses            6603     6675      +72
Impacted Files Coverage Δ
src/NeoSharp.Application/DI/VMModule.cs 0% <0%> (ø) :arrow_up:
...rc/NeoSharp.Core/SmartContract/MessageContainer.cs 0% <0%> (ø)
src/NeoSharp.Core/SmartContract/ScriptTable.cs 0% <0%> (ø)
...chain/Processing/InvocationTransactionPersister.cs 0% <0%> (ø) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 8b8e0b3...cce9b55. Read the comment docs.

shargon commented 5 years ago

Do you have Windows or Linux?

shargon commented 5 years ago

GetMessage on IMessageProvider should return the TX content, so we can't reuse the same ExecutionEngineArgs because the signature of the TX is checked here

https://github.com/neo-project/neo-vm/blob/master/src/neo-vm/ExecutionEngine.cs#L590

osmirnov commented 5 years ago

I am on Windows. We use yours HyperVM not NeoVM. How is your comment applicable?

shargon commented 5 years ago

Maybe we have to make Transaction and Block inherit from IMessageProvider but for check witness this is necessary, we need to pass the current "Message"

osmirnov commented 5 years ago

Now I stuck with "FaultByGas" error. HyperVM thinks I owe some Gas to it.

image

belane commented 5 years ago

Perhaps because is not taking in account that transactions have 10 free gas execution, most of the invokations in the blockchain are bellow 10 of gas.

If you call engine.Execute((uint)transaction.Gas.Value)) with zero, HyperVM will run out of gas and returns FAULT_BY_GAS.

So it's running https://github.com/CityOfZion/neo-hypervm/blob/master/src/Neo.HyperVM/ExecutionEngine.cpp#L115-L117 with 0 and many OpCodes add Gas execution (https://github.com/CityOfZion/neo-hypervm/blob/master/src/Neo.HyperVM/ExecutionEngine.cpp#L317) and will break contract execution https://github.com/CityOfZion/neo-hypervm/blob/master/src/Neo.HyperVM/ExecutionEngine.h#L72

Would you try to run transactions whit less than 10 gas with a default 10 engine.Execute(10u) ?

osmirnov commented 5 years ago

It is not my transaction. I've got them from blockchain during sync. I expect the gas they have should reflect consumed gas as they are in historical blocks.

osmirnov commented 5 years ago

@shargon It would be nice if you integrated HyperVM. I currently work on NeoVM integration and my secondary goal to making VMs swappable. So you finish this part I can integrate both VMs faster.