Make sure triples and presignatures are not reused #931
Conversation
| @@ -8,7 +8,7 @@ use near_account_id::AccountId; | |||
| type TripleResult<T> = std::result::Result<T, anyhow::Error>; | |||
|
|
|||
| // Can be used to "clear" redis storage in case of a breaking change | |||
| const TRIPLE_STORAGE_VERSION: &str = "v1"; | |||
| const TRIPLE_STORAGE_VERSION: &str = "v2"; | |||
There was a problem hiding this comment.
Incrementing the storage version here to start from scratch.
volovyks
changed the title
| @@ -187,6 +187,17 @@ impl PresignatureManager { | |||
|
|
|||
| pub async fn insert(&mut self, presignature: Presignature) { | |||
| tracing::debug!(id = ?presignature.id, "inserting presignature"); | |||
| if self.contains(&presignature.id).await { | |||
| tracing::error!(id = presignature.id, "mine presignature already inserted"); | |||
| if self.contains_used(&presignature.id).await { | ||
| tracing::error!( | ||
| id = presignature.id, | ||
| "tried to insert used mine presignature" |
| @@ -433,11 +482,6 @@ impl PresignatureManager { | |||
| participants = ?presig_participants.keys_vec(), | |||
| "running: the intersection of participants is less than the threshold" | |||
| ); | |||
|
|
|||
| // Insert back the triples to be used later since this active set of | |||
| // participants were not able to make use of these triples. | |||
There was a problem hiding this comment.
I think instead of wasting the triples, we can actually change the order of ```
if let Some((triple0, triple1)) = triple_manager.take_two_mine().await
and
if presig_participants.len() < self.threshold```
basically check participants.len() first, if that passes, then take_mine
| @@ -665,7 +663,6 @@ impl SignatureManager { | |||
| ?err, | |||
| "failed to retry signature generation: trashing presignature" | |||
| ); | |||
| failed_presigs.push(presignature); | |||
There was a problem hiding this comment.
if the presignature is not pushed back into storage, then we should not run retry_failed_generation() with the same presignature at all. I think if we don't want to recycle the presignature, then we should pick another unused presignature.
There was a problem hiding this comment.
It is possible
Check the PR description, I want to rethink how we do retries
| @@ -691,17 +687,10 @@ impl SignatureManager { | |||
| my_request.time_added, | |||
| cfg, | |||
| ) { | |||
| failed_presigs.push(presignature); | |||
| @@ -96,10 +110,17 @@ impl PresignatureRedisStorage { | |||
| Ok(result) | |||
| } | |||
|
|
|||
| pub async fn len_used(&self) -> PresigResult<usize> { | |||
There was a problem hiding this comment.
seems like we never clear the used presignatures. So the used presignatures will grow indefinitely as we generate and use more presignatures.
I think we should add a mechanism that get rid of the very old presignatures in the used_key
There was a problem hiding this comment.
Redis allows to add time it will be stored in the DB, we can use that
| @@ -89,10 +102,17 @@ impl TripleRedisStorage { | |||
| Ok(result) | |||
| } | |||
|
|
|||
| pub async fn len_used(&self) -> TripleResult<usize> { | |||
There was a problem hiding this comment.
same problem as the used presignature. this will grow indefinitely
| "tried to insert used mine presignature" | ||
| ); | ||
| return; | ||
| } | ||
| // Remove from taken list if it was there | ||
| self.gc.remove(&presignature.id); |
There was a problem hiding this comment.
gc field is actually not necessary in PresignatureManager now that you add the used presignatures to storage.
You can delete it. Same for TripleManager.
There was a problem hiding this comment.
GC is used for multiple purposes. I want to discuss that with @ChaoticTempest
We should be more strict on how we manage triples and presignatures. Not sure if it's causing this https://github.com/near/transfer/issues/18 and other issues, but we should make this change anyway. Rare occasions where we lose triples or presignatures is a low price for robustness.
Triples:
Presignatures:
UPDATE: This PR needs to be discussed and designed better. It does not cover all edge cases.
I think that many issues in our project are caused by retry, reinsert, etc. approaches. Also, such design patterns can hide many issues.