Tenure extend when the previous tenure is bad

[obycode]

Description

Updates the miner to attempt to continue its tenure if the winner of the next tenure commits to the wrong block or if it mines no blocks within some grace period.

Applicable issues

• fixes Miner should generate a tenure-extend and continue mining if the next miner does not start mining #5361

Additional info (benefits, drawbacks, caveats)

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

[obycode]

Ok, the case where the winning miner has committed to the wrong parent tenure is complete. Now I just need to figure out a clean way to attempt a tenure extend after some time has passed and the new miner has not mined a block.

[obycode]

There is a problem that I discovered which is unrelated to these changes, but becomes a challenge due to these changes.

stacks-core/testnet/stacks-node/src/event_dispatcher.rs

Lines 110 to 123 in 6e0bd5a

	pub static STACKER_DB_CHANNEL: StackerDBChannel = StackerDBChannel::new();
	/// This struct receives StackerDB event callbacks without registering
	/// over the JSON/RPC interface. To ensure that any event observer
	/// uses the same channel, we use a lazy_static global for the channel (this
	/// implements a singleton using STACKER_DB_CHANNEL).
	///
	/// This is in place because a Nakamoto miner needs to receive
	/// StackerDB events. It could either poll the database (seems like a
	/// bad idea) or listen for events. Registering for RPC callbacks
	/// seems bad. So instead, it uses a singleton sync channel.
	pub struct StackerDBChannel {
	sender_info: Mutex<Option<InnerStackerDBChannel>>,
	}

This global becomes a problem when we have tests that run multiple miners. If multiple miners attempt to propose a block at the same time, then we end up having problems.

[kantai]

This global becomes a problem when we have tests that run multiple miners. If multiple miners attempt to propose a block at the same time, then we end up having problems.

The only clear option to me is to make this a Arc+Mutex an instance variable in the EventObserver struct. This requires that the EventObserver only be constructed once, and then cloned whenever copies are needed. I think that wouldn't be too hard (there's already a lot of passing around of the object I think, rather than it being continuously reconstructed from the config).

[obycode]

This PR handles the tenure extend for the case where the next miner has committed to the wrong parent tenure. The other case mentioned in #5361, where a tenure extend is allowed after some time limit has passed with no blocks from the new miner, will be completed in a separate PR.

The test for that behavior is already in this PR, but it is not enabled yet.

[hstove]

@obycode can we create a new issue for the timeout case, and then rename #5361 to be more about "if the new miner commits to the wrong block"? Or some derivation of that?

[hstove]

Overall LGTM! Mainly requesting a change in the SignerDB migration and then a few extra assertions in your test.

[hstove]

Is this true? Looking at line 962, the case where sn.winning_stacks_block_hash == highest_tenure_bhh - wouldn't that mean the current burn block has a good sortition? Or is the fact that we're in continue_tenure mean that my assertion is wrong? Sorry, it's tricky to follow all the code paths here.

[obycode]

Right, we're trying to find the last good sortition, given that the current one is already bad since we arrived in continue_tenure. We get the last snapshot with a sortition, but if we determine that is bad, then we go back to the sortition of the tip.

[hstove]

At line 984 we run this exact query. With your change, is that always necessary? If it's maybe necessary, can we change 984 to something like:

let canonical_block_snapshot = if last_good_block_election_snapshot
    .canonical_stacks_tip_consensus_hash
    == canonical_stacks_tip_ch
{
    last_good_block_election_snapshot
} else {
    SortitionDB::get_block_snapshot_consensus(self.sortdb.conn(), &canonical_stacks_tip_ch)
        .map_err(|e| {
            error!("Relayer: failed to get block snapshot for canonical tip: {e:?}");
            NakamotoNodeError::SnapshotNotFoundForChainTip
        })?
        .ok_or_else(|| {
            error!("Relayer: failed to get block snapshot for canonical tip");
            NakamotoNodeError::SnapshotNotFoundForChainTip
        })?
};

[obycode]

I do think it's still necessary. We can definitely reuse it here and avoid looking it up again.

[obycode]

Hmm... maybe it's not. I'll look at this some more.

[hstove]

IMO I would appreciate some ELI5(ish) comments on some of the sortdb lookups to explain why/what they're doing. There's already great comments in here, I think it's just tricky to follow when I have to constantly look up with a specific function does

[obycode]

Ok, I tried to clean this function up a bit. What do you think about this refactoring? 2178846

[obycode]

I also improved the names and added more comments in another commit just now.

[obycode]

Next step will be to remove the tenure extend disable flag and instead update those forking tests to expect this new behavior. This will be lower priority though as I pivot to the design for #5434 before coming back to this.