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.

Sign up for GitHub

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

Jump to bottom

eth/stagedsync: fixes for mining on devnet #8874

Closed
wants to merge 5 commits into from
Closed

eth/stagedsync: fixes for mining on devnet #8874

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5982292
eth/stagedsync: fixes for mining on devnet
manav2401 Dec 1, 2023
a5ad560
Merge branch 'devel' into manav/mining_span_fix
manav2401 Dec 1, 2023
cc4bcbd
Merge branch 'devel' into manav/mining_span_fix
manav2401 Dec 4, 2023
947307f
Merge branch 'devel' into manav/mining_span_fix
manav2401 Dec 9, 2023
d021f93
Merge branch 'devel' into manav/mining_span_fix
manav2401 Dec 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion eth/stagedsync/chain_reader.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,5 +86,9 @@ func (cr ChainReader) BorEventsByBlock(hash libcommon.Hash, number uint64) []rlp
}

func (cr ChainReader) BorSpan(spanId uint64) []byte {
panic("")
spanBytes, err := cr.BlockReader.Span(context.Background(), cr.Db, spanId)
if err != nil {
log.Error("BorSpan failed", "err", err)
}
return spanBytes
}
44 changes: 21 additions & 23 deletions eth/stagedsync/stage_bor_heimdall.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,33 +142,29 @@ func BorHeimdallForward(
}
}

if mine {
minedHeader := cfg.miningState.MiningBlock.Header

if minedHeadNumber := minedHeader.Number.Uint64(); minedHeadNumber > headNumber {
// Whitelist service is called to check if the bor chain is
// on the cannonical chain according to milestones
if service != nil {
if !service.IsValidChain(minedHeadNumber, []*types.Header{minedHeader}) {
logger.Debug("[BorHeimdall] Verification failed for mined header", "hash", minedHeader.Hash(), "height", minedHeadNumber, "err", err)
dataflow.HeaderDownloadStates.AddChange(minedHeadNumber, dataflow.HeaderInvalidated)
s.state.UnwindTo(minedHeadNumber-1, ForkReset(minedHeader.Hash()))
return fmt.Errorf("mining on a wrong fork %d:%x", minedHeadNumber, minedHeader.Hash())
}
}
} else {
return fmt.Errorf("attempting to mine %d, which is behind current head: %d", minedHeadNumber, headNumber)
}
}
// if mine {
Copy link
Contributor Author

@manav2401 manav2401 Dec 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Need some discussion around this (more details in the PR description - see Issue 3).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can safely skip this checks as the block anyways goes through this stage (with mine = false) as if it's syncing.

Copy link
Contributor

Choose a reason for hiding this comment

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

@manav2401 bor heimdall stage is called with mine=true in the mining stage loop. There are 2 separate loops - syncing and mining. My understanding is that when we are in the mine mode we need to get the header of the mined block from the mining state using cfg.miningState.MiningBlock.Header. However now that the mine block stage has been moved after the bor heimdall stage that means the mined header will not be available? We need the header in order to fetch and persist the state sync events I believe. This makes me think that we would need 2 stages - 1 before the mine block stage to fetch and persist spans and 1 after the mine block to fetch the state sync events. What are your thoughts & is my understanding so far correct?

Copy link
Contributor

Choose a reason for hiding this comment

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

(note I believe currently we are not correctly handling the state sync events for the mine=true case as the mined header is not correctly passed to the fetchAndWriteBorEvents function)

Copy link
Contributor

@taratorio taratorio Dec 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

thinking about it more maybe splitting is not necessary since in iteration N the bor heimdall stage will process state sync events for the block created in iteration N-1 since the header for that block will be available

however if we need to persist state sync events then we may need to fix that bug I mentioned with passing the mined header to fetchAndWriteBorEvents - but this is something that 1) I'm not sure if is a requirement for the block producer (do you happen to know?) and 2) can be done in another PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @taratorio, you're correct that we need header for processing certain things in the bor heimdall stage. I was initially of the view that we'll anyways pass through the same stage when syncing (because when we mine block N, the data like header, bodies, exec results are cached and then it's again sent to all the sync stages where most of them just directly use the cached values and persists the data in DB and perform the remaining operations). That time we do have a header available and that should be sufficient to perform all the validations.

Re. you questions, I don't think we need the whole header looking at the fetchAndWriteBorEvents function. But, I am a bit confused because I didn't check that part of the code and looks like writing these things (not snapshot, but writing in contracts) should happen via bor consensus and not an external bor heimdall stage. At least, that's how it's done in bor. In this function, we could definitely persist the data (i.e. the state sync events) in snapshot and then bor consensus should be able to utilise it when required. Wdyt?

// minedHeader := cfg.miningState.MiningBlock.Header

// if minedHeadNumber := minedHeader.Number.Uint64(); minedHeadNumber > headNumber {
// // Whitelist service is called to check if the bor chain is
// // on the cannonical chain according to milestones
// if service != nil {
// if !service.IsValidChain(minedHeadNumber, []*types.Header{minedHeader}) {
// logger.Debug("[BorHeimdall] Verification failed for mined header", "hash", minedHeader.Hash(), "height", minedHeadNumber, "err", err)
// dataflow.HeaderDownloadStates.AddChange(minedHeadNumber, dataflow.HeaderInvalidated)
// s.state.UnwindTo(minedHeadNumber-1, ForkReset(minedHeader.Hash()))
// return fmt.Errorf("mining on a wrong fork %d:%x", minedHeadNumber, minedHeader.Hash())
// }
// }
// } else {
// return fmt.Errorf("attempting to mine %d, which is behind current head: %d", minedHeadNumber, headNumber)
// }
// }

if err != nil {
return fmt.Errorf("getting headers progress: %w", err)
}

if s.BlockNumber == headNumber {
return nil
}

// Find out the latest event Id
cursor, err := tx.Cursor(kv.BorEvents)
if err != nil {
Expand Down Expand Up @@ -206,12 +202,14 @@ func BorHeimdallForward(
nextSpanId = binary.BigEndian.Uint64(k) + 1
}
snapshotLastSpanId := cfg.blockReader.(LastFrozen).LastFrozenSpanID()
if snapshotLastSpanId+1 > nextSpanId {
if s.BlockNumber != 0 && snapshotLastSpanId+1 > nextSpanId {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is needed because even though we don't have 0th span, the function LastFrozenSpanID will return 0 which makes this check assume that we have 0th span and need the 1st span. This is not the case when we start to mine from scratch in devnets.

nextSpanId = snapshotLastSpanId + 1
}
var endSpanID uint64
if headNumber > zerothSpanEnd {
endSpanID = 2 + (headNumber-zerothSpanEnd)/spanLength
} else {
endSpanID = 1
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Initially, we can simply set this to 1 so that on the first block itself, it fetches both 0th and 1st span.

}

lastBlockNum := s.BlockNumber
Expand Down
18 changes: 9 additions & 9 deletions eth/stagedsync/stagebuilder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,15 +27,6 @@ func MiningStages(
finish MiningFinishCfg,
) []*Stage {
return []*Stage{
{
ID: stages.MiningCreateBlock,
Description: "Mining: construct new block from tx pool",
Forward: func(firstCycle bool, badBlockUnwind bool, s *StageState, u Unwinder, tx kv.RwTx, logger log.Logger) error {
return SpawnMiningCreateBlockStage(s, tx, createBlockCfg, ctx.Done(), logger)
},
Unwind: func(firstCycle bool, u *UnwindState, s *StageState, tx kv.RwTx, logger log.Logger) error { return nil },
Prune: func(firstCycle bool, u *PruneState, tx kv.RwTx, logger log.Logger) error { return nil },
},
{
ID: stages.BorHeimdall,
Description: "Download Bor-specific data from Heimdall",
Expand All @@ -52,6 +43,15 @@ func MiningStages(
return BorHeimdallPrune(p, ctx, tx, borHeimdallCfg)
},
},
{
ID: stages.MiningCreateBlock,
Description: "Mining: construct new block from tx pool",
Forward: func(firstCycle bool, badBlockUnwind bool, s *StageState, u Unwinder, tx kv.RwTx, logger log.Logger) error {
return SpawnMiningCreateBlockStage(s, tx, createBlockCfg, ctx.Done(), logger)
},
Unwind: func(firstCycle bool, u *UnwindState, s *StageState, tx kv.RwTx, logger log.Logger) error { return nil },
Prune: func(firstCycle bool, u *PruneState, tx kv.RwTx, logger log.Logger) error { return nil },
},
{
ID: stages.MiningExecution,
Description: "Mining: execute new block from tx pool",
Expand Down
Loading