-
Notifications
You must be signed in to change notification settings - Fork 0
Implements persistence for Events, and snapshot loading #17
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
jbearer
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a few suggestions but I think the only blocker is the question about computing/storing the stake for the full node set
| // Mark all delegations to this node as exits | ||
| let result = sqlx::query( | ||
| "UPDATE delegation | ||
| SET unlocks_at = $1, withdrawal_amount = '0' | ||
| WHERE delegator = $2 AND node = $3", | ||
| ) | ||
| .bind(withdrawal.available_time as i64) | ||
| .bind(withdrawal.delegator.to_string()) | ||
| .bind(withdrawal.node.to_string()) | ||
| .execute(&mut **tx) | ||
| .await?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a bit unfortunate that by breaking up the diff into one event for each wallet, this becomes one UPDATE for each wallet when it could be a single statement with just WHERE node = $. This is an issue with the design, since we use the same event stream for updating our own database as for updating clients...but there is a separate client for each wallet, while there is only one database, so the database become a bottleneck.
It seems like maybe we should have storage handle a new type of update, WalletsDiff, which are events generated that affect the entire set of wallets, not just one at at time. Then we could have a diff like NodeExited(address) without specifying any particular delegator. But anyways, this can be left for future work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
jbearer
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. It looks like the CI failure is timing out while the demo is pulling Docker images. I hope this PR might fix it
This PR implements SQLITE persistence for events. It includes migration for all the tables needed.
It saves the genesis l1 block, stores all the diffs and loads the finalized snapshot.