Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: add bitcoin reorg test to bitcoind/stacks-node/signers integrations #4989

Merged
merged 2 commits into from
Jul 26, 2024

Conversation

kantai
Copy link
Member

@kantai kantai commented Jul 22, 2024

This PR adds a bitcoin reorg integration test to the nakamoto/signers integration test suite.

There's a couple of behaviors to note here:

  1. During the reorg, the miner can't win a sortition, because of the Bitcoin MEV logic: after the reorg, the miner is marked as "not mining often enough", so has to issue some losing commits before they can win a commit.
  2. During this time, the miner tries to submit a TenureExtend, but this is rejected by the signer set. The reason this happens is that (1) the bitcoin reorg creates a child of the common fork ancestor which has a successful sortition in it, (2) this new sortition is the "latest sortition" that the signer set sees, (3) the signer set doesn't allow the previous sortition (i.e., the parent of the latest sortition) to submit a TenureExtend because the latest sortition is valid and didn't do anything "wrong" yet, and (4), the miner doesn't submit a BlockFound tenure at this "latest sortition", because the nakamoto runloop (which behaves basically like the neon runloop) doesn't notify the relayer thread of sortitions until they've overtaken the old block height. This means that during the reorg, until a new sortition "wins", no nakamoto blocks are being mined.

Otherwise, the test asserts that the miner and signers recover from the reorg: blocks are mined after the reorg once the sortitions overcome behavior (1).

I think there's a valid question about whether or not something should be done to address (2). There's a couple options for how to address it. One is to update the runloop so that the relayer thread is notified of the reorging sortitions (or otherwise refactor the relayer thread to not depend on those notifications?), this would mean that the relayer thread should produce a new tenure at the new winning sortition, and then issue a tenure extend (to the latest block or to each block in the reorg?). I think this would require updating the miner thread's logic as well, because it would need to produce a block even though the bitcoin view has "changed". The second would be to update the signer set logic to mark the latest miner as "invalid" for some reason. The options for that heuristic I think are either (1) some amount of time passes before they receive a block proposal or (2) a subsequent bitcoin block has been mined before they produced a block. Of these heuristics, my initial suspicion is that (1) is easier and also perhaps better, because it also encourages miners to produce their first block proposal quickly, and that solving it from the signer set behavior is going to be simpler than updating the miner logic. At a higher level, though, its worth considering whether or not adding more logic to the signer/miner right now is worth it for this case. None of these changes would require consensus changes, so it may makes sense to try to opt for a simpler implementation for the first version of the nakamoto miner and signer binaries.

@kantai kantai requested a review from a team as a code owner July 22, 2024 14:40
@kantai kantai changed the title test: add bitcoin reorg test to bitcoind/stacks-node/signers integrat… test: add bitcoin reorg test to bitcoind/stacks-node/signers integrations Jul 22, 2024
@saralab saralab requested review from jcnelson and obycode July 22, 2024 15:12
* add bitcoind reorg test to bitcoind / stacks-node / signers integration test suite
* add test to bitcoin-tests.yml
@kantai kantai requested a review from a team as a code owner July 22, 2024 20:02
@saralab saralab requested a review from jbencin July 26, 2024 13:58
@kantai kantai added this pull request to the merge queue Jul 26, 2024
Merged via the queue into develop with commit 95a6bb7 Jul 26, 2024
1 check passed
@blockstack-devops
Copy link
Contributor

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.

@stacks-network stacks-network locked as resolved and limited conversation to collaborators Oct 28, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[testing] Test a bitcoin fork and ensure that the miner/signer flow recovers
5 participants