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

API for VM-Kernel interaction #734

Closed
AlexandraRoatis opened this issue Nov 28, 2018 · 10 comments
Closed

API for VM-Kernel interaction #734

AlexandraRoatis opened this issue Nov 28, 2018 · 10 comments
Assignees
Labels
enhancement New feature or request

Comments

@AlexandraRoatis
Copy link
Contributor

Refactor the current code to extract a common interface layer to a new API for the VM repository in order to decouple the kernel implementation from the specific VM used by it.

The kernel will use the new VM-API jar and a specific implementation jar (no longer relying on git submodules).

FVM, AVM and the hybrid combination of the two would be options for the implementation jar.

Changes which must be made on the kernel side:

  • DB must support arbitrary-length keys and values (at the "repository" level)
  • allow writing 0-values to DB -- don't want it to delete the data?
  • Aion Transaction type field to define "deploy to AVM" as being distinct from "deploy to FVM"
  • TransactionExecutor replaced by BulkExecutor
@AlexandraRoatis AlexandraRoatis added the enhancement New feature or request label Nov 28, 2018
@AlexandraRoatis AlexandraRoatis added this to the 0.3.3 milestone Nov 28, 2018
@aionick
Copy link
Contributor

aionick commented Nov 28, 2018

The API should consist of two elements: data types (for which the implementation is provided in the API) and interfaces.

The following is a list of core aspects of the VM API. There may of course be oversights here and this list may grow, though I don't expect it to grow substantially and I also do not expect much of what is here to not make it into the actual API:

(note, some names on here may soon end up becoming stale)
Data Types

  • ResultCode: holds the 'exit status' of a transaction that was executed.
  • TransactionResult: the side-effects (including state change commits), output and energy consumption of an executed transaction.
  • TransactionContext: the pertinent world-state contextual details at the start of executing some transaction.
  • Log: a smart contract event log fired off as a result of executing some transaction.
  • Address: the public address of an account.
  • ExternalTransaction: a transaction that is not spawned by some other transaction.
  • InternalTransaction: a transaction that is spawned only as a consequence of executing some other transaction.
  • Block: a block in the blockchain.
  • LightweightFuture: a Java Future but much more lightweight since we only need the get() method. This is of course an optional feature since it is provided by the JDK, but would prove more efficient and less cumbersome for our purposes.

Interfaces

  • VirtualMachine: the interface of an arbitrary virtual machine.
  • KernelInterface: the interface of an arbitrary kernel implementation. The goal of this interface is not so that we can swap out kernels but so that a virtual machine has a line of communication back to the kernel.
  • TransactionInterface: an interface over transaction types, particularly the two transaction types listed above.

@aionick
Copy link
Contributor

aionick commented Dec 5, 2018

TransactionResult is now replacing TransactionResult (previous kernel version), ExecutionResult, AbstractExecutionResult, and IExecutionResult. With this change, ResultCode in the api is also replacing the old kernel one.

I'm moving onto TransactionContext now, which will drag with it a bunch of the other classes.

One question we have to answer soon is: Where should new contract addresses be generated?

Currently transactions generate them, which doesn't make much sense. Also, depending on how we solve the problem of knowing which vm to use to run a contract call, this responsibility may have to fall on the virtual machines themselves.

@aionick
Copy link
Contributor

aionick commented Dec 6, 2018

The classes we have to finish the replacements for before we move on to TransactionContext:

  • TransactionInterface
  • InternalTransactionInterface
  • IExecutionLog
  • IBloomFilter
  • Address

@aionick
Copy link
Contributor

aionick commented Dec 7, 2018

Going to begin work on integrating TransactionContext into the aion project now.

@aionick
Copy link
Contributor

aionick commented Dec 7, 2018

I think it may be worth discussing the implications of ResultCode as an enum. This certainly works in a single vm model, but in designing a more generalized interface I don't think this is the way to go.

Different virtual machines should have the liberty to define their own error codes. Not every VM has the same error scenarios in the first place, and we don't want a single centralized place where every error code gets dumped, since each VM uses just a subset of them.

Instead I think ResultCode should be refactored so that it is just down to the isSuccess(), isFailed(), isRejected() methods, which all error codes must be defined in terms of, as well as a toString() implementation so that the error itself can be communicated beyond these three categories. This still allows the virtual machines themselves to implement all of this in terms of an enum.

If you're fine with this change @AlexandraRoatis then I can do this, since it was part of my TransactionResult change.

