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: use last finalized block on startup #3737

Merged
merged 5 commits into from
Feb 12, 2024

Conversation

jimjbrettj
Copy link
Contributor

Changes

  • When starting the syncing service we should be using the last finalized block that we have, not the best block that we have. This PR makes this change.
  • After startup we are okay to go back to using the best block

Tests

go test -tags integration github.com/ChainSafe/gossamer

Issues

Primary Reviewer

@EclesioMeloJunior

@jimjbrettj jimjbrettj added T-bug this issue covers unexpected and/or wrong behaviour. S-sync-paseo related to particular network syncing. labels Feb 2, 2024
Copy link

codecov bot commented Feb 2, 2024

Codecov Report

Attention: 7 lines in your changes are missing coverage. Please review.

Comparison is base (d1ca7aa) 50.51% compared to head (53a8579) 50.23%.
Report is 6 commits behind head on development.

❗ Current head 53a8579 differs from pull request most recent head 2d1561e. Consider uploading reports for the commit 2d1561e to get more accurate results

Additional details and impacted files
@@               Coverage Diff               @@
##           development    #3737      +/-   ##
===============================================
- Coverage        50.51%   50.23%   -0.29%     
===============================================
  Files              230      230              
  Lines            29006    29003       -3     
===============================================
- Hits             14653    14570      -83     
- Misses           12856    12935      +79     
- Partials          1497     1498       +1     

Copy link
Member

@EclesioMeloJunior EclesioMeloJunior left a comment

Choose a reason for hiding this comment

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

Instead of using this flag property (isStartup) I would suggest to: inside the bootstrapSync method inserting the call to GetHighestFinalisedHeader before starting the for loop, which means that the process will start using the latest finalized header.

Also, I've suggested to transform currentSyncInformation method into isBootstrapSync where it doesn't need to return the block but only a bool indicating if we keep on bootstrap or change

func (cs *chainSync) bootstrapSync() {
  currentBlock, err := cs.blockState.GetHighestFinalisedHeader()
  if err != nil {
    panic("cannot find highest finalised header")
  }

  for {
    ...
    isBootstrap := cs.isBootstrapSync(currentBlock)
    
    if isBootstrap {
      cs.workerPool.useConnectedPeers()
      err = cs.requestMaxBlocksFrom(currentBlock, networkInitialSync)
      if err != nil {
        logger.Errorf("requesting max blocks from best block header: %s", err)
      }

      currentBlock, err = cs.blockState.BestBlockHeader()
      if err != nil {
        return nil, false, fmt.Errorf("getting best block header: %w", err)
      }
    } else {
      ...
    }
  }
}

func (cs *chainSync) isBootstrapSync(syncHeader *types.Header) bool {
  syncTarget := cs.peerViewSet.getTarget()
  return syncHeader.Number+network.MaxBlocksInResponse < syncTarget
}

Copy link
Member

@EclesioMeloJunior EclesioMeloJunior left a comment

Choose a reason for hiding this comment

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

LGTM!

@jimjbrettj jimjbrettj force-pushed the jimmy/useLastFinalizedOnStartup branch from 53a8579 to bb167b6 Compare February 12, 2024 18:57
@jimjbrettj jimjbrettj force-pushed the jimmy/useLastFinalizedOnStartup branch from bb167b6 to 2d1561e Compare February 12, 2024 19:08
@jimjbrettj jimjbrettj merged commit c262642 into development Feb 12, 2024
22 of 23 checks passed
@jimjbrettj jimjbrettj deleted the jimmy/useLastFinalizedOnStartup branch February 12, 2024 19:45
github-actions bot pushed a commit that referenced this pull request Mar 1, 2024
# [0.9.0](v0.8.0...v0.9.0) (2024-3-1)

### Bug Fixes

