das: DASing of past headers #473

renaynay · 2022-02-23T12:18:29Z

This PR implements DASing of past headers in a single routine.

Eventually, this will be parallelised.

TODO

figure out what to do in the instance that DASer runs and samples new headers [100:150] and DASer is stopped, so sample routine stores latest checkpoint to disk as last DASed height (150) but catchUp routine has only sampled from [1:40] so there is a gap missing from (40: 100)?
fix constructor
cleanup
lint
doc.go
open issue for parallelisation of DASer
open issue for DASState

Resolves #181.

liamsi

Left some nits. Overall looks very good.

das/daser.go

das/daser_test.go

das/doc.go

das/checkpoint_store.go

das/daser.go

renaynay · 2022-03-07T18:27:35Z

I think I will address the first TODO in a separate issue:

figure out what to do in the instance that DASer runs and samples new headers [100:150] and DASer is stopped, so sample routine stores latest checkpoint to disk as last DASed height (150) but catchUp routine has only sampled from [1:40] so there is a gap missing from (40: 100)?

Wondertan

Generally looks good, but there are two issues:

Stopping logic will panic if there were two or more catchups
A few changes still needed to be done before share: Cache Availability #180 becomes helpful

das/doc.go

das/daser.go

Wondertan

Ok, so the last two issues are fixed. Here is another review with suggestions, thoughts, and one bug catch.

Also, I think I found a drawback in a solution for the last TODO. Think of a common case:

A Node is started and needs/starts a catch-up
It finishes catching up while the sample routine is and was going for thousands of heights
The node is stopped along with the DASser and the very old checkpoint is saved
On startup, DASer will load this old checkpoint and will start dasing from it

You may think that #180 will fix this and this is true to some extent, but #180 is about the new ShareAvailable implementation which stores the fact of availability on disk and to check it we need to do an IO per height, so on long height ranges, this can be an IO waste issue. I think we can justify this issue in the case of a gap described originally, but for the common case I mentioned above this is not acceptable IMO. Also, the logs will be misleading and false stating, as it would say catching up over already sampled headers.

The issue can be fixed by checking: if there were no catchUp routines terminated(meaning we fully caught up on everything), then store the checkpoint not from the catchUpScheduler, but the one in sample like it was before.

das/daser.go

adlerjohn · 2022-03-17T00:44:27Z

CI won't run with merge conflicts IIRC, so could you fix the conflict as you push?

das/daser.go

Wondertan · 2022-03-21T17:42:11Z

@renaynay, did you get a chance to run DASer manually?

das/daser.go

…AS history (#20)

Co-authored-by: Hlib Kanunnikov <[email protected]>

…is not on the Node

… at a time (#24)

Wondertan

No more serious issues I am aware of, only nits left. Almost there.

das/daser.go

Wondertan

liamsi

What is with the 2 TODOs mentioned in the opening comment?

das/checkpoint_store.go

das/daser.go

Co-authored-by: Ismail Khoffi <[email protected]>

renaynay · 2022-03-28T11:20:28Z

@liamsi the 2 TODOs mentioned initially in the comment of this PR are related to an issue that can only be resolved once #427 is implemented.

liamsi

I find the naming confusing but the docs clarify it. So LGTM

renaynay added the area:shares Shares and samples label Feb 23, 2022

renaynay force-pushed the dasing-of-past-headers branch from a034dbe to 8a540d9 Compare February 23, 2022 12:29

Bidon15 mentioned this pull request Feb 23, 2022

das: Implement DASer public API #427

Closed

2 tasks

renaynay force-pushed the dasing-of-past-headers branch 2 times, most recently from 7621a53 to 76e4016 Compare February 24, 2022 17:28

renaynay marked this pull request as ready for review February 24, 2022 19:49

renaynay requested review from liamsi and Wondertan as code owners February 24, 2022 19:49

renaynay force-pushed the dasing-of-past-headers branch from 548ce3e to 2b6edea Compare February 24, 2022 20:07

renaynay mentioned this pull request Feb 28, 2022

das: Parallelise catchUp method #483

Closed

liamsi reviewed Feb 28, 2022

View reviewed changes

adlerjohn reviewed Mar 1, 2022

View reviewed changes

renaynay force-pushed the dasing-of-past-headers branch from 4eeac67 to ee95275 Compare March 1, 2022 13:41

renaynay mentioned this pull request Mar 7, 2022

das: Ensure there are no gaps in DASed past headers in case of stopped node #504

Closed

Wondertan requested changes Mar 10, 2022

View reviewed changes

das/doc.go Show resolved Hide resolved

das/daser.go Outdated Show resolved Hide resolved

das/daser.go Outdated Show resolved Hide resolved

das/daser.go Outdated Show resolved Hide resolved

renaynay mentioned this pull request Mar 15, 2022

das: Improve management of catch-up threads and prevent gaps in DAS history renaynay/celestia-node#20

Merged

Wondertan reviewed Mar 16, 2022

View reviewed changes

renaynay force-pushed the dasing-of-past-headers branch from 4889821 to 329c004 Compare March 21, 2022 14:10

Wondertan reviewed Mar 21, 2022

View reviewed changes

das/daser.go Outdated Show resolved Hide resolved

Wondertan requested changes Mar 21, 2022

View reviewed changes

das/daser.go Outdated Show resolved Hide resolved

das/daser.go Outdated Show resolved Hide resolved

renaynay mentioned this pull request Mar 21, 2022

share: Cache Availability #180

Closed

renaynay force-pushed the dasing-of-past-headers branch from d474f09 to 21045cb Compare March 21, 2022 18:20

renaynay requested review from Wondertan, liamsi and adlerjohn March 21, 2022 18:38

renaynay commented Mar 21, 2022

View reviewed changes

das/daser.go Outdated Show resolved Hide resolved

renaynay commented Mar 21, 2022

View reviewed changes

das/daser.go Outdated Show resolved Hide resolved

renaynay and others added 20 commits March 26, 2022 12:41

refactor: remove Head method from HeaderGetter interface

52cd504

refactor: remove height var from sample routine

2170142

chore: remove white space

27523d6

refactor: improve checkpoint store init and fix comments

347eb2a

refactor: remove word random from tests

7ce4a94

docs: better document the HeaderGetter

81d1691

feature: Improve management of catch-up threads and prevent gaps in D…

7075b91

…AS history (#20)

refactor: Apply suggestions from code review

feb658d

Co-authored-by: Hlib Kanunnikov <[email protected]>

refactor: add jobsCh channel to DASer and construct it in Start

7b7b90d

refactor: wrap jobs chan write with a select in case of block

fed5e96

test: fix race in DASer restart test

65f04b4

refactor: construct jobs channel once and never close it out

95d65b0

test: remove duplicate wrapping of ds from tests

36b939e

fix(node/services): use proper datastore.Batching as otherwise DASer …

fc8c46e

…is not on the Node

fix: wrap write to jobsCh in daser w select case

b0b8b63

refactor: log only on debug level during catch-up

b4c4b24

refactor: shorten log

cfed6dc

refactor: move catchup scheduler call to Start

a1879f4

refactor: return if failed to get next header inside a catchUp routine

378ff97

fix | refactor: ensure checkpoint storing isn't racey, one catchUpJob…

c01a057

… at a time (#24)

renaynay force-pushed the dasing-of-past-headers branch from 8dd3c00 to c01a057 Compare March 26, 2022 12:41

Wondertan reviewed Mar 27, 2022

View reviewed changes

das/daser.go Outdated Show resolved Hide resolved

das/daser.go Outdated Show resolved Hide resolved

das/daser.go Outdated Show resolved Hide resolved

das/daser.go Outdated Show resolved Hide resolved

das/daser.go Show resolved Hide resolved

fix: addressing hlibs comments

ad2975d

Wondertan approved these changes Mar 28, 2022

View reviewed changes

liamsi reviewed Mar 28, 2022

View reviewed changes

das/checkpoint_store.go Show resolved Hide resolved

das/checkpoint_store.go Outdated Show resolved Hide resolved

das/daser.go Show resolved Hide resolved

docs: clarify docs for loadCheckpoint

ffd0bfc

Co-authored-by: Ismail Khoffi <[email protected]>

liamsi approved these changes Mar 28, 2022

View reviewed changes

renaynay merged commit c5ee765 into celestiaorg:main Mar 28, 2022

renaynay deleted the dasing-of-past-headers branch March 28, 2022 11:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

das: DASing of past headers #473

das: DASing of past headers #473

renaynay commented Feb 23, 2022 •

edited

Loading

liamsi left a comment

renaynay commented Mar 7, 2022

Wondertan left a comment

Wondertan left a comment •

edited

Loading

adlerjohn commented Mar 17, 2022

Wondertan commented Mar 21, 2022

Wondertan left a comment

Wondertan left a comment

liamsi left a comment

renaynay commented Mar 28, 2022

liamsi left a comment

das: DASing of past headers #473

das: DASing of past headers #473

Conversation

renaynay commented Feb 23, 2022 • edited Loading

TODO

liamsi left a comment

Choose a reason for hiding this comment

renaynay commented Mar 7, 2022

Wondertan left a comment

Choose a reason for hiding this comment

Wondertan left a comment • edited Loading

Choose a reason for hiding this comment

adlerjohn commented Mar 17, 2022

Wondertan commented Mar 21, 2022

Wondertan left a comment

Choose a reason for hiding this comment

Wondertan left a comment

Choose a reason for hiding this comment

liamsi left a comment

Choose a reason for hiding this comment

renaynay commented Mar 28, 2022

liamsi left a comment

Choose a reason for hiding this comment

renaynay commented Feb 23, 2022 •

edited

Loading

Wondertan left a comment •

edited

Loading