Skip to content

Do not recycle presignatures#697

Merged
volovyks merged 12 commits intodevelopfrom
serhii/do-not-recycle-presignatures
Apr 10, 2026
Merged

Do not recycle presignatures#697
volovyks merged 12 commits intodevelopfrom
serhii/do-not-recycle-presignatures

Conversation

@volovyks
Copy link
Copy Markdown
Contributor

It was a part of the State Sync PR initially, but it does not belong there.
Recycling is not part of our design; rather, it's the opposite.
It can cause or hide issues, more motivation here: #472 (comment)

@volovyks volovyks requested a review from ChaoticTempest March 12, 2026 10:56
@volovyks volovyks assigned jakmeier and unassigned jakmeier Mar 12, 2026
@volovyks volovyks requested a review from jakmeier March 12, 2026 10:57
/// Stockpile triples if the amount of unspent triples is below the minimum
/// and the maximum number of all ongoing generation protocols is below the maximum.
async fn stockpile(&mut self, participants: &[Participant], cfg: &ProtocolConfig) {
if participants.len() < self.threshold {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check is already done before calling the stockpile() function

self.ongoing_owned.remove(&id);
let _ = ongoing_gen_tx.send(self.ongoing.len());
}
_ = stockpile_interval.tick(), if active.len() >= self.threshold => {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Logic is the same here. I've only added logging.

Copy link
Copy Markdown
Contributor

@jakmeier jakmeier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but let's wait for @ChaoticTempest to see if the presiganture burning issue is still present.

Copy link
Copy Markdown
Contributor

@ChaoticTempest ChaoticTempest left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not entirely sure if this issue is still present. @volovyks can you add a test where we have 100 presignatures with 100 signature requests. If we can get through with ~95% of the signatures produced, then it'd be great. Ideally it should be as close to 100% chance as possible but we don't have to guarantee that with this PR

@volovyks
Copy link
Copy Markdown
Contributor Author

I do not think that having loose tests is a good idea. I've created a couple of very strict tests here: #710
They pass and seem stable. Check details in the PR.

@volovyks
Copy link
Copy Markdown
Contributor Author

@ChaoticTempest @jakmeier do not lose triples, and presignatures tests are passing
I think that waste of presignatures is still possible if we have many round of signature generation, but I'm ok with merging this anyway

@ChaoticTempest
Copy link
Copy Markdown
Contributor

I would like to hold off merging this PR until #723 is merged since that race conditions causes some sign requests to go through several 1000s of rounds

@volovyks volovyks merged commit b21509e into develop Apr 10, 2026
3 checks passed
@volovyks volovyks deleted the serhii/do-not-recycle-presignatures branch April 10, 2026 11:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants