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.

Sign up for GitHub

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

Jump to bottom

cmd: add flag to start node on remote core #196

Merged
merged 8 commits into from
Nov 17, 2021
Merged

cmd: add flag to start node on remote core #196

merged 8 commits into from
Nov 17, 2021

Conversation

vgonkivs
Copy link
Member

  • Added flag that tells node to connect to the remote address.

Closes #142

P.S. Unfortunately, I was not able to test it properly. I have tried to create and run StartRemoteClient() from core/testing.go but got an error Error: node: failed to start: client not running. Did I do something wrong?

Wondertan
Wondertan requested changes Nov 14, 2021
View reviewed changes
Copy link
Member

@Wondertan Wondertan left a comment

Choose a reason for hiding this comment

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

@vgonkivs, this should work over ‘start’ command and not ‘init’. We should start the node with a remote core, even when the configuration is initialized to work with an embedded one. Essentially, flags should override configurations in runtime, not overwrite them on disk.

@vgonkivs
Copy link
Member Author

Thanks for explanation @Wondertan . Do we need to handle it for dev node?

@Wondertan
Copy link
Member

@vgonkivs, you can omit it this for the dev mode, as it is going to be reworked.

@renaynay
Copy link
Member

@vgonkivs there is a failing test for starting the remote client - let me know if you can resolve this. Happy to help as well.

@vgonkivs
Copy link
Member Author

Hi, @renaynay. Currently investigating what went wrong. Panic happened on newly added test for node starting over remote core.

github.com/celestiaorg/celestia-core/libs/pubsub.(*Subscription).Out(0x0, 0x0) /home/runner/go/pkg/mod/github.com/celestiaorg/celestia-core@v0.0.2-0.20210924001615-488ac31b4b3c/libs/pubsub/subscription.go:43 +0x5 github.com/celestiaorg/celestia-core/rpc/core.Subscribe.func1(0x1b25db0, 0x0, 0xc016c2c090, 0x13, 0x1b0a3e0, 0x25bee00, 0xc06dcb9920, 0xc08337ca40, 0xf) /home/runner/go/pkg/mod/github.com/celestiaorg/celestia-core@v0.0.2-0.20210924001615-488ac31b4b3c/rpc/core/events.go:50 +0x4c created by github.com/celestiaorg/celestia-core/rpc/core.Subscribe /home/runner/go/pkg/mod/github.com/celestiaorg/celestia-core@v0.0.2-0.20210924001615-488ac31b4b3c/rpc/core/events.go:47 +0x4bd

Based on the stack trace, it seems that somehow pubsub subscription was not created...
Strange behaviour, but all tests have been passed several times on my machine.
Will be glad to get help from you.

@vgonkivs
Copy link
Member Author

Now all my test have passed but another issue appears on TestSubscriber on pipeline with 1.16

@renaynay
Copy link
Member

renaynay commented Nov 16, 2021 •
edited
Loading

Don't worry @vgonkivs, there is something wrong with the subscriber stuff on IPFS side I believe, so we're gonna ignore the flakeyness for now.

@renaynay renaynay mentioned this pull request Nov 16, 2021
Closed
1 task
@renaynay renaynay added area:config CLI and config area:core_and_app Relationship with Core node and Celestia-App labels Nov 16, 2021
Bidon15
Bidon15 previously approved these changes Nov 16, 2021
View reviewed changes
Copy link
Member

@Bidon15 Bidon15 left a comment

Choose a reason for hiding this comment

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

Works flawlessly. No hickups so far 👏

Wondertan
Wondertan reviewed Nov 16, 2021
View reviewed changes
Copy link
Member

@Wondertan Wondertan left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@vgonkivs, man, this is amazing, thank you!

Left a few comments, but otherwise LGTM!

node/fxutil/util.go Outdated Show resolved Hide resolved
node/config.go Outdated Show resolved Hide resolved
cmd/start.go Outdated Show resolved Hide resolved
node/core/core.go Show resolved Hide resolved
node/fxutil/util.go Show resolved Hide resolved
@Wondertan
Copy link
Member

@vgonkivs, one more thing. We recently made an update in #175, where we changed all the imports from celestiaorg/celestia-core to tendermint/termint. This change also affects your PR so pls do a rebase on the main branch.

renaynay
renaynay reviewed Nov 17, 2021
View reviewed changes
Copy link
Member

@renaynay renaynay left a comment

Choose a reason for hiding this comment

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

some nits but otherwise LGTM! and yes we need a rebase before merge :)

cmd/start.go Outdated Show resolved Hide resolved
core/testing.go Outdated Show resolved Hide resolved
@vgonkivs vgonkivs force-pushed the add_flag_to_start_node_on_remote branch from 4df8c0f to 4eed2e2 Compare November 17, 2021 11:06
@vgonkivs
Copy link
Member Author

Please let me know if everything is ok , so I can squash commits into one.

@liamsi
Copy link
Member

liamsi commented Nov 17, 2021

Awesome! Thanks! Did someone try running this? @vgonkivs @Bidon15 ?

@vgonkivs
Copy link
Member Author

vgonkivs commented Nov 17, 2021 •
edited
Loading

Hi @liamsi . It works for me with --core.remote on mocked Node and without flag(embedded. I mean no regression was found).

@Bidon15
Copy link
Member

Bidon15 commented Nov 17, 2021

Awesome! Thanks! Did someone try running this? @vgonkivs @Bidon15 ?

Yeap. All good for me 👍🔥

Wondertan
Wondertan previously approved these changes Nov 17, 2021
View reviewed changes
Copy link
Member

@Wondertan Wondertan left a comment

Choose a reason for hiding this comment

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

Besides two nits, LGTM

cmd/start.go Outdated Show resolved Hide resolved
Wondertan
Wondertan reviewed Nov 17, 2021
View reviewed changes
Copy link
Member

@Wondertan Wondertan left a comment

Choose a reason for hiding this comment

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

Awesome! Thanks again @vgonkivs.

@Wondertan Wondertan merged commit 5027f63 into celestiaorg:main Nov 17, 2021
@renaynay renaynay mentioned this pull request Nov 19, 2021
Merged
@vgonkivs vgonkivs deleted the add_flag_to_start_node_on_remote branch June 6, 2022 13:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Wondertan Wondertan Wondertan approved these changes

@Bidon15 Bidon15 Bidon15 approved these changes

@liamsi liamsi Awaiting requested review from liamsi

@mattdf mattdf Awaiting requested review from mattdf

@renaynay renaynay Awaiting requested review from renaynay renaynay is a code owner

Assignees

@vgonkivs vgonkivs

Labels
area:config CLI and config area:core_and_app Relationship with Core node and Celestia-App
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add flag to pass address of remote Core node
5 participants
@vgonkivs @Wondertan @renaynay @liamsi @Bidon15