Go bindings #41

chfast · 2018-07-12T12:44:36Z

Can you help with the review @gballet, @fjl?

Appropriate go-ethereum PR: ethereum/go-ethereum#17050

axic · 2018-07-13T15:27:00Z

bindings/go/evmc/evmc.go

+inline int set_option(struct evmc_instance* instance, _GoString_ name, _GoString_ value)
+{
+	if (instance->set_option)
+		return instance->set_option(instance, name.p, value.p);


This could use the helper now?

It's easier to use _GoString_ type here, than use malloc/free to pass the string to the original helper.

I mean instead of the if and return here, just wrap evmc_set_option?

Oh, yes. Good idea.

axic · 2018-07-13T15:27:17Z

bindings/go/evmc/evmc.go

+	memcpy(msg.value.bytes, value.p, sizeof(msg.value));
+	memcpy(msg.code_hash.bytes, code_hash.p, sizeof(msg.code_hash));
+
+    struct extended_context ctx = {{&fn_table}, context_index};


Indentation?

axic · 2018-07-13T15:28:00Z

bindings/go/evmc/evmc.go

+	memcpy(msg.destination.bytes, destination.p, sizeof(msg.destination));
+	memcpy(msg.sender.bytes, sender.p, sizeof(msg.sender));
+	memcpy(msg.value.bytes, value.p, sizeof(msg.value));
+	memcpy(msg.code_hash.bytes, code_hash.p, sizeof(msg.code_hash));


Should these have assertions for the length property (.n?) on the GoBytes?

I'm not sure using a C assert is good idea here.

Because only this Go module can use this function we know exactly what size it is (still prone to future mistakes).

Because this does not compile with Go 1.9, this might be rewritten anyway.

axic · 2018-07-13T15:31:50Z

bindings/go/evmc/evmc.go

+{
+	if (instance->set_option)
+		return instance->set_option(instance, name.p, value.p);
+	return -1;


Note: the evmc_set_option wrapper returns 0 in this case. Which one is preferred?

Here it returns 1, 0 or -1. This is not super needed, but I have different warnings for 0 and -1.

axic · 2018-07-13T15:35:38Z

@gballet would you mind having a look at the Go code? :)

Also please tag anyone else from the Go team who you think makes sense.

axic · 2018-07-13T15:37:32Z

bindings/go/evmc/evmc.go

+
+func evmcUint256be(in common.Hash) C.struct_evmc_uint256be {
+	out := C.struct_evmc_uint256be{}
+	for i := 0; i < len(in); i++ {


Can we assert that len(in) equals that of uint256be?

Is there an option for static (compile time) assert in Go?

There is no static assert. You can't even check the struct size (without "unsafe" package). But this is not a big problem here, because the array access is safe (will panic if wrong, but I have not checked if the go compiler is able to optimize this).

This has been fixed by the assertion commit.

axic · 2018-07-13T15:38:22Z

bindings/go/evmc/evmc.go

+
+func evmcAddress(address common.Address) C.struct_evmc_address {
+	r := C.struct_evmc_address{}
+	for i := 0; i < len(address); i++ {


This has been fixed by the assertion commit.

axic · 2018-07-13T15:39:23Z

bindings/go/evmc/host.go

+	if size == 0 {
+		return []byte{}
+	}
+	return (*[1 << 30]byte)(unsafe.Pointer(data))[:size:size]


What is this magic doing? :)

Simple. You cast the data pointer to a pointer to a huge array and then take a slice of size size of it. This is a cgo why to treat C array as a Go slice.

axic · 2018-07-13T15:41:22Z

bindings/go/evmc/host.c

+
+#include <stdlib.h>
+
+const struct evmc_context_fn_table fn_table = {


I'm not sure how linking works and whether this global symbol would pollute any consumer of this go library, but probably it makes sense prefixing these.

Yes. I agree. I can also add "hidden" attribute to it.

axic · 2018-07-13T18:05:28Z

bindings/go/evmc/evmc.go

+#include <stdlib.h>
+#include <string.h>
+
+inline int set_option(struct evmc_instance* instance, _GoString_ name, _GoString_ value)


Not sure about linking visibility in cgo, but perhaps this should be static to limit the scope to this file?

gumb0 · 2018-07-23T17:39:04Z

bindings/go/evmc/evmc.go

+	_GoBytes_ destination, _GoBytes_ sender, _GoBytes_ value, _GoBytes_ input, _GoBytes_ code_hash, int64_t gas,
+	int32_t depth, uint32_t flags, _GoBytes_ code)
+{
+	struct evmc_message msg;


Would it be good idea to default-initialize it here first? ( = {}; is it valid in C?)

Not in the pedantic C, but here it should work.

gumb0 · 2018-07-24T16:18:16Z

bindings/go/evmc/evmc.go

+	case C.EVMC_LOADER_SYMBOL_NOT_FOUND:
+		err = fmt.Errorf("evmc loader: the EVMC create function not found in %s", filename)
+	case C.EVMC_LOADER_INVALID_ARGUMENT:
+		panic("evmc loader: filename argument is empty")


also can be too long

Why not filename argument is invalid (empty, too long, etc.)?

axic · 2018-07-25T16:42:25Z

Basically I would like to have some guarantees that memory sizes equal between the C and Go data structures. Is that achievable somehow?

axic · 2018-07-25T16:43:55Z

bindings/go/evmc/host.go

+	DelegateCall = C.EVMC_DELEGATECALL
+	CallCode     = C.EVMC_CALLCODE
+	Create       = C.EVMC_CREATE
+)


Need to add Create2 here.

axic · 2018-07-25T17:02:26Z

Needs to be rebased to pick up latest ABI.

gumb0 · 2018-07-26T14:05:36Z

bindings/go/evmc/evmc.go

+
+extern const struct evmc_context_fn_table evmc_go_fn_table;
+
+static struct evmc_result execute_wrapper(struct evmc_instance* instance, int64_t context_index, enum evmc_revision rev,


Should this now have create2_salt parameter?

ah it's only for calls...

It should be passed back to the Host interface. I have not done this yet.

gumb0 · 2018-07-26T14:15:21Z

bindings/go/evmc/evmc.go

+}
+
+const (
+	Failure = Error(C.EVMC_FAILURE)


Why consts only for two status codes?

I'm waiting for a comment from Go devs, because sometimes they predefined errors as consts and sometimes create errors at hoc.

gumb0 · 2018-07-26T14:43:19Z

bindings/go/evmc/host.go

+//export accountExists
+func accountExists(pCtx unsafe.Pointer, pAddr *C.struct_evmc_address) C.int {
+	idx := int((*C.struct_extended_context)(pCtx).index)
+	ctx := getHostContext(idx)


Is it possible that idx points to non-existing context? Should we check it here or in getHostContext ?

This should be always valid. Otherwise Go will raise an exception.

axic · 2018-07-27T12:15:08Z

bindings/go/evmc/evmc.go

+}
+
+func (instance *Instance) Execute(ctx HostContext, rev Revision,
+	destination common.Address, sender common.Address, value common.Hash, input []byte, codeHash common.Hash, gas int64, depth int, static bool,


Why is value of Hash type?

Because this is more closely to big-endian representation of integer in EVM. go-ethereum uses big.Big type for it but can easily convert with common.BigToHash(): https://github.com/ethereum/go-ethereum/pull/17050/files#diff-9633a3aa7f6fd345efaab4431bab4ae8R253.

However, this is inconsistent with Host.Call() where big.Int is used.

axic · 2018-07-27T12:19:44Z

bindings/go/evmc/evmc.go

+
+type Error int32
+
+func (err Error) IsInternalError() bool {


This doesn't seem to be used below (or anywhere?).

axic · 2018-07-27T12:20:08Z

bindings/go/evmc/evmc.go

+	return err < 0
+}
+
+func (err Error) Error() string {


Would a name like ErrorToString or EVMCErrorString be more descriptive?

This is implementation of error interface.

type error interface { Error() string }

Ah ok, so all the wrappers return a standard Error object which contain an internal error value and this can be used to turn that into a string.

It means the evmc.Error type can used as the builtin error type.

axic · 2018-07-27T12:22:36Z

bindings/go/evmc/evmc.go

+	msg.input_size = input.n;
+	msg.gas = gas;
+	msg.depth = depth;
+	msg.kind = EVMC_CALL;


This means there's no way to report a CREATE transaction?

The VM does not care. I'm not sure if this information is available in go-ethereum context.

Hera is because if it is a create it must do the metering on the input and the returned data.

axic · 2018-07-27T12:37:20Z

bindings/go/evmc/evmc.go

+		return fmt.Sprintf("evmc: unknown internal error (%d)", int32(code))
+	}
+
+	panic(fmt.Sprintf("evmc: unknown status code %d", int32(code)))


Not sure it is a good idea making this a panic?

axic · 2018-07-27T12:42:17Z

bindings/go/evmc/loader.c

+ * Licensed under the MIT License. See the LICENSE file.
+ */
+
+#include "../../../lib/loader/loader.c"


Why not make this file a symlink instead? Is that not allowed by govendor or package managers?

Do symlinks work on Windows?

Git allows symlinks, I am sure it resolves it somehow on Windows. @gumb0 do you know by any chance what git is doing in that case?

axic · 2018-07-27T12:47:02Z

bindings/go/evmc/evmc_test.go

+	"testing"
+)
+
+func TestLoad(t *testing.T) {


Where is this used? What file is it loading?

These are unit tests. You can run them with go test .... It will load file pointed by EVMC_PATH.

gumb0 · 2018-07-31T16:01:15Z

Needs to be udated for EXTCODEHASH support

chfast · 2018-08-16T15:00:47Z

Needs to be udated for EXTCODEHASH support

EXTCODEHASH is included.

axic · 2018-08-18T17:52:30Z

@chfast is anything blocking the merging of this?

chfast · 2018-08-18T17:58:34Z

Missing reviews?

I'd go and merge this if no more obvious issues. Maybe I should fix one FIXME comment from go-ethereum.

Also this might be not compatible with the master branch. I base this on v5.0.0. But I can handle this.

The benefit of merging this is to have it tested on CI in new PRs so the Go bindings will match the recent changes in the EVMC API.

axic · 2018-08-18T18:15:08Z

Even if the geth team hasn't reviewed this I'd merge it for the reasons you've said.

axic · 2018-08-18T18:18:25Z

bindings/go/evmc/evmc.go

-	if (instance->set_option)
-		return instance->set_option(instance, name.p, value.p);
-	return 0;
+	int r = evmc_set_option(instance, name, value);


ret instead of r? It doesn't matter too much.

axic · 2018-08-18T18:18:54Z

bindings/go/evmc/evmc.go

-	return 0;
+	int r = evmc_set_option(instance, name, value);
+	free(name);
+	free(value);


So C.CString allocates a new pointer but throws away ownership of it?

Yes.

// Go string to C string // The C string is allocated in the C heap using malloc. // It is the caller's responsibility to arrange for it to be // freed, such as by calling C.free (be sure to include stdlib.h // if C.free is needed). func C.CString(string) *C.char

axic · 2018-08-18T18:22:23Z

bindings/go/evmc/evmc.go

+	s := evmcAddress(sender)
+	v := evmcUint256be(value)
+	ch := evmcUint256be(codeHash)
+	r := C.execute_wrapper(instance.handle, C.int64_t(ctxId), uint32(rev), &d, &s, &v, input,


Why not use more meaningful variable names?

axic · 2018-08-18T18:24:34Z

bindings/go/evmc/evmc.go

+		gas,
+		depth,
+		kind,
+		flags,


Why not keep the named version? Can you have named members here?

I'm counting on compiler warning in case a field is missed.

chfast · 2018-08-20T12:21:44Z

Rebased on master and squashed.
Works fine with geth.
If not objections I'm going to merge this.

axic · 2018-08-20T13:53:16Z

bindings/go/evmc/host.c

+    (evmc_emit_log_fn)emitLog,
+};
+
+void evmc_go_free_result_output(const struct evmc_result* result)


Actually instead of this one could use evmc_release_result from helpers.h.

Actually not. This is the implementation. evmc_release_result is just a wrapper that calls the implementation.

My bad, too early.

chfast force-pushed the go branch 2 times, most recently from d946800 to d52cb9f Compare July 13, 2018 15:22

chfast requested review from axic and gumb0 July 13, 2018 15:24

axic reviewed Jul 13, 2018

View reviewed changes

chfast force-pushed the go branch from d52cb9f to 5e86a03 Compare July 13, 2018 15:56

axic reviewed Jul 13, 2018

View reviewed changes

chfast mentioned this pull request Jul 13, 2018

EVMC support ethereum/go-ethereum#17050

Closed

ethereum deleted a comment from axic Jul 16, 2018

chfast force-pushed the go branch 3 times, most recently from a9ebe2f to 0b5d0e4 Compare July 16, 2018 12:24

gumb0 reviewed Jul 23, 2018

View reviewed changes

gumb0 reviewed Jul 24, 2018

View reviewed changes

axic reviewed Jul 25, 2018

View reviewed changes

chfast force-pushed the go branch from c4be8db to 53a2613 Compare July 26, 2018 12:22

gumb0 reviewed Jul 26, 2018

View reviewed changes

axic reviewed Jul 27, 2018

View reviewed changes

bindings/go/evmc/evmc.go

type Error int32

func (err Error) IsInternalError() bool {

Copy link

Member

axic Jul 27, 2018

Choose a reason for hiding this comment

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

This doesn't seem to be used below (or anywhere?).

axic reviewed Jul 27, 2018

View reviewed changes

chfast force-pushed the go branch 3 times, most recently from 66d2208 to 9242887 Compare August 16, 2018 13:38

axic reviewed Aug 18, 2018

View reviewed changes

chfast added this to the 5.1 milestone Aug 20, 2018

chfast force-pushed the go branch from a456a1f to 60f6192 Compare August 20, 2018 12:14

Add Go bindings

71df6b8

chfast force-pushed the go branch from 60f6192 to 71df6b8 Compare August 20, 2018 12:56

axic approved these changes Aug 20, 2018

View reviewed changes

axic merged commit 58a8d22 into master Aug 20, 2018

axic deleted the go branch August 20, 2018 13:31

axic reviewed Aug 20, 2018

View reviewed changes


		#include <stdlib.h>

		const struct evmc_context_fn_table fn_table = {


		extern const struct evmc_context_fn_table evmc_go_fn_table;

		static struct evmc_result execute_wrapper(struct evmc_instance* instance, int64_t context_index, enum evmc_revision rev,

Go bindings #41

Go bindings #41

Conversation

chfast commented Jul 12, 2018 • edited by axic Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

axic commented Jul 13, 2018

axic Jul 13, 2018 • edited Loading

Choose a reason for hiding this comment

chfast Jul 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

axic commented Jul 25, 2018

Choose a reason for hiding this comment

axic commented Jul 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

axic Jul 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

axic Jul 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gumb0 commented Jul 31, 2018

chfast commented Aug 16, 2018

axic commented Aug 18, 2018

chfast commented Aug 18, 2018

axic commented Aug 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chfast commented Aug 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chfast commented Jul 12, 2018 •

edited by axic

Loading

axic Jul 13, 2018 •

edited

Loading

chfast Jul 13, 2018 •

edited

Loading

axic Jul 27, 2018 •

edited

Loading

axic Jul 27, 2018 •

edited

Loading