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

Simplify code to only apply blocks from cache #701

Open
danwt opened this issue Apr 17, 2024 · 3 comments
Open

Simplify code to only apply blocks from cache #701

danwt opened this issue Apr 17, 2024 · 3 comments

Comments

@danwt
Copy link
Contributor

danwt commented Apr 17, 2024

We have unnecessary branching in our code because we, currently

  1. On receiving a gossiped block, we apply it or add it to cache (

    dymint/block/manager.go

    Lines 242 to 253 in 7c19785

    // if height is expected, apply
    // if height is higher than expected (future block), cache
    if block.Header.Height == m.store.NextHeight() {
    err := m.applyBlock(context.Background(), &block, &commit, blockMetaData{source: gossipedBlock})
    if err != nil {
    m.logger.Error("apply gossiped block", "err", err)
    }
    } else if block.Header.Height > m.store.NextHeight() {
    m.prevBlock[block.Header.Height] = &block
    m.prevCommit[block.Header.Height] = &commit
    m.logger.Debug("Caching block", "block height", block.Header.Height, "store height", m.store.Height())
    }
    )
  2. On syncing from DA, we apply it or add it to cache

    dymint/block/retriever.go

    Lines 88 to 100 in df192a9

    for _, batch := range batchResp.Batches {
    for i, block := range batch.Blocks {
    if block.Header.Height != m.store.NextHeight() {
    continue
    }
    err := m.applyBlock(ctx, block, batch.Commits[i], blockMetaData{source: daBlock, daHeight: daMetaData.Height})
    if err != nil {
    return err
    }
    }
    }
    err := m.attemptApplyCachedBlocks(ctx)

Instead, we should just always add blocks to the cache, and then apply from the cache
We should remove the path to apply directly
It will be less code and more maintainable

@mtsitrin
Copy link
Contributor

handled by #700

@danwt
Copy link
Contributor Author

danwt commented May 1, 2024

Oh yeah, that's a nice improvement in 700 👍
I think an ever better next step will be to only have 1 caller of apply cached blocks

syncer -> cache
gossip -> cache
some third party: cache -> apply

@danwt
Copy link
Contributor Author

danwt commented Jul 17, 2024

something like this can also work

I think it would be good if whenever we get a block, we send it to channel
and there is a goroutine to listen on this channel and apply to cache, and he will apply the blocks when the cache has the next one needed
this way we get rid of all mutex related to cache access and simplify the flow
ie. there should only be one applyBlock call in the code

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

No branches or pull requests

2 participants