Burn state handling in the Clarity VM #4789

obycode · 2024-05-14T01:08:43Z

Description

In Nakamoto, a transaction can access the current burn block, while in epoch 2.x, it can only access the burn block of its parent. This changeset updates the behavior, so that every block in a tenure, including the first block, can access the burn block information of the tenure's burn block.

Note, this is built off of #4745. I currently have this PR targeting that branch, so the new changes are clear. Once that one is merged, I will set this back to target develop.

Applicable issues

fixes Nakamoto: Burn state handling in the Clarity VM #4333

Checklist

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

In epoch 2, a Stacks block can only access the burn block associated with its parent, since the block is buily before its burn block is known. In epoch 3, all Nakamoto blocks can access the current burn block.

clarity/src/vm/database/clarity_db.rs

Co-authored-by: Jeff Bencin <jeff.bencin@gmail.com>

kantai

This mostly looks good to me -- I had a handful of comments, but I think the approach for SortitionDBConn needs to be changed.

kantai · 2024-05-17T01:57:49Z

clarity/src/vm/database/clarity_db.rs

-                .into()
-            })
+        // In epoch 2, we can only access the burn block associated with the last block
+        if self.get_clarity_epoch_version()? < StacksEpochId::Epoch30 {


I think we should prefer defining characteristics of epochs as methods of StacksEpochId:

/// Whether or not this epoch uses the tip for reading Clarity burn block information (3.0+ behavior) /// or should use the parent block's burn block (behavior before 3.0) pub fn clarity_uses_tip_burn_block(&self) -> bool

And then, self.get_clarity_epoch_version().clarity_uses_tip_burn_block().

I think the advantages to this are that (1) we don't repeat the same gating logic in multiple places, and (2) the StacksEpochId can become a pretty reliable place for seeing which features activated when.

Good idea. I now see the other similar methods there. I see most of these methods use a match to make it explicit for each epoch. Is that preferred over just using a >= so that it needs to be considered for each new epoch?

Added method in e29ab98.

kantai · 2024-05-17T02:04:51Z

clarity/src/vm/tests/mod.rs

+        let mut db = self.0.as_clarity_db();
+        db.begin();
+        db.set_clarity_epoch_version(epoch).unwrap();
+        db.commit().unwrap();
+        if epoch >= StacksEpochId::Epoch30 {
+            db.begin();
+            db.set_tenure_height(1).unwrap();
+            db.commit().unwrap();
+        }
+        let mut owned_env = OwnedEnvironment::new(db, epoch);


Should these changes have happened in the previous PR? I thought memory env tests were passing okay in that PR without this.

Yes, they should have, but I believe the problem wasn't hit in the previous PR because of the special case handling of block 0.

kantai · 2024-05-17T02:29:43Z

stackslib/src/clarity_vm/database/mod.rs

+    fn get_tip_burn_block_height(&self) -> Option<u32> {
+        let tip = SortitionDB::get_canonical_burn_chain_tip(self.conn()).ok()?;
+        tip.block_height.try_into().ok()
+    }
+
+    fn get_tip_sortition_id(&self) -> Option<SortitionId> {
+        let tip = SortitionDB::get_canonical_burn_chain_tip(self.conn()).ok()?;
+        Some(tip.sortition_id)
+    }


I'm not 100% sure that this is safe. I think the SortitionDBConn implementation is used by the mining path, while the SortitionHandleTx is used by the block processor. I think that this leads to the "correct" behavior: the SortitionHandleTx has its chain_tip field set to the "burn_view", and when mining, the canonical tip should be the tip (but I'm not sure that it always will be -- maybe a sortition could be processed between the block starting mining and finishing mining?). Either way, this seems like something that could come back to bite us someday (I think we've been bitten before by a difference between the SortitionDBConn and SortitionHandleTx impls).

My suggestion is to replace impl BurnStateDB for SortitionDBConn with impl BurnStateDB for SortitionHandleConn (which would require updating any consumers of this impl to create handles). I think this is safer in the long run (and HandleConn and HandleTx are close enough to each other, that maybe at some point, we use either a macro or the SortitionHandle trait to specify the exact same impl for both).

Ok, I pushed a change for this, but it's possible that I may have gone overboard and let this extend out further than necessary. I'll re-review this change tomorrow.

Also, SortitionDB::get_canonical_burn_chain_tip() is NOT safe to use in consensus code. Both SortitionDBConn and SortitionHandleTx have self.context.sortition_id, which represents their respective views of the burnchain tip. They're guaranteed not to change over the course of their respective lifetimes, whereas the result of SortitionDB::get_canonical_burn_chain_tip() can change from call to call from outside of the coordinator thread.

Furthermore, even within the coordinator thread, SortitionDB::get_canonical_burn_chain_tip() can return arbitrarily different values between Nakamoto blocks, which would lead to a chain split If used in this manner. For example, suppose tenure T had 10 blocks, labeled T0 through T9. It's entirely possible that node A processes a new burnchain block in-between processing T3 and T4, while node B processes the same new burnchain block between processing T5 and T6. Now, nodes A and B may diverge due to different return values from this impl.

To further elaborate, the only safe place to call SortitionDB::get_canonical_burn_chain_tip() in consensus code is within code reachable from either ChainsCoordinator::handle_new_epoch2_burnchain_block() or ChainsCoordinator::handle_new_nakamoto_burnchain_block(). Only in these code paths is the value returned by SortitionDB::get_ccanonical_burn_chain_tip() guaranteed to be consistent, because these are the only two places where new sortitons can get created.

Use `SortitionHandleConn` instead of `SortitionDBConn`. This change required propagation through many locations.

jcnelson · 2024-05-22T19:00:45Z

stackslib/src/chainstate/coordinator/mod.rs

@@ -3274,7 +3274,7 @@ impl<
                    if let Some(ref mut estimator) = self.cost_estimator {
                        let stacks_epoch = self
                            .sortition_db
-                            .index_conn()
+                            .index_handle_at_tip()


Can you avoid using this in consensus code? It can panic instead of propagating an underlying DB error. Perhaps add a fallible variant of index_handle_at_tip?

jcnelson · 2024-05-22T19:02:23Z

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

-                    })
-                })
+                .with_read_only_unconfirmed_clarity_tx(
+                    &sortdb.index_handle_at_tip(),


Same here -- let's use a fallible variant

Does it matter here, since this is in a test?

jcnelson · 2024-05-22T19:02:36Z

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

-                    })
-                })
+                .with_read_only_clarity_tx(
+                    &sortdb.index_handle_at_tip(),


jcnelson · 2024-05-22T19:03:29Z

stackslib/src/clarity_vm/database/mod.rs

+        let readonly_marf = self
+            .index
+            .reopen_readonly()
+            .expect("BUG: failure trying to get a read-only interface into the sortition db.");


Can you just return an error here instead?

jcnelson · 2024-05-22T19:15:27Z

stackslib/src/net/api/callreadonly.rs

-                    )
-                })
+                chainstate.maybe_read_only_clarity_tx(
+                    &sortdb.index_handle_at_tip(),


Please use something fallible here

jcnelson · 2024-05-22T19:16:02Z

stackslib/src/net/api/getaccount.rs

-                                .unwrap_or_else(|| (STXBalance::zero(), None))
-                        };
+                chainstate.maybe_read_only_clarity_tx(
+                    &sortdb.index_handle_at_tip(),


Please use something fallible here

This gets used all over the place in this PR, so I'm just going to summarize my feelings in a review comment instead.

jcnelson

There are at least two consensus bugs here:

You cannot call SortitionDB::get_canonical_burn_chain_tip() from chainstate-processing code, since it can change arbitrarily between two successive calls. Only the chains coordinator can safely call this, and only in specific places. This also applies to anything related to loading "canonical" data, including SortitionDB::get_canonical_sortition_tip() (see next point).
It is also not safe to call SortitionDB::index_handle_at_tip() or SortitionDB::tx_begin_at_tip(). Both of these call SortitionDB::get_canonical_sortition_tip() internally, and furthermore, both of these contain a needless .unwrap(). These two functions really need to be marked as #[cfg(any(test, feature = "testing"))], since they're not meant to be used in any other context.

obycode · 2024-05-29T00:55:31Z

I'm struggling trying to get an implementation of this SortitionDB method to work correctly:

pub fn index_handle_at_block<'a>(
        &'a self,
        stacks_block_id: &StacksBlockId,
    ) -> Result<SortitionHandleConn<'a>, db_error> { ... }

