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

Simplify Octane EventLogs #2106

Open
corverroos opened this issue Oct 9, 2024 · 2 comments
Open

Simplify Octane EventLogs #2106

corverroos opened this issue Oct 9, 2024 · 2 comments
Labels
network-upgrade Resolving this issue breaks consensus

Comments

@corverroos
Copy link
Collaborator

corverroos commented Oct 9, 2024

Problem to Solve

Currently the Octane MsgExecutionPayload contains both the new payload/block being proposed/added, as well as the previous block event logs.

type MsgExecutionPayload struct {
	ExecutionPayload  []byte     
	PrevPayloadEvents []*EVMEvent 
}

@chmllr indicated that this could actually be simplified. EVMEvents are deterministic given a finalized block. They can simply be queried after ForkChoiceUpdated in FinalizeBlock and provided to the EVMEventProcessors.

This reduces the size of consensus blocks. It also simplifies the code.
This also avoids the 1 block lag, with execution event logs being applied in the same consensus block.

Note that querying EVM EventLogs is generally only possible for finalized blocks (post ForkchoiceUpdate). So this would mean that verifying event logs during ProcessProposal wouldn't be possible. But since it is deterministic and event logs cannot be modified or blocked or omitted, they all need to be processed in anycase. Failing if they are not valid for some reason. So this ins't a problem I think.

Note this would require a network upgrade.

Proposed Solution

  • Remove PrevPayloadEvents from MsgExecutionPayload.
  • Don't generate them during PrepareProposal and don't verify them during ProcessProposal.
  • During FinalizeBlock in EVMEngine msgServer.ExecutionPayload, after calling ForkchoiceUpdatedV3
  • Fetch the newly finalized block evm events via s.evmEvents and pass those to s.deliverEvents
  • Ensure events are processing in correct order

Also take the opportunity to improve the EVMEventProcessor interface:

type EvmEventProcessor interface {
	// Name returns the name of the processor.
	Name() string
	// FilterParams returns the addresses and topics to filter events for.
	FilterParams() ([]common.Address, [][]common.Hash)
	// Deliver processes the EVM log event.
	Deliver(ctx context.Context, blockHash common.Hash, log *EVMEvent) error
}

Note that on the network upgrade height itself, we should process both legacy previous block events as well as current block events. Otherwise we will miss events for upgrade height-1.

@corverroos corverroos added the network-upgrade Resolving this issue breaks consensus label Oct 9, 2024
@corverroos
Copy link
Collaborator Author

This a known optimisation at this point, we haven't planned it on the Omni roadmap as the advantages are marginal.

@corverroos
Copy link
Collaborator Author

Note that this should fix the issue raised by story

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
network-upgrade Resolving this issue breaks consensus
Projects
None yet
Development

No branches or pull requests

2 participants