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

Feat/signers write transactions to stackerdb #4301

Merged
merged 20 commits into from
Feb 3, 2024

Conversation

jferrant
Copy link
Collaborator

Description

Signers must broadcast their cast aggregate public key transaction to the mempool if in pre nakamoto rules, and always broadcast the transaction to stackerdb for signers and miners to observe when validating and building the block, respectively.

Applicable issues

For this to be fully tested, we need to have the cast aggregate public key transaction happening

Copy link

codecov bot commented Jan 27, 2024

Codecov Report

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

Comparison is base (dd006d4) 82.86% compared to head (fc7149b) 59.46%.

Files Patch % Lines
stacks-signer/src/runloop.rs 66.36% 148 Missing ⚠️
stacks-signer/src/client/stacks_client.rs 63.92% 127 Missing ⚠️
stacks-signer/src/client/stackerdb.rs 67.17% 43 Missing ⚠️
libsigner/src/messages.rs 94.01% 7 Missing ⚠️
stacks-signer/src/utils.rs 72.72% 6 Missing ⚠️
testnet/stacks-node/src/tests/signer.rs 97.05% 5 Missing ⚠️
stacks-signer/src/config.rs 80.95% 4 Missing ⚠️
stacks-signer/src/main.rs 0.00% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             next    #4301       +/-   ##
===========================================
- Coverage   82.86%   59.46%   -23.40%     
===========================================
  Files         443      443               
  Lines      315904   316885      +981     
===========================================
- Hits       261769   188448    -73321     
- Misses      54135   128437    +74302     

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

@jferrant jferrant force-pushed the feat/signers-write-transactions-to-stackerdb branch 7 times, most recently from 2c514e8 to 72d13eb Compare February 1, 2024 15:08
if let Some(data) = chunk {
if let Ok(message) = bincode::deserialize::<SignerMessage>(data) {
if let SignerMessage::Transactions(chunk_transactions) = message {
let signer_id = *signer_ids.get(i).expect(
Copy link
Member

Choose a reason for hiding this comment

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

What happens if chunk_ack.len() >= signer_ids.len()?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This cannot happen. The call get_latest_chunks will just return None for slots that don't exist so we will always return as many chunks as we queried. and we query an equal number of slots to signer_ids.If this was to happen, it would be a BUG in the system hence the expect.

@jferrant jferrant force-pushed the feat/signers-write-transactions-to-stackerdb branch from dc9f26a to d693a5e Compare February 2, 2024 00:43
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 superficial comments.

stacks-signer/src/client/stackerdb.rs Outdated Show resolved Hide resolved
stacks-signer/src/utils.rs Outdated Show resolved Hide resolved
stacks-signer/src/runloop.rs Outdated Show resolved Hide resolved
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.

LGTM; just address Aaron's comments before merging and make sure CI is passing.

@jferrant jferrant force-pushed the feat/signers-write-transactions-to-stackerdb branch 4 times, most recently from a550240 to ed4f3d3 Compare February 2, 2024 22:50
…update tests

Signed-off-by: Jacinta Ferrant <jacinta@trustmachines.co>
Signed-off-by: Jacinta Ferrant <jacinta@trustmachines.co>
Signed-off-by: Jacinta Ferrant <jacinta@trustmachines.co>
jferrant and others added 16 commits February 2, 2024 15:28
Signed-off-by: Jacinta Ferrant <jacinta@trustmachines.co>
Signed-off-by: Jacinta Ferrant <jacinta@trustmachines.co>
Signed-off-by: Jacinta Ferrant <jacinta@trustmachines.co>
Signed-off-by: Jacinta Ferrant <jacinta@trustmachines.co>
Signed-off-by: Jacinta Ferrant <jacinta@trustmachines.co>
…and signers to observe

Signed-off-by: Jacinta Ferrant <jacinta@trustmachines.co>
Signed-off-by: Jacinta Ferrant <jacinta@trustmachines.co>
Signed-off-by: Jacinta Ferrant <jacinta@trustmachines.co>
…verifying transactions

Signed-off-by: Jacinta Ferrant <jacinta@trustmachines.co>
…r and add a test

Signed-off-by: Jacinta Ferrant <jacinta@trustmachines.co>
Signed-off-by: Jacinta Ferrant <jacinta@trustmachines.co>
Signed-off-by: Jacinta Ferrant <jacinta@trustmachines.co>
Signed-off-by: Jacinta Ferrant <jacinta@trustmachines.co>
Signed-off-by: Jacinta Ferrant <jacinta@trustmachines.co>
Signed-off-by: Jacinta Ferrant <jacinta@trustmachines.co>
@jferrant jferrant force-pushed the feat/signers-write-transactions-to-stackerdb branch from ed4f3d3 to 6f589a4 Compare February 2, 2024 23:46
Signed-off-by: Jacinta Ferrant <jacinta@trustmachines.co>
@jferrant jferrant force-pushed the feat/signers-write-transactions-to-stackerdb branch from c7c904b to fc7149b Compare February 3, 2024 00:54
@jferrant jferrant merged commit 4440414 into next Feb 3, 2024
1 of 2 checks passed
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.

None yet

3 participants