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

Nakamoto: Validate stacker signature of a Nakamoto Block #4039

Merged
merged 26 commits into from
Nov 30, 2023

Conversation

jferrant
Copy link
Collaborator

@jferrant jferrant commented Nov 7, 2023

Description

Add functionality to check if a Nakamoto block has been correctly signed with the Signers (Stackers) WSTS signature associated with the aggregate public key.

Applicable issues

Copy link

codecov bot commented Nov 7, 2023

Codecov Report

Attention: 421 lines in your changes are missing coverage. Please review.

Comparison is base (1bb5331) 0.04% compared to head (6eeff19) 0.04%.
Report is 3 commits behind head on next.

Files Patch % Lines
stacks-common/src/util/secp256k1.rs 0.00% 86 Missing ⚠️
stackslib/src/chainstate/nakamoto/mod.rs 0.00% 74 Missing ⚠️
stackslib/src/chainstate/nakamoto/tests/node.rs 0.00% 72 Missing ⚠️
...kslib/src/chainstate/nakamoto/coordinator/tests.rs 0.00% 57 Missing ⚠️
stackslib/src/chainstate/stacks/boot/mod.rs 0.00% 39 Missing ⚠️
stackslib/src/net/relay.rs 0.00% 30 Missing ⚠️
testnet/stacks-node/src/mockamoto.rs 0.00% 25 Missing ⚠️
stackslib/src/chainstate/stacks/transaction.rs 0.00% 12 Missing ⚠️
stacks-signer/src/runloop.rs 0.00% 11 Missing ⚠️
stackslib/src/chainstate/burn/db/sortdb.rs 0.00% 7 Missing ⚠️
... and 5 more
Additional details and impacted files
@@            Coverage Diff             @@
##             next    #4039      +/-   ##
==========================================
- Coverage    0.04%    0.04%   -0.01%     
==========================================
  Files         419      419              
  Lines      297652   297992     +340     
==========================================
  Hits          136      136              
- Misses     297516   297856     +340     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@obycode obycode left a comment

Choose a reason for hiding this comment

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

LGTM! I just had one minor suggestion.
May the Schwartz be with you.

stackslib/src/chainstate/burn/db/sortdb.rs Outdated Show resolved Hide resolved
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.

LGTM -- only had a minor comment

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.

I don't think we should be using MessageSignature to encapsulate a Schnorr signature. I was using that as a placeholder for a Schnorr-specific signature type.

If you can instead create a Schnorr signature container, and use that in place of MessageSignature in the block header, then I'll approve 👍

@jferrant jferrant force-pushed the feat/nakamoto-validate-signer branch 2 times, most recently from 88d0745 to 7574a6c Compare November 8, 2023 19:29
@jferrant
Copy link
Collaborator Author

jferrant commented Nov 8, 2023

I don't think we should be using MessageSignature to encapsulate a Schnorr signature. I was using that as a placeholder for a Schnorr-specific signature type.

If you can instead create a Schnorr signature container, and use that in place of MessageSignature in the block header, then I'll approve 👍

Done. Just working on adding "aggregate_public_key" to the RewardCycleInfo

@jferrant jferrant requested a review from jcnelson November 8, 2023 19:31
@jferrant jferrant force-pushed the feat/nakamoto-validate-signer branch from de9b632 to 87a6c9c Compare November 8, 2023 23:05
@jferrant
Copy link
Collaborator Author

I am also inclined in this PR to update stacker_signature and all references to it to signers_signature as technically stackers may have offloaded their responsibilities to some other signer operator. Does anyone have objections to this? @jcnelson @kantai

@jcnelson
Copy link
Member

Sounds good to me 👍

@jferrant jferrant force-pushed the feat/nakamoto-validate-signer branch 3 times, most recently from 2717979 to 7c3aea9 Compare November 14, 2023 17:44
@CLAassistant
Copy link

CLAassistant commented Nov 16, 2023

CLA assistant check
All committers have signed the CLA.

@jferrant jferrant force-pushed the feat/nakamoto-validate-signer branch 2 times, most recently from 510367a to 220099a Compare November 20, 2023 17:55
@jferrant jferrant force-pushed the feat/nakamoto-validate-signer branch 3 times, most recently from 3500cb3 to d3e0aa8 Compare November 28, 2023 00:46
…nd verify it against the provided signature

Signed-off-by: Jacinta Ferrant <[email protected]>
…ard cycle to determine the aggregate public key

Signed-off-by: Jacinta Ferrant <[email protected]>
Signed-off-by: Jacinta Ferrant <[email protected]>
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.

LGTM -- I think we should just flag a relevant issue for the block that the agg key is fetched from, I don't think it needs to block this PR.

stackslib/src/chainstate/nakamoto/mod.rs Outdated Show resolved Hide resolved
stackslib/src/chainstate/nakamoto/mod.rs Outdated Show resolved Hide resolved
stackslib/src/net/relay.rs Show resolved Hide resolved
@jferrant jferrant merged commit b2f0b39 into next Nov 30, 2023
1 check passed
@blockstack-devops
Copy link
Contributor

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@stacks-network stacks-network locked as resolved and limited conversation to collaborators Nov 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants