Introduce new rollback strategy #423
Conversation
common/src/caryatid.rs
Outdated
| /// Topic to publish on | ||
| topic: String, | ||
|
|
||
| // When did we publish our last non-rollback message |
There was a problem hiding this comment.
When = slot number, by context? Could note here
| // Get a mutable state | ||
| let mut state = history.lock().await.get_current_state(); | ||
|
|
||
| // Read per-block topics in parallel |
There was a problem hiding this comment.
Ah I see you're cleaning this up too, nice!
| .await | ||
| } | ||
|
|
||
| /// Publish a rollback message, if we have anything to roll back |
There was a problem hiding this comment.
Is there some trick to make this common across all publishers? In C++ it would just be a RollbackEnabledPublisher superclass (I miss inheritance sometimes!)
There was a problem hiding this comment.
There's no formal concept of a "publisher" in this codebase at all, just a bunch of existing structs named Publisher with similar logic. I moved that logic into a RollbackAwarePublisher struct, which all of the existing structs are now just wrappers for.
| context.run(async move { | ||
| // Get promise of params message so the params queue is cleared and | ||
| // the message is ready as soon as possible when we need it | ||
| let mut params_message = params_subscription.read(); |
There was a problem hiding this comment.
Ooh, more TypeScript mindset showing!
| loop { | ||
| let (blk_g, gov_procs) = Self::read_governance(&mut governance_s).await?; | ||
| let (_, message) = governance_s.read().await?; | ||
| //let (blk_g, gov_procs) = Self::read_governance(&mut governance_s).await?; |
There was a problem hiding this comment.
Commented code alert... Is read_governance() no longer used?
There was a problem hiding this comment.
Removed.
read_governance is no longer used, because now this stream can emit two different messages (CardanoMessage::GovernanceProcedures or CardanoMessage::Rollback) and we want to handle both of them. Hiding the match-on-message-type logic inside a helper function would have made that complicated.
sandtreader
left a comment
sandtreader
left a comment
There was a problem hiding this comment.
Looks great!
Some minor comments, nothing big.
Description
Right now, all the state modules handle rollbacks at the same time that they process new messages. After a rollback has occurred, the
statusfield on the nextBlockInfowill beRolledBack. When modules read this status from messages on their "synchronizer" topic (the topic they use to decide where on the chain they are), they roll back all state to right before the newly received message before processing it.The trouble with this is
What this PR does
Introduce a new StateTransition message, with a Rollback variant. The system is prepared to send Rollback state transition messages throughout itself. When a module receives a Rollback on its "synchronizer" topic, it will forward that Rollback to every topic it writes to. When a module receives a Rollback on any other topic it reads from, it will ignore it.
I've added two utilities to make this pattern easier to follow:
Subscription.read_ignoring_rollbackswill read from a subscription until it finds any message which is not a Rollback, and then return that.RollbackAwareProducerwrites messages to a specific topic, but will only write Rollback messages to that topic if they they would roll back the most recently published non-rollback message. The idea is that we only propagate rollback messages to downstream topics if there is anything from the topic to actually roll back.I also added a
Pointtype to use in the point message, and madePeerNetworkInterfaceaccept that type in itsFindIntersectcommand. That's because rollbacks can theoretically be all the way to the origin, and indexing could start at the origin.What this PR does not do
Nothing is actually producing these new state transition messages yet. The PeerNetworkInterface will eventually publish them to
cardano.block.available, but it doesn't yet.Nothing depends on state transitions. They are forwarded around the system, but no functionality depends on them. Everything still relies on checking for
block_info.status == RolledBack.Related Issue(s)
Contributes to #421
How was this tested?
Ran omnibus, didn't see any behavioral differences from before. Which is what I expected, because the new messages aren't actually threaded through.
Checklist
Impact / Side effects
None
Reviewer notes / Areas to focus
I think the overall architecture is more relevant in this PR than any specific code.