Feat/nakamoto coordinator #4009

jcnelson · 2023-10-24T19:04:19Z

This is a PR which implements the full Nakamoto coordinator. It makes the following notable changes:

Nakamoto blocks now contain only the consensus hash and parent StacksBlockId to identify their placement in the block history. No more parent consensus hash, and no more burn_view.
It reuses the canonical Stacks tip memoization logic from the 2.x days to enable the Sortition DB to track the canonical Stacks tip as blocks get processed
It splits the coordinator's main loop into a 2.x and a Nakamoto subroutine. It will use the 2.x coordinator loop up until the PoX anchor block for the first Nakamoto reward cycle is processed. At this point, the Nakamoto coordinator loop will be used.
It prevents the 2.x coordinator logic from processing Stacks and Bitcoin blocks beyond the start of the Nakamoto epoch.
It requires that the Nakamoto PoX anchor block be the parent of the first Nakamoto block mined in the prepare phase. In doing so, it requires that the node calculate the reward set at the beginning of the prepare phase, instead of at the end.

The last bullet point imposes three additional design requirements: the Nakamoto epoch must start ~100 blocks into a reward phase (so Stackers can come online and DKG can happen), the last 2.x reward cycle must be a PoX reward cycle, and there needs to be a "Nakamoto bootstrap" contract deployed to mainnet in which Stackers can register their signing keys prior to the first Nakamoto reward cycle. This contract would be used just for this purpose, and it cannot be a boot contract.

I still need to do a lot of testing in this PR, but I'm posting it as a draft for now to get early feedback in the design.

…Coordinator

…s (update the structs and DB tables to reflect this). Also, add top-level Nakamoto block-processing.

…t_canonical_block_header()

…o state

…ompatible with both StacksBlock and NakamotoBlock

…th epoch2) and make the remaining directives compatible with epoch3

codecov · 2023-10-24T19:05:21Z

Codecov Report

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

Comparison is base (487126a) 0.16% compared to head (7cf69c7) 0.15%.
Report is 1 commits behind head on next.

Files	Patch %	Lines
stackslib/src/chainstate/nakamoto/tests/mod.rs	0.00%	698 Missing ⚠️
stackslib/src/chainstate/nakamoto/tests/node.rs	0.00%	628 Missing ⚠️
stackslib/src/chainstate/nakamoto/miner.rs	0.00%	533 Missing ⚠️
...ackslib/src/chainstate/nakamoto/coordinator/mod.rs	0.00%	525 Missing ⚠️
...kslib/src/chainstate/nakamoto/coordinator/tests.rs	0.00%	451 Missing ⚠️
stackslib/src/chainstate/stacks/transaction.rs	0.00%	373 Missing ⚠️
stackslib/src/chainstate/stacks/miner.rs	0.00%	311 Missing ⚠️
stackslib/src/core/mod.rs	0.00%	306 Missing ⚠️
stackslib/src/chainstate/burn/db/sortdb.rs	0.00%	222 Missing ⚠️
clarity/src/vm/database/structures.rs	0.00%	204 Missing ⚠️
... and 59 more

Additional details and impacted files

@@            Coverage Diff             @@
##             next    #4009      +/-   ##
==========================================
- Coverage    0.16%    0.15%   -0.01%     
==========================================
  Files         351      355       +4     
  Lines      292666   297188    +4522     
==========================================
  Hits          469      469              
- Misses     292197   296719    +4522

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

kantai

This looks great to me, just have some review comments.

stackslib/src/burnchains/mod.rs

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

stackslib/src/chainstate/nakamoto/mod.rs

testnet/stacks-node/src/event_dispatcher.rs

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

muneeb-ali · 2023-10-30T21:03:54Z

Great to see this PR!

I still need to do a lot of testing in this PR, but I'm posting it as a draft for now to get early feedback in the design.

@jcnelson really appreciate the effort to get early feedback. A lot of devs are really interested in learning/knowing more Nakamoto implementation and sharing things early even when they don't feel fully ready helps a ton with that 🙏 🙌

