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: first integration #53

Merged
merged 56 commits into from
Aug 21, 2018
Merged

VM: first integration #53

merged 56 commits into from
Aug 21, 2018

Conversation

semuxgo
Copy link
Contributor

@semuxgo semuxgo commented Aug 18, 2018

This is a standalone EVM implementation derived from EthereumJ v1.8.1. The main changes are:

  • Added a facade interface between VM and kernel
  • Made it independent and embedable into any Java-based blockchain project, the only dependencies are
    • Commons Lang
    • Bouncycastle
    • SLF4J
    • JUnit and Mockito (test)
  • Made DataWord immutable and refactored a few classes

@orogvany could you help review the code? also we need to implement all the interfaces in the org.ethereum.vm.client package, in the semux project.

}

public byte getByte(int index) {
if (index < 0 || index >= SIZE) {
Copy link
Contributor

@witoldsz witoldsz Aug 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please correct me if I am wrong, but isn't the bounds check embedded within Java arrays? I thought it's the primary reason why the arrays are slow – because one cannot disable the bounds check.

@orogvany
Copy link
Collaborator

orogvany commented Aug 20, 2018

It's unclear which are modifications and which are verbatim copies.

I think we do want to fork the project into a module, so we can continue to get bugfixes from mainline.

This combined with license restrictions makes for compelling reason to modularize.

I'm fine with LGPL and using this if you like it better than etheriumJ 1.5.0. I'd rather have bugfixes and modern version with ability to get fixes (even if we need to merge heavily as they diverge)

@orogvany
Copy link
Collaborator

I looked through the client interfaces, and it looks like we should be able to support them fairly easily. just a few new fields. looks good to me to start work on integration

@semuxgo
Copy link
Contributor Author

semuxgo commented Aug 20, 2018

This derived EVM is already highly modularized, and it's not using any classes from org.semux.

There isn't much development around EthereumJ, especially after the leave of their team leads, Roman and Nash. They are not even actively implementing the upcoming Constantinople fork, ethereum/pm#53. We have to put resources to maintain the VM and develop new features by ourselves, to stay up-to-date.

In case we want to merge bugfixes from upstream, we can simply follow https://github.com/ethereum/ethereumj/commits/develop/ethereumj-core/src/main/java/org/ethereum/vm. There're only less than 50 commits in the past year.

@orogvany
Copy link
Collaborator

orogvany commented Aug 21, 2018

Ah, I did not know that. I'd still prefer to keep it separated for the licensing issue, even though it does make the build chain a little more difficult.

Alternatively, we could just split the project into 2 sub-modules (or more) if we wanted to keep them in the same github repo. Or we could ask nicely to get it dual licensed as MIT again, so we can continue development without worry of license taint. Long shot, as I'm not sure history of moving towards lgpl, but as its sunsetting development for now, they may be more open to it.

It's also possible we can keep them in the same module but use some mvn tools to get 2 jars output, but I'm not sure if that complies with the lgpl lib requirements. Having 2 libs enforces better separation IMO.


public class BytecodeCompiler {

public static void main(String[] args) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should remove this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could be useful as an independent tool for de-compile opcode string.

Copy link
Collaborator

@orogvany orogvany Aug 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough, javadoc here might be appropriate to avoid confusion


public interface BlockStore {

byte[] getBlockHashByNumber(int index);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be a long, right?
In the above Block class getNumber is a long, and we use long internally.


public interface Block {

BigInteger getGasLimit();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should gasLimit be set per-block or via configuration?
It seems like potentially each validator can define its own gas limit?
In eth, this is voted on by miners apparently, but I'm not sure how that works with dPOS

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct. The block gasLimit is determined by miners through consensus rule. This VM module is designed for generic blockchain project, not tailed for Semux BFT.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, we'll just handle in our client impl

* the account address
* @return true if account exist, false otherwise
*/
boolean isExist(byte[] address);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this reads weird. Can we rename to accountExists(address) or exists(address) ?

* the account address
* @ImplNote trigger account creation
*/
void createAccount(byte[] address);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this createAccount/deleteAccount compatible with our database?
It's not immediately clear if accounts in VM != addresses ?

* the account address
* @return balance of the account as a <code>BigInteger</code> value
*/
BigInteger getBalance(byte[] address);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's probably some discrepancy here. We use long as our nano sem in Amount class. There seems to be little point in using BigInt, as we do not support over longs

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is because the base unit in Ethereum is 10^-18 ETH, while the base unit is 10^-9 SEM in Semux. Using Biginteger to handle any decimals.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, that could get tricky. We'll probably want to add a toBigInt to amount class to make sure there's no errors.

Will need to figure out if this means we should move to 10^-18 in our db to match, or what.

@semuxgo semuxgo merged commit c97d0af into semuxproject:develop Aug 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants