Reactor State Dependencies

[alexanderbez]

As we refactor the existing reactors in context of the new p2p design (ref: #5670), we need to address and come up with a clean way to handle cases where reactors depend on external/shared state, as we want reactors to be as isolated as possible and have clear boundary semantics.

Here are the finding I've compiled after reviewing all the reactors. TL;DR if there is any dependency on external/shared state, it is either a peer and/or the p2p switch.

/cc @tessr @erikgrinaker

---

Reactor State Dependencies

State Sync

The state sync reactor relies on no external or shared state (yay 🎉 ).

Evidence

The evidence reactor relies on the following external or shared state:

1. Peer

The peer height is used to determine what evidence to send to a connected peer. Specifically, if the peer's height is less than or equal to the height of the evidence, then we ignore sending that piece of evidence to the peer.

Removing this state requirement is trivial but it means sending more messages over the wire such that the receiving peer will ignore it.

Blockchain v0

The blockchain v0 reactor relies on the following external or shared state:

1. P2P Switch

The reactor relies on the switch directly to get peers to broadcast messages to during poolRoutine. This isn't a problem though because the peer ID is part of the BlockRequest, so we have all the info we need to send messages to that peer.

There is one place where where we need the number of peers, but that is used in a log only which I suppose we can just omit.

Finally, the reactor depends on the switch to call an API on an entirely different reactor:

conR, ok := bcR.Switch.Reactor("CONSENSUS").(consensusReactor)
if ok {
  conR.SwitchToConsensus(state, blocksSynced > 0 || stateSynced)
}

I'm not clear on how handle this situation. Maybe we can completely remove v0 or a figure out a way to augment the reactor such that we can call this API without needing the switch (e.g. passing a function to the constructor).

Blockchain v1

The blockchain v1 reactor relies on the following external or shared state:

1. P2P switch

The reactor relies on a p2p switch which is wrapped with a private switchIO type. This is somewhat problematic. The reliance on the TrySend and Broadcast APIs are not problematic. However, we face the same problem as we do in the blockchain v0 reactor -- SwitchToConsensus.

PEX

The pex reactor relies on the following external or shared state:

1. Peer

A call to the peer's NodeInfo is made during AddPeer so we can add the resulting NetAddress to the address book. Various other APIs are called such as IsOutbound, FlushStop, and SocketAddr.

2. P2P Switch

Various APIs are called on the p2p switch such as IsDialingOrExistingAddress, NumPeers, IsPeerPersistent, DialPeerWithAddress, and MaxNumOutboundPeers.

Mempool

The mempool reactor relies on the following external or shared state:

1. Peer

Various APIs are called on the peer in the mempool such as getting the peer's height via Get, IsRunning and Quit.

Consensus

The consensus reactor relies on the following external or shared state:

1. Peer

The consensus reactor uses the peer's state, specifically to call APIs such as RecordVote, RecordBlockPart and SetHasProposalBlockPart. In fact the reactor also calls methods such as InitPeer which internally set the peer's state.

2. P2P Switch

The reactor uses the switch to get all known peers so it can further call APIs on the peer's state such as RecordVote and RecordBlockPart. In addition, it does peer management on the switch via API calls such as MarkPeerAsGood and MarkPeerAsGood.

Summary

Across all the reactors, the most common calls we make to external or shared state include direct API calls on the p2p Switch and the peer's state. Some reactors only rely on a peer's height, whereas other reactors mutate peer state directly.

[erikgrinaker]

the reactor depends on the switch to call an API on an entirely different reactor (SwitchToConsensus)

Yeah, the proper fix for this would be #4644, which I think we should tackle separately. A temporary hack like passing a function to the constructor is fine.

The pex reactor relies on the following external or shared state

This one's going to be a bit more tricky. I'll look into this as I prototype the router and peer store. Most likely we'll need to rewrite this reactor quite significantly with other abstractions or responsibilites.

Mempool: Various APIs are called on the peer in the mempool such as getting the peer's height via Get, IsRunning and Quit.

We'll need to come up with something for height (e.g. additional peer updates), the rest I believe can be replaced by the existing peer updates.

Consensus: The reactor uses the switch to get all known peers so it can further call APIs on the peer's state such as RecordVote and RecordBlockPart.

This peer state is a consensus-local concept, and can be replaced by local state tracking e.g. in a map, using peer updates to keep track of peers coming and going.

However, this state is also exposed directly via RPC. :(

tendermint/rpc/core/consensus.go

Line 56 in 56911ee

peerState, ok := peer.Get(types.PeerStateKey).(*cm.PeerState)

This probably needs the RPC layer to have access to the consensus reactor so it can request this information via a method call.

In addition, it does peer management on the switch via API calls such as MarkPeerAsGood and MarkPeerAsGood.

This can be replaced by something like peer errors (or possibly a broader concept like peer quality).

Thanks for gathering this info @alexanderbez! I don't think this sounds too bad, and we should be able to come up with reasonable solutions for most of this -- let's chew on it for a couple of days.

[tessr]

Nice, thanks for auditing all of these. A few thoughts:

1. I assume "blockchain v1" should be "blockchain v2"? We've removed v1 from master.
2. I'm not sure we need to get all the way into Node cleanup (Node startup phases #4644) right now, and I think that's probably a whole can of worms. But we might think about what the ideal relationships between all the Reactors and the Router would look like, if we had a nice Node startup process. Would some Reactors keep references to each other, so they can call into one another? Would all cross-Reactor communication happen through the Router? Or something different? (Maybe I'm thinking about this in the wrong way!)
3. I recommend holding off on introducing a generalized "peer quality," at least for now - it seems like a slightly premature abstraction, or perhaps like icing that we can add on top of the P2P cake once it's a little more fully baked 🍰

[erikgrinaker]

Summary of discussion on call about reactors sharing state:

• We should implement basic peer state handling in the P2P core first (i.e. addresses, connection attempts, etc), then figure out how to integrate it with reactors (if at all).

• PEX reactor is the odd one out, we'll have to deal with it separately.

• As for other reactors, they mostly need to keep track of peer heights. blockchain reactor currently queries this from peers directly, while mempool shares it with consensus. A few options:

  1. Share the state between reactors, either via messages or via shared read/write access to the peer store. This introduces dependencies between reactors. For example, if the consensus reactor is responsible for keeping track of peer heights, then it must be running when the other reactors are running, otherwise they won't get height information. We should probably avoid this.

  2. Have the P2P core responsible for exchanging basic status information (e.g. heights), and make this information available to reactors via some API (messages or peer store). Removes reactor dependencies, but requires protocol additions.

  3. Have each reactor query the information it needs itself. This needs e.g. mempool reactor to have new messages for querying heights.

We'll mull on this over the holidays.

[alexanderbez]

Currently I am the most inclined to proceed with (2) as it's the easiest to reason about and removes the requirement for reactors to manually have to poll for info.

[alexanderbez]

How do we close discussions @marbar3778?

[tac0turtle]

Either lock or delete it. There isn't a way to close it, unfortunately