stackslib/src/burnchains/tests/mod.rs

jferrant · 2023-11-13T14:28:44Z

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

+        Ok(())
+    }
+
+    /// Get a pre-processed reawrd set.


zone117x · 2023-11-13T14:37:17Z

Are these changes integrated and working with the mockamoto mode?

jferrant · 2023-11-13T14:54:33Z

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

+    pub fn set_nakamoto_signing_key(&mut self, pubkey_hash160: &Hash160) {
+        if self.memo.len() < 20 {
+            let mut new_memo = vec![0; 20];
+            new_memo[0..self.memo.len()].copy_from_slice(&self.memo);


I am a bit confused by this.Not sure I am reading this correclty, but this is what I think is happening:

new_memo is intended to pad self.memo with 0s (if it is less than 20 bytes in length), preserving the original self.memo.len() contents of memo. But then it immediately overwrites this with the contents of the pubkey_hash160. What I don't understand is why bother copying the contents of self.memo back into new_memo as it will get overwritten on line 93? Can't we just remove line 90 for the exact same result?

I agree this looks overly complicated. It looks like you're just trying to resize self.memo. How about just doing:

self.memo.resize(20);

In Nakamoto, the memo field has been repurposed to contain the hash160 of the miner's public key (which is 20 bytes). I don't think this is enforced by this PR, but I just opened an issue for it. It'll ship with Neon.

jferrant · 2023-11-13T16:12:25Z

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

+    // did we already calculate the reward cycle info?  If so, then return it.
+    let first_sortition_id = if let Some(first_sn) = prepare_phase_sortitions.first() {
+        if let Some(persisted_reward_cycle_info) =
+            SortitionDB::get_preprocessed_reward_set(sort_db.conn(), &first_sn.sortition_id)?


Is it possible that a reward set is preprocessed BEFORE DKG is set and then this call occurs AFTER DKG is set, but the preprocessed reward set is not updated correctly to include this? (commenting mostly for my PR built ontop of this one)

Yes, that will need to be addressed in your PR. The reward set will need to be updated with the aggregate public key if it has since become available. Perhaps the aggregate public key could be stored in a separate column in the same table? Then you don't have to load the entire reward set to check each time.

jbencin

I'm going to submit what I have and save the rest of this PR for tomorrow. Most of my comments are pretty minor and about Rust style/safety/optimization. I don't understand the codebase well enough yet to find the sort of logic errors that you and Aaron can.

clarity/src/vm/analysis/mod.rs

jbencin · 2023-11-13T16:33:27Z

stackslib/src/burnchains/tests/mod.rs

+        let new_seed = if let Some(new_seed) = new_seed {
+            new_seed
+        } else {
+            // prove on the last-ever sortition's hash to produce the new seed
+            let proof = miner
+                .make_proof(&leader_key.public_key, &last_snapshot.sortition_hash)
+                .expect(&format!(
+                    "FATAL: no private key for {}",
+                    leader_key.public_key.to_hex()
+                ));
+
+            let new_seed = VRFSeed::from_proof(&proof);
+            new_seed
+        };


Somewhat confusing logic here, consider using unwrap_or_else():

Suggested change

let new_seed = if let Some(new_seed) = new_seed {

new_seed

} else {

// prove on the last-ever sortition's hash to produce the new seed

let proof = miner

.make_proof(&leader_key.public_key, &last_snapshot.sortition_hash)

.expect(&format!(

"FATAL: no private key for {}",

leader_key.public_key.to_hex()

));

let new_seed = VRFSeed::from_proof(&proof);

new_seed

};

let new_seed = new_seed.unwrap_or_else(|| {

// prove on the last-ever sortition's hash to produce the new seed

let proof = miner

.make_proof(&leader_key.public_key, &last_snapshot.sortition_hash)

.expect(&format!(

"FATAL: no private key for {}",

leader_key.public_key.to_hex()

));

VRFSeed::from_proof(&proof)

});

jbencin · 2023-11-13T16:45:34Z

clarity/src/vm/database/structures.rs

+                if *unlock_height >= (v3_unlock_height as u64) {
+                    v3_unlock_height as u64


I know u32 as u64 is perfectly safe, but IMO it's better to always use from() and try_from() to save developers the cognitive load of having to think about whether any particular use of as is safe:

Suggested change

if *unlock_height >= (v3_unlock_height as u64) {

v3_unlock_height as u64

if *unlock_height >= (u64::from(v3_unlock_height)) {

u64::from(v3_unlock_height)

This applies to other uses of as in the PR as well. Ideally, we wouldn't have any usage of as in our codebase and could enable the Clippy lint for it

jbencin · 2023-11-13T17:05:35Z

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

+            debug!(
+                "Memoized canonical Stacks chain tip is now {}/{}",
+                &ch, &bhh
+            );


Minor style nit, but IMO putting variables inside the format string is cleaner and easier to read:

Suggested change

debug!(

"Memoized canonical Stacks chain tip is now {}/{}",

&ch, &bhh

);

debug!("Memoized canonical Stacks chain tip is now {ch}/{bhh}");

jbencin · 2023-11-13T17:36:13Z

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

+            .optional()
+            .map_err(db_error::from)?;


map_err() is unnecessary here, ? automatically converts for error types where trait From is implemented:

Suggested change

.optional()

.map_err(db_error::from)?;

.optional()?;

jbencin · 2023-11-13T21:45:45Z

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

+            sort_db.conn(),
+            &sns.last()
+                .as_ref()
+                .expect("FATAL; unreachable: sns is never empty")


Typo?

Suggested change

.expect("FATAL; unreachable: sns is never empty")

.expect("FATAL: unreachable: sns is never empty")

jbencin · 2023-11-13T21:47:16Z

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

+    };
+    sort_db
+        .get_next_block_recipients(burnchain, sortition_tip, reward_cycle_info.as_ref())
+        .map_err(|e| Error::from(e))


Suggested change

.map_err(|e| Error::from(e))

.map_err(Error::from)

jbencin · 2023-11-13T21:57:41Z

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

+                BurnchainDB::get_burnchain_block(&self.burnchain_blocks_db.conn(), &cursor)
+                    .map_err(|e| {
+                        warn!(
+                            "ChainsCoordinator: could not retrieve  block burnhash={}",


Typo, extra space:

Suggested change

"ChainsCoordinator: could not retrieve block burnhash={}",

"ChainsCoordinator: could not retrieve block burnhash={}",

jbencin · 2023-11-13T22:05:16Z

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

+        debug!(
+            "Unprocessed burn chain blocks [{}]",
+            dbg_burn_header_hashes.join(", ")
+        );


It's probably more efficient debug print a string vector:

Suggested change

debug!(

"Unprocessed burn chain blocks [{}]",

dbg_burn_header_hashes.join(", ")

);

debug!(

"Unprocessed burn chain blocks {dbg_burn_header_hashes:?}"

);

jbencin · 2023-11-13T22:46:09Z

stackslib/src/chainstate/nakamoto/miner.rs

+
+        let burn_tip = SortitionDB::get_canonical_chain_tip_bhh(burn_dbconn.conn())?;
+        let burn_tip_height =
+            SortitionDB::get_canonical_burn_chain_tip(burn_dbconn.conn())?.block_height as u32;


u64 as u32 can truncate. Yes it will take thousands of years at current Bitcoin block rate, but suppose one day Bitcoin decides on faster blocks, or this code gets used elsewhere (like Subnets, or another chain). Or suppose we add integration tests with unrealistic values for burn_height. This sort of bug is unlikely to occur, but very difficult to track down, so it's better to make it impossible

Suggested change

SortitionDB::get_canonical_burn_chain_tip(burn_dbconn.conn())?.block_height as u32;

u32::try_from(SortitionDB::get_canonical_burn_chain_tip(burn_dbconn.conn())?.block_height).expect("block_height overflow");

jferrant · 2023-11-14T13:35:35Z

stackslib/src/chainstate/nakamoto/mod.rs

+        }
+
+        // tenure-changes must all come first, and must be in order
+        for i in 0..tenure_change_positions.len() {


Prob better to do iter.enumerate() so you don't do a direct array access

jferrant · 2023-11-14T13:38:53Z

stackslib/src/chainstate/nakamoto/mod.rs

+        chain_id: u32,
+        epoch_id: StacksEpochId,
+    ) -> bool {
+        if self.txs.len() == 0 {


NIT: will prob get fixed in a clippy PR, but in general when doing len() checks against 0, should use is_empty() (otherwise silence the clippy warning)

jferrant · 2023-11-14T14:12:53Z

stackslib/src/core/mod.rs

-pub static STACKS_EPOCH_3_0_MARKER: u8 = 0x0a;
+pub static STACKS_EPOCH_2_5_MARKER: u8 = 0x0a;
+
+/// Stacks 3.0 epoch marker.  All block-commits in 2.4 must have a memo bitfield with this value


should this say "All block-commits in 3.0 must have a memo..."?

jferrant · 2023-11-14T14:16:54Z

stackslib/src/clarity_vm/clarity.rs

@@ -2471,6 +2610,10 @@ mod tests {
                u32::MAX
            }

+            fn get_v3_unlock_height(&self) -> u32 {


Is there a purpose to keeping these unlock height and activation height functions separated into v3/v1 and pox_3/pox_4? they always seem to be the same in which case it makes more sense to keep just a single "get_unlock_height" and a "get_pox_activation_height" ?

Yes -- they have to be different block heights. Specifically, the new PoX contract needs to be instantiated first, and then after that, the tokens in the old PoX contract must unlock. But, this can't happen in the same block.

jferrant · 2023-11-14T14:45:51Z

stackslib/src/chainstate/stacks/mod.rs

@@ -658,7 +658,13 @@ pub struct SchnorrThresholdSignature {
    //pub scalar: wsts::Scalar,
 }

-/// Reasons why a `TenureChange` transaction can be de
+impl SchnorrThresholdSignature {


Oh I didn't see this when i was building off this branch. Perhaps I should be using this then instead of my SchnorrSignature in my own PR.

Is there a reason you don't want to use the wsts::common::Signature directly? Are we going to be implementing a struct wrapper around the wsts point and scalar types to basically add our own custom SchnorrThresholdSignature?

I think this might be a @jbencin and @xoloki question. IIRC @xoloki told him to use wsts::Point and wsts::Scalar to encode the signature, but that was a while ago. It's possible that wsts::common::Signature is more appropriate now.

You should do whatever makes the most sense in your PR :) This PR only needs this struct to encode/decode; it doesn't care about validating the contents.

ThresholdSignature is the same as wsts::common::Signature, I defined a struct in this repo so that I could implement StacksMessageCodec for it (can't do that with types defined outside this crate)

jferrant · 2023-11-14T15:01:08Z

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

@@ -8544,9 +8546,15 @@ pub mod test {
            fn get_v2_unlock_height(&self) -> u32 {
                u32::MAX
            }
+            fn get_v3_unlock_height(&self) -> u32 {


Similar comment as before. Not sure why these are separated by version and pox since they are all identical

They're not identical on mainnet. This trait impl is just for testing transaction-handling.

jferrant

All my comments are nits or about typos really. Don't think I know enough to find logic errors, so take this review with a grain of salt XD

Sooooo

LGTM

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

stackslib/src/chainstate/coordinator/mod.rs

stackslib/src/chainstate/nakamoto/mod.rs

kantai · 2023-11-14T16:31:16Z

stackslib/src/chainstate/nakamoto/mod.rs

+    /// Find all positionally-valid tenure changes in this block.
+    /// They must be the first transactions.
+    /// Return their indexes into self.txs
+    fn find_tenure_changes(&self) -> Vec<usize> {


I think the caller of this might be simplified if this returned Vec<&TenureChangePayload> because you wouldn't need to check the payload type again when validating the payloads.

True, but that would also require lifetimes, which could be an issue depending on when/how the result is used. Also, minor nit: Using filter_map() would simplify and clarify this logic a bit, and it might make more sense to return an Iterator than a Vec

kantai · 2023-11-14T16:33:08Z

stackslib/src/chainstate/nakamoto/mod.rs

+        }
+        if let Some(false) = wellformed {
+            // block isn't well-formed
+            return None;


Should this case error and invalidate the block?

It does, by way of validate_transactions_static(), which gets called by validate_nakamoto_block_burnchain(), which in turn is called by accept_block().

So, an invalid Nakamoto block (i.e. one that's missing a coinbase) will never get stored to the staging DB.

As an added safety check, the case where the call to get_coinbase_tx() returns None causes append_block() to fail with an invalid-block error.

stackslib/src/chainstate/nakamoto/mod.rs

kantai

LGTM -- just had a few comments, and one or two questions about behavior.

jbencin

More comments...

jbencin · 2023-11-14T15:46:32Z

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

+                PaidRewards {
+                    pox: vec![],
+                    burns: 0,
+                }


Might be a little cleaner to #[derive(default)] for this struct and then do:

Suggested change

PaidRewards {

pox: vec![],

burns: 0,

}

PaidRewards::default()

jbencin · 2023-11-15T15:18:09Z

stackslib/src/chainstate/nakamoto/miner.rs

+            .find(|txn| {
+                if let TransactionPayload::TenureChange(..) = txn.payload {
+                    true
+                } else {
+                    false
+                }
+            })


Can simplify this to:

Suggested change

.find(|txn| {

if let TransactionPayload::TenureChange(..) = txn.payload {

true

} else {

false

}

})

.find(|txn| matches!(txn.payload, TransactionPayload::TenureChange(..)))

jbencin · 2023-11-15T15:48:51Z

stackslib/src/chainstate/nakamoto/mod.rs

+    /// Find all positionally-valid tenure changes in this block.
+    /// They must be the first transactions.
+    /// Return their indexes into self.txs
+    fn find_tenure_changes(&self) -> Vec<usize> {


True, but that would also require lifetimes, which could be an issue depending on when/how the result is used. Also, minor nit: Using filter_map() would simplify and clarify this logic a bit, and it might make more sense to return an Iterator than a Vec

jbencin · 2023-11-15T16:05:49Z

stackslib/src/chainstate/nakamoto/mod.rs

+        let wellformed = self.is_wellformed_first_tenure_block();
+        if wellformed.is_none() {
+            // block isn't a first-tenure block, so no valid tenure changes
+            return None;
+        } else if let Some(false) = wellformed {
+            // this block is malformed
+            info!("Block is malformed";
+                  "block_id" => %self.block_id());
+            return Some(false);
+        }


If a function returns an Option, you can use the ? operator with Option types:

Suggested change

let wellformed = self.is_wellformed_first_tenure_block();

if wellformed.is_none() {

// block isn't a first-tenure block, so no valid tenure changes

return None;

} else if let Some(false) = wellformed {

// this block is malformed

info!("Block is malformed";

"block_id" => %self.block_id());

return Some(false);

}

if !self.is_wellformed_first_tenure_block()? {

// this block is malformed

info!("Block is malformed";

"block_id" => %self.block_id());

return Some(false);

}

jbencin · 2023-11-15T16:11:26Z

stackslib/src/chainstate/nakamoto/mod.rs

+        let wellformed = self.is_wellformed_first_tenure_block();
+        if wellformed.is_none() {
+            // block isn't a first-tenure block, so no coinbase
+            return None;
+        }
+        if let Some(false) = wellformed {


Simplify:

Suggested change

let wellformed = self.is_wellformed_first_tenure_block();

if wellformed.is_none() {

// block isn't a first-tenure block, so no coinbase

return None;

}

if let Some(false) = wellformed {

if !self.is_wellformed_first_tenure_block()? {

jbencin · 2023-11-15T16:13:08Z

stackslib/src/chainstate/nakamoto/mod.rs

+        self.txs.iter().find(|tx| {
+            if let TransactionPayload::Coinbase(..) = &tx.payload {
+                true
+            } else {
+                false
+            }
+        })


Simplify:

Suggested change

self.txs.iter().find(|tx| {

if let TransactionPayload::Coinbase(..) = &tx.payload {

true

} else {

false

}

})

self.txs.iter().find(|tx| mathes!(&tx.payload, TransactionPayload::Coinbase(..)))

jbencin · 2023-11-15T16:25:12Z

stackslib/src/chainstate/nakamoto/mod.rs

+            .map(|coinbase_tx| {
+                if let TransactionPayload::Coinbase(_, _, vrf_proof) = &coinbase_tx.payload {
+                    vrf_proof.as_ref()
+                } else {
+                    // actually unreachable
+                    None
+                }
+            })
+            .flatten()


Simplify:

Suggested change

.map(|coinbase_tx| {

if let TransactionPayload::Coinbase(_, _, vrf_proof) = &coinbase_tx.payload {

vrf_proof.as_ref()

} else {

// actually unreachable

None

}

})

.flatten()

.and_then(|coinbase_tx| {

if let TransactionPayload::Coinbase(_, _, vrf_proof) = &coinbase_tx.payload {

vrf_proof.as_ref()

} else {

// actually unreachable

None

}

})

jbencin

Approving... I left plenty of Rust suggestions but nothing critical. Only thing you should definitely fix are truncating as conversions

jbencin · 2023-11-15T17:38:12Z

stackslib/src/chainstate/nakamoto/mod.rs

+                block_commit.key_block_ptr as u64,
+                block_commit.key_vtxindex as u32,


Suggested change

block_commit.key_block_ptr as u64,

block_commit.key_vtxindex as u32,

u64::from(block_commit.key_block_ptr),

u32::from(block_commit.key_vtxindex),

jbencin · 2023-11-15T19:03:03Z

stackslib/src/chainstate/stacks/block.rs

-                        return false;
-                    }
+        for tx in txs.iter() {
+            if let TransactionPayload::Coinbase(_, ref recipient_opt, ref proof_opt) = &tx.payload {


Should this be a match statement rather than a series of if lets?

jbencin · 2023-11-15T19:08:40Z

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

    pub burn_header_timestamp: u64,
+    /// Size of the block corresponding to `anchored_header` in bytes


Thank you for adding doc comments to struct fields, really helps those of us who are not so familiar with the codebase

jbencin · 2023-11-15T19:14:17Z

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

+        match &self {
+            StacksBlockHeaderTypes::Nakamoto(ref x) => Some(x),


Lots of unnecessary ref/deref operations in this codebase:

Suggested change

match &self {

StacksBlockHeaderTypes::Nakamoto(ref x) => Some(x),

match self {

StacksBlockHeaderTypes::Nakamoto(x) => Some(x),

jbencin · 2023-11-15T19:57:58Z

testnet/stacks-node/src/event_dispatcher.rs

+            .registered_observers
+            .iter()
+            .enumerate()
+            .filter(|(obs_id, _observer)| self.miner_observers_lookup.contains(&(*obs_id as u16)))


usize as u16 can truncate

Suggested change

.filter(|(obs_id, _observer)| self.miner_observers_lookup.contains(&(*obs_id as u16)))

.filter(|(obs_id, _observer)| self.miner_observers_lookup.contains(&(u16::try_from(*obs_id).expect("obs_id overflow"))))

jbencin · 2023-11-15T20:02:44Z

testnet/stacks-node/src/event_dispatcher.rs

+            cost: consumed.clone(),
+            tx_events,
+        })
+        .unwrap();


This probably should never fail, but it's still preferable to use expect() in non-test code

Suggested change

.unwrap();

.expect("Deserializing MinedNakamotoBlockEvent failed");

jbencin · 2023-11-15T20:04:42Z

testnet/stacks-node/src/neon_node.rs

-            parent_block_burn_height: parent_block.block_height,
-            parent_block_total_burn: parent_block.total_burn,
+            parent_block_burn_height: parent_block_height,
+            parent_block_total_burn: parent_block_total_burn,


Suggested change

parent_block_total_burn: parent_block_total_burn,

parent_block_total_burn,

jbencin · 2023-11-15T20:13:47Z

testnet/stacks-node/src/tests/neon_integrations.rs

+                let inner_obj = if let Some(inner_obj) = txevent_obj.get("Success") {
+                    inner_obj
+                } else if let Some(inner_obj) = txevent_obj.get("ProcessingError") {
+                    inner_obj
+                } else if let Some(inner_obj) = txevent_obj.get("Skipped") {
+                    inner_obj
+                } else {
+                    panic!("TransactionEvent object should have one of Success, ProcessingError, or Skipped")
+                };
+                inner_obj
+                    .as_object()


Can be simplified:

Suggested change

let inner_obj = if let Some(inner_obj) = txevent_obj.get("Success") {

inner_obj

} else if let Some(inner_obj) = txevent_obj.get("ProcessingError") {

inner_obj

} else if let Some(inner_obj) = txevent_obj.get("Skipped") {

inner_obj

} else {

panic!("TransactionEvent object should have one of Success, ProcessingError, or Skipped")

};

inner_obj

.as_object()

txevent_obj.get("Success")

.or_else(|| txevent_obj.get("ProcessingError"))

.or_else(|| txevent_obj.get("Skipped"))

.expect("TransactionEvent object should have one of Success, ProcessingError, or Skipped")

.as_object()

CLAassistant · 2023-11-16T04:54:35Z

All committers have signed the CLA.

jcnelson · 2023-11-16T19:06:26Z

Hi @jbencin, I appreciate your feedback, but I'm going to defer on most of the ergonomics changes for now. I'm already too anxious about this PR blocking other work, and these changes don't affect the behavior or even the legibility of the code. I did take the time to go through all the files I touched and use explicit integer conversions, however.

…s really needs a clippy pass)

jcnelson added 23 commits October 24, 2023 14:58

chore: pull thru is_prepare_phase_start

7045a4d

feat: add is_prepare_phase_start to PoxConstants

8b4fc43

feat: track last processed reward cycle in sortition DB MARF

6479175

refactor: add support for both epoch2 and epoch3 processing to Chains…

35c0fd3

…Coordinator

chore: use StacksBlockEventData instead of StacksBlock for event handler

e7a17c3

feat: only track consensus_hash and parent_block_id in Nakamoto block…

9e02660

…s (update the structs and DB tables to reflect this). Also, add top-level Nakamoto block-processing.

chore: update Nakamoto block tests to reflect new structs

5c55d04

feat: is_first_index_block_hash

0eed15a

feat: remove get_stacks_chain_tip() in favor of new Nakamoto-aware ge…

18de400

…t_canonical_block_header()

chore: document StacksHeaderInfo fields, and add accessor for nakamot…

defd316

…o state

feat: capture event observer data in StacksBlockEventData, which is c…

489f385

…ompatible with both StacksBlock and NakamotoBlock

chore: remove compiler warning

28eedab

chore: remove nakamoto acceptance period

0337f1e

chore: remove analyze-fees directive (since it's only compatible wi…

1895e45

…th epoch2) and make the remaining directives compatible with epoch3

chore: use get_canonical_block_header

30331a7

chore: use get_canonical_block_header() and document test methods

e47ab75

chore: use get_canonical_block_header

7e75e42

chore: use get_canonical_block_header

144de2d

feat: use get_canonical_block_header

a7ab086

chore: use StacksBlockEventData

7705961

chore: use get_canonical_block_header

d4b68a3

chore: use StacksBlockEventData

11b2bb6

feat: WIP initial coordinator implementation for Nakamoto

85f925d

jcnelson requested review from kantai, obycode, jbencin and jferrant October 24, 2023 19:04

kantai reviewed Oct 26, 2023

View reviewed changes

jferrant reviewed Nov 13, 2023

View reviewed changes

stackslib/src/burnchains/tests/mod.rs Show resolved Hide resolved

jferrant reviewed Nov 13, 2023

View reviewed changes

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

Ok(())

}

/// Get a pre-processed reawrd set.

Copy link

Collaborator

jferrant Nov 13, 2023

Choose a reason for hiding this comment

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

nit: Typo

jferrant reviewed Nov 13, 2023

View reviewed changes

jbencin reviewed Nov 13, 2023

View reviewed changes

jferrant reviewed Nov 14, 2023

View reviewed changes

jferrant self-requested a review November 14, 2023 15:18

jferrant approved these changes Nov 14, 2023

View reviewed changes

kantai reviewed Nov 14, 2023

View reviewed changes

stackslib/src/chainstate/burn/db/sortdb.rs Outdated Show resolved Hide resolved

kantai reviewed Nov 14, 2023

View reviewed changes

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

kantai reviewed Nov 14, 2023

View reviewed changes

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

kantai reviewed Nov 14, 2023

View reviewed changes

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

kantai reviewed Nov 14, 2023

View reviewed changes

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

jferrant mentioned this pull request Nov 14, 2023

Nakamoto: Validate stacker signature of a Nakamoto Block #4039

Merged

kantai approved these changes Nov 14, 2023

View reviewed changes

jbencin reviewed Nov 15, 2023

View reviewed changes

jbencin approved these changes Nov 15, 2023

View reviewed changes

chore: address PR feedback; use explicit integer conversions (but thi…

7cf69c7

…s really needs a clippy pass)

jcnelson merged commit ae0f434 into next Nov 16, 2023
2 checks passed

saralab mentioned this pull request Nov 17, 2023

Nakamoto: NakamotoCoordinator #3869

Closed

		if *unlock_height >= (v3_unlock_height as u64) {
		v3_unlock_height as u64

	.expect("FATAL; unreachable: sns is never empty")
	.expect("FATAL: unreachable: sns is never empty")

	"ChainsCoordinator: could not retrieve block burnhash={}",
	"ChainsCoordinator: could not retrieve block burnhash={}",

	SortitionDB::get_canonical_burn_chain_tip(burn_dbconn.conn())?.block_height as u32;
	u32::try_from(SortitionDB::get_canonical_burn_chain_tip(burn_dbconn.conn())?.block_height).expect("block_height overflow");

		block_commit.key_block_ptr as u64,
		block_commit.key_vtxindex as u32,

		pub burn_header_timestamp: u64,
		/// Size of the block corresponding to `anchored_header` in bytes

		match &self {
		StacksBlockHeaderTypes::Nakamoto(ref x) => Some(x),

	.filter(\|(obs_id, _observer)\| self.miner_observers_lookup.contains(&(*obs_id as u16)))
	.filter(\|(obs_id, _observer)\| self.miner_observers_lookup.contains(&(u16::try_from(*obs_id).expect("obs_id overflow"))))

	.unwrap();
	.expect("Deserializing MinedNakamotoBlockEvent failed");

	parent_block_total_burn: parent_block_total_burn,
	parent_block_total_burn,

Feat/nakamoto coordinator #4009

Feat/nakamoto coordinator #4009

Conversation

jcnelson commented Oct 24, 2023

codecov bot commented Oct 24, 2023 • edited

Codecov Report

kantai left a comment

Choose a reason for hiding this comment

muneeb-ali commented Oct 30, 2023

Choose a reason for hiding this comment

zone117x commented Nov 13, 2023

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

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

jbencin Nov 13, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jferrant Nov 14, 2023 • 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

jferrant Nov 14, 2023 • 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

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

kantai left a comment

Choose a reason for hiding this comment

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

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

jbencin Nov 15, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CLAassistant commented Nov 16, 2023 • edited

jcnelson commented Nov 16, 2023

codecov bot commented Oct 24, 2023 •

edited

jbencin Nov 13, 2023 •

edited

jferrant Nov 14, 2023 •

edited

jferrant Nov 14, 2023 •

edited

jbencin Nov 15, 2023 •

edited

CLAassistant commented Nov 16, 2023 •

edited