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.

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

modify nakamoto block header to use Vec<MessageSignature> #4781

Merged
merged 61 commits into from
Jun 4, 2024

Conversation

hstove
Copy link
Contributor

@hstove hstove commented May 13, 2024

This builds on top of #4778, to try and reduce duplicate work and conflicts.

The verify_signer_signatures function has a few rules:

  • The Vec<Signature> can be smaller than Vec<Signer>, but it's still order dependent.
    • What this means is that if you have signers A,B,C, you can have signatures sA,sC, but not sC,sA.
  • This depends on the signers being unique by public key (which I think is true, but if not it requires refactoring)
  • I'm summing the total weight of the RewardSet and using 70% as the threshold
  • No duplicates, no signatures from non-signers, etc

Tracking the work included in this PR:

  • Updates the signer_signature field in NakamotoBlockHeader to be Vec<MessageSignature>
  • Adds a new verify_signer_signatures method to NakamotoBlockHeader with tests
  • Verify the block header in accept_block
  • Update TestSigners and TestSigningChannel to sign the block using the new format
  • Update blind_signer related code for Nakamoto integrations

What I think (?) is out of scope:

@hstove hstove requested a review from kantai May 13, 2024 03:04
@hstove hstove force-pushed the feat/header-signer-signatures branch from 6abb574 to 8c92e2d Compare May 13, 2024 22:33
@jcnelson
Copy link
Member

Hi @hstove, I suspect that you have discovered that in order to ship this PR, a lot of unit test infrastructure needs to be changed as well (see #4796). I wonder if it makes sense to create a feature branch in the Stacks blockchain for switching over from WSTS to multisig?

@hstove hstove changed the base branch from chore/signer-traits to feat/add-v0-signer May 15, 2024 20:10
@hstove hstove force-pushed the feat/header-signer-signatures branch from fe3ca6f to d14e5b4 Compare May 15, 2024 20:11
@hstove
Copy link
Contributor Author

hstove commented May 15, 2024

@jcnelson I think that's possibly the right idea, but I'm finding it to be actually pretty reasonable to swap the testing infra without really messing it up for when we go back to threshold sigs. There are a few places (ie TestSigners, SignCoordinator) where I'm keeping essentially two implementations, and only one of them is actively used.

I have a number of TODO comments in this PR, which are mainly related to where you'd need to do epoch-gated behavior. I'd love to see what you think about how to proceed given my first attempt at this.

@hstove
Copy link
Contributor Author

hstove commented May 15, 2024

@jferrant @kantai @obycode tagging you in as I think this PR is in a good place to get a review. I'll want to see a full CI run to see exactly what's still TBD, but a lot of the initial pieces are in place.

I've also rebased this on top of #4788, as that includes some type changes that are utilized.

@saralab saralab marked this pull request as ready for review May 16, 2024 13:51
Copy link
Member

@kantai kantai left a comment

Choose a reason for hiding this comment

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

This LGTM -- just a few comments. Exciting stuff!

stackslib/src/chainstate/nakamoto/mod.rs Outdated Show resolved Hide resolved
stackslib/src/chainstate/nakamoto/mod.rs Outdated Show resolved Hide resolved
stackslib/src/chainstate/nakamoto/mod.rs Outdated Show resolved Hide resolved
stackslib/src/chainstate/nakamoto/mod.rs Outdated Show resolved Hide resolved
stackslib/src/chainstate/nakamoto/mod.rs Outdated Show resolved Hide resolved
stackslib/src/chainstate/nakamoto/mod.rs Outdated Show resolved Hide resolved
testnet/stacks-node/src/nakamoto_node/sign_coordinator.rs Outdated Show resolved Hide resolved
…natures

feat: gather v0 block signatures from stackerdb
@hstove hstove requested a review from a team as a code owner May 31, 2024 17:21
@hstove hstove linked an issue May 31, 2024 that may be closed by this pull request
@jcnelson jcnelson self-requested a review May 31, 2024 20:14
jcnelson
jcnelson previously approved these changes May 31, 2024
Copy link
Member

@jcnelson jcnelson left a comment

Choose a reason for hiding this comment

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

Overall LGTM. Just a couple merge conflicts and a couple places where an error shouldn't be fatal.

@smcclellan smcclellan requested a review from jferrant June 3, 2024 15:07
@hstove
Copy link
Contributor Author

hstove commented Jun 3, 2024

@jcnelson I've refactored run_miner to return a Result, along with removing any panics. Can you give 55567d7 a look and see if that's on track with your suggestions?

@jcnelson jcnelson self-requested a review June 3, 2024 20:25
jcnelson
jcnelson previously approved these changes Jun 3, 2024
@hstove hstove requested a review from kantai June 4, 2024 13:46
jferrant
jferrant previously approved these changes Jun 4, 2024
kantai
kantai previously approved these changes Jun 4, 2024
@kantai kantai dismissed stale reviews from jferrant, jcnelson, and themself via 7c8df31 June 4, 2024 18:09
@kantai
Copy link
Member

kantai commented Jun 4, 2024

Approving.

I pushed 7c8df31. The miner thread joining logic needs to allow the prior mining thread to error (the prior miner will exit with an error if it detects the burnchain change). I am pretty sure that this was what was causing the CI failures (it caused those tests to fail locally for me).

@hstove
Copy link
Contributor Author

hstove commented Jun 4, 2024

Nice catch, thank you @kantai!

@jcnelson jcnelson self-requested a review June 4, 2024 19:22
@kantai kantai added this pull request to the merge queue Jun 4, 2024
Merged via the queue into develop with commit 690701f Jun 4, 2024
1 check passed
@kantai kantai deleted the feat/header-signer-signatures branch June 4, 2024 21:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants