Tenure extend when the previous tenure is bad

stacks-network / stacks-core

The Stacks blockchain implementation

https://docs.stacks.co

GNU General Public License v3.0

3.01k stars 672 forks source link

Tenure extend when the previous tenure is bad #5452

Open obycode opened 1 week ago

obycode commented 1 week ago

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 #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 commented 1 week ago

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 commented 1 week ago

There is a problem that I discovered which is unrelated to these changes, but becomes a challenge due to these changes. https://github.com/stacks-network/stacks-core/blob/6e0bd5ae66b65797e76b3a48fdf99be22f73459f/testnet/stacks-node/src/event_dispatcher.rs#L110-L123

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 commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

@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?

obycode commented 6 days ago

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 commented 3 days ago

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

obycode commented 3 days ago

@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 ba2faf7035b9e3c472d21685121c75f139f02dd2 and 965f58ba294f965741079c83d69c974310df2edd.

hstove commented 2 days ago

FYI I've stripped all of the shadow blocks stuff, which was merged into this PR. The old branch is now in https://github.com/stacks-network/stacks-core/tree/feat/tenure-extend-no-blocks-with-shadow

obycode commented 2 days ago

My last commit adds a new test, tenure_extend_after_2_bad_commits which will currently fail due to the signerdb issue Hank identified. That can be enabled once that problem is resolved (or determined that we shouldn't fix it).