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(hardfork): l2 hardfork #1212

Open
wants to merge 93 commits into
base: main
Choose a base branch
from
Open

feat(hardfork): l2 hardfork #1212

wants to merge 93 commits into from

Conversation

srene
Copy link
Contributor

@srene srene commented Nov 9, 2024

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.

@srene srene requested a review from a team as a code owner November 9, 2024 15:11
block/modes.go Outdated

func (m *Manager) subscribeFullNodeEvents(ctx context.Context) {
// Subscribe to new (or finalized) state updates events.
go uevent.MustSubscribe(ctx, m.Pubsub, syncLoop, settlement.EventQueryNewSettlementBatchAccepted, m.onNewStateUpdate, m.logger)

Check notice

Code scanning / CodeQL

Spawning a Go routine

Spawning a Go routine may be a possible source of non-determinism
block/modes.go Outdated
func (m *Manager) subscribeFullNodeEvents(ctx context.Context) {
// Subscribe to new (or finalized) state updates events.
go uevent.MustSubscribe(ctx, m.Pubsub, syncLoop, settlement.EventQueryNewSettlementBatchAccepted, m.onNewStateUpdate, m.logger)
go uevent.MustSubscribe(ctx, m.Pubsub, validateLoop, settlement.EventQueryNewSettlementBatchFinalized, m.onNewStateUpdateFinalized, m.logger)

Check notice

Code scanning / CodeQL

Spawning a Go routine

Spawning a Go routine may be a possible source of non-determinism
go uevent.MustSubscribe(ctx, m.Pubsub, validateLoop, settlement.EventQueryNewSettlementBatchFinalized, m.onNewStateUpdateFinalized, m.logger)

// Subscribe to P2P received blocks events (used for P2P syncing).
go uevent.MustSubscribe(ctx, m.Pubsub, p2pGossipLoop, p2p.EventQueryNewGossipedBlock, m.OnReceivedBlock, m.logger)

Check notice

Code scanning / CodeQL

Spawning a Go routine

Spawning a Go routine may be a possible source of non-determinism

// Subscribe to P2P received blocks events (used for P2P syncing).
go uevent.MustSubscribe(ctx, m.Pubsub, p2pGossipLoop, p2p.EventQueryNewGossipedBlock, m.OnReceivedBlock, m.logger)
go uevent.MustSubscribe(ctx, m.Pubsub, p2pBlocksyncLoop, p2p.EventQueryNewBlockSyncBlock, m.OnReceivedBlock, m.logger)

Check notice

Code scanning / CodeQL

Spawning a Go routine

Spawning a Go routine may be a possible source of non-determinism
@srene srene marked this pull request as draft November 9, 2024 15:52
@srene srene marked this pull request as ready for review November 9, 2024 17:28
block/fork.go Fixed Show fixed Hide fixed
proto/types/dymint/state.proto Outdated Show resolved Hide resolved
types/pb/dymensionxyz/dymension/rollapp/query.pb.go Outdated Show resolved Hide resolved
types/validation.go Outdated Show resolved Hide resolved
block/fork.go Show resolved Hide resolved
@srene
Copy link
Contributor Author

srene commented Nov 13, 2024

  1. All the frozen stuff should be done with context cancellation, rather than adhoc
  2. What is the purpose of frozen compared to shutting down the node? I assume it's to avoid looping daemon restarts. Can you document this?
  3. Please encapsulate block.header.version.app in one place and everywhere call it by revision
  4. Revision should go on the batch type. There shouldn't be a need to change method signatures to include revision.
  5. What is the semantic difference between the revision on the state.version, vs the revision on the state of the last block?
  6. Let's ditch state.Version. The only relevant thing is the block version. Revision should be first class concept.
  7. Block ID should be <height,revision> pair, but everything is still stored using just height. Maybe fine for now but I think down the road this is going to make things harder to reason about.
  8. I don't see what is stopping the full node from using a different DRS than the one the proposer actually uses for the new revision?
  9. Do we need to document all the fields/state that need to be set correctly by rollback assumption? Where is the information about the actual rollback?

i think we addressed everything but there are things i dont think need to be changed now. ill add issues for what is ncessary:

  • state.version stays the same because it is not something added or modified in this pr and change it now may require a dymint refactor that i think should not be part of this pr.
  • as we discussed before, nothing prevents the fullnode to start with a wrong version after rollback, but after applying the first block will fail and it will require rolling back again. that it could be maybe improved in the way we validate rollappparams or but imo is out of scope of this pr.
  • consensus msgs for upgrade will be sent in all cases and the rdk will decide what to do based on current rollapp params status. this avoids overloading the state or adding the previous drs in the instruction, which could lead to errors.

block/fork.go Outdated
Comment on lines 39 to 40
if err := m.checkForkUpdate(ctx); err != nil {
continue
Copy link
Contributor

Choose a reason for hiding this comment

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

dont you want error here?

block/fork.go Outdated
Comment on lines 24 to 42
if !types.InstructionExists(m.RootDir) {
err := m.checkForkUpdate(ctx)
if err != nil {
return err
}
}

ticker := time.NewTicker(LoopInterval) // TODO make this configurable
defer ticker.Stop()

for {
select {
case <-ctx.Done():
return nil
case <-ticker.C:
if err := m.checkForkUpdate(ctx); err != nil {
continue
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

can dry out and put checkForUpdate at top of for loop?

block/fork.go Outdated
Comment on lines 162 to 167
// Block Creation Rules:
// - Two blocks are considered in this process:
// - First block: Contains consensus messages for the fork
// - Second block: Should be empty (no messages or transactions)
// - Total height increase should be 2 blocks from RevisionStartHeight
func (m *Manager) handleCreationOfForkBlocks(instruction types.Instruction, consensusMsgs []proto.Message) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

just call it create fork blocks?

block/fork.go Outdated
if err := m.validateExistingBlocks(instruction, 1); err != nil {
return err
}
return m.createNewBlocks(consensusMsgs, 1) // Create only the second block
Copy link
Contributor

Choose a reason for hiding this comment

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

why does second block have cons message?

block/fork.go Outdated
Comment on lines 240 to 250
// Create first block with consensus messages if blocksToCreate == 2
if blocksToCreate == 2 {
if _, _, err := m.ProduceApplyGossipBlock(context.Background(), true); err != nil {
return fmt.Errorf("producing first block: %v", err)
}
}

// Create second empty block for both cases (blocksToCreate == 1 or 2)
if _, _, err := m.ProduceApplyGossipBlock(context.Background(), true); err != nil {
return fmt.Errorf("producing second block: %v", err)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

it's allow empty, it should be forced empty

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.

5 participants