Step 1: kernel integration (use vm API) #774
Conversation
aionick
requested review from
AlexandraRoatis,
AionJayT,
jeff-aion and
aion-kelvin
aionick
added
the
wip
- This change allows us to remove the old, overly complex structure relating to this concept which consisted of: ExecutionResult, TransactionResult (kernel side), AbstractExecutionResult, and IExecutionResult. These are now all gone and replaced by the new TransactionResult. - All changes to source code are simple replacements of these old classes with the new class. - All changes to test code preserve the test logic prior to this change and are only replacements and simple adjustments to new semantics, of which there are few.
- Note that the BloomFilter class does not implement IBloomFilter. This is because the VM-related components do not interact with BloomFilter but rather with Bloom.
- The old kernel Address class is now AionAddress, and it implements the Address interface in the VM API. - The kernel Log class now implements IExecutionLog. - The bulk of these changes are all renamings to allow for this swapping.
- All of the bulk changes here simply relate to using this new type and handling some of the side-effects that come with it. For example, different method names and the Address type that some of its methods return. - Also making a method in AionTransaction synchronized again (but we should be looking into this ... it seems like improper synchronization).
- All changes here are simply related to this new interface type.
- All of the changes here are re-namings related to this change. - One exception: the old ExecutionHelper is now a SideEffects implementation and its old 'merge' method no longer performs a conditional merge. We have separated the logic of this conditional merge out into two different methods to better suit an API and as such we have replaced each occurrence of that merge with this new logic, which is equivalent.
- The ExecutionContext class now implements the TransactionContext interface in the VM API.
- These above mentioned classes are now interfaces and the kernel side provdes two FastVM implementations of them. All changes here simply relate to this new change.
- An implementation for the FastVM has been created. No one is using it yet.
- This is an implementation of KernelInterface for the fastvm. - Most chanegs here relate to how we are now using this in the fastvm (namely in the Callback stack).
- updated aion_fastvm, modAionImpl, modPrecompiled, modAion, modApiServer, modVM, modMcf, aion_api with interface changes
- The precompiled module will soon be detached from the fastvm and so it will also need to provide its own implementation of ResultCode and TransactionResult, that's what this commit introduces. - Currently these classes are unused. - Also note that they are copies of the fastvm logic. This is because the two shared a single path of execution on these points prior to this separation, and so this similarity needs be preserved.
- All classes in modPrecompiled have changed to now deal with its specific ResultCode and TransactionResult implementation in order to move towards decoupling this module from the fastvm. - Note that its implementation is identical to the fastvm implementation since the two used to share these classes.
We are no longer using the libnsc jar directly, and some other small fixes.
aionick
force-pushed
the
incremental-pr
branch
from
d816384
to
65ed64a
Compare
AlexandraRoatis
removed
the
wip
There was a problem hiding this comment.
Hmm, so I wrote like 24 comments and then refreshed the page and then there were only 10. I'm not sure what happened to them. I think some of the comments were on things that you have since modified when you added more commits to the PR.
Have a few comments and suggestions below; and two general comments:
(1) I noticed some interfaces are prefixed with "I" and some are suffixed with "Interface." for the new ones that you add (I thnik some aren't actually in this PR), could we stick with one of them? I personally much prefer the "I" one.
(2) In the in-line comments I mention Address/AionAddress a lot. I'm guessing some IDE refactor/rename caused all the Address to become AionAddress. I don't think we have to fix every instance of it right now, but I think we should get them right in method signature in interfaces and, if we have time, method signature of non-private methods
| return (this.to == null || this.to.isEmptyAddress()); | ||
|
|
||
| // TODO: all this is a temporary solution. | ||
| if (this.to == null) { |
There was a problem hiding this comment.
What was wrong with this.to.isEmptyAddress() ?
| * | ||
| * @return a cope of the IDataWord. | ||
| */ | ||
| BigInteger value(); |
There was a problem hiding this comment.
what happened to the Javadoc comments?
modPrecompiled/test/org/aion/precompiled/TRS/TRSstateContractTest.java
Outdated
Show resolved
Hide resolved
modPrecompiled/test/org/aion/precompiled/TRS/TRSstateContractTest.java
Outdated
Show resolved
Hide resolved
modPrecompiled/test/org/aion/precompiled/TRS/TRSstateContractTest.java
Outdated
Show resolved
Hide resolved
| @@ -78,5 +79,5 @@ | |||
| * @param key the key at which the date will be stored | |||
| * @param value the data to be stored | |||
| */ | |||
| void addStorageRow(Address address, DW key, DW value); | |||
| void addStorageRow(Address address, ByteArrayWrapper key, ByteArrayWrapper value); | |||
There was a problem hiding this comment.
I noticed that as a result of this change, a lot of calls to it IRepositoryCache.addStorageRow has changed from:
IRepositoryCache.addStorageRow(someAddress, new DataWord(someKey1), new DataWord(someKey2))
to
IRepositoryCache.addStorageRow(someAddress, new DataWord(someKey1).toWrapper(), new DataWord(someKey2).toWrapper())
This happens all over the code and tests, i.e. StandaloneBlockchain.java:313-314.
Would it make more sense to avoid that "add toWrapper() to everything" change by doing something like this:
- create an interface called ByteArrayWrappable with a method toWrapper()
- make IDataWord implement ByteArrayWrappable (and any other suitable class)
- either: (1) replace IRepositoryCache.addStorageRow ByteArrayWrapper arguments with ByteArrayWrappable and in each implementation, call ByteArrayWrappable.toWrapper() to get the ByteArrayWrapper
- or: (2) add a default method into IRepositoryCache: addStorageRow(Address, ByteArrayWrappable, ByteArrayWrappable) and all it does is call toWrapper() and pass it on to addStorageRow(Address address, ByteArrayWrapper key, ByteArrayWrapper value)
There was a problem hiding this comment.
We could make this comment an issue that can be verified at the end, once all stages of the PR are added.
| public interface ITransaction extends Cloneable { | ||
|
|
||
| byte[] getHash(); | ||
| public interface ITransaction extends Cloneable, TransactionInterface { |
There was a problem hiding this comment.
The ITransaction/TransactionInterface thing is a bit confusing, but as we talked about in-person, it's ok as like a intermediate step that will be eventually refactored. Could you just either add a javadoc or TODO comment here or make a Github/Jira issue about this so we remember to revisit some time in the near future?
| return false; | ||
| } | ||
| } | ||
| return true; |
There was a problem hiding this comment.
this method might need to benchmark to see this implementation is faster than pure byte compares by order later.
aionick
changed the title
This is the first half of the work done on PR #761. The point of this separate PR is to try and get this batch of commits moved into
master-pre-merge, since these have been identified as mostly simple re-naming and moving refactorings or the like. There is none of the core logic here.Once we've established this branch is passing the CI and the other branch, rebased onto this (there were a couple inconsistencies to resolve), is also passing, this can probably be merged in right away if all approve (it seems so far we do) and the other PR can be updated to the rebased versions I've done on separate branches (for now).