* add a limit of number of bytes while scale decoding a slice ([#3733](#3733)) ([5edbf89](5edbf89))
* **docs:** Fixing link to polkadot runtime fundamentals to the right one ([#3763](#3763)) ([a785d32](a785d32))
* don't panic if we fail to convert hex to bytes ([#3734](#3734)) ([12234de](12234de))
* **dot/sync:** execute p2p handshake when there is no target ([#3695](#3695)) ([a9db0ec](a9db0ec))
* fix index out of range undeterministic error in rpc test ([#3718](#3718)) ([d099384](d099384))
* fix non deterministic  panic during TestStableNetworkRPC integration test ([#3756](#3756)) ([ee3d243](ee3d243))
* **lib/trie:** use `MustBeHashed` for V1 trie nodes with larger storage values ([#3739](#3739)) ([f5e48a9](f5e48a9))
* **mocks:** Set fixed version for uber mockgen in CI ([#3656](#3656)) ([ea9877e](ea9877e))
* **runtime/storage:** support nested storage transactions ([#3670](#3670)) ([3e99f6d](3e99f6d))
* segfault on node restart ([#3736](#3736)) ([d1ca7aa](d1ca7aa))
* **state-version:** should be uint8 instead of uint32 ([#3779](#3779)) ([c8fdb14](c8fdb14))
* update paseo chain spec ([#3770](#3770)) ([6a54f28](6a54f28))
* use last finalized block on startup ([#3737](#3737)) ([c262642](c262642))

### Features

* **config:** dynamically set version based on environment ([#3693](#3693)) ([5c534c9](5c534c9))
* **staging:** Expose RPC on Westend Staging Node ([#3687](#3687)) ([c374eaa](c374eaa))
* **tests/scripts:** create script to retrieve trie state via rpc ([#3714](#3714)) ([5ccea40](5ccea40))
Copy link

github-actions bot commented Mar 1, 2024

🎉 This PR is included in version 0.9.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

timwu20 pushed a commit that referenced this pull request Apr 19, 2024
# [0.9.0](v0.8.0...v0.9.0) (2024-3-1)

### Bug Fixes

* add a limit of number of bytes while scale decoding a slice ([#3733](#3733)) ([5edbf89](5edbf89))
* **docs:** Fixing link to polkadot runtime fundamentals to the right one ([#3763](#3763)) ([a785d32](a785d32))
* don't panic if we fail to convert hex to bytes ([#3734](#3734)) ([12234de](12234de))
* **dot/sync:** execute p2p handshake when there is no target ([#3695](#3695)) ([a9db0ec](a9db0ec))
* fix index out of range undeterministic error in rpc test ([#3718](#3718)) ([d099384](d099384))
* fix non deterministic  panic during TestStableNetworkRPC integration test ([#3756](#3756)) ([ee3d243](ee3d243))
* **lib/trie:** use `MustBeHashed` for V1 trie nodes with larger storage values ([#3739](#3739)) ([f5e48a9](f5e48a9))
* **mocks:** Set fixed version for uber mockgen in CI ([#3656](#3656)) ([ea9877e](ea9877e))
* **runtime/storage:** support nested storage transactions ([#3670](#3670)) ([3e99f6d](3e99f6d))
* segfault on node restart ([#3736](#3736)) ([d1ca7aa](d1ca7aa))
* **state-version:** should be uint8 instead of uint32 ([#3779](#3779)) ([c8fdb14](c8fdb14))
* update paseo chain spec ([#3770](#3770)) ([6a54f28](6a54f28))
* use last finalized block on startup ([#3737](#3737)) ([c262642](c262642))

### Features

* **config:** dynamically set version based on environment ([#3693](#3693)) ([5c534c9](5c534c9))
* **staging:** Expose RPC on Westend Staging Node ([#3687](#3687)) ([c374eaa](c374eaa))
* **tests/scripts:** create script to retrieve trie state via rpc ([#3714](#3714)) ([5ccea40](5ccea40))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-sync-paseo related to particular network syncing. T-bug this issue covers unexpected and/or wrong behaviour.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants