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

feat(l2 fork): Handle fork #1199

Closed
wants to merge 39 commits into from

Conversation

faultytolly
Copy link
Contributor

PR Standards

Opening a pull request should be able to meet the following requirements

--

PR naming convention: https://hackmd.io/@nZpxHZ0CT7O5ngTp0TP9mg/HJP_jrm7A


Close #XXX

<-- Briefly describe the content of this pull request -->

For Author:

  • Targeted PR against correct branch
  • included the correct type prefix in the PR title
  • Linked to Github issue with discussion and accepted design
  • Targets only one github issue
  • Wrote unit and integration tests
  • All CI checks have passed
  • Added relevant godoc comments

For Reviewer:

  • confirmed the correct type prefix in the PR title
  • Reviewers assigned
  • confirmed all author checklist items have been addressed

After reviewer approval:

  • In case targets main branch, PR should be squashed and merged.
  • In case PR targets a release branch, PR should be rebased.

@testinginprod testinginprod changed the base branch from main to feat/l2-hard-fork November 5, 2024 21:27
unknown unknown added 4 commits November 5, 2024 22:34
# Conflicts:
#	block/manager.go
#	mocks/github.com/dymensionxyz/dymint/settlement/mock_ClientI.go
…nsition

# Conflicts:
#	block/executor.go
#	block/manager.go
Copy link
Contributor

@srene srene left a comment

Choose a reason for hiding this comment

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

i havent checked the part of upgrade drs, since im not sure yet how it should work. i think there are missing things in the p2p syncing, but i think that could be dealt in a separate pr.

block/fork.go Outdated
panic(fmt.Sprintf("produce empty block: %v", err))
}

err = m.applyBlock(block, commit, types.BlockMetaData{Source: types.Produced})
Copy link
Contributor

Choose a reason for hiding this comment

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

is there any reason to not gossip these two blocks to the p2p?

Copy link
Contributor

Choose a reason for hiding this comment

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

if there is no specific reason to not gossiping the blocks, i would call the already existing ProduceApplyGossipBlock() func.

@faultytolly faultytolly marked this pull request as ready for review November 6, 2024 10:30
@faultytolly faultytolly requested a review from a team as a code owner November 6, 2024 10:30
Copy link
Contributor

@srene srene left a comment

Choose a reason for hiding this comment

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

tests are failing

@mtsitrin mtsitrin changed the base branch from feat/l2-hard-fork to main November 7, 2024 09:13
@mtsitrin mtsitrin mentioned this pull request Nov 7, 2024
12 tasks
}

if m.shouldStopNode(rollapp, lastBlock) {
err = m.createInstruction(rollapp)
Copy link
Contributor

Choose a reason for hiding this comment

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

will be called every LoopInterval = 15 * time.Second until manual reboot?
the freezeNode doesn't freeze this loop

// 2. If the block's app version (equivalent to revision) is less than the rollapp's revision
func (m *Manager) shouldStopNode(rollapp *types.Rollapp, block *types.Block) bool {
revision := block.Header.Version.App
if m.State.Height() >= rollapp.RevisionStartHeight && revision < rollapp.Revision {
Copy link
Contributor

Choose a reason for hiding this comment

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

why we need to check the lastBlock? isn't redundant with m.State.Height()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

where is the last block?

Copy link
Contributor

Choose a reason for hiding this comment

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

block *types.Block in the arg

	lastBlock, err := m.Store.LoadBlock(m.State.Height())
	if err != nil {
		return err
	}

	if m.shouldStopNode(rollapp, lastBlock) {
	```

@@ -23,8 +23,8 @@ import (
const minBlockMaxBytes = 10000

type ExecutorI interface {
CreateBlock(height uint64, lastCommit *types.Commit, lastHeaderHash, nextSeqHash [32]byte, state *types.State, maxBlockDataSizeBytes uint64, enforceEmpty bool) *types.Block
Copy link
Contributor

Choose a reason for hiding this comment

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

enforceEmpty i think it can be removed because not used

) *types.Block {
maxBlockDataSizeBytes = min(maxBlockDataSizeBytes, uint64(max(minBlockMaxBytes, state.ConsensusParams.Block.MaxBytes)))
mempoolTxs := e.mempool.ReapMaxBytesMaxGas(int64(maxBlockDataSizeBytes), state.ConsensusParams.Block.MaxGas)

Copy link
Contributor

Choose a reason for hiding this comment

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

enforceEmpty is always false so it can be removed

block/fork.go Outdated
panic(fmt.Sprintf("produce empty block: %v", err))
}

err = m.applyBlock(block, commit, types.BlockMetaData{Source: types.Produced})
Copy link
Contributor

Choose a reason for hiding this comment

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

if there is no specific reason to not gossiping the blocks, i would call the already existing ProduceApplyGossipBlock() func.

@danwt
Copy link
Contributor

danwt commented Nov 11, 2024

closing in favor of #1212

@danwt danwt closed this Nov 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants