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

[sovereign] Do not request incoming headers upon receiving #6585

Conversation

mariusmihaic
Copy link
Contributor

@mariusmihaic mariusmihaic commented Nov 5, 2024

Reasoning behind the pull request

  • This PR addresses an "edge case" issue that arises when a sovereign chain’s round duration is greater than the main chain’s and incoming headers are quickly added to the pool. Due to go routines notifying the pool and process handlers asynchronously, it’s possible for an extended header with nonce X+2 to be added before one with nonce X+1.
  • When the function doJobOnReceivedCrossNotarizedHeader receives a new main chain header, it notifies all relevant handlers via crossBlockNotifier, which then calls ComputeLongestChain. If a header with nonce X+2 is processed first, unnecessary network requests are made to receive headers with nonce X+1.

Proposed changes

  • In normal processing chain code, if we have no registered handler inside crossBlockNotifier,we return early
  • We also have no registered handler, so returned earlier .

Testing procedure

Pre-requisites

Based on the Contributing Guidelines the PR author and the reviewers must check the following requirements are met:

  • was the PR targeted to the correct branch?
  • if this is a larger feature that probably needs more than one PR, is there a feat branch created?
  • if this is a feat branch merging, do all satellite projects have a proper tag inside go.mod?

@mariusmihaic mariusmihaic self-assigned this Nov 5, 2024
@mariusmihaic mariusmihaic marked this pull request as ready for review November 5, 2024 15:24
@sasurobert sasurobert self-requested a review November 6, 2024 09:17
sasurobert
sasurobert previously approved these changes Nov 6, 2024
axenteoctavian
axenteoctavian previously approved these changes Nov 6, 2024
)

// GetCurrentSovereignHeader returns current sovereign chain block handler from blockchain hook
func GetCurrentSovereignHeader(nodeHandler process.NodeHandler) data.SovereignChainHeaderHandler {
return nodeHandler.GetChainHandler().GetCurrentBlockHeader().(data.SovereignChainHeaderHandler)
}

// CreateSovereignRunTypeComponents will create sovereign run type components
func CreateSovereignRunTypeComponents(args runType.ArgsRunTypeComponents, sovereignExtraConfig config.SovereignConfig) (factory.RunTypeComponentsHolder, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The cleanest solution would be to move this function somewhere in /sovereignnode/runtype and use it in both chainSimualtor and sovereignNodeRunner

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not quite same usage, just verified.
Will leave it as it is

Base automatically changed from MX-15961-sov-node-trust-mode to feat/sovereign-mainchain-header-sync November 8, 2024 10:36
@mariusmihaic mariusmihaic dismissed stale reviews from sasurobert and axenteoctavian November 8, 2024 10:36

The base branch was changed.

@mariusmihaic mariusmihaic merged commit a9dcce7 into feat/sovereign-mainchain-header-sync Nov 8, 2024
4 checks passed
@mariusmihaic mariusmihaic deleted the MX-15984-fix-request-incoming-header branch November 8, 2024 11:31
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.

3 participants