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

SputnikVM Integration #413

Merged
merged 72 commits into from Dec 9, 2017

Conversation

Projects
None yet
3 participants
@sorpaas
Copy link
Contributor

sorpaas commented Nov 22, 2017

(Copy information from below so it is easier to read)

You can help test go-ethereum with SputnikVM integration using a nightly build and run geth with ./geth --sputnikvm.

Current build status:

  • We use a build tag system. Current build command in README continues to work, but build without the sputnikvm library. Run the compiled binary with --sputnikvm argument gives out correct errors.
  • Use this comment (#413 (comment)) for build instructions for building with sputnikvm library.
  • CI and deployment builds geth with sputnikvm library.

Current test status:

  • Regression test against mainnet all passed.
  • go-ethereum tests using classic vm is unaffected.
  • go-ethereum tests using sputnikvm passed. 4 additional tests are disabled due to various reasons. Refer to the diff and commits for explanation.

@sorpaas sorpaas changed the title SputnikVM Integration [WIP] SputnikVM Integration Nov 22, 2017

@sorpaas sorpaas changed the title [WIP] SputnikVM Integration SputnikVM Integration Dec 3, 2017

@sorpaas

This comment has been minimized.

Copy link
Contributor

sorpaas commented Dec 3, 2017

Thanks to sputnikvm-ffi, this branch is working right now and I'm able to sync to at least a large part of the network using the SputnikVM processor. So I'm planning to use it to conduct another regression test for SputnikVM using this branch (whose time can be reduced to several hours / one or two day). Help appreciated for code review and/or testing. go test ./... (with manual setting of UseSputnikVM = true) also mostly passes now, except for several ones which the processor cannot figure out the correct Patch to use.

From my reading of the code, it looks like go-ethereum uses heap allocated big integers and hash representations, which could be a bottleneck for performance currently. So I have a feeling that SputnikVM might be able to be faster right now, although we need to test it.

To reproduce the regression, have Rust (>= 1.21) and Golang (>= 1.9) installed. use multivm-sputnikvm branch of go-ethereum. And then:

cd $GOPATH/src/github.com/ethereumproject
git clone https://github.com/ethereumproject/sputnikvm-ffi
cd sputnikvm-ffi/c/ffi
cargo build --release
cp $GOPATH/src/github.com/ethereumproject/sputnikvm-ffi/c/ffi/target/release/libsputnikvm_ffi.a $GOPATH/src/github.com/ethereumproject/sputnikvm-ffi/c/libsputnikvm.a

And then build geth with CGO_LDFLAGS:

cd $GOPATH/src/github.com/ethereumproject/go-ethereum/cmd/geth
CGO_LDFLAGS="$GOPATH/src/github.com/ethereumproject/sputnikvm-ffi/c/libsputnikvm.a -ldl" go build -tags=sputnikvm .

In macOS:

cd $GOPATH/src/github.com/ethereumproject/go-ethereum/cmd/geth
CGO_LDFLAGS="$GOPATH/src/github.com/ethereumproject/sputnikvm-ffi/c/libsputnikvm.a -ldl -lresolv" go build -tags=sputnikvm .

Run geth with the --sputnikvm flag to enable the SputnikVM processor:

./geth --sputnikvm --verbosity 6

The fastest way to test is to export your current Ethereum blockchain, use a new folder to re-import it.

./geth export blockchain.db
mv ~/.ethereum-classic ~/.ethereum-classic-old
./geth --sputnikvm import blockchain.db

@sorpaas sorpaas requested review from tzdybal and whilei Dec 3, 2017

@sorpaas

This comment has been minimized.

Copy link
Contributor

sorpaas commented Dec 3, 2017

I'm on block 1295000 without any failure. Everything looks green right now. The time it takes is around 10000 transactions per 30 seconds. So would be glad to have some review.

@splix @whilei @tzdybal

@sorpaas sorpaas requested a review from splix Dec 3, 2017

@sudachen sudachen self-requested a review Dec 4, 2017

@sorpaas

This comment has been minimized.

Copy link
Contributor

sorpaas commented Dec 4, 2017

The regression passed 2.5 million blocks without an issue. 🎏

From this point on, I don't have the blockchain data locally so it needs to use sputnikvm1. Would be glad if someone can help test. @whilei @tzdybal

@sorpaas

This comment has been minimized.

Copy link
Contributor

sorpaas commented Dec 4, 2017

(Also just a side note. We currently have 6 patches (including ECIP1017) in ETC chain. Only four of them are related to EVM. They're Frontier, Homestead, EIP150 and EIP160. That's why you see in the state processor it only checks those four hard forks.)

@sorpaas

This comment has been minimized.

Copy link
Contributor

sorpaas commented Dec 4, 2017

My Internet connection seems fast today. Just passed 4 million blocks in localhost. Everything still looks green.

1 more million to go...

@sorpaas

This comment has been minimized.

Copy link
Contributor

sorpaas commented Dec 4, 2017

Reached 4.9 million blocks in my localhost. Regression test is now finished and no errors reported! 🎏

break Loop
case sputnikvm.RequireAccount:
address := ret.Address()
if statedb.Exist(address) {

This comment has been minimized.

@whilei

whilei Dec 4, 2017

Contributor

In Go there's a general convention to avoid else statements, preferring "breaking" patterns instead, so we could use

		case sputnikvm.RequireAccountCode:
			address := ret.Address()
			if statedb.Exist(address) {
				vm.CommitAccountCode(address, statedb.GetCode(address))
                break
			}
			vm.CommitNonexist(address)

noting that break will break the switch, not the for Loop.

This comment has been minimized.

@whilei

whilei Dec 4, 2017

Contributor

It's only a style thing, though.

This comment has been minimized.

@sorpaas

sorpaas Dec 4, 2017

Contributor

Hmm, I'll fix this. But can you explain why? Code above is actually harder to read for me compared with an else statement.

This comment has been minimized.

@sorpaas

sorpaas Dec 4, 2017

Contributor

Fixed.

This comment has been minimized.

@tzdybal

tzdybal Dec 4, 2017

Member

I totally disagree with @whilei. This is perfectly legal use of else statement. They are typically omitted only when code itself returns/continues/breaks, etc., see Effective Go.
I think that if/else block is much more readable.

This comment has been minimized.

@whilei

whilei Dec 4, 2017

Contributor

Noted @tzdybal -- I definitely don't have my heart set on it, and think your take is valid; in fact I think it's more readable as Wei wrote it originally too. Whatever you want, @sorpaas

receipt.TxHash = tx.Hash()
receipt.GasUsed = new(big.Int).Set(totalUsedGas)
if MessageCreatesContract(tx) {
from, _ := tx.From()

This comment has been minimized.

@whilei

whilei Dec 4, 2017

Contributor

Don't we already have from from up top on line 23?

This comment has been minimized.

@sorpaas

sorpaas Dec 4, 2017

Contributor

Does golang has variable scope? So this line defines a new from variable, and out of this if scope from would still be the above one, right? It is really common to do this kind of thing in Rust, does it work in go?

Anyway, from is never used below again so this shouldn't cause any trouble.

This comment has been minimized.

@whilei

whilei Dec 4, 2017

Contributor

I feel like I might be not understanding something here [posted before page refresh]

yea, go does have variable scope. I think I'm just not understanding why it's needed to reestablish the value? Can tx.From() change once the vm has run?

This comment has been minimized.

@sorpaas

sorpaas Dec 4, 2017

Contributor

Ooops you're right. Fixed by removing this line.


glog.V(logger.Debug).Infoln(receipt)

vm.Free()

This comment has been minimized.

@whilei

whilei Dec 4, 2017

Contributor

Would it save any performance to bump vm.Free() up to after where it is last used in the function (line 144)? I like the "readability" of having it at the bottom, but just a thought.

This comment has been minimized.

@sorpaas

sorpaas Dec 4, 2017

Contributor

I don't think so. It would just mean that VM is freed early.

The problem is that Go only execute one transaction at a time, so it's always one VM created, one VM freed. We don't save any time by moving it up.

This comment has been minimized.

@whilei

whilei Dec 4, 2017

Contributor

roger that

This comment has been minimized.

@whilei

whilei Dec 4, 2017

Contributor

and we can't just slap the whole thing into a go routine because there might be conflicts in state, yes?

This comment has been minimized.

@sorpaas

sorpaas Dec 4, 2017

Contributor

Won't be any conflict. Each VM executes independently and they never share any memory. But if indeed we try to execute multiple VMs in parallel, moving it up might give us a really small performance improvement.

However, right now I prefer readability. Besides I don't think this is the performance bottleneck. We can always modify it easily in a later PR.

@sorpaas sorpaas force-pushed the multivm-sputnikvm branch from 60ae07a to 5371655 Dec 7, 2017

sorpaas added some commits Dec 7, 2017

@tzdybal
Copy link
Member

tzdybal left a comment

@sorpaas (CC: @whilei) - latest changes reviewed. I like the build tags thing.

@whilei why we keep the 'incorrect' test cases, why we can't just remove them?

@sorpaas

This comment has been minimized.

Copy link
Contributor

sorpaas commented Dec 7, 2017

@tzdybal Which one do you mean are the "incorrect" test cases?

@ethereumproject ethereumproject deleted a comment from sorpaas Dec 8, 2017

@ethereumproject ethereumproject deleted a comment from sorpaas Dec 8, 2017

@ethereumproject ethereumproject deleted a comment from sorpaas Dec 8, 2017

@sudachen sudachen removed their request for review Dec 8, 2017

@tzdybal

tzdybal approved these changes Dec 8, 2017

@tzdybal

This comment has been minimized.

Copy link
Member

tzdybal commented Dec 8, 2017

@sorpaas I mean the ones disabled in the test code long time ago.

@sorpaas

This comment has been minimized.

Copy link
Contributor

sorpaas commented Dec 8, 2017

@tzdybal I created a new issue #427 for discussing those tests.

Everyone, please take a last look of the code here. If there's no objection, I'll merge it after 24 hours tomorrow.

rel (fix #416, part of milestone #372).

@sorpaas

This comment has been minimized.

Copy link
Contributor

sorpaas commented Dec 8, 2017

Current build status:

  • We use a build tag system. Current build command in README continues to work, but build without the sputnikvm library. Run the compiled binary with --sputnikvm argument gives out correct errors.
  • Use this comment (#413 (comment)) for build instructions for building with sputnikvm library.
  • CI and deployment builds geth with sputnikvm library.

Current test status:

  • Regression test against mainnet all passed.
  • go-ethereum tests using classic vm is unaffected.
  • go-ethereum tests using sputnikvm passed. 4 additional tests are disabled due to various reasons. Refer to the diff and commits for explanation.
@whilei

whilei approved these changes Dec 8, 2017

@sorpaas sorpaas merged commit 8150230 into master Dec 9, 2017

4 checks passed

continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@tzdybal tzdybal deleted the multivm-sputnikvm branch Mar 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment