status-im / status-console-client

Status messaging console user interface
Mozilla Public License 2.0
10 stars 2 forks source link

[WIP/ PoC] Nimbus test #79

Closed oskarth closed 5 years ago

oskarth commented 5 years ago

Experimental Nimbus support / PoC spike ready to merge.

Changes have been fairly isolated so it should hopefully not impact other parts of the code.

Please see modified README (https://github.com/status-im/status-console-client/pull/79/files?short_path=04c6e90#diff-04c6e90faac2675aa89e2176d2eec7d8) in commit for setup.

Old description

See https://github.com/status-im/nimbus/pull/331 for relevant issue Hacky code, just trying to get the thing to compile/link.

See issue above for current issue. The problem is dealing with static linking of secp256k1 which is different in both projects.


Update: secp issue was solved by exposing a shared library

Update 2: Other problem was deadlock, this was solved by some threading FFI magic on Nimbus side together with locking go procs to one OS thread

Follow up issues

See https://github.com/status-im/status-console-client/issues/123 and https://github.com/status-im/status-nim/issues/1

oskarth commented 5 years ago

https://github.com/oskarth/nimbus/commits/status-c-api my (in flux) branch for reference

oskarth commented 5 years ago

@adambabik Please have a look. It works end to end as a spike now. The changes should be isolated enough to make this mergable and only expose spike with the right build and runtime flags.

For further enhancements, we can create issues later. Right now the bridge has been created, which was the goal of this.

oskarth commented 5 years ago

@adambabik please have a look again, created a patch to address the issue of isolating changes while keeping things simple

adambabik commented 5 years ago

@oskarth Let's not add this patches stuff into another repo... It's one of the worst ideas I have seen.

diff --git a/main.go b/main.go
index c8b6766..aad4d16 100644
--- a/main.go
+++ b/main.go
@@ -59,6 +59,8 @@ var (

        // flags for external node
        providerURI = fs.String("provider", "", "an URI pointing at a provider")
+
+       useNimbus = fs.Bool("nimbus", false, "use Nimbus node")
 )

 func main() {
@@ -75,6 +77,10 @@ func main() {
                os.Exit(0)
        }

+       if *useNimbus {
+               startNimbus()
+       }
+
        var privateKey *ecdsa.PrivateKey

        if *keyHex != "" {
diff --git a/nimbus.go b/nimbus.go
index 06ab7d0..dc4877c 100644
--- a/nimbus.go
+++ b/nimbus.go
@@ -1 +1,17 @@
+// +build nimbus
+
 package main
+
+import (
+       statusnim "github.com/status-im/status-nim"
+       "runtime"
+)
+
+func init() {
+       runtime.LockOSThread()
+}
+
+func startNimbus() {
+       statusnim.Start()
+       statusnim.ListenAndPost()
+}
diff --git a/nimbus_disabled.go b/nimbus_disabled.go
index 06ab7d0..2878301 100644
--- a/nimbus_disabled.go
+++ b/nimbus_disabled.go
@@ -1 +1,7 @@
+// +build !nimbus
+
 package main
+
+func startNimbus() {
+       panic("executable needs to be built with -tags nimbus")
+}

^ Would this work? You would need to built it with go build -mod=vendor -ldflags="-r /usr/local/lib" -tags nimbus -o ./bin/status-term-client .

There are more concerns actually:

oskarth commented 5 years ago

Thanks for comments. I think it is important to understand what the goal here is. The primary goal of spiking out functionality end to end has been done. Now it's about coming to a checkpoint, which means saving the progress so far so we can build on it. The goal right now is not to get something production ready, nor figure out all the interfaces, nor have a slick build process, etc. How can we work towards that and get it merged as soon as possible (like today)?

@oskarth Let's not add this patches stuff into another repo... It's one of the worst ideas I have seen.

A patch is rough and doesn't lend itself to extensibility, but it (a) actually solves the problem of self-contained changes (b) is quick (c) is easy to reason about and fixup later. Seems like hyperbole :P That said:

build flag sketch

Thanks, this seems OK.

status-im/nimbus#331 is not merged and I guess this is needed to build libnimbus_api.so? Why do we want to merge this PR then?

It has been merged now.

How can I build libnimbus_api.so? I can't find any instruction in README.md just information that I should probably copy it into some dir.

See https://github.com/status-im/nimbus/blob/devel/nimbus/api/build_status_api.sh This needs to be prettied up a lot, but it's outside the scope of this spike PR.

Do we need all this new stuff in vendor/?

You are right, it was used before to get around static library limitations, but assuming we use Nimbus as a shared library (and there's no reconciliation in geth and Nimbus secp256k1 libs) it should be OK.

adambabik commented 5 years ago

@oskarth I am not against adding that, just had concerns of the previous form. Now it looks great!

oskarth commented 5 years ago

Thanks for feedback and help with conditional build flags in Go!