persist triples #444
Merged
persist triples #444
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Terraform Feature Environment (dev-444)Terraform Initialization ⚙️
|
ppca
force-pushed
the
xiangyi/big_table
branch
from
5aa0fa6
to
6b2911f
Compare
volovyks
previously approved these changes
Feb 8, 2024
There was a problem hiding this comment.
LGTM, all the comments are optional or can be addressed in a separate PR.
Overall the only thing that concerns me with triple persistence is more edge cases, stale triples, etc. We should design it in a way that will not introduce new challenges for us and just help to reduce the number of triples that we need to generate after reboot.
- Do we have a mechanism to delete stale triples? Otherwise, our DB can grow indefinitely.
- I assume that if some of the triples are not in sync between all the nodes, we will drop this particular protocol, choose the other 2 triples, and try again. Is that so @ChaoticTempest ? It should allow us to overcome read/write failures to DB.
Integration tests:
- Separate tests for DB are not a priority, let's not waste time on this for now
- A top-level test that checks the number of triples after node load would be nice to have (separate PR)
| .await?; | ||
|
|
||
| let storage_options = mpc_recovery_node::storage::Options { | ||
| gcp_project_id: Some("multichain-integration".to_string()), |
There was a problem hiding this comment.
Probably we want to reuse the variable defined above.
| @@ -38,6 +38,15 @@ async fn test_triples_and_presignatures() -> anyhow::Result<()> { | |||
| assert_eq!(state_0.participants.len(), 3); | |||
| wait_for::has_at_least_triples(&ctx, 2).await?; | |||
| wait_for::has_at_least_presignatures(&ctx, 2).await?; | |||
| // TODO: add test that checks #triples in datastore | |||
There was a problem hiding this comment.
I would rather create a separate test. In that test, I would:
- Start 3 nodes
- Wait for a generation of N triples
- Stop all 3 nodes
- Start one of them and check that it has N triples (it will not be able to generate new triples, since other nodes are down)
There was a problem hiding this comment.
Yeah I wanted to do this but was not able to access the datastore correctly. The error is as shown a few lines down
|
|
||
| #[derive(Debug, Clone)] | ||
| pub enum Value { | ||
| BooleanValue(bool), |
There was a problem hiding this comment.
Is it hard to create a place for common code between FastAuth and Multichain codebases? Code duplication is never a good idea. But I understand, we do not want to touch the old codebase.
There was a problem hiding this comment.
Yeah I don't want to mess up mpc code since it's working and I'm not sure maybe we retire mpc code base altogether someday so decided to write a new one.
|
Do we have a mechanism to delete stale triples? Otherwise, our DB can grow indefinitely. |
I think currently if one node goes offline while a protocol is in progress, then that protocol will be stuck forever. |
ppca
force-pushed
the
xiangyi/big_table
branch
from
5e6ff54
to
dd2567f
Compare
This PR should be discussed together with #446. Check my last proposition, it may help with this issue, |
node/src/protocol/triple.rs
Outdated
| ) -> Self { | ||
| let mut mine: VecDeque<TripleId> = VecDeque::new(); | ||
| let mut all_triples = HashMap::new(); | ||
| if let Some(triple_data_vec) = triples.clone() { |
There was a problem hiding this comment.
is this clone() necessary here?
node/src/protocol/triple.rs
Outdated
Comment on lines
125
to
150
| let mine1 = self.mine.contains(&triple1.id); | ||
| let mine2 = self.mine.contains(&triple2.id); | ||
| let mut write_lock = self.triple_storage.write().await; | ||
| let account_id = &write_lock.account_id(); | ||
| match write_lock | ||
| .delete(TripleData { | ||
| account_id: account_id.clone(), | ||
| triple: triple1.clone(), | ||
| mine: mine1, | ||
| }) | ||
| .await | ||
| { | ||
| Ok(()) => tracing::info!(id0, "successfully deleted triple"), | ||
| Err(error) => tracing::info!(id0, ?error, "delete triple failed"), | ||
| } | ||
| match write_lock | ||
| .delete(TripleData { | ||
| account_id: account_id.clone(), | ||
| triple: triple2.clone(), | ||
| mine: mine2, | ||
| }) | ||
| .await | ||
| { | ||
| Ok(()) => tracing::info!(id1, "successfully deleted triple"), | ||
| Err(error) => tracing::info!(id1, ?error, "delete triple failed"), | ||
| } |
There was a problem hiding this comment.
let's make this into a separate function for easier readability
node/src/protocol/triple.rs
Outdated
Comment on lines
295
to
310
| let mut write_lock = self.triple_storage.write().await; | ||
| let account_id = write_lock.account_id().clone(); | ||
| for triple in async_triples_to_insert { | ||
| let mine = self.mine.contains(&triple.id); | ||
| match write_lock | ||
| .insert(TripleData { | ||
| account_id: account_id.clone(), | ||
| triple, | ||
| mine, | ||
| }) | ||
| .await | ||
| { | ||
| Ok(()) => tracing::info!("successfully inserted triple"), | ||
| Err(error) => tracing::info!("triple insertion failed: {}", error), | ||
| } | ||
| } |
There was a problem hiding this comment.
this seems like this could be another separate function inside TripleStorage. Now that I see it again, let's not log on every Ok. The logs will get spammed otherwise
node/src/protocol/triple.rs
Outdated
| .map(|me| TripleManager::new(participants.clone(), *me, number as usize, 0)) | ||
| let participants: Vec<Participant> = range.clone().map(Participant::from).collect(); | ||
| let managers = range | ||
| .clone() |
There was a problem hiding this comment.
hmm, why is this a clone instead of an iter or into_iter?
node/src/protocol/triple.rs
Outdated
Comment on lines
431
to
432
| let runtime = tokio::runtime::Runtime::new().unwrap(); | ||
| runtime.block_on(async { |
There was a problem hiding this comment.
you can also do a #[tokio::test] instead on top of the function just so the code here doesn't shift into a different scope
node/src/gcp/mod.rs
Outdated
Comment on lines
332
to
333
| pub datastore: Option<DatastoreService>, | ||
| pub secret_manager: Option<SecretManagerService>, |
There was a problem hiding this comment.
why are these options? Would be unwieldy to use later when we go and directly reference them everytime?
node/src/protocol/consensus.rs
Outdated
Comment on lines
688
to
691
| let triple_data = match triple_data_result { | ||
| Ok(vec_triple_data) => Some(vec_triple_data), | ||
| _ => None, | ||
| }; |
There was a problem hiding this comment.
youu can shorten this to:
Suggested change
| let triple_data = match triple_data_result { | |
| Ok(vec_triple_data) => Some(vec_triple_data), | |
| _ => None, | |
| }; | |
| let triple_data = match triple_data_result.ok(); |
node/src/protocol/state.rs
Outdated
| pub struct StartedState(pub Option<PersistentNodeData>); | ||
| pub struct StartedState { | ||
| pub persistent_node_data: Option<PersistentNodeData>, | ||
| pub triple_data: Option<Vec<TripleData>>, |
There was a problem hiding this comment.
why is this an option? I'd imagine this could just be an empty vec by default otherwise
ChaoticTempest
approved these changes
Feb 13, 2024
|
Nice work @ppca! Merging. |
Terraform Feature Environment Destroy (dev-444)Terraform Initialization ⚙️
|
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Included in this PR:
About tests:
About encryption:
will do a followup for encryption after this one merges.