ethereum-optimism / specs

OP Stack Specifications
https://specs.optimism.io
Creative Commons Zero v1.0 Universal
90 stars 85 forks source link

plasma: default commitment type conflicts with derivation version #65

Closed tuxcanfly closed 7 months ago

tuxcanfly commented 7 months ago

Is your feature request related to a problem? Please describe.

Currently the specs for plasma describe the commitment type as 0 for keccak256 commitment of the data.

Commitments are encoded as commitment_type_byte ++ commitment_bytes, where commitment_bytes depends on the commitment_type_byte where [0, 128) are reserved for official implementations:

commitment_type commitment
0 keccak256(tx_payload)

There is small but unfortunate chance of mis-interpreting the commitment type as the derivation version due to both being calldata, for example if a sequencer node enables plasma but disables it later. Since configuration for plasma is part of the --plasma cli flags and not part of rollup.json, the likelihoood of this is high especially during fork migrations.

Describe the solution you'd like

Please consider removing the ambiguity between the derivation version and commitment type by using distinct sentinel bytes.

Describe alternatives you've considered Alternatively, please consider making the plasma config part of the rollup config.

tchardin commented 7 months ago

Hey @tuxcanfly, thanks for the suggestion. We do have a last PR to complete the op-plasma implementation that will add a proper plasma switch as part of system config. Once that is done, the flag will tell derivation whether to interpret the first byte of the tx data as a derivation version or commitment version at a given block. Will make sure to reflect that in the spec.

tynes commented 7 months ago

@tchardin Do you mean "at a given block" or do you mean "for a given block"? My understanding is there is no concept of going between plasma and rollup mode. "At a given block" implies you can switch between the two

@tuxcanfly Do you feel like this is sufficient?

tchardin commented 7 months ago

After chat with @trianglesphere, we're going to go for bumping the batcher tx version byte to 1 and use that to detect whether tx data should be interpreted as plasma commitment and then keep a derivationVersion0 on the input batch to forward to the frame decoder. (Commitment type prefix is kept after the batcher tx type)

tuxcanfly commented 7 months ago

@tchardin there is no batcher tx version right now, so this would be a new addition correct?

tchardin commented 7 months ago

https://github.com/ethereum-optimism/specs/blob/main/specs/protocol/derivation.md#batcher-transaction-format this is a bit ambiguous because it's also called derivationVersion in the implementation so in rollup mode the derivation version byte (0) is the batcher tx version byte too.

tuxcanfly commented 7 months ago

@tchardin actually now that the rollup config includes usePlasma flag I'm not totally sure if this is required. We would need to move the version byte out of the frame txData into the plasma input. The additional byte also adds to the cost of L1 calldata that is submitted very frequently.

tuxcanfly commented 7 months ago

@tchardin would this enable switching between rollup mode and plasma mode if we switch the derivation mode based on the version byte on a frame-by-frame basis?

tuxcanfly commented 7 months ago

We can also enable challenges only for plasma mode by having a pre-condition that the version byte must be non-zero. This also derisks bugs or issues with the smart contract system itself, by having a fallback to the rollup mode.

emilianobonassi commented 7 months ago

@tchardin re: last PR to complete the op-plasma implementation, will get merged before ecotone?

tchardin commented 7 months ago

@tuxcanfly still pending review but plan is using a tx data type byte as in https://github.com/latticexyz/redstone/pull/20/files, this means op plasma chain can still process regular input from L1 DA but not the way around, so a rollup mode chain would skip input commitments.

@emilianobonassi ecotone is already merged. If you mean before ecotone is activated, then yes hopefully will be merged before March 14.

emilianobonassi commented 7 months ago

@tchardin @tuxcanfly i think we can close since this is merged, is that correct?

https://github.com/ethereum-optimism/optimism/pull/9845

trianglesphere commented 7 months ago

Implemented in

https://github.com/ethereum-optimism/optimism/pull/9845 https://github.com/ethereum-optimism/specs/pull/90