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

store: change to save DA and headers #218

Closed
wants to merge 9 commits into from
Closed

Conversation

tac0turtle
Copy link
Contributor

@tac0turtle tac0turtle commented Mar 16, 2021

Description

Implement saving of header and DA header

Closes: #182

@Wondertan Wondertan mentioned this pull request Mar 19, 2021
@lgtm-com
Copy link

lgtm-com bot commented Mar 20, 2021

This pull request introduces 2 alerts when merging 493c7e8 into 74c5b7d - view on LGTM.com

new alerts:

  • 2 for Useless assignment to local variable

Comment on lines 86 to 99
// todo: use IPLD plugin interface
// func Block(ctx *rpctypes.Context, heightPtr *int64) (*ctypes.ResultBlock, error) {
// height, err := getHeight(env.BlockStore.Height(), heightPtr)
// if err != nil {
// return nil, err
// }

// block := env.BlockStore.LoadBlock(height)
// blockMeta := env.BlockStore.LoadBlockMeta(height)
// if blockMeta == nil {
// return &ctypes.ResultBlock{BlockID: types.BlockID{}, Block: block}, nil
// }
// return &ctypes.ResultBlock{BlockID: blockMeta.BlockID, Block: block}, nil
// }
Copy link
Member

Choose a reason for hiding this comment

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

This would be a very simple function, that a) gets the block hash for the requested height, b) gets the DA header for that hash, c) uses the DA header to actually get the block data (e.g. via RetrieveBlockData of adr002 / the PRs that @evan-forbes is working on). Not sure if a) + b) could be one step instead.

@tac0turtle
Copy link
Contributor Author

Note to self: Blockchain reactor and possibly consensus need to fetch the block in order to conduct replay against the app.

@lgtm-com

This comment has been minimized.

@liamsi
Copy link
Member

liamsi commented Mar 24, 2021

This is blocked on the reading portion of ADR 002, right? #194

@github-actions

This comment has been minimized.

@github-actions github-actions bot added the Stale label Apr 4, 2021
@tac0turtle tac0turtle added S:blocked Status Blocked and removed Stale labels Apr 4, 2021
@tac0turtle
Copy link
Contributor Author

This is blocked on the reading portion of ADR 002, right? #194

yes, otherwise there is nothing to verify for fast sync and consensus .

@github-actions

This comment has been minimized.

@github-actions github-actions bot added the Stale label Apr 15, 2021
@liamsi liamsi removed the Stale label Apr 15, 2021
@liamsi
Copy link
Member

liamsi commented Apr 15, 2021

Removed the stale label. We want to attack in the next few weeks now that the read portion is also there.
cc @evan-forbes @Wondertan @marbar3778

@liamsi
Copy link
Member

liamsi commented Apr 16, 2021

Quoting @marbar3778:

Note to self: Blockchain reactor and possibly consensus need to fetch the block in order to conduct replay against the app.

So one way to unblock this PR, or rather chunk up the missing pieces into smaller would be to disable fast-sync first and use RetrieveBlockData during consensus. We can later figure out how to do syncing (basically fetching the DA header and calling RetrieveBlockData too).
Is this a correct summary of our brief discussion, Marko?

Just for context @Wondertan and @evan-forbes started working or will start working shortly on the using-RetrieveBlockData during consensus part. Does it make sense to hold off merging this until that part is already done?

@Wondertan
Copy link
Member

basically fetching the DA header and calling RetrieveBlockData too

@liamsi, that's for full node, but in case I am a light node, how does chain syncing works for me? Is this simply fetching headers from last to first and sampling related blocks in the same order?

@liamsi
Copy link
Member

liamsi commented Apr 16, 2021

Essentially, yes. There are basically two kinds of light nodes:

  • Nodes that only rely on DA during consensus: they receive the DA header with a proposal, and yes, they sample as you said.
  • Actual light-clients can either:
    • rely on an honest majority assumption; these only follow headers and verify signatures - basically how the light-client works in tendermint right now
    • do DA sampling, so they first have to get the DA header too and sample as you mentioned above too

Nodes that want to actually sync data and replay state still need to be defined more concretely. But they could download the data in parallel (doesn't have to be in order).

@Wondertan
Copy link
Member

Wondertan commented Apr 16, 2021

@liamsi, I might have missed mentioning this in the question, but I meant a case where a new node joins an existing network. Imagine the network is more than 10000 epochs large and the new node that does not want to rely on honest majority needs to sync the chain.

Btw, is the case above called a replay in tm terms? Or replay is only related to state, while chain sync is specific for chain validation? AFAIK, most blockchains combine those two terms, but in this case, state validity is not a concern for Celestial consensus, thus they are separated.

@adlerjohn
Copy link
Member

adlerjohn commented Apr 16, 2021

The particular deployed node can define how it's parametrized. Light nodes work as @Wondertan described (download all block headers, do DAS for each block), while superlight nodes rely on an honest majority assumption. See: https://github.com/lazyledger/lazyledger-specs/blob/de5f4f74f56922e9fa735ef79d9e6e6492a2bad1/specs/node_types.md#node-type-definitions

Which ones is used depends on what the node runner wants.

@liamsi liamsi removed the S:blocked Status Blocked label Apr 29, 2021
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.

@marbar3778 we decided to store the DAHeader inside of BlockMeta for now instead. Any chance you could revive this PR including this change (basically merging in master and some minor changes to the API) soon?
Related:

@tac0turtle
Copy link
Contributor Author

@marbar3778 we decided to store the DAHeader inside of BlockMeta for now instead. Any chance you could revive this PR including this change (basically merging in master and some minor changes to the API) soon?
Related:

* #374

* specifically: https://github.com/lazyledger/lazyledger-core/pull/374/files#r645077665

yea will resurrect it tomorrow and over the weekend. Thanks for the ping

@liamsi
Copy link
Member

liamsi commented Aug 16, 2021

@marbar3778 we decided that for our devnet (see #491) we will try to keep the changes to tendermint as minimal as possible. Hence, we will just keep the vanilla tendermint store and use a dedicated "Celestia node" that connects to a tendermint node e.g. via RPC to retrieve the block, erasure code it (again) and store and serve it to DAS nodes.

I'm going to close this issue for now but please do not delete the branch. We might come back to this at a later point in time to only store the block data once.

@liamsi liamsi closed this Aug 16, 2021
@tac0turtle tac0turtle deleted the marko/store branch September 2, 2021 07:58
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.

Store: only store header + DA header
4 participants