fix: prevent miner from reorging itself #6513
Conversation
|
Pulling in develop led to a compile error due to a return type changing - fixed in 8c828f6! |
|
These integration tests aren't just flaky, looks like I actually broke them. Let me spend some time on this. Sorry! |
|
LGTM once the tests are fixed :) |
| "blocks_before" => blocks_before, | ||
| "blocks_processed" => blocks_processed, | ||
| "blocks_processed_before" => blocks_processed_before, | ||
| "block_in_tenure" => block_in_tenure, |
There was a problem hiding this comment.
I assume you are waiting on Aaron's feedback , but I think the miner views Tenure C as invalid and now builds off of block b because it was the prior miner. So its no longer seeing block b as a fork as this test is trying to do. It sees it as the last valid mined block and so it extends its prior tenure B rather than mining Tenure C. i.e. I don't think its really a forking test anymore. in a multi miner setup, I would expect the prior miner to extend Tenure B and the new incoming miner be unable to mine at all. This makes me wonder if we even should have the timeout anymore for signers to enable an incoming miner to reorg a poorly timed block. Or does that depend on signer global state being merged to remove this timeout?
| use clarity::boot_util::boot_code_addr; | ||
| use clarity::vm::costs::{ExecutionCost, LimitedCostTracker}; | ||
| use clarity::vm::representations::ContractName; | ||
| use clarity::vm::types::{PrincipalData, QualifiedContractIdentifier, StandardPrincipalData}; |
There was a problem hiding this comment.
Weird, in the "Files changed" view, this shows on the correct line.
|
Reviewers: I've added a new integration test, which is a |
| /// - Miner B wins tenure B, with 2 blocks | ||
| /// - Miner A wins tenure C, but with a block commit to tip A | ||
| /// - We verify that Miner B extends Tenure B | ||
| fn tenure_extend_after_stale_commit_different_miner() { |
There was a problem hiding this comment.
I think we might already actually have this test in tenure_extend_after_bad_commit? Not 100% sure. Your test seems more clear though so not against replacing it, but prob would delete the old test (though the old one also confirms that Miner A can win the following one after the extended Tenure B and correctly builds so you might want to expand this test if you do decide to replace)
|
I think the nakamoto test is kinda nice to have in place even since it removes the signers from the equation and just test the miner functionality solo basically. But I think if you keep it, it needs to have its description updated since it now confirms that tenure_C builds actually on b_0 NOT a_x. Also, it prob should be renamed to "bad_commit_does_not_trigger_fork" or something like that? EDIT: granted I guess a bad commit COULD trigger a fork in a multiple miner set up (see |
|
@jferrant agreed regarding renaming the test (and fixing the description) - updated! |
I still think you have the duplicate test, but will approve once that is updated/removed. See |
There was a problem hiding this comment.
This LGTM!
The changes to the forked_tenure_is_ignored() tests make sense to me. The scenario those tested are now handled differently (i.e., we expect a tenure-extend). The assertion that those tests were making (that the miner always tries to build off of their committed-to tenure) is now covered by your new test tenure_extend_after_stale_commit_different_miner (by "Miner 1's proposal for C is rejected"), which I think is the right place for it.
|
@jferrant I realized that the tests were doing two slightly separate things - one was verifying that we wouldn't reorg based on there being 2 blocks in a tenure, and another is based on the reorg attempt being longer than |
This adds logic to an edge case where:
Previously, the miner would issue a normal tenure change, which would attempt (and fail) to reorg itself. This PR fixes the logic such that the miner will issue an extension.