Fix: 2771

[lgalabru]

Address #2771.
The test added consists in producing a canonical chain with 210 blocks. Once done, we invalidate the block 208, and instead of rebuilding directly a longer fork with N blocks (at done in the bitcoind_forking_test) we slowly add some more blocks.
Without the patch, this behavior ends up crashing the node with errors like:

WARN [1626791307.078061] [src/chainstate/coordinator/mod.rs:535] [chains-coordinator] ChainsCoordinator: could not retrieve  block burnhash=40bdbf0dda349642bdf4dd30dd31af4f0c9979ce12a7c17485245d0a6ddd970b
WARN [1626791307.078098] [src/chainstate/coordinator/mod.rs:308] [chains-coordinator] Error processing new burn block: NonContiguousBurnchainBlock(UnknownBlock(40bdbf0dda349642bdf4dd30dd31af4f0c9979ce12a7c17485245d0a6ddd970b))

And the burnchain db end up in the same state we ended up while investing 2771.

With this patch, the node is able to entirely register this new canonical fork, and then able to make some progress.

[gregorycoppola]

Thanks for the fix, @lgalabru !

Looks pretty good assuming that the test passes.. Just had a few requests mostly for comments and readability.

[gregorycoppola]

Hello... Mostly my comments are small wording suggestions for comments.

But, I have one bigger questions, which is: can we really add this test to automated tests? Is that what you're doing in the Dockerfile change?

Because, I thought this test doesn't actually "fail" on failure.. it just kind of hangs.. is that OK with the GitHub actions framework?

[gregorycoppola]

thanks for the fix!

[pavitthrap]

my only concern is the use of a while loop here (in case the new blocks take a while to reach the db_height). It's my impression that elsewhere in the code base we "fail" and try again - for example, by returning burnchain_error::TrySyncAgain.

I'm still approving, but wondering what your thoughts are on this.

[lgalabru]

There are a few places in the codebase where we block on operations.
This patch is clearly an edge case that we saw on Bitcoin Testnet (way more instable), and will probably never see on Mainnet. The TrySyncAgain was my initial approach, however on the next retry, we would be loosing the information "we just reorgd and waiting for more blocks from the new canonical fork", and would be trying to fetch blocks using the burnchain tip of the previous fork as a point of reference (which is the real issue).

This approach is limiting the patching footprint, I think the clean solution would be introducing a DB schema modification, where we would be updating the BurnchainDB when we reorg, and marking the blocks from former canonical fork as such.
The approach that I've just described is way more invasive and subject to introduce regression, which is something I'd like to avoid for the next few weeks :).

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