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.

[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.

[hstove]

@obycode this was the PR where it was discussed that I could help out on the integration tests, right?

[obycode]

@obycode this was the PR where it was discussed that I could help out on the integration tests, right?

Yes, see my last comment. I had previously adjusted some tests to disable this feature, but then @kantai suggested (and I agree) that it would be better to instead adjust those forking tests to expect this behavior.

You can probably revert ba2faf7 and 965f58b.

[obycode]

Thanks for addressing those remaining items for me @hstove! 🙌 Looks good!

[jferrant]

Really just the one nit about combining the schemas to reduce the number of unnecessary versions. :)

[blockstack-devops]

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.