Sovereign-Labs / sovereign-sdk

A framework for building seamlessly scalable and interoperable rollups that can run on any blockchain
https://sovereign.xyz
Apache License 2.0
366 stars 104 forks source link

Are both `rollup_config.toml` and `rollup_config` crate needed? #684

Open LiveDuo opened 1 year ago

LiveDuo commented 1 year ago

Right now there are two configs in the examples folder. There's const-rollup-config and rollup_config.toml.

Is there a conceptual distinction in between the two configs? If not, can const-rollup-config refactored into rollup_config.toml for better clarity and to avoid hardcoding ROLLUP_NAMESPACE_RAW and SEQUENCER_DA_ADDRESS?

preston-evans98 commented 1 year ago

Hey @LiveDuo! Great question - yes both configs are needed. The const-rollup-config crate is required at compile time, and sets configuration that needs to be baked into the rollup's underlying zk circuit. Any changes to this config are equivalent to a hard fork of the rollup. On the other hand, the rollup_config.toml file has runtime params that are only relevant to one particular full node at runtime.

preston-evans98 commented 1 year ago

cc @citizen-stig, maybe we can rename const-rollup-config to consensus-params or something? And maybe rollup_config.toml would be clearer as node-config.toml?

This will be partially addressed by #316

LiveDuo commented 1 year ago

@preston-evans98 Thanks for clearing it out.

One thing that does not make complete sense yet is: if da_sequencer_address is required in const-rollup-config and whether a feature like https://github.com/Sovereign-Labs/sovereign-sdk/issues/408 would remove the hardcoded dependency on the sequencer address?

citizen-stig commented 1 year ago

@preston-evans98 I thinks it is good idea to rename those to match their actual usage!

preston-evans98 commented 1 year ago

@LiveDuo, good question! Unforunately, #408 on its own won't remove the need for a hardcoded initial sequencer. For DOS protection, we require sequencers to stake some tokens before we begin processing their blobs, so you need at least one sequencer to be registered at genesis.

We do have a long-term plan for removing this restriction, but it isn't currently a priority. If you need that feature, let me know and we'll bump it up on the roadmap.

For context, the rough idea of what we'll do is process a limited number of blobs from unregistered sequencers in each DA layer block, with the requirement that each of these blobs must begin with a transaction which registers the sequencer in order for the rest of the blob to be processed. This puts an upper bound on the amount of "wasted" work that the rollup will have to do if someone malicious tries to spam the chain, but still gives us a fallback mechanism to recover if all of the registered sequencers are unavailable.

LiveDuo commented 1 year ago

@preston-evans98 I kind of missed the DOS attack vector.

with the requirement that each of these blobs must begin with a transaction which registers the sequencer in order for the rest of the blob to be processed

It seems that this restriction might overlap with forced transactions (ie. submitted directly to DA) as they don't come from a registered sequencer either. My view is that unregistered sequencers are more important than forced transactions as they are still permission-less (with the require a bond) but provide a better user experience. I think users would value more the fallback unregistered sequencers provide but very few would go through the process of submitting the transaction to the DA manually.

If you need that feature, let me know and we'll bump it up on the roadmap.

As of right now, these specific features are just "good to have". The important bit is understanding the overall architecture and how the different pieces fit together.