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

core: initialization and Repository #81

Merged
merged 11 commits into from
Sep 28, 2021
Merged

core: initialization and Repository #81

merged 11 commits into from
Sep 28, 2021

Conversation

Wondertan
Copy link
Member

@Wondertan Wondertan commented Sep 23, 2021

Context

The penultimate piece for #30. It introduces initialization flow for the embedded Core node along with repository to access Core' stored data. Basically, it implements some of the important functionality for future init command.

Others

  • Implements custom serialization for core.Config(rationale is in the code)
  • Starts own collection of utilities
  • Provides FS, InMem and Mock repositories

Refs

Depends on #71 and #74
Moves towards done #30

@Wondertan Wondertan self-assigned this Sep 23, 2021
@Wondertan Wondertan force-pushed the hlib/core-repo branch 2 times, most recently from a2c165d to c6ebf9d Compare September 24, 2021 11:16
@Wondertan

This comment has been minimized.

Copy link
Member

@liamsi liamsi left a comment

Choose a reason for hiding this comment

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

Can you also merge #71 and rebase this on master again?

@Wondertan

This comment has been minimized.

@Wondertan Wondertan mentioned this pull request Sep 27, 2021
2 tasks
@Wondertan

This comment has been minimized.

@Wondertan

This comment has been minimized.

@liamsi
Copy link
Member

liamsi commented Sep 27, 2021

with repository to access Core' stored data

isn't the whole idea that celestia-node only communicates with core (includes accessing data) via the RPC interface (even if local)?

Copy link
Member

@liamsi liamsi left a comment

Choose a reason for hiding this comment

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

On first sight it seems this PR breaks somewhat with the design paradigm we have settled on (namely that the the node only knows about very few RPC endpoints and that is the only thing it needs from core). The motivation for that seems to be that the future node-specific data and config are written into the same "repo" as the core node's?
I understand why this might be desirable but that is really not important for devnet.

We have to be careful to focus on the DA-specific parts!

core/init.go Outdated Show resolved Hide resolved
core/init.go Show resolved Hide resolved
core/init.go Show resolved Hide resolved
@liamsi
Copy link
Member

liamsi commented Sep 27, 2021

Implements custom serialization for core.Config(rationale is in the code)

Which custom serialzation (toml is not custom)? Where is the rationale?

@Wondertan
Copy link
Member Author

Remember, we are treating tendermint as a black-box.

Yeeeaaah, on a high level, but the deeper you go, the less it black-boxable 🤷🏻‍♂️

@Wondertan
Copy link
Member Author

Wondertan commented Sep 27, 2021

The motivation for that seems to be that the future node-specific data and config are written into the same "repo" as the core node's?
I understand why this might be desirable but that is really not important for devnet.

The motivations here are:

  • Make embedded Core work
  • Simplify deployment of things by having everything related to initialization in one place.
    • This is a crucial part for devnet and future nets. Instead of having separate hacky initialization scripts for Core and Node, we control where it actually needs to be controlled. The PR just implements initialization complexity earlier than later.
  • Thoroughly understand tendermint initialization flow and its requirement

@Wondertan
Copy link
Member Author

Which custom serialzation (toml is not custom)?

Not custom serialization, but custom serialization logic.

Where is the rationale?

Next LoadConfig func

core/config.go Show resolved Hide resolved
@liamsi
Copy link
Member

liamsi commented Sep 27, 2021

Yeeeaaah, on a high level, but the deeper you go, the less it black-boxable 🤷🏻‍♂️

Let's still try to stick to that paradigm as much as possible.

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.

couple nits, I'm okay with this but I hate the idea that we need to do this much work to get core to work embedded within node. It's not that much reward per amount of work that's gone into it so far. It's fine otherwise though. We aim to remove all this regardless after Devnet.

core/config.go Outdated Show resolved Hide resolved
core/config.go Outdated Show resolved Hide resolved
core/config.go Outdated Show resolved Hide resolved
core/config.go Show resolved Hide resolved
@Wondertan
Copy link
Member Author

Wondertan commented Sep 27, 2021

We aim to remove all this regardless after Devnet.

@renaynay, WDYM?

@Wondertan
Copy link
Member Author

According to offsite. We treat Tendermint as a black box for devnet. Later on, we will have to decouple it into more pieces, so this code is likely to stay preserving control over initialization.

@renaynay
Copy link
Member

I mean, we will not run celestia-node and celestia-core processes together. This is a crutch for devnet. After devnet, all should be communicating via p2p layer (meaning celestia-node does not need to know / care about celestia-core ).

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.

OK Thank you!

Copy link
Member

@liamsi liamsi left a comment

Choose a reason for hiding this comment

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

👏🏼 LGMT

@Wondertan Wondertan merged commit 1bd0e90 into main Sep 28, 2021
@Wondertan Wondertan deleted the hlib/core-repo branch September 28, 2021 10:17
@Wondertan Wondertan restored the hlib/core-repo branch January 7, 2022 14:40
@Wondertan Wondertan deleted the hlib/core-repo branch January 7, 2022 15:55
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.

3 participants