[DRAFT] [miner] Reduce fork and orphan rate with an interruptable miner

[jcnelson]

This PR addresses #3334, #3324, and #3323.

I was looking at ways by which flashblocks and forks can originate at random, especially when the mempool is under load. It turns out, there are a few common ways by which a node can be prevented from learning the latest chain tip in a timely manner, which increases the likelihood of these two phenomena. These ways are both related to how write-locks are acquired in the StacksChainState database state:

• Relayer / ChainsCoordinator conflict: When the relayer thread trying to mine a block, it must currently open a write-lock to the underlying chainstate DB's MARF because it will ultimately store a state trie representing the new block (or microblock state) to the database. This in turn prevents the ChainsCoordinator thread from processing new blocks. The reverse is also true -- the ChainsCoordinator can prevent the relayer from mining. The effect of a congested mempool here is that the relayer can sometimes prevent the ChainsCoordinator thread from making progress for some time, even if it's trying to write a new Stacks block. Worse, the block the relayer is mining is going to be an orphan if it wins sortition.

• Relayer / P2P conflict: Currently, the relayer processes a block or microblock it produces before it advertises it to the peer network. But, the relayer can get stuck while doing this if the ChainsCoordinator is actively processing new chainstate. This prevents the peer from quickly announcing their block to the peer network, which in turn delays other peers from learning about a new chain tip, which in turn gives their relayer threads a better chance of mining an orphan. Having a congested mempool amplifies this effect, because the relayer thread can start mining and get stuck mining for a long, long time before the ChainsCoordinator thread can act on a newly-announced block.

This PR does the following to address it (see system schematic):

/// System schematic.
/// Legend:
///    |------|    Thread
///    /------\    Shared memory
///    @------@    Database
///    .------.    Code module
///
///
///                           |------------------|
///                           |  RunLoop thread  |   [1,7]
///                           |   .----------.   |--------------------------------------.
///                           |   .StacksNode.   |                                      |
///                           |---.----------.---|                                      |
///                    [1]        |     |    |     [1]                                  |
///              .----------------*     |    *---------------.                          |
///              |                  [3] |                    |                          |
///              V                      |                    V                          V
///      |----------------|             |    [9,10]   |---------------| [11] |--------------------------|
/// .--- | Relayer thread | <-----------|-----------> |   P2P Thread  | <--- | ChainsCoordinator thread | <--.
/// |    |----------------|             V             |---------------|      |--------------------------|    |
/// |            |     |          /-------------\    [2,3]    |    |              |          |               |
/// |        [1] |     *--------> /   Globals   \ <-----------*----|--------------*          | [4]           |
/// |            |     [2,3,7]    /-------------\                  |                         |               |
/// |            V                                                 V [5]                     V               |
/// |    |----------------|                                 @--------------@        @------------------@     |
/// |    |  Miner thread  | <------------------------------ @  Mempool DB  @        @  Chainstate DBs  @     |
/// |    |----------------|             [6]                 @--------------@        @------------------@     |
/// |                                                                                        ^               |
/// |                                               [8]                                      |               |
/// *----------------------------------------------------------------------------------------*               |
/// |                                               [7]                                                      |
/// *--------------------------------------------------------------------------------------------------------*
///
/// [1]  Spawns
/// [2]  Synchronize unconfirmed state
/// [3]  Enable/disable miner
/// [4]  Processes block data
/// [5]  Stores unconfirmed transactions
/// [6]  Reads unconfirmed transactions
/// [7]  Signals block arrival
/// [8]  Store blocks and microblocks
/// [9]  Pushes retrieved blocks and microblocks
/// [10] Broadcasts new blocks, microblocks, and transactions
/// [11] Notifies about new transaction attachment events

First, it refactors testnet/src/stacks-node/neon_node.rs from a hairball of hard-to-follow functions into a set of properly defined structs and struct implementations that contain the state for the threads they represent. It also refactors all shared-thread state into a Globals struct to keep it accessible. I was forced to do this refactoring in order to effectively reason about how the node behaves; it was getting very hard to follow.

Second, it addresses these two problems by doing the following:

• Interruptible Miner Thread: Mining blocks and microblocks now occurs in a dedicated miner thread, which the relayer thread spawns. This miner thread can be interrupted when new Stacks block data or burnchain data arrives, which in turn causes the miner to exit and in doing so, releases the write lock. The ChainsCoordinator, Relayer, and RunLoop threads now instruct the miner to stop whenever they need to process new Stacks blocks or burnchain blocks.

