Feat: Add `TenureChange` transaction type #3974

jbencin · 2023-09-21T20:33:00Z

Description

Add a new Stacks transaction type, TenureChange, that Stackers will use to control mining tenure in Nakamoto release.

This PR adds the enum variant TransactionPayload::TenureChange and establishes the wire format for a tenure change. It does not do much in the way of checking whether the trenure change is valid. That will come in a separate PR.

Applicable issues

Nakamoto: New TenureChange Stacks transaction #3944

Additional info (benefits, drawbacks, caveats)

Checklist

Add TenureChange variant to TransactionPayload enum
In db/transactions.rs, verify that the current block is a Nakamoto block and that the StacksBlockId of the last block from the previous tenure is the current block's parent
Add step in NakamotoChainState that checks for the presence of a TenureChange transaction, confirming that the current block was signed by the appropriate hash
Test coverage for new or modified code paths
Changelog is updated
Required documentation changes (e.g., docs/rpc/openapi.yaml and rpc-endpoints.md for v2 endpoints, event-dispatcher.md for new events)
New clarity functions have corresponding PR in clarity-benchmarking repo
New integration test(s) added to bitcoin-tests.yml

codecov · 2023-09-21T20:34:10Z

Codecov Report

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

Comparison is base (543466f) 0.04% compared to head (252b77f) 0.04%.
Report is 1 commits behind head on next.

Files	Patch %	Lines
stackslib/src/chainstate/stacks/transaction.rs	0.00%	63 Missing ⚠️
stackslib/src/chainstate/nakamoto/tests/mod.rs	0.00%	33 Missing ⚠️
testnet/stacks-node/src/mockamoto.rs	0.00%	11 Missing ⚠️
stackslib/src/chainstate/stacks/mod.rs	0.00%	8 Missing ⚠️
stackslib/src/chainstate/nakamoto/mod.rs	0.00%	2 Missing ⚠️
stackslib/src/chainstate/nakamoto/tests/node.rs	0.00%	1 Missing ⚠️
stackslib/src/chainstate/stacks/block.rs	0.00%	1 Missing ⚠️
stackslib/src/chainstate/stacks/db/transactions.rs	0.00%	1 Missing ⚠️
testnet/stacks-node/src/run_loop/neon.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             next    #3974      +/-   ##
==========================================
- Coverage    0.04%    0.04%   -0.01%     
==========================================
  Files         419      419              
  Lines      297581   297643      +62     
==========================================
  Hits          136      136              
- Misses     297445   297507      +62

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

stackslib/src/chainstate/stacks/mod.rs

stackslib/src/chainstate/stacks/transaction.rs

jcnelson · 2023-10-04T20:37:56Z

stackslib/src/chainstate/stacks/db/transactions.rs

+                    return Err(Error::InvalidStacksTransaction(msg, false));
+                }
+
+                // TODO: More checks before adding to block?


A few:

This must be the first block in the miner's tenure. It cannot be in a subsequent block

This TenureChange must be for last_tenure + 1. No TenureChanges can be skipped.

It must point to the last active miner's block-commit's StacksBlockId

It must count up the number of blocks since the last block with a TenureChange (it may be zero; a block can have multiple TenureChange transactions per the design doc)

It must be signed by the current Stacker set (this could be checked on consensus_deserialize)

It must precede any other kinds of transactions (this could be checked in StacksBlock::consensus_deserialize)

I think this should live as a separate issue from this PR and let this PR function mostly just as a wire format implementation (#4014)

stackslib/src/chainstate/stacks/mod.rs

jcnelson · 2023-10-04T20:44:45Z

stackslib/src/chainstate/stacks/transaction.rs

+            TransactionPayload::TenureChange(tc) => {
+                write_next(fd, &(TransactionPayloadID::TenureChange as u8))?;
+                tc.consensus_serialize(fd)?;
+            }


You're going to somehow need to epoch-gate consensus_deserialize below so that this new transaction variant does not even decode until epoch 3.0. I think we're going to need a separate trait for this (see here: #3710 (comment))

Yeah this makes sense. I'll check with Vlad to see if he's working on this for his PR

Right now, there are at least two PRs in flight (Vlad's and Cyle's) that do this. We should just bite the bullet and get one of them shipped, and then base this PR off of it.

Actually, @kantai has already done this in #3992. Can you rebase off of this?

Sure. I guess we should let @fess-v know as well, since he's working on this too

Hey @jbencin @jcnelson ! Can't find in #3992 the logic for DeserializeWithEpoch traits for StacksTransaction and StacksBlock, I'm currently working on it and will finish soon, do you think I should wait for that PR to implement this logic and drop mine?

No -- #3992 isn't going to implement that logic -- so you should continue with your work.

Got it, thanks @kantai

Just to close the loop on this PR: I don't think it's strictly necessary to epoch gate this deserialization. All that matters is whether or not the transaction is valid for inclusion in a block. During block processing, the transaction processor should just yield an invalid block error if this transaction payload is matched.

Yeah that's fine for now. Furthermore, the block validation code reached from accept_block() can reject blocks that include them too early. I'm already doing this in my coordinator branch.

Cargo.lock

jcnelson · 2023-10-18T11:05:19Z

stackslib/src/chainstate/stacks/transaction.rs

+}
+
+impl TenureChangePayload {
+    pub fn validate(&self) -> Result<(), TenureChangeError> {


This is going to require access to a lot more context, such as access to the chainstate databases. In fact, the code for this might instead belong in NakamotoChainState.

jcnelson · 2023-11-06T16:19:41Z

Before this comes out of draft, we need unit tests for the new functionality.

jbencin · 2023-11-07T15:12:23Z

@jcnelson or @kantai, I'm trying to finish up this PR now, and I've got a couple minor questions regarding how to sign. Right now I have the following struct:

pub struct TenureChangePayload {
    pub previous_tenure_end: StacksBlockId,
    pub previous_tenure_blocks: u16,
    pub cause: TenureChangeCause,
    pub pubkey_hash: Hash160,
    pub signature: ThresholdSignature,
    pub signers: Vec<u8>,
}

What I'm wondering is:

Does the signature field need to cover all other fields, including signers?
How to do the signature? My guess is that it should be done while the struct is encoded with StacksMessageCodec and treat that as a u8 slice. So then I'd need to pull out signature like this:

pub struct TenureChange {
    pub previous_tenure_end: StacksBlockId,
    pub previous_tenure_blocks: u16,
    pub cause: TenureChangeCause,
    pub pubkey_hash: Hash160,
    pub signers: Vec<u8>,
}

pub struct TenureChangePayload {
    pub tenure_change: TenureChange,
    pub signature: ThresholdSignature,
}

so I can serialize TenureChange and compute the signature

kantai · 2023-11-07T15:56:40Z

@jcnelson or @kantai, I'm trying to finish up this PR now, and I've got a couple minor questions regarding how to sign. Right now I have the following struct:
pub struct TenureChangePayload {
    pub previous_tenure_end: StacksBlockId,
    pub previous_tenure_blocks: u16,
    pub cause: TenureChangeCause,
    pub pubkey_hash: Hash160,
    pub signature: ThresholdSignature,
    pub signers: Vec<u8>,
}
What I'm wondering is:

Does the signature field need to cover all other fields, including signers?

Yes, it needs to cover signers.

How to do the signature? My guess is that it should be done while the struct is encoded with StacksMessageCodec and treat that as a u8 slice. So then I'd need to pull out signature like this:

I would recommend just adding a function:

impl TenureChangePayload {
    /// Serialize the parts of this payload to `out` required for signing and validating
    /// stacker signatures.
    pub fn serialize_for_sign<W: Write>(&self, out: &mut W) -> Result<(), CodecError> {
          s.previous_tenure_end.consensus_serialize(out)?;
          ...
    }
}

jbencin · 2023-11-07T16:04:35Z

I would recommend just adding a function:

Cool, this works too. The only other change then I'll need is to move signature as the last field in the struct, so serialize_for_sign() can be called in TenureChangePayload::consensus_serialize().

jbencin · 2023-11-09T16:47:21Z

I've finished up with the structure and wire format for a TenureChange transaction. I'm limiting the scope of this PR and pushing most of the validation work off to a later one. With the change in scope this PR is ready for review.

There's one thing that needs to be fixed before merging: There are still 2 unit tests failing. I've been looking into this since yesterday but haven't made any progress:

thread 'chainstate::nakamoto::tests::nakamoto_advance_tip_simple' panicked at stackslib/src/chainstate/nakamoto/tests.rs:167:6:
called `Result::unwrap()` on an `Err` value: InvalidStacksBlock("Failed to load total tenure cost from parent. parent_stacks_block_id = 55c9861be5cff984a20ce6d99d4aa65941412889bdc665094136429b84f8c2ee")


failures:
    chainstate::nakamoto::tests::nakamoto_advance_tip_multiple
    chainstate::nakamoto::tests::nakamoto_advance_tip_simple

test result: FAILED. 1241 passed; 2 failed; 125 ignored; 0 measured; 0 filtered out; finished in 416.59s

Any ideas what in here might have broken these tests @kantai?

jcnelson · 2023-11-09T16:54:29Z

I've scrapped those two tests in my coordinator PR and replaced them into the TestPeer-derived tests (but it's not committed yet). Going forward, there's going to be a lot of things that these tests will need to do in order to produce validate-able Nakamoto blocks, which TestPeer already does for free.

kantai · 2023-11-09T19:26:32Z

Any ideas what in here might have broken these tests @kantai?

I'm not sure -- I know that I needed to fix those tests when I merged some of this work into my previous PR.

I'd recommend rebasing this PR off of next -- that might fix those tests for you. If not, as @jcnelson mentions, those tests are getting supplanted in the coordinator PR anyways, so it may be easier to simply disable them.

jbencin · 2023-11-10T20:07:54Z

so it may be easier to simply disable them.

Okay, I'm just adding #[ignore] to these tests for now

kantai · 2023-11-10T20:27:27Z

Can you rebase this to next?

…sts`

jbencin · 2023-11-10T22:47:21Z

Rebased, force pushed, and changed target to next

kantai

LGTM!

CLAassistant · 2023-11-16T06:30:54Z

All committers have signed the CLA.

jcnelson

A good chunk of this got merged to next as part of the coordinator PR. If you rebase to next then the rest of it is fine with me. Thanks!

jbencin · 2023-11-17T22:41:05Z

There's one more thing I want to do with TenureChange: I want it to use the wsts::common::Signature struct so that we can use it's verify() method. The only problem is that Signature does not implement the necessary derives like Debug and Serialize (even though Point and Scalar do), so I can't put it in TenureChange

I'll make a PR to WSTS adding the derives, and I'll make a separate PR for this to stacks-core

cc @xoloki

jbencin · 2023-11-19T22:45:41Z

Hey @jcnelson I've resolved the conflicts with next and this is ready to merge. Would you like me to wait until you've merged #4074, so we don't risk breaking anything there?

jbencin force-pushed the feat/add-tenure-change branch from 05ab9e9 to 40836f7 Compare September 22, 2023 16:35

jbencin changed the base branch from feat/nakamoto-blocks-pre-rebase to feat/nakamoto-blocks September 22, 2023 17:25

jcnelson reviewed Sep 26, 2023

View reviewed changes

stackslib/src/chainstate/stacks/mod.rs Outdated Show resolved Hide resolved

jcnelson reviewed Sep 26, 2023

View reviewed changes

stackslib/src/chainstate/stacks/transaction.rs Outdated Show resolved Hide resolved

jbencin force-pushed the feat/add-tenure-change branch 2 times, most recently from 2af12b5 to 036153d Compare September 27, 2023 19:45

zone117x mentioned this pull request Oct 4, 2023

[Nakamoto] Support TenureChange Stacks transaction hirosystems/stacks-blockchain-api#1733

Closed

jcnelson reviewed Oct 4, 2023

View reviewed changes

stackslib/src/chainstate/stacks/mod.rs Outdated Show resolved Hide resolved

jcnelson reviewed Oct 4, 2023

View reviewed changes

stackslib/src/chainstate/stacks/mod.rs Show resolved Hide resolved

jcnelson reviewed Oct 4, 2023

View reviewed changes

EmbeddedAndroid linked an issue Oct 17, 2023 that may be closed by this pull request

Nakamoto: New TenureChange Stacks transaction #3944

Closed

jbencin mentioned this pull request Oct 17, 2023

Feat: Signature order-independence fix for multisig tx #3710

Merged

5 tasks

jcnelson reviewed Oct 18, 2023

View reviewed changes

Cargo.lock Outdated Show resolved Hide resolved

jcnelson reviewed Oct 18, 2023

View reviewed changes

zone117x mentioned this pull request Oct 26, 2023

feat: support for TenureChange transaction types hirosystems/stacks-encoding-native-js#14

Merged

jbencin force-pushed the feat/add-tenure-change branch from 8a1b9c9 to 28dd0a6 Compare November 7, 2023 19:23

jbencin requested review from jcnelson and kantai November 10, 2023 20:09

Address a couple comments from Jude

97bfaa4

jbencin added 2 commits November 10, 2023 17:23

Add threshold signature to TenureChange message using types from `w…

694b760

…sts`

Add tests for TenureChangePayload ser/deser

f287648

jbencin force-pushed the feat/add-tenure-change branch from 7ff13df to 2c5593d Compare November 10, 2023 22:43

Factor signature out of TenureChangePayload

f98f013

jbencin force-pushed the feat/add-tenure-change branch from 2c5593d to 34b07e8 Compare November 10, 2023 22:45

Ignore nakamoto_advance_tip_* tests

34b07e8

jbencin changed the base branch from feat/nakamoto-blocks to next November 10, 2023 22:46

kantai approved these changes Nov 15, 2023

View reviewed changes

jcnelson approved these changes Nov 17, 2023

View reviewed changes

jbencin marked this pull request as ready for review November 17, 2023 23:10

jbencin force-pushed the feat/add-tenure-change branch from 35045e2 to c15f8b7 Compare November 19, 2023 22:40

Merge 'stacks/next' into feat/add-tenure-change

c15f8b7

Merge branch 'stacks/next' into feat/add-tenure-change

252b77f

jbencin merged commit 2980e17 into stacks-network:next Nov 21, 2023
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat: Add `TenureChange` transaction type #3974

Feat: Add `TenureChange` transaction type #3974

jbencin commented Sep 21, 2023 •

edited

Loading

codecov bot commented Sep 21, 2023 •

edited

Loading

jcnelson Oct 4, 2023

kantai Oct 27, 2023

jcnelson Nov 6, 2023

jcnelson Oct 4, 2023

jbencin Oct 16, 2023

jcnelson Oct 18, 2023

jcnelson Oct 18, 2023

jbencin Oct 18, 2023

fess-v Oct 18, 2023

kantai Oct 18, 2023

fess-v Oct 18, 2023

kantai Oct 27, 2023

jcnelson Nov 6, 2023

jcnelson Oct 18, 2023

jcnelson commented Nov 6, 2023

jbencin commented Nov 7, 2023

kantai commented Nov 7, 2023

jbencin commented Nov 7, 2023

jbencin commented Nov 9, 2023

jcnelson commented Nov 9, 2023

kantai commented Nov 9, 2023

jbencin commented Nov 10, 2023

kantai commented Nov 10, 2023

jbencin commented Nov 10, 2023

kantai left a comment

CLAassistant commented Nov 16, 2023 •

edited

Loading

jcnelson left a comment

jbencin commented Nov 17, 2023

jbencin commented Nov 19, 2023

Feat: Add TenureChange transaction type #3974

Feat: Add TenureChange transaction type #3974

Conversation

jbencin commented Sep 21, 2023 • edited Loading

Description

Applicable issues

Additional info (benefits, drawbacks, caveats)

Checklist

codecov bot commented Sep 21, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcnelson commented Nov 6, 2023

jbencin commented Nov 7, 2023

kantai commented Nov 7, 2023

jbencin commented Nov 7, 2023

jbencin commented Nov 9, 2023

jcnelson commented Nov 9, 2023

kantai commented Nov 9, 2023

jbencin commented Nov 10, 2023

kantai commented Nov 10, 2023

jbencin commented Nov 10, 2023

kantai left a comment

Choose a reason for hiding this comment

CLAassistant commented Nov 16, 2023 • edited Loading

jcnelson left a comment

Choose a reason for hiding this comment

jbencin commented Nov 17, 2023

jbencin commented Nov 19, 2023

Feat: Add `TenureChange` transaction type #3974

Feat: Add `TenureChange` transaction type #3974

jbencin commented Sep 21, 2023 •

edited

Loading

codecov bot commented Sep 21, 2023 •

edited

Loading

CLAassistant commented Nov 16, 2023 •

edited

Loading