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

fix(da): full-nodes can sync from p2p while DA light client is down or out of sync #857

Merged
merged 22 commits into from
May 18, 2024

Conversation

srene
Copy link
Contributor

@srene srene commented May 13, 2024

PR Standards

The main objective of this PR is the startup process for Celestia DA for full-nodes. With the modifications, full nodes will not wait for a DA to be in sync (or up) and it will do the syncing process in the background, while the full-node can start receiving and caching gossiped blocks from P2P. It also solves a nil pointer error that was happening when DA light finished syncing and a full-node was started when was not still in sync. Without the modifications of this PR, a node could start receiving gossiped blocks, but everything received while DA was syncing was disregarded because the block manager was not started, waiting for the celestia light node to finish the syncing process.

Opening a pull request should be able to meet the following requirements


Close #856 , #799 , #770

<-- Briefly describe the content of this pull request -->

For Author:

  • Targeted PR against correct branch
  • included the correct type prefix in the PR title
  • Linked to Github issue with discussion and accepted design
  • Targets only one github issue
  • Wrote unit and integration tests
  • All CI checks have passed
  • Added relevant godoc comments

For Reviewer:

  • confirmed the correct type prefix in the PR title
  • Reviewers assigned
  • confirmed all author checklist items have been addressed

After reviewer approval:

  • In case targets main branch, PR should be squashed and merged.
  • In case PR targets a release branch, PR should be rebased.

@srene srene requested a review from a team as a code owner May 13, 2024 15:25
@srene srene self-assigned this May 13, 2024
@srene srene marked this pull request as draft May 13, 2024 15:26
@srene srene marked this pull request as ready for review May 14, 2024 08:51
@srene srene linked an issue May 15, 2024 that may be closed by this pull request
@danwt
Copy link
Contributor

danwt commented May 15, 2024

conflicts @srene

@danwt
Copy link
Contributor

danwt commented May 15, 2024

btw, solves #770?

@srene
Copy link
Contributor Author

srene commented May 15, 2024

btw, solves #770?

yes, the problem was coming from da stuck in the starting process when celestia was syncing meanwhile the node was starting to get batches. I opened a new issue for that, so i think we can close 770

@danwt
Copy link
Contributor

danwt commented May 15, 2024

Please do gofumpt -w . to format

@srene srene force-pushed the srene/856-fullnode-dontwait-da branch from 83fcec0 to 50bec52 Compare May 15, 2024 14:38
@srene srene force-pushed the srene/856-fullnode-dontwait-da branch from 22f0caf to 09d08ae Compare May 15, 2024 20:25
@srene srene marked this pull request as draft May 16, 2024 09:21
@srene srene marked this pull request as ready for review May 16, 2024 09:33
@srene srene requested review from danwt and mtsitrin May 16, 2024 09:33
Copy link
Contributor

@danwt danwt left a comment

Choose a reason for hiding this comment

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

looks good just some nits

@srene srene requested a review from danwt May 16, 2024 13:01
@srene srene requested a review from mtsitrin May 16, 2024 21:02
@srene srene force-pushed the srene/856-fullnode-dontwait-da branch from d020e95 to 31ab15b Compare May 17, 2024 09:07
Comment on lines +612 to +614
go func() {
done <- rpc.Header.SyncWait(c.ctx)
}()

Check notice

Code scanning / CodeQL

Spawning a Go routine Note

Spawning a Go routine may be a possible source of non-determinism
}

c.logger.Info("Celestia-node is synced.", "height", state.ToHeight)
go c.sync(rpc)

Check notice

Code scanning / CodeQL

Spawning a Go routine Note

Spawning a Go routine may be a possible source of non-determinism
@srene srene requested a review from omritoptix May 18, 2024 08:56
@omritoptix omritoptix merged commit 2b5eb07 into main May 18, 2024
6 checks passed
@omritoptix omritoptix deleted the srene/856-fullnode-dontwait-da branch May 18, 2024 09:04
omritoptix pushed a commit that referenced this pull request May 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

fix(da): full-nodes should not wait for da to start syncing celestia da nil pointer error
4 participants