Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Embed public and private shares into DkgBegin messages #927

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Embed public and private shares into DkgBegin messages #927

wants to merge 5 commits into from

Conversation

xoloki
Copy link
Contributor

@xoloki xoloki commented Nov 22, 2024 •
edited
Loading

Description

This change fixes the race condition that arises during DKG from the interplay of our networking stack, usage of WSTS, and WSTS itself. It uses recent WSTS changes to embed public and private shares into the various DkgBegin messages. This avoids the case where a signer receives DKG public or private shares before the corresponding DkgBegin.

Closes: #929

Changes

Testing Information

Checklist:

  • I have performed a self-review of my code
  • My changes generate no new warnings
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@djordon djordon added bug Something isn't working signer communication Communication across sBTC bootstrap signers. signer coordination The actions executed by the signer coordinator. labels Nov 23, 2024
@djordon djordon added this to the sBTC Release Ready milestone Nov 23, 2024
@djordon djordon mentioned this pull request Nov 23, 2024
Open
3 tasks
@xoloki xoloki force-pushed the fix/demo-fix-wsts-embed branch from 5fd51b5 to 3f8be89 Compare November 25, 2024 13:57
@xoloki xoloki changed the base branch from fix/demo-fix to main December 2, 2024 20:22
@xoloki
PoC embedding dkg shares into begin messages
9e518af 
fmt fixes

remove commented out branches

underscore msg_public_key since we aren't using it for now, will remove later

init tracing subscriber so i can see what's going on
@xoloki xoloki force-pushed the fix/demo-fix-wsts-embed branch from e8e0735 to 9e518af Compare December 2, 2024 20:24
@xoloki xoloki changed the title PoC embedding dkg shares into begin messages Embed public and private shares into DkgBegin messages Dec 3, 2024
@xoloki xoloki requested a review from djordon December 3, 2024 02:27
@xoloki xoloki marked this pull request as ready for review December 3, 2024 02:40
@xoloki
Copy link
Contributor Author

xoloki commented Dec 3, 2024

@matteojug can you point me to the added sleep statements? I want to try disabling them.

@djordon
Copy link
Collaborator

djordon commented Dec 3, 2024

I think this is the one: https://github.com/stacks-network/sbtc/pull/927/files#diff-2d16f548dd0cc67e9c08474479e9c73ff36c6004bafe17d3ab0af140d14d5954R557-R562

@xoloki
Copy link
Contributor Author

xoloki commented Dec 3, 2024

I think this is the one: https://github.com/stacks-network/sbtc/pull/927/files#diff-2d16f548dd0cc67e9c08474479e9c73ff36c6004bafe17d3ab0af140d14d5954R557-R562

Ok, the sleep is disabled in main.rs but the code is left intact. I ran a devnet demo with no problem, please try to reproduce the error on this branch @matteojug @cylewitruk @djordon

xoloki
xoloki commented Dec 3, 2024
View reviewed changes
signer/src/proto/convert.rs Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@djordon djordon Awaiting requested review from djordon

At least 1 approving review is required to merge this pull request.

Assignees

@xoloki xoloki

Labels
bug Something isn't working signer communication Communication across sBTC bootstrap signers. signer coordination The actions executed by the signer coordinator.
Projects
sBTC
Status: In Review
Milestone
sBTC: Nice to have
Development

Successfully merging this pull request may close these issues.

[Bug]: Race condition during DKG
2 participants
@xoloki @djordon