Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Miner should extend over "bad tenure" into an empty sortition #5810

Open
kantai opened this issue Feb 6, 2025 · 0 comments
Open

Miner should extend over "bad tenure" into an empty sortition #5810

kantai opened this issue Feb 6, 2025 · 0 comments
Milestone

Comments

@kantai
Copy link
Member

kantai commented Feb 6, 2025

Yesterday, the network experienced a stall in which:

  1. At Bitcoin Block N, Miner A won and mined a normal tenure
  2. At Bitcoin Block N+1, Miner B won, but their commit reorged Miner A. The signer set marks Miner B invalid, and Miner A successfully tenure extends into Bitcoin block N+1 and mines blocks.
  3. At Bitcoin Block N+2, there's an empty sortition. Miner A stops mining and does not attempt to further extend their tenure into this one.

The reason this happens is somewhat complex, but the upside is basically this function in nakamoto_node/relayer.rs:

    /// Is the given sortition a valid sortition?
    /// I.e. whose winning commit's parent tenure ID is on the canonical Stacks history,
    /// and whose consensus hash corresponds to the ongoing tenure or a confirmed tenure?
    fn is_valid_sortition(
        chain_state: &mut StacksChainState,
        stacks_tip_id: &StacksBlockId,
        stacks_tip_sn: &BlockSnapshot,
        burn_tip_ch: &ConsensusHash,
        sn: &BlockSnapshot,
    ) -> Result<bool, NakamotoNodeError> {
        if !sn.sortition {
            // definitely not a valid sortition
            debug!("Relayer: Sortition {} is empty", &sn.consensus_hash);
            return Ok(false);
        }

        // check that this commit's parent tenure ID is on the history tipped at
        // `stacks_tip_id`
        let mut ic = chain_state.index_conn();
        let parent_tenure_id = StacksBlockId(sn.winning_stacks_block_hash.clone().0);
        let height_opt = ic.get_ancestor_block_height(&parent_tenure_id, stacks_tip_id)?;
        if height_opt.is_none() {
            // parent_tenure_id is not an ancestor of stacks_tip_id
            debug!(
                "Relayer: Sortition {} has winning commit hash {}, which is not canonical",
                &sn.consensus_hash, &parent_tenure_id
            );
            return Ok(false);
        }

        if sn.consensus_hash == *burn_tip_ch {
            // sn is the sortition tip, so this sortition must commit to the tenure start block of
            // the ongoing Stacks tenure.
            let highest_tenure_start_block_header = NakamotoChainState::get_tenure_start_block_header(
                &mut ic,
                stacks_tip_id,
                &stacks_tip_sn.consensus_hash
            )?
            .ok_or_else(|| {
                error!(
                    "Relayer: Failed to find tenure-start block header for stacks tip {stacks_tip_id}"
                );
                NakamotoNodeError::ParentNotFound
            })?;

            let highest_tenure_start_block_id =
                highest_tenure_start_block_header.index_block_hash();
            if highest_tenure_start_block_id != parent_tenure_id {
                debug!("Relayer: Sortition {} is at the tip, but does not commit to {} so cannot be valid", &sn.consensus_hash, &parent_tenure_id;
                    "highest_tenure_start_block_header.block_id()" => %highest_tenure_start_block_id);
                return Ok(false);
            }
        }

        Ok(true)
     }

The intent of this function is to return true if the given sn corresponds to a sortition that extends the current stacks tenure. I'd suggest renaming this function to something like extends_canonical_tenure().

The basic issue in this function is the if statement if sn.consensus_hash == *burn_tip_ch { .. }. The checks inside that statement should be applied in all cases, not just at the burn_tip_ch.

Fixing this issue just means removing that "if guard", and just applying the internal checks in all cases. This should be included with a new integration test with the above scenario, though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Status: 🆕 New
Development

No branches or pull requests

1 participant