types migration #2478
types migration #2478
Conversation
types/src/v0/traits.rs
Outdated
| self.migrate_anchor_leaf().await?; | ||
| self.migrate_da_proposals().await?; | ||
| self.migrate_vid_shares().await?; | ||
| self.migrate_undecided_state().await?; | ||
| self.migrate_quorum_proposals().await?; | ||
| self.migrate_quorum_certificates().await |
There was a problem hiding this comment.
Can we gate this by versions(and the other parts we start storing the new types) so we can merge this and not worry about deploying it
|
|
||
|
|
||
| CREATE TABLE undecided_state2 ( | ||
| -- The ID is always set to 0. Setting it explicitly allows us to enforce with every insert or |
There was a problem hiding this comment.
Did you consider adding a constraint for only allowing the ID to be 0?
There was a problem hiding this comment.
can add, these tables are just copy of older ones
| // `drb_seed` no longer exists on `Leaf2` | ||
| // if leaf2.drb_seed != [0; 32] && leaf2.drb_result != [0; 32] { | ||
| // panic!("Downgrade of Leaf2 to Leaf will lose DRB information!"); | ||
| // } |
There was a problem hiding this comment.
@ss-es @lukaszrzasik maybe you can comment on this? I think the downgrade is mainly used in the query service now when paths to API v0 are invoked?
sequencer/src/persistence/fs.rs
Outdated
| .context("failed to create anchor leaf 2 dir")?; | ||
|
|
||
| let decided_leaf_path = inner.decided_leaf_path(); | ||
| if !decided_leaf_path.is_dir() { |
There was a problem hiding this comment.
@imabdulbasit When would this not be a directory?
There was a problem hiding this comment.
if I spin up a new node with PoS version
sequencer/src/persistence/fs.rs
Outdated
| } | ||
|
|
||
| tracing::warn!("migrating decided leaves.."); | ||
| for entry in fs::read_dir(decided_leaf_path)? { |
There was a problem hiding this comment.
Do I understand correctly that if the migration is interrupted it will just migrate everything again the next time? I don't think that's a problem, as long as things eventually get migrated and we don't end up in a state where the node can't start up because the migration fails.
There was a problem hiding this comment.
Yeah, we can keep the offsets after each batch upsert and resume from there too. I didn't think about that but can add it after doing some rewards work
sequencer/src/persistence/fs.rs
Outdated
| let proposal = bincode::deserialize::<Proposal<SeqTypes, DaProposal<SeqTypes>>>(&bytes) | ||
| .context(format!("parsing da proposal {}", path.display()))?; | ||
|
|
||
| let file_path = da2_path.join(view.to_string()).with_extension("txt"); |
There was a problem hiding this comment.
I think in these functions we could be a bit more explicit with naming to make it easier to read. As in for example old_path new_path, old_file, new_file or something like that, instead of path, file, file_path, file.
There was a problem hiding this comment.
done in 0ea3d2b
| let mut tx = self.db.write().await?; | ||
| query.execute(tx.as_mut()).await?; | ||
| tx.commit().await?; | ||
| if rows.len() < batch_size as usize { |
There was a problem hiding this comment.
Some of the migration functions have this condition and some don't. Is that intentional? I think we don't really need it because the next query should return nothing in which case we already stop.
edit: i'm probably wrong the condition is present everywhere. Still wonder why it's there though.
There was a problem hiding this comment.
So I added this to avoid an extra query but I guess that doesn't really makes any performance difference. there is also a if rows.is_empty() { break} at the start so removing this wouldn't affect anything
There was a problem hiding this comment.
This statement should be there for all SQL migrations. I think it is missing for undecided_state but that is fine because there can be only one entry for undecided_state
There was a problem hiding this comment.
LGTM. But some remaining questions. I don't think any of that is critical though. What I guess would be nice if we had some kind of data dump from an existing node somewhere that we can try the migration on. I think the tests you added are good and I think it should work but it's always nice to also test big migrations on a real databases. I know we don't currently have good infrastructure for doing that but I think it's something we should consider building.
This PR migrates existing types, such as leaf1 and VID, to the new V2 types. There are separate migrations for both query service storage and consensus storage types.
To ensure data safety, the migration creates new files for filesystem storage and new tables for SQL storage. This approach allows us to retain the old data as a fallback in case of any issues. The old data can be removed in a future release once this migration has been successfully deployed on decaf/mainnet.
Query Service Storage:
Consensus Storage:
followup work: