Nakamoto staging blocks #3992

kantai · 2023-10-06T01:45:08Z

This PR has the initial work for handling nakamoto staging blocks.

Currently, the staging blocks table is part of the same chainstate sqlite db as the old staging blocks table. This simplifies things initially, but part of the nakamoto workstream will be putting that in its own sqlite db.

This extends the TenureChange PR with a few more items:

Adds Epoch30. This patch will need to be reconciled with Add epoch 3.0 #3954, but that should be relatively easy.
It adds miner pubkey hash tracking to the sortition db. This isn't discussed in the nakamoto SIP, but I think this is a useful thing for the implementation: the miner's VRF key register transaction should include the Hash160 of the miner's pubkey to be used in its tenures. Otherwise, stacks nodes would have to wait until they've processed the tenure change transaction to validate a signature.

It also adds three methods for the staging blocks: getting the next ready block, marking a block as processed, and marking a burn block as processed. Because a block's "burn view" is somewhat independent of the burn block that selected its tenure, this should be tracked separately, so the nakamoto staging blocks table tracks "burn_attachable" and "stacks_attachable".

…king in sortition db, add epoch30

codecov · 2023-10-06T01:46:30Z

Codecov Report

Merging #3992 (e985b17) into next (5f310ec) will decrease coverage by 0.01%.
Report is 2 commits behind head on next.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             next    #3992      +/-   ##
==========================================
- Coverage    0.16%    0.16%   -0.01%     
==========================================
  Files         350      350              
  Lines      291672   292190     +518     
==========================================
  Hits          469      469              
- Misses     291203   291721     +518

Files	Coverage Δ
clarity/src/vm/analysis/mod.rs	`0.00% <ø> (ø)`
stacks-common/src/util/hash.rs	`0.00% <ø> (ø)`
stacks-common/src/util/macros.rs	`0.00% <ø> (ø)`
stackslib/src/chainstate/burn/mod.rs	`0.00% <ø> (ø)`
stackslib/src/cost_estimates/fee_scalar.rs	`0.00% <ø> (ø)`
clarity/src/vm/analysis/type_checker/mod.rs	`0.00% <0.00%> (ø)`
clarity/src/vm/costs/mod.rs	`0.00% <0.00%> (ø)`
clarity/src/vm/functions/mod.rs	`0.00% <0.00%> (ø)`
clarity/src/vm/version.rs	`0.00% <0.00%> (ø)`
stackslib/src/chainstate/stacks/block.rs	`0.00% <0.00%> (ø)`
... and 20 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

jcnelson · 2023-10-09T14:22:20Z

stackslib/src/chainstate/burn/db/sortdb.rs

+    /// current chain tip: only recent tenures can receive blocks this
+    /// way. Otherwise, the `BlockHeaderHash` must have been
+    /// explicitly confirmed by a block commit.
+    pub fn expects_blocks_from_tenure(


I'm having a hard time reconciling the docstring with the code body. The code body returns the latest BlockSnapshot whose block-signing key hash matches the given public key, if there any such snapshots within the last NAKAMOTO_TENURE_BLOCK_ACCEPTANCE_PERIOD sortitions. What does this have to do with "expect[ing] to receive unknown blocks?" Also, it seems miners can reuse the same signing key in multiple tenures, so if there were two snapshots from the same miner in this window, this method would not return the earlier one. Is this desired?

Good point. I think this should be a bool instead.

At first, I intended to use this result to figure out the consensus hash that started the miner's tenure. However, for exactly the reason you point out, it cannot do that (because the miner's key could be reused). Instead, I think that the Nakamoto block headers should contain the consensus hash of their tenure. There's other ways to figure out their associated tenure, I'm sure, but it dramatically simplifies things if the block's include it. I know it's a somewhat counter-intuitive change from Stacks 2.0's perspective: in 2.0, a miner cannot know their consensus hash when they produce their block, but because block production happens after their tenure starts in 3.0, they actually do know their consensus hash.

Instead, I think that the Nakamoto block headers should contain the consensus hash of their tenure.

I agree -- we should use this instead of BurnchainHeaderHash for burn_view.

I'll do this when working on the coordinator

jcnelson · 2023-10-09T14:23:02Z

stackslib/src/chainstate/burn/operations/leader_key_register.rs

@@ -77,6 +77,12 @@ impl LeaderKeyRegisterOp {
        Some(LeaderKeyRegisterOp::new(&prover_pubk))
    }

+    /// Interpret the first 20 bytes of the key registration's memo field as the Hash160 of
+    ///  of the public key that will sign this miner's nakamoto blocks.
+    pub fn interpret_nakamoto_signing_key(&self) -> Option<Hash160> {


Ah, interesting idea! This should get added to the SIP and design doc. Can you add a quick one-sentence comment in both places so we remember to merge it in? Thanks!

Also, this field should probably be explicit in LeaderKeyRegisterOp as an Option<..>. The field would be required to be Some(..) in epoch 3.0, but it's not considered in earlier epochs.

I think there is a little bit of code clarity tradeoff with adding a new field to the struct, and I'm genuinely not sure which would work out more elegantly. If there's a new field in the struct, there's two kind of immediate impacts:

The db schema of the Sortition DB needs to change and FromRow/ToSql need to handle the new field. The db schema of the burnchain DB doesn't change, but the deserialization of the opcode would need to make sure that it handles the absence of the field correctly. Or the schema of the SortitionDB doesn't change and the FromRow / Insert method handles shuttling the new field via the existing memo column.

The consensus deserialize/serialize needs to be sure to be compatible. This is pretty straight forward and I'm not worried about its impact.

In light of (1), do you still think there should be a new field for this struct?

I think that in each breaking change, we've required users to spin up from genesis anyway, so introducing a new field which renders the Stacks 2.x chainstate incompatible I think is fine (especially if it simplifies the consensus code).

I can take this on

jcnelson · 2023-10-09T14:25:05Z

stackslib/src/chainstate/coordinator/mod.rs

@@ -2141,6 +2141,16 @@ impl<
        self.inner_handle_new_burnchain_block(&mut HashSet::new())
    }

+    /// Are affirmation maps active during the epoch?


Good news -- they're going away in Epoch 3.0. There's no longer a need for them because the chain can't fork.

Yeah, I agree. But I think the code may have to stay in place for a while, and the intention here is to have a function that encapsulates the decision on whether an affirmation map check should occur.

Ah, I meant to say that the check could return false for epoch 3.0.

jcnelson · 2023-10-09T17:02:51Z

stackslib/src/chainstate/nakamoto/mod.rs

-    pub fn tenure_changed(&self) -> bool {
-        // TODO: when tenure change txs are implemented, this must be updated
-        true
+    pub fn tenure_changed(&self, parent: &StacksBlockId) -> bool {


I think it might make more sense to return the list of TenureChange transactions here. The tenure can change multiple times in a block, since it has to contain all missing TenureChange transactions as well.

I think that will be necessary once the TenureChange transactions are actually getting validated, but right now, all the stacks block processor cares about is whether or not a tenure change occurred.

jcnelson · 2023-10-09T17:04:45Z

stackslib/src/chainstate/nakamoto/mod.rs

+            return ChainstateError::InvalidStacksBlock("Unrecoverable miner public key".into());
+        })?;
+
+        if sortdb


I think this whole if-block needs to be dropped. A node can accept any block back to the last PoX anchor block, especially while booting up.

I think there needs to be two different acceptance paths for blocks:

Any block that has been committed to by a leader block commit -- leader block commits still commit to block hashes, but those are not the chain tip. These are used to accept blocks while booting up, catching up to the chain tip.

The if block in the code above: while operating at the chain tip, the only blocks that should be accepted are ones in recent tenures.

jcnelson · 2023-10-18T11:11:34Z

stackslib/src/chainstate/nakamoto/tests.rs

-        "0c5e890e95e2f92ef36934bc0e5d71a6715974593f7d952b07ee6f959dae3f1c",
-        "0cd7696c4920ea5bc498ea46ee1df8566e06ea0bc8fd16a1e0ffd292d55f746e",
-        "84177188b1c02af772d2442b760fd9215b9dfadeed5723504acda2c94b068d15",
+        "c76d48e971b2ea3c78c476486455090da37df260a41eef355d4e9330faf166c0",


I ended up changing these hashes when I was working on the coordinator (since I had to change some of the header structs, which changes the state root hash). That's okay, right?

Yeah, it's fine -- perhaps after factoring the mining routine in the mockamoto node, we should update this test anyways so that its able to figure out its own hashes.

jcnelson

Thanks @kantai!

saralab · 2023-10-28T14:48:21Z

@obycode @jbencin : We need your attention on this PR, can you start reviewing this? This is critical for Mockamoto readiness

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

obycode · 2023-10-30T02:44:45Z

stackslib/src/core/mod.rs

@@ -428,6 +432,10 @@ pub static STACKS_EPOCH_2_3_MARKER: u8 = 0x08;
 /// *or greater*.
 pub static STACKS_EPOCH_2_4_MARKER: u8 = 0x09;

+/// Stacks 3.0 epoch marker.  All block-commits in 2.4 must have a memo bitfield with this value


Suggested change

/// Stacks 3.0 epoch marker. All block-commits in 2.4 must have a memo bitfield with this value

/// Stacks 3.0 epoch marker. All block-commits in 3.0 must have a memo bitfield with this value

stackslib/src/core/mod.rs

stackslib/src/chainstate/burn/db/sortdb.rs

stackslib/src/chainstate/nakamoto/mod.rs

muneeb-ali · 2023-10-30T20:57:28Z

Awesome to see this being reviewed!

"miner pubkey hash tracking to the sortition db. This isn't discussed in the nakamoto SIP"

If this is not in the SIP already then probably worth adding. The SIPs will be the first place a lot of devs will look for information and can help their understanding of how the code works.

obycode

LGTM! 🚀

kantai · 2023-10-31T14:45:42Z

Awesome to see this being reviewed!

"miner pubkey hash tracking to the sortition db. This isn't discussed in the nakamoto SIP"

If this is not in the SIP already then probably worth adding. The SIPs will be the first place a lot of devs will look for information and can help their understanding of how the code works.

Yep -- opened as a PR against the current SIP (stacksgov/sips#158)

blockstack-devops · 2024-11-09T00:20:55Z

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.

jbencin and others added 7 commits September 27, 2023 15:40

feat: Add TenureChange transaction type

9c491ed

fix: Do not allow TenureChange transactions in mempool

036153d

tenure_change() returns true if there is a TenureChange transaction

e325a89

Working on tenure_changed()...

2481a49

Merge branch 'feat/nakamoto-blocks' into feat/nakoordinator

c53dd0f

feat: initial work on nakamoto_staging_blocks, miner pubkey hash trac…

b966e95

…king in sortition db, add epoch30

add initial test case for nakamoto_staging_blocks

3c2093a

fix sortdb migration test

ccc9c58

jcnelson reviewed Oct 9, 2023

View reviewed changes

jcnelson and others added 2 commits October 9, 2023 16:54

Merge branch 'feat/nakamoto-blocks' into feat/nakoordinator

45dd615

Merge changes from feat/nakamoto-blocks

fa8ec88

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

Nakamoto: Chain State Table for Nakamoto Headers #3859

Closed

jcnelson mentioned this pull request Oct 18, 2023

Feat: Add TenureChange transaction type #3974

Merged

8 tasks

jcnelson reviewed Oct 18, 2023

View reviewed changes

jcnelson approved these changes Oct 18, 2023

View reviewed changes

kantai requested a review from jbencin October 25, 2023 17:46

kantai changed the title ~~DRAFT: Nakamoto staging blocks~~ Nakamoto staging blocks Oct 25, 2023

kantai changed the base branch from feat/nakamoto-blocks to next October 25, 2023 17:47

kantai self-assigned this Oct 26, 2023

saralab requested a review from obycode October 28, 2023 14:42

obycode reviewed Oct 30, 2023

View reviewed changes

obycode mentioned this pull request Oct 30, 2023

Epoch 3.0 gating (in blockchain repo) stacks-network/clarity-wasm#91

Closed

address PR review

e985b17

kantai requested a review from obycode October 30, 2023 20:38

saralab requested a review from jferrant October 30, 2023 21:17

obycode approved these changes Oct 30, 2023

View reviewed changes

kantai merged commit d267ca6 into next Oct 31, 2023
1 check passed

This was referenced Nov 3, 2023

Nakamoto: Chain State Table for Nakamoto Headers #3859

Closed

Nakamoto: Chain State Interface for Nakamoto Block Processing #3861

Closed

Nakamoto: New DB for Nakamoto Staging Blocks #3860

Closed

Nakamoto: Query method for canonical Stacks block #3862

Closed

jcnelson mentioned this pull request Feb 13, 2024

Separate Nakamoto DB for staging blocks #4373

Merged

blockstack-devops added the locked label Nov 9, 2024

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Nakamoto staging blocks #3992

Nakamoto staging blocks #3992

kantai commented Oct 6, 2023

codecov bot commented Oct 6, 2023 •

edited

Loading

jcnelson Oct 9, 2023

kantai Oct 9, 2023

jcnelson Oct 9, 2023

jcnelson Oct 9, 2023

jcnelson Oct 9, 2023

jcnelson Oct 9, 2023

kantai Oct 9, 2023

jcnelson Oct 9, 2023

jcnelson Oct 9, 2023

jcnelson Oct 9, 2023

kantai Oct 9, 2023

jcnelson Oct 9, 2023

jcnelson Oct 9, 2023

kantai Oct 9, 2023

jcnelson Oct 9, 2023

kantai Oct 9, 2023

jcnelson Oct 18, 2023

kantai Oct 18, 2023

jcnelson left a comment

saralab commented Oct 28, 2023 •

edited

Loading

obycode Oct 30, 2023

muneeb-ali commented Oct 30, 2023 •

edited

Loading

obycode left a comment

kantai commented Oct 31, 2023

blockstack-devops commented Nov 9, 2024

	/// Stacks 3.0 epoch marker. All block-commits in 2.4 must have a memo bitfield with this value
	/// Stacks 3.0 epoch marker. All block-commits in 3.0 must have a memo bitfield with this value

Nakamoto staging blocks #3992

Nakamoto staging blocks #3992

Conversation

kantai commented Oct 6, 2023

codecov bot commented Oct 6, 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

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 left a comment

Choose a reason for hiding this comment

saralab commented Oct 28, 2023 • edited Loading

Choose a reason for hiding this comment

muneeb-ali commented Oct 30, 2023 • edited Loading

obycode left a comment

Choose a reason for hiding this comment

kantai commented Oct 31, 2023

blockstack-devops commented Nov 9, 2024

codecov bot commented Oct 6, 2023 •

edited

Loading

saralab commented Oct 28, 2023 •

edited

Loading

muneeb-ali commented Oct 30, 2023 •

edited

Loading