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

Miner can continue mining after an empty tenure followed by empty sortition #5411

Closed hstove closed 1 week ago

hstove commented 3 weeks ago

(hopefully eventually)

This PR currently just contains an integration test to try and reproduce the scenario described in #5400.

The basic flow of the test is:

I thought this would reproduce the issue, but things seem fine - the miner creates a TenureExtend off of block A and signers approve it.

JACINTA EDIT:

So I modified the logic to check during continue_tenure to see if we failed to produce any blocks in our tenure which means we really never issued a tenure extend. Now we issue one. I verified this works with multiple empty sortitions, and that we can build off this tenure change payload that shows up late essentially. I also ensured that we do issue a traditional tenure extend in the case where we successfully issued the tenure extend payload. This did require me to chagne the vrf proof generation to use the block election snapshot and not the current burn block to verify. I also found a bug in the way we were passing the election snapshot along (caused miners to never issue a block proposal at all because it would attempt to write to the wrong slot).

Now I know @kantai you wanted the miner to issue a continue tenure after the successful tenure change but I think this is a lot more complicated and has more edge cases that I don't like...Now I can continue to try this though if you think this fix is insufficient / or if there are edge cases that already exist that I am just not seeing.

Will see what CI does...hopefully haven't broken anything

jferrant commented 3 weeks ago

I think you might need to have 2 sep miners to get this to work. We need to

hstove commented 3 weeks ago

Yeah you may be right about needing 2 miners, but I swear this situation involved the same miner winning the first two blocks (though I should double check). I think it's also very possible that there was a different "root cause" in that case.

jferrant commented 2 weeks ago

Yeah you may be right about needing 2 miners, but I swear this situation involved the same miner winning the first two blocks (though I should double check). I think it's also very possible that there was a different "root cause" in that case.

It could have been done with one miner . It just was not happening because we were failing to produce an empty tenure as easily in the one miner case because of the boot to nakamoto logic. it def can be done. Having two miners makes it just a bit easier and allows a more expansive test as well :)

hstove commented 2 weeks ago

Nice, this is looking pretty good!

blockstack-devops commented 6 days ago

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.