• Announce Before Processing: The node now announces blocks before it processes them. This is fine because a correct miner will produce a valid block, so there's no need to process it first. It's more important for the network's health that other nodes can receive it and process it.

Regarding testing, this PR adds the beginnings of a couple of "smoke tests" that are too expensive to run in CI, but demonstrate that a set of nodes will produce a high-quality chain even when the mempool is congested with CPU-bound transactions, which forces the miner thread to be active for nearly 100% of the burnchain block time. I'm labeling this PR as a draft until we have a strategy in place for smoke testing.

[obycode]

I'll start a mock miner now and review some time tonight.

[obycode]

Ok, to the best of my knowledge, this looks good to me. Thanks for the great documentation. That was very helpful. Might be a good idea to get another review from someone who has looked at neon_node before this.

[gregorycoppola]

https://github.com/stacks-network/stacks-blockchain/pull/3335/files/e5e39b1b04f497e4b703d81a0875842d5e3f6c69#r992508541

are these tests set up to run properly now? i still set it as TODO

[jcnelson]

@gregorycoppola Yes, they're still disabled in CI. There's no real way to run them in github CI because they take too long. But separately, we could create a smoke test CI that ran them nightly and reported results. I think that's best handled in a separate PR, because it'll take some time to settle on a smoke test procedure we're happy with.

[jcnelson]

@obycode Thanks! I've added @lgalabru, who wrote the original neon_node implementation.

[lgalabru]

Thanks @jcnelson! Rewriting the runloop was in my "someday" to do list.
I had a radically different design in mind for the next iteration. I suspected a performance issue, but another big concern that I would love to see addressed somehow is testability of the runloop, which would require a lot of decoupling work.
Anyway, I'm glad you were able to get there!

Question concerning the spec. I have a good understanding of why any new burnchain data needs to interrupt the miner, but not sure to follow why new Stacks block should be interrupting the mining loop? could this be an attack vector? De facto, miners are trusting their bitcoin node, but they don't have this relationship with the stacks-node peers?

Concerning the failing tests, are they passing on your machine? It's a new runloop so we probably need to be diligent.

Beside my question on interruption caused by Stacks blocks, I think my only concern with this PR is around testing: I see a good amount of assertions being removed, and maybe not enough assertions being introduced.
Feel free to merge if you think that the manual testing that went (and/or planned) through this upgrade gives us enough confidence!

[lgalabru]

I see the arrange and act phases, but can't see the asserts. Are the assertions happening somewhere else?

[jcnelson]

Right -- this is the beginning of a smoke test that won't run in CI, but would run as part of a nightly run (it takes too long to run in CI).

[lgalabru]

Why are we getting rid of these tests?

[jcnelson]

This deletion is part of mining_transactions_is_fair.

[lgalabru]

Why are we getting rid of these tests?

[jcnelson]

This gets rid of mining_transactions_is_fair. This test no longer reflects the mining strategy we use. If Alice sends 10 transactions collectively worth 10 STX, but Bob sends 2 transactions worth 100 STX, and the block can only fit Alice's or Bob's transactions, then Bob's transactions should be prioritized over Alice's. This isn't "fair" in that Alice's low-value transactions will get ignored for being low-fee, but it produces a higher profit for miners (which is what we really want).

[jcnelson]

@lgalabru Question concerning the spec. I have a good understanding of why any new burnchain data needs to interrupt the miner, but not sure to follow why new Stacks block should be interrupting the mining loop? could this be an attack vector? De facto, miners are trusting their bitcoin node, but they don't have this relationship with the stacks-node peers?

So, the problem we're trying to address is that a node could start mining before it has received the latest Stacks data for the canonical chain tip. If this happens, then the block the miner will produce will be a sibling block to the chain tip, and could end up getting mined (leading to a short-term fork). By making it so that the arrival of new Stacks blocks or microblocks (i.e. that this node doesn't already have) interrupts the miner, it gives the system a chance to process them first, and in doing so, learn the canonical chain tip before it tries mining again.

I see that the the check that does this here is blindingly trusting the NetworkResult data, which does open a DDoS vector whereby someone can just post the same blocks over and over again and stall mining. I'll fix it so that the miner is only interrupted by the arrival of new block or microblock data, which can be checked and reported by the Relayer::process_network_result() method.

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