feat: set initial fcs from db #335
Conversation
frisitano
left a comment
frisitano
left a comment
There was a problem hiding this comment.
Left a comment inline.
crates/engine/src/fcs.rs
Outdated
| /// Creates a [`ForkchoiceState`] instance setting the `head`, `safe` and `finalized` hash to | ||
| /// the appropriate starting values by reading from the database. | ||
| pub async fn from_db<DB: DatabaseTransactionProvider>(db: DB) -> Option<Self> { | ||
| // TODO: important to add retry for these operations. | ||
| let tx = db.tx().await.ok()?; | ||
| let latest_l2_block = tx.get_l2_blocks().await.ok()?.next().await?.ok()?; | ||
| let (latest_safe_block, _) = tx.get_latest_safe_l2_info().await.ok()??; | ||
| Some(Self { head: latest_l2_block, safe: latest_safe_block, finalized: latest_safe_block }) | ||
| } |
There was a problem hiding this comment.
Is it also suitable to set the safe and finalized heads using the database? I wonder if we should be setting the safe and finalized using the EN provider and the head using the database. My concern is that we could accidentally issue an FCU containing a finalized head, which is older than the finalized head of the EN. This would be an invalid FCU. For the safe and finalized they are deterministic and therefore will be eventually consistent so I don't think it's an issue to use the EN provider to set them.
There was a problem hiding this comment.
My concern is that we could accidentally issue an FCU containing a finalized head, which is older than the finalized head of the EN.
Yes that could be a concern, let me update this
crates/node/src/args.rs
Outdated
| // Update the head block info from the database if available. | ||
| if let Some(latest_block) = db.tx().await?.get_l2_blocks().await?.next().await { | ||
| let latest_block = latest_block?; | ||
| fcs.update_head_block_info(latest_block); | ||
| } |
There was a problem hiding this comment.
We only persist safe l2 blocks in the database. Unsafe blocks are not persisted in the database, we only persist a mapping of L1 message -> L2 block number. This may be a shortcoming of the data model that needs to be revised. We may need to revert to persisting unsafe blocks in the database as well.
There was a problem hiding this comment.
That's true... one solution would be to take the latest executed L1 message's L2 block number. We could on top of that (as a later improvement) iterate blocks from there to the tip using the L2 provider and take the shallowest block which doesn't include L1 messages (as suggested by @Thegaram).
There was a problem hiding this comment.
This works, but it could lead to relatively deep reorgs if L1 messages are not included in a large number of blocks. An alternative solution would be to track the current chain head in the database metadata table.
greged93
force-pushed
the
feat/fcs-from-db
branch
from
a9c50df to
c41cc9e
Compare
frisitano
left a comment
frisitano
left a comment
There was a problem hiding this comment.
Looks good, left a comment inline about updating the metadata table function in another location - this would logically be after it has been imported by the engine.
Sets the fcs at startup from the database instead of the provider. Avoids edge cases on the initial L1 message index for the sequencer, where an incorrect state could arise during restart from a crash.
Resolves #334