@aionick
Copy link
Contributor

aionick commented Dec 7, 2018

I'm also noticing we really have two ways of classifying transactions, and right now we are poorly and quite confusingly calling these the transaction type and the transaction kind.

The real difference between these is that type is being used at a higher level than kind is, though there hasn't been much effort yet to determine the true scope of type. It does seem, however, that type will simply represent the type of VM that will process the transaction. This may be the wrong level, however, because the same VM may be able to process contracts written in multiple languages, and it may actually be that we want type to provide us with which language the contract is written in.

On the other hand, kind distinguishes transactions based on what kind of transactional logic they will cause to be executed. For example, contract creation, contract call, delegate call.

We need to formalize this distinction properly for our api layer.

@aionick
Copy link
Contributor

aionick commented Dec 17, 2018

The VM API is in a more-or-less finalized form for this phase of the integration. At this point it consists only of interfaces without any shared data types. This may not be the final phase of the API, I suspect that it will make sense to pull some data types out into it, but that's not a priority at the moment.

As for the shape of the kernel integration, we are currently very close to having modVM interact with the fast vm in the desired way, all that remains is to make the BulkExecutor actually accept transactions in bulk and to call through to the fastvm using its VirtualMachine interface (it is still using the TransactionExecutor inside the fvm). Once this is in place, fitting the Avm in should be relatively easy - however, the Avm will have to begin using the VM API interfaces, so there's still all of these changes to be done on that side.

The kernel & fastvm have a number of cosmetic & design related refactorings left to do. However, these are not a priority at the moment, our priority is now getting the kernel-fvm relationship set up properly and adding the Avm in.

Some tasks left to do:

  • Repository needs a flushTo() method so that we can re-direct state updates to an unrelated repository.
  • BulkExecutor needs to receive transactions in bulk from its callers and process them in bulk.
  • Avm classes need to implement the interfaces in the VM API that are related to this. (in progress)
  • Ideally, some more consensus-breaking tests that check special cases like failures, rejections, internal transactions, should be added, or some other meaningful tests that ensure we've been correct in some of these areas where we currently don't have much testing support.
  • Ideally remove the VM API submodule, build a version 1 of it and start using that jar directly in the kernel.

@aion-kelvin
Copy link
Contributor

Hey, have a small request for the API.

There is a plumbing issue for when transactions fail currently -- somewhere in or between the kernel and the VM, some details of the error are lost. Would be good to make sure the new API can fix it, or at least is set up in a way that helps us fix it.

The problem is that when transactions fail because of either invalid nonce, invalid energy limit, or insufficient balance, all three cases get reported back to the kernel as "transaction dropped." As a result, when users send in a transaction from RPC (i.e. via Web3), the kernel just reports "transaction dropped" instead of something like "insufficient balance."

  1. Insufficient balance gets detected by AbstractExecutor#prepare and saves it in an IExecutionResult - https://github.com/aionnetwork/aion/blob/master/modVM/src/org/aion/vm/AbstractExecutor.java#L147-L154
  2. TransactionExecutor#finish checks that IExecutionResult to find out why it failed. However, it does not save the fact that is was due to insufficient balance: https://github.com/aionnetwork/aion/blob/master/modVM/src/org/aion/vm/TransactionExecutor.java#L243-L246
  3. TransactionExecutor#finish returns an AionTxExecSummary, but the fact of "insufficient balance" is not saved in that AionTxExecSummary object -- https://github.com/aionnetwork/aion/blob/master/modVM/src/org/aion/vm/TransactionExecutor.java#L264-L268

Basically, I think in the new API, AionTxExecSummary (or its replacement) needs to have a field that holds a reason for why a tx is dropped/rejected. Then TransactionExecutor#finish (or its replacement) needs to save that reason into the summary object.

@AlexandraRoatis
Copy link
Contributor Author

AlexandraRoatis commented Jan 21, 2019

AVM kernel integration progress tracking and remaining to do list:

@AlexandraRoatis
Copy link
Contributor Author

AlexandraRoatis commented Feb 13, 2019

Transaction type checks were disabled in b84bdcd and should be re-enabled after establishing when they will be introduced to both kernel implementations.

@AlexandraRoatis AlexandraRoatis modified the milestones: 0.3.3, 0.4.0 Feb 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants
@AlexandraRoatis @AionJayT @aionick @aion-kelvin and others