omni-network / omni

Monorepo for Omni node, contracts and other related tools
https://omni.network
GNU General Public License v3.0
88 stars 54 forks source link

Simplify Octane EventLogs #2106

Open corverroos opened 3 weeks ago

corverroos commented 3 weeks ago

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

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 commented 3 weeks ago

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

corverroos commented 3 days ago

Note that this should fix the issue raised by story