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

ipfs: move repo intialization to node start instead of in tendermint init #314

Closed
liamsi opened this issue May 9, 2021 · 3 comments · Fixed by #334
Closed

ipfs: move repo intialization to node start instead of in tendermint init #314

liamsi opened this issue May 9, 2021 · 3 comments · Fixed by #334
Labels
C:data-availability Component: Data Availability Proofs C:dht DHT and p2p related issues (IPFS mostly)

Comments

@liamsi
Copy link
Member

liamsi commented May 9, 2021

See: https://github.com/lazyledger/lazyledger-core/blob/50f722a510dd2ba8e3d31931c9d83132d6318d4b/cmd/tendermint/commands/init.go#L119-L120

This is more relevant for the light client (mvp).
See also: https://github.com/lazyledger/lazyledger-core/blob/9a08608a49c19d587e988eb33dfce11357cea580/docs/lazy-adr/adr-004-mvp-light-client.md#running-an-ipfs-node

The init command should only create a config that can be edited afterwards without fully initializing the repo. Intitialzing the repo should happen on demand when the node is spun up the first time. The same mechanism should be reusable for the light client or for testing purposes.

@liamsi liamsi added C:data-availability Component: Data Availability Proofs C:dht DHT and p2p related issues (IPFS mostly) labels May 9, 2021
@Wondertan
Copy link
Member

I don't think doing shadow initialization of IPFS repo is a good idea, instead, if a user wants to start a node with one and only command it is better for him to provide a related bool option, e.g --ipfs-init for IPFS initialization not to do that silently.

IPFS repo initialization is similar to tm's one where mostly directory and config are created for further operations. Keeping ipfs repo initialization within tendermint init makes great sense as their behavior is identical. It would allow config amends after initialization before running the actual daemon.

We should also consider addition of --ipfs--addr string option to support work with existing ipfs repos and daemons over HTTP CoreAPI implementation and parse standard IPFS_API env var to autodetect running IPFS daemons on the same machine.

@liamsi
Copy link
Member Author

liamsi commented May 10, 2021

That is a good point. Thanks @Wondertan! I also think it is a good idea to add this flag.

IPFS repo initialization is similar to tm's one where mostly directory and config are created for further operations.

I think the problem here is that we have IPFS configs in both files: the tendermint conf and the ipfs conf. I think the IPFS config section in the tendermint should at least be removed. The only thing tendermint needs to know is where the repo (or the real ipfs config) is. Everything else is configured in that very repo. So instead of this:
https://github.com/lazyledger/lazyledger-core/blob/59ceaf8c44a069a55d5f8f8f3bea50b23a186146/config/ipfs_config.go#L3-L18
and this:
https://github.com/lazyledger/lazyledger-core/blob/59ceaf8c44a069a55d5f8f8f3bea50b23a186146/config/toml.go#L437-L458

we could have code-only transforms/profiles. This would be more similar to ipfs: https://github.com/ipfs/go-ipfs-config/blob/9a8ef0f5c436319b76187606e82cf9f8bc74df0b/profile.go#L10-L18

So the config would be created from a default profile either on init, or, via the flag you mentioned when the node is started. Instead of currently starting IPFS OnStart, we could (or maybe must) pass in a CoreAPI object to NewNode directly. The latter is related to using the same block store for everything. @evan-forbes brought the latter up in our 1:1.

@Wondertan
Copy link
Member

Wondertan commented May 11, 2021

Agree, IPFS config in tendermint config should be removed. Also, I like the idea to use Transformer

cmwaters pushed a commit that referenced this issue Mar 13, 2023
cmwaters pushed a commit that referenced this issue Mar 13, 2023
* Add changelog entry for #314

Signed-off-by: Thane Thomson <[email protected]>

* Build changelog with unreleased changes for pre-release

Signed-off-by: Thane Thomson <[email protected]>

* version: Set to v0.34.27-alpha.1

Signed-off-by: Thane Thomson <[email protected]>

* ci: Update to incorporate/backport changes from #317

Signed-off-by: Thane Thomson <[email protected]>

* ci: Simplify release workflow further

Signed-off-by: Thane Thomson <[email protected]>

* ci: Add step descriptions for pre-release and release workflows to explain what they do

Signed-off-by: Thane Thomson <[email protected]>

---------

Signed-off-by: Thane Thomson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:data-availability Component: Data Availability Proofs C:dht DHT and p2p related issues (IPFS mostly)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants