Feat/tenure change validation #4114

jcnelson · 2023-12-01T19:55:53Z

This PR addresses #4014 by way of #4053. Tenure-change transactions are now fully validated. In particular:

Tenures can now span multiple sortitions if there was no new miner chosen
Nakamoto can keep making blocks even if a miner does not produce any blocks in its sortition
Tenures can have their runtime budgets extended

Leaving as a draft for now so I can add more unit tests and let CI run on the existing ones.

…cessed tenures and verifying that tenure-changes account for any missed sortitions or tenures. This deviates from the SIP slightly -- there is at most one TC per block, and we only distinguish between TCs that extend the budget and those that create new tenures.

…ase validation, and tenure table structure and queries

…correctly fill in the tenure-change fields

…us hash

…not yet linked to a block-commit it has (e.g. if the commit was the first nakamoto commit)

jcnelson · 2023-12-01T20:32:07Z

Not sure why but CI does not want to run.

stackslib/src/chainstate/nakamoto/mod.rs

xoloki

Minor comments about crypto libs and little things, but LGTM. However I'm not well versed in this code.

stacks-common/src/types/chainstate.rs

stacks-common/src/util/secp256k1.rs

xoloki · 2023-12-01T20:39:25Z

stacks-common/src/util/secp256k1.rs

@@ -156,6 +157,7 @@ impl From<&WSTSSignature> for SchnorrSignature {
        SchnorrSignature(buf)
    }
 }
+*/

 impl Secp256k1PublicKey {


Ideally we'd also move away from other libsecp256k1 wrappers. p256k1 wraps everything, including public keys, ecdsa/schnorr sigs, etc.

stackslib/src/chainstate/nakamoto/tests/node.rs

kantai

Have a few comments. Most are superficial, but there's a handful that I think merit some discussion.

stackslib/src/chainstate/nakamoto/mod.rs

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

stackslib/src/clarity_vm/clarity.rs

stackslib/src/chainstate/nakamoto/mod.rs

stackslib/src/chainstate/stacks/mod.rs

jbencin

Made some comments, minor stuff. I'll let Aaron give the second approval though because I'm not confident that I understand all the details here

stackslib/src/chainstate/nakamoto/mod.rs

jbencin · 2023-12-06T21:31:33Z

stackslib/src/chainstate/nakamoto/mod.rs

            parent_microblocks_cost: ExecutionCost::zero(),
            anchored_block_cost: block_execution_cost,
            parent_burn_block_hash,
-            parent_burn_block_height,
+            parent_burn_block_height: u32::try_from(parent_burn_block_height).unwrap_or(0), // shouldn't be fatal


If parent_burn_block_height is to large for u32 should it default to u32::MAX?

I wanted the error to be very obvious

stackslib/src/chainstate/stacks/transaction.rs

stackslib/src/chainstate/nakamoto/mod.rs

kantai

This looks good to me, I just had a few comments which are mostly superficial. This PR also currently breaks the stacks-node target, so that must be fixed before it can be approved.

$ cargo check
...
error: could not compile `stacks-node` (bin "stacks-node") due to 7 previous errors; 2 warnings emitted

jferrant · 2023-12-11T14:49:22Z

stacks-common/src/util/secp256k1.rs

@@ -118,45 +118,6 @@ impl Default for Secp256k1PublicKey {
    }
 }

-pub struct SchnorrSignature(pub [u8; 65]);


Really nice to see this go XD Didn't like it in there to begin with.

…o return the height, and add a backwards-compatible wrapper. In addition, add a test method for setting the canonical tip

…processed_tenure()

jferrant · 2023-12-11T21:10:44Z

stackslib/src/chainstate/nakamoto/coordinator/tests.rs

+            assert_eq!(matured_reward.parent_miner.coinbase, 1000_000_000);
+        }
+
+        if i < 11 {


Super nit especially since in a test, but this would be clearer to me with a match on a std::cmp
Like so

match i.cmp(&11) { Ordering::Less => {...}, Ordering::Greater => {...}, Ordering::Equal => {...}, }

jferrant · 2023-12-11T21:11:23Z

stackslib/src/chainstate/nakamoto/coordinator/tests.rs

+        } else {
+            assert_eq!(miner_reward.coinbase, 1000_000_000);
+        }
+        if i < 10 {


Similar to above comment.

kantai

LGTM, thanks for the changes!

…acks-network/stacks-blockchain into feat/tenure-change-validation

blockstack-devops · 2024-11-08T00:20:29Z

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.

jcnelson added 13 commits December 1, 2023 12:21

chore: getters for previous mock miner's block-commits and blocks

ab8a906

refactor: use new matured miner reward struct

4348679

testing: expand unit testing for tenure-start block validation, coinb…

948595c

…ase validation, and tenure table structure and queries

testing: require nodes to begin Nakamoto off of an epoch2 block, and …

13b0f1b

…correctly fill in the tenure-change fields

chore: sync with tenure-change struct

bf865d4

feat: add support for tenure budget extension

b753a7c

feat: tenure-changes identify current and previous tenures by consens…

60fee44

…us hash

testing: find the last anchored block in the mock miner even if it's …

3a700cb

…not yet linked to a block-commit it has (e.g. if the commit was the first nakamoto commit)

chore: try_as_tenure_change()

a0386c2

chore: add reset_cost() function for tenure-change budget extension

461b282

chore: log block rejection

ad1f178

chore: tenure-change requires prev_consensus_hash

a276e50

jcnelson requested review from xoloki, kantai, jbencin and jferrant December 1, 2023 19:55

Merge branch 'next' into feat/tenure-change-validation

690fcdc

jcnelson marked this pull request as ready for review December 1, 2023 20:31

jcnelson marked this pull request as draft December 1, 2023 20:31

fix: workspace dependency

5f92195

kantai reviewed Dec 1, 2023

View reviewed changes

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

xoloki approved these changes Dec 1, 2023

View reviewed changes

chore: remove commented-out code

d650b72

kantai reviewed Dec 1, 2023

View reviewed changes

zone117x reviewed Dec 6, 2023

View reviewed changes

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

jbencin reviewed Dec 6, 2023

View reviewed changes

jcnelson marked this pull request as ready for review December 7, 2023 14:50

chore: cargo fmt

38af405

kantai reviewed Dec 7, 2023

View reviewed changes

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

kantai reviewed Dec 7, 2023

View reviewed changes

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

kantai reviewed Dec 7, 2023

View reviewed changes

jcnelson requested review from kantai and jbencin December 8, 2023 17:32

chore: address PR feedback and get testnet to build

37e134b

janniks mentioned this pull request Dec 8, 2023

feat: handle new nakamoto wire format hirosystems/clarinet#1283

Closed

jferrant reviewed Dec 11, 2023

View reviewed changes

jcnelson added 3 commits December 11, 2023 15:01

Merge branch 'next' into feat/tenure-change-validation

3f02431

chore: extend SortitionDB::get_canonical_stacks_tip_block_hash to als…

8155592

…o return the height, and add a backwards-compatible wrapper. In addition, add a test method for setting the canonical tip

chore: modify tests to work with new NakamotoChainState::get_highest_…

ded3b54

…processed_tenure()

jferrant reviewed Dec 11, 2023

View reviewed changes

kantai approved these changes Dec 11, 2023

View reviewed changes

jcnelson added 9 commits December 11, 2023 17:15

chore: pull forward canonical block header pointer

863ae14

chore: API sync

5dfbb25

chore: remove dead code

6934d7d

chore: query highest tenure from memoized canonical stacks chain tip

e52e570

fix: get fork test to work with new highest-tenure query

375db58

Merge branch 'feat/tenure-change-validation' of https://github.com/st…

0b835e0

…acks-network/stacks-blockchain into feat/tenure-change-validation

Merge branch 'next' into feat/tenure-change-validation

221b4ba

Merge branch 'next' into feat/tenure-change-validation

6ef458f

Merge branch 'next' into feat/tenure-change-validation

f751c40

saralab added the neon3.0 label Dec 12, 2023

saralab added this to the Nakamoto Milestone 0.2 (code name: Neon) milestone Dec 12, 2023

jcnelson merged commit 58e15c8 into next Dec 12, 2023
2 checks passed

zone117x mentioned this pull request Dec 13, 2023

feat: update to latest TenureChange wire-format hirosystems/stacks-encoding-native-js#16

Merged

blockstack-devops added the locked label Nov 8, 2024

stacks-network locked as resolved and limited conversation to collaborators Nov 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/tenure change validation #4114

Feat/tenure change validation #4114

jcnelson commented Dec 1, 2023

jcnelson commented Dec 1, 2023

xoloki left a comment

xoloki Dec 1, 2023

kantai left a comment

jbencin left a comment

jbencin Dec 6, 2023

jcnelson Dec 7, 2023

kantai left a comment

jferrant Dec 11, 2023

jferrant Dec 11, 2023 •

edited

Loading

jferrant Dec 11, 2023

kantai left a comment

blockstack-devops commented Nov 8, 2024

Feat/tenure change validation #4114

Feat/tenure change validation #4114

Conversation

jcnelson commented Dec 1, 2023

jcnelson commented Dec 1, 2023

xoloki left a comment

Choose a reason for hiding this comment

xoloki Dec 1, 2023

Choose a reason for hiding this comment

kantai left a comment

Choose a reason for hiding this comment

jbencin left a comment

Choose a reason for hiding this comment

jbencin Dec 6, 2023

Choose a reason for hiding this comment

jcnelson Dec 7, 2023

Choose a reason for hiding this comment

kantai left a comment

Choose a reason for hiding this comment

jferrant Dec 11, 2023

Choose a reason for hiding this comment

jferrant Dec 11, 2023 • edited Loading

Choose a reason for hiding this comment

jferrant Dec 11, 2023

Choose a reason for hiding this comment

kantai left a comment

Choose a reason for hiding this comment

blockstack-devops commented Nov 8, 2024

jferrant Dec 11, 2023 •

edited

Loading