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

Nimbus integration #4254

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

Nimbus integration #4254

wants to merge 2 commits into from

Conversation

vitvly
Copy link
Member

@vitvly vitvly commented Nov 2, 2023

This PR updates the nimbus interaction layer to use the most recent version of Nimbus proxy
Also

  • adds a more proper cleanup procedures for Client.
  • updated block roots are stored in DB
  • retrofitted proxy-wrapper <-> status-go communication

@status-im-auto
Copy link
Member

status-im-auto commented Nov 2, 2023

Jenkins Builds

Click to see older builds (23)
Commit #️⃣ Finished (UTC) Duration Platform Result
✖️ 9de3742 #1 2023-11-02 12:35:49 ~3 min tests 📄log
✔️ 9de3742 #1 2023-11-02 12:39:04 ~7 min linux 📦zip
✔️ 9de3742 #1 2023-11-02 12:40:09 ~8 min android 📦aar
✔️ 1d5b0b3 #2 2023-11-03 12:40:00 ~5 min ios 📦zip
✔️ 1d5b0b3 #2 2023-11-03 12:41:09 ~6 min android 📦aar
✖️ 1d5b0b3 #2 2023-11-03 12:41:32 ~6 min tests 📄log
✔️ 1d5b0b3 #2 2023-11-03 12:41:39 ~6 min linux 📦zip
✖️ 4a27e4b #3 2024-01-14 17:59:52 ~2 min tests 📄log
✔️ 4a27e4b #3 2024-01-14 18:00:54 ~3 min linux 📦zip
✔️ 4a27e4b #3 2024-01-14 18:02:42 ~5 min android 📦aar
✔️ 6503f66 #4 2024-01-15 09:16:40 ~1 min linux 📦zip
✖️ 6503f66 #4 2024-01-15 09:17:18 ~2 min tests 📄log
✔️ 6503f66 #4 2024-01-15 09:20:02 ~5 min android 📦aar
✖️ d3ec1fd #5 2024-01-15 09:57:50 ~2 min tests 📄log
✔️ d3ec1fd #5 2024-01-15 09:58:37 ~3 min linux 📦zip
✔️ d3ec1fd #5 2024-01-15 10:00:42 ~5 min android 📦aar
✖️ 04631f7 #6 2024-01-17 16:38:39 ~1 min tests 📄log
✔️ 04631f7 #6 2024-01-17 16:39:59 ~2 min android 📦aar
✔️ 04631f7 #6 2024-01-17 16:40:57 ~3 min linux 📦zip
✖️ 5aec7ce #7 2024-01-18 10:21:32 ~1 min tests 📄log
✔️ 5aec7ce #7 2024-01-18 10:21:55 ~1 min linux 📦zip
✔️ 5aec7ce #7 2024-01-18 10:22:44 ~2 min android 📦aar
✖️ 5aec7ce #8 2024-01-19 06:15:30 ~2 min tests 📄log
Commit #️⃣ Finished (UTC) Duration Platform Result
✖️ a44e50b #9 2024-02-10 13:20:09 ~2 min tests 📄log
✔️ a44e50b #8 2024-02-10 13:21:12 ~3 min linux 📦zip
✔️ a44e50b #8 2024-02-10 13:22:52 ~5 min android 📦aar
✖️ e49fd45 #10 2024-02-12 09:22:49 ~1 min tests 📄log
✔️ e49fd45 #9 2024-02-12 09:23:27 ~1 min android 📦aar
✔️ e49fd45 #9 2024-02-12 09:24:31 ~2 min linux 📦zip

@vitvly vitvly marked this pull request as ready for review February 12, 2024 09:21
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to update the migration file timestamp here before we can merge this file?

@jakubgs
Copy link
Member

jakubgs commented Feb 13, 2024

You need to rebase this PR, it won't run with old CI labels.

Copy link
Member

@richard-ramos richard-ramos left a comment

Choose a reason for hiding this comment

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

Interesting feature!
What would happen in mobile if we merge this PR? would this version still be usable?
I'm also not entirely sure how packaging nimbus would work when generating the gomobile bindings.

// Run light client proxy
ctx, cancel := context.WithCancel(context.Background())
proceed:
fmt.Println("after waitForProxyHeaders")
Copy link
Member

Choose a reason for hiding this comment

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

Should probably be a debug log?

Suggested change
fmt.Println("after waitForProxyHeaders")
fmt.Println("after waitForProxyHeaders")

// Let's check if handlers have been installed
_, found := client.handler("eth_getBalance")
if found {
fmt.Println("Proceed")
Copy link
Member

Choose a reason for hiding this comment

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

Debug log instead of Println

@@ -88,7 +88,7 @@ require (
github.com/meirf/gopart v0.0.0-20180520194036-37e9492a85a8
github.com/mutecomm/go-sqlcipher/v4 v4.4.2
github.com/schollz/peerdiscovery v1.7.0
github.com/siphiuel/lc-proxy-wrapper v0.0.0-20230516150924-246507cee8c7
github.com/vitvly/lc-proxy-wrapper v0.0.0-20240210131537-0f48377f5a71
Copy link
Member

Choose a reason for hiding this comment

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

This repo has no license. Probably a good idea to add a MIT/Apache2 and/or move it into status-im

@@ -312,7 +312,7 @@ func (n *StatusNode) setupRPCClient() (err error) {
if err != nil {
return
}
n.rpcClient, err = rpc.NewClient(gethNodeClient, n.config.NetworkID, n.config.UpstreamConfig, n.config.Networks, n.appDB)
n.rpcClient, err = rpc.NewClient(gethNodeClient, n.config.NetworkID, n.config.UpstreamConfig, n.config.Networks, n.config.NimbusProxyConfig.Enabled, n.appDB)
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if at this point if it wouldn't be a good idea to create a config object instead of adding more parameters to the function?

Comment on lines +809 to +811
if err != nil {
return "", err
}
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible for this query to return no rows and be valid? because you might get a sql.ErrNoRows so I wonder if we should add a

	if errors.Is(err, sql.ErrNoRows) {
		return "", nil
	}


blockRoot, err := nodecfg.GetNimbusTrustedBlockRoot(c.db)
if err != nil {
fmt.Println("verif_proxy GetNimbusTrustedBlockRoot error", err)
Copy link
Member

Choose a reason for hiding this comment

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

Error log instead of Println

if !proxyInitialized {
proxyInitialized = true
// Create RPC client using verification proxy endpoint
endpoint := "http://" + cfg.RpcAddress + ":" + fmt.Sprint(cfg.RpcPort)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
endpoint := "http://" + cfg.RpcAddress + ":" + fmt.Sprint(cfg.RpcPort)
endpoint := fmt.Sprintf("http://%s:%d", cfg.RpcAddress, cfg.RpcPort)

if proxyInitialized && ev.EventType == proxytypes.FinalizedHeader {
err = storeUpdatedBlockRoot(c, ev.Msg)
if err != nil {
fmt.Println("verif_proxy storeUpdatedBlockRoot", err)
Copy link
Member

Choose a reason for hiding this comment

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

Error log instead of println

// Invoke a simple RPC method in order to ascertain that proxy is up and running
ctx, _ := context.WithTimeout(c.ctx, 5*time.Second)
_, err := blockNumber(ctx, proxyClient)
fmt.Println("verif_proxy blockNumber result", err)
Copy link
Member

Choose a reason for hiding this comment

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

Error log instead of println, only if err != nil

@friofry
Copy link
Contributor

friofry commented May 24, 2024

Hi @vitvly,
I wonder if this feature will be merged to develop? There is code on the desktop side to enable=true in NimbusProxyConfig. I'm fixing the API to enable NimbusProxyConfig. But I'll probably leave the status-go part unimplemented for now

@vitvly
Copy link
Member Author

vitvly commented May 28, 2024

@friofry hi, i will push updated PR with review comments fixed and hopefully it can be merged.

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.

None yet

6 participants