-
Notifications
You must be signed in to change notification settings - Fork 200
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
Karlb/stop #2330
Karlb/stop #2330
Conversation
…h command, fixes forkid issue
… logic to shut down entire node by listening to chain head events
Coverage from tests in coverage: 54.8% of statements across all listed packagescoverage: 68.2% of statements in consensus/istanbul coverage: 63.3% of statements in consensus/istanbul/announce coverage: 56.9% of statements in consensus/istanbul/backend coverage: 0.0% of statements in consensus/istanbul/backend/backendtest coverage: 24.3% of statements in consensus/istanbul/backend/internal/replica coverage: 65.4% of statements in consensus/istanbul/core coverage: 50.0% of statements in consensus/istanbul/db coverage: 0.0% of statements in consensus/istanbul/proxy coverage: 64.2% of statements in consensus/istanbul/uptime coverage: 52.4% of statements in consensus/istanbul/validator coverage: 79.2% of statements in consensus/istanbul/validator/random |
miner/worker.go
Outdated
if w.chainConfig.IsL2Migration(nextBlockNum) { | ||
if w.isRunning() { | ||
log.Info("The next block is the L2 migration block, stopping block construction", "currentBlock", w.chain.CurrentBlock().NumberU64(), "hash", w.chain.CurrentBlock().Hash(), "nextBlock", nextBlockNum.Uint64()) | ||
w.stop() | ||
} | ||
return | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With this removed, could the local node produce blocks after the migration? Or are those also checked by the block fetcher header validity check?
We also wouldn't stop the worker anymore. Is that a problem (resource usage, irritating logs or rpc responses)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the local node can produce blocks any more, because if you look at consensus we can see that sendPreprepareV2
is the point at which the proposer sends the initial preprepare message.
func (c *core) sendPreprepareV2(request *istanbul.Request, roundChangeCertificateV2 istanbul.RoundChangeCertificateV2) {
logger := c.newLogger("func", "sendPreprepareV2")
// If I'm the proposer and I have the same sequence with the proposal
if c.current.Sequence().Cmp(request.Proposal.Number()) == 0 && c.isProposer() {
m := istanbul.NewPreprepareV2Message(&istanbul.PreprepareV2{
View: c.current.View(),
Proposal: request.Proposal,
RoundChangeCertificateV2: roundChangeCertificateV2,
}, c.address)
logger.Debug("Sending preprepareV2", "m", m)
c.broadcast(m)
}
}
If you dig down into the broadcast
function you can see that the preprepare message eventually gets sent back to the proposer, for it to handle in the same way as all the other messages eventually arriving at handlePreprepareV2
which calls verifyProposal
on the block.
So to summarise consensus nodes, including proposers would not be able to progress past the preprepare phase of consensus if the block does not pass validation, and therefore not be able to produce new blocks.
We also wouldn't stop the worker anymore. Is that a problem (resource usage, irritating logs or rpc responses)?
I think that's a benefit, because we don't end up messing about with the internal stopping mechanisms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, thanks for the explanation!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. The test seems to cover the important cases (and has proven useful when trying different approaches) and the actual changes (ignoring the flag passing) are really small.
So @piersy @karlb the approach here is just to change Does this affect both flows:
And for (2). What happens instead? Proposer keep trying to produce blocks and failing? Then comes a new proposer, and so on... is this some kind of "busy wait"? |
Hey @mcortesi
Yes.
As far as I can see this would prevent any fetching of L2 blocks and and from my previous investigation it seems that it also prevents creation of L2 blocks.
Yep, but probably not that "busy" because the istanbul timeouts will increase exponentially. |
Try alternative approaches to #2322