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

Karlb/stop #2330

Merged
merged 24 commits into from
Sep 19, 2024
Merged

Karlb/stop #2330

merged 24 commits into from
Sep 19, 2024

Conversation

karlb
Copy link
Contributor

@karlb karlb commented Sep 17, 2024

Try alternative approaches to #2322

Copy link

github-actions bot commented Sep 17, 2024

Coverage from tests in ./e2e_test/... for ./consensus/istanbul/... at commit ff274ca

coverage: 54.8% of statements across all listed packages
coverage:  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

Copy link

github-actions bot commented Sep 17, 2024

5889 passed, 45 skipped

miner/worker.go Outdated
Comment on lines 404 to 410
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
}
Copy link
Contributor Author

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)?

Copy link
Contributor

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.

Copy link
Contributor Author

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!

@karlb karlb marked this pull request as ready for review September 18, 2024 11:02
Copy link
Contributor

@piersy piersy left a comment

Choose a reason for hiding this comment

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

Well, I approve, but these are partly my changes so maybe @palango or @karlb could take a look

Copy link
Contributor Author

@karlb karlb left a 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.

@mcortesi
Copy link
Contributor

So @piersy @karlb the approach here is just to change Engine.verifyHeader() to fail if header is for a L2 block?

Does this affect both flows:

  1. Syncing flow: No full node will accept a block of L2
  2. Consensus Flow: There won't be consensus on a block of L2

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"?

@piersy
Copy link
Contributor

piersy commented Sep 18, 2024

Hey @mcortesi

here is just to change Engine.verifyHeader() to fail if header is for a L2 block?

Yes.

Does this affect both flows:

Syncing flow: No full node will accept a block of L2
Consensus Flow: There won't be consensus on a block of L2

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.

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"?

Yep, but probably not that "busy" because the istanbul timeouts will increase exponentially.

@piersy piersy merged commit ff274ca into master Sep 19, 2024
27 checks passed
@piersy piersy deleted the karlb/stop branch September 19, 2024 10:32
@alecps
Copy link
Contributor

alecps commented Sep 19, 2024

This looks great! Much cleaner than #2322. Thanks @karlb @piersy

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.

4 participants