For example, this would be called in places like StacksChainState::eval_boot_code_read_only to get the iconn at the appropriate block. What would be the correct way to do this?

obycode · 2024-05-29T02:40:12Z

I currently am working with this version that adds a chainstate parameter. Will push an update soon. A few tests still fail after this change.

    pub fn index_handle_at_block<'a>(
        &'a self,
        chainstate: &StacksChainState,
        stacks_block_id: &StacksBlockId,
    ) -> Result<SortitionHandleConn<'a>, db_error> {
        let (consensus_hash, bhh) = match chainstate.get_block_header_hashes(stacks_block_id) {
            Ok(Some(x)) => x,
            _ => return Err(db_error::NotFoundError),
        };
        let snapshot = SortitionDB::get_block_snapshot_consensus(&self.conn(), &consensus_hash)?
            .ok_or(db_error::NotFoundError)?;
        Ok(self.index_handle(&snapshot.sortition_id))
    }

obycode force-pushed the fix/burn-state branch 2 times, most recently from 6ebdb01 to 822813f Compare May 15, 2024 17:38

obycode requested review from a team as code owners May 15, 2024 17:38

obycode changed the base branch from develop to feat/stacks-block-height May 15, 2024 17:39

obycode changed the base branch from feat/stacks-block-height to develop May 15, 2024 22:02

obycode force-pushed the fix/burn-state branch from 6ab5130 to 9eb0995 Compare May 15, 2024 22:38

obycode added 9 commits May 15, 2024 18:39

feat: access current burn chain state in epoch 3

7d81d22

In epoch 2, a Stacks block can only access the burn block associated with its parent, since the block is buily before its burn block is known. In epoch 3, all Nakamoto blocks can access the current burn block.

test: update check_block_heights for new behavior

cbff7f0

docs: update readme

ecf1763

tests: add new integration test to bitcoin-tests.yml

b5e9069

fix: impl missing methods in docs

f3bed34

test: fill in methods in test structs

18bac17

test: update test_block_heights

228179b

fix: update test_get_burn_block_info_eval

a316dac

refactor: simplify test setup

029d5ef

obycode force-pushed the fix/burn-state branch from 9eb0995 to d94f88d Compare May 15, 2024 22:40

fix: set default block height in test implementation

9439cdc

obycode force-pushed the fix/burn-state branch from d94f88d to 9439cdc Compare May 15, 2024 23:02

jbencin reviewed May 16, 2024

View reviewed changes

clarity/src/vm/database/clarity_db.rs Outdated Show resolved Hide resolved

jbencin reviewed May 16, 2024

View reviewed changes

clarity/src/vm/database/clarity_db.rs Outdated Show resolved Hide resolved

obycode and others added 2 commits May 15, 2024 22:34

refactor: remove unnecessary impls

9e270b7

refactor: simplify expression

44dd3dc

Co-authored-by: Jeff Bencin <jeff.bencin@gmail.com>

jbencin previously approved these changes May 16, 2024

View reviewed changes

kantai reviewed May 17, 2024

View reviewed changes

refactor: add method clarity_uses_tip_burn_block

e29ab98

obycode dismissed jbencin’s stale review via e29ab98 May 17, 2024 14:27

refactor: implement BurnStateDB for SortitionHandleConn

6cfbb17

Use `SortitionHandleConn` instead of `SortitionDBConn`. This change required propagation through many locations.

obycode force-pushed the fix/burn-state branch from f715eeb to 6cfbb17 Compare May 21, 2024 05:11

Merge branch 'develop' into fix/burn-state

40e7517

jcnelson reviewed May 22, 2024

View reviewed changes

jcnelson requested changes May 22, 2024

View reviewed changes

obycode added 2 commits May 28, 2024 22:52

fix: use sortition handle at correct tip

9920ce5

fix: index_handle_at_block for Nakamoto blocks

fa482af

obycode force-pushed the fix/burn-state branch from 7c76f94 to fa482af Compare May 29, 2024 17:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Burn state handling in the Clarity VM #4789

Burn state handling in the Clarity VM #4789

obycode commented May 14, 2024 •

edited

kantai left a comment

kantai May 17, 2024

obycode May 17, 2024 •

edited

obycode May 17, 2024

kantai May 17, 2024

obycode May 17, 2024

kantai May 17, 2024

obycode May 21, 2024

jcnelson May 22, 2024

jcnelson May 22, 2024

jcnelson May 22, 2024

jcnelson May 22, 2024

jcnelson May 22, 2024

obycode May 24, 2024

jcnelson May 22, 2024

jcnelson May 22, 2024

jcnelson May 22, 2024

jcnelson May 22, 2024

jcnelson May 22, 2024

jcnelson left a comment

obycode commented May 29, 2024 •

edited

obycode commented May 29, 2024

Burn state handling in the Clarity VM #4789

Are you sure you want to change the base?

Burn state handling in the Clarity VM #4789

Conversation

obycode commented May 14, 2024 • edited

Description

Applicable issues

Checklist

kantai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

obycode May 17, 2024 • edited

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

obycode commented May 29, 2024 • edited

obycode commented May 29, 2024

obycode commented May 14, 2024 •

edited

obycode May 17, 2024 •

edited

obycode commented May 29, 2024 •

edited