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

⚗️ Re-architecting state machine #576

Closed
D4nte opened this Issue Dec 16, 2018 · 6 comments

Comments

Projects
None yet
3 participants
@D4nte
Copy link
Member

D4nte commented Dec 16, 2018

DoD:

  • Presentation of the new architecture including upsides and downsides.
  • New issues ready to be groomed, under a new Epic.
  • Current "Refund" issues updated/deleted.
  • How do we get refund with this re-architecture (without new "refundable" states).
  • How do we add "secret reveal" before redeem transaction is confirmed (mempool).
@LLFourn

This comment has been minimized.

Copy link
Contributor

LLFourn commented Jan 8, 2019

Forward

While doing this spike I realised that the problems with the state machine can be split into 3:

  1. What we store in the storage layer is too tightly coupled to our state machine.
  2. We shouldn't parameterize our state machine by the Role. You don't need to know what role you are to watch the blockchains.
  3. I don't think we should have a single state machine across both ledgers but rather individual state machines for alpha and beta ledgers.

I decided that fixing (1) first is the most useful because once that's done we can restructure the state machines however we want. I think that (2) can be done at the same time because it arises from the same underlying design issue.

Problem

Right now we have a very tight coupling between states in the state machine and actions (Actions is implemented on the state). The HTTP controllers are able to get the current state from the state_store. This means that if we want to add new actions or represent new knowledge we have to do so by adding new states. For example:

  • To add refund actions we have to create new Refundable states for every situation where you could refund one or the other
  • To represent the fact that you can learn the secret from a tx in the mempool before it's confirmed you'd have to have some a new pair of states e.g. SecretRevealedAlphaFundedBetaFunded, SecretRevealedAlphafundedBetaRefunded (and even more with the Refundable thing!)

Obviously doing the above leads to a combinatorial explosion of new states. I don't want to solve that issue now but at least make it so if we do have double the number of states in the state machine we don't have to deal with all that complexity when producing actions or HTTP responses.

Solution

Instead of retrieving state machine states from the storage layer we get Alice or Bob structs. These structs represent the actor's current view of the world and their secret knowledge. This includes their view of the ledgers, the request/response data and anything else they learn during the execution of the protocol (e.g. learning the secret early from the mempool). From the outside we retrieve Alice or Bob and ask them for the actions or details about what they know. For example, here's what an Actor trait might look like that Alice and Bob implement (we may not even need this trait but it's here to help the exposition):

/// This is what we are able to pull out of the DB. Each variant represents the transactions on the blockchain at the time
enum LedgerState<L: Ledger> {
    None,
    //probably a Deploy in here too but ommitted for clarity
    Funded{
       htlc_location: L::HtlcLocation,
       // we might not even need this if we transition to time based locks rather than block height
       refundable: bool
    },
    Redeem{ htlc_location: L::HtlcLocation, redeem_tx: RedeemTransaction<L> },
    Refund{ htlc_location: L::HtlcLocation, refund_tx: RefundTransaction<L> }
}

trait Actor<AL, BL, AA, BA> {
    /// The enum of actions
    type ActionKind;

    /// The actions the actor can do right now
    fn actions(&self) -> Vec<Self::ActionKind>;

    /// The actor's view of what has happened on the ledgers
    fn alpha_ledger_state(&self) -> LedgerState<AL>;
    fn beta_ledger_state(&self) -> LedgerState<BL>;

    /// Returns something like what `Initiation` contains now
    /// alpha_ledger_refund_identity, alpha_ledger, secret_hash etc
    /// (whatever we specify as the initial data in rfc003 + rfc002)
    fn initial_data(&self) -> InitialData<AL,BL,AA,BA>;

    /// What we specify should be in the accept/reject resoinse (if we have it)
    fn response_data(&self) -> Option<Result<AcceptResponse<AL,BL>, SwapReject>>;
}

Implementation

Here's how we can realise the above API. We take the Alice and Bob types that implement Role and restructure them so they can implement Actor. An implementation of Bob might look like (with some things omitted):

struct Bob<AL, BL, AA, BA> {
    /// The swap request data
    initial_data: InitialData<AL, BL, AA, BA>,
    alpha_ledger_state: LedgerState<AL>,
    beta_ledger_state: LedgerState<BL>,
    /// When the secret gets revealed before the transaction is confirmed in the mempool.
    /// How this happens is out of scope for this.
    early_secret_reveal: Option<Secret>

    /// This is something similar to what `response_sender` is right now but it actually stores
    /// the response once the accept/decline action is taken. Note the response in this thing is
    /// what we currently call a `StateMachineResponse` (i.e. it may have private keys)
    response_state: ResponseState,

    /// we may have ended in failure and aborted execution
    error: Option<rfc003::Erorr>
}

impl Actor for Bob<Bitcoin, Ethereum, BitcoinQuantity, EtherQuantity> {
    /// Pretty much what we have now
    type ActionKind = alice::ActionKind;

    fn actions(&self) -> Vec<Self::ActionKind> {
        match (self.alpha_ledger_state, self.beta_ledger_state) {
            (LedgerState::None, LedgerState::None) => {
                // This is very rough
                self.response_state.get_response() {
                    NoResponseYet(accept_action, decline_action) => vec![accept_action, decline_action],
                    _ => vec![],
                }
            },
            (LedgerState::Funded{...}, LedgerState::None) => {
                vec![bob::ActionKind::Fund(swap.fund_action())]
            },
            (LedgerState::Funded{...}, LedgerState::Funded{refundable, beta_htlc_location}) => {
                if refundable {
                    vec![bob::ActionKind::Refund(...)]
                }
                else {
                    vec![]
                }
            }
            // etc
        }
    }

    // The other methods just return the data bob has access to
}

Changes to state machine and storage layer

The state machine itself wouldn't have to changed except to improve it by removing the role and the parameterization by the role. Instead, we would just pass a Sender instead of a RwLock<..> as the SaveState. Then on the outside we'd get a stream of each state change and interpret each state change and adapt it to the the appropriate storage layer changes.

// Code for this would go in the "Spawners"
let alpha_ledger_state_handle = storage_layer.alpha_ledger_state_handle(&swap_id);
let beta_ledger_state_handle = storage_layer.beta_ledger_state_handle(&swap_id);
let response_state_handle = storage_layer.response_state_handle(&swap_id);

stream_of_states.for_each(move |new_state| {
    match new_state {
        SwapStates::AlphaFunded(..) => alpha_ledger_state_handle.write(LedgerState::Funded{ ... }),
         ...
    }
})

Actually I'm not sure if there could be one .response_state_handle given the data you write there is different for Alice and Bob 🤔. The implementor would have to think of the right way to do it for the cases of Alice and Bob.

Cons of the solution

The only downside of this solution I can see is that it makes it a bit harder to go restart the state machine after restarting the node because we are no longer just storing the state machine state in the storage layer. In order to restart the state machine we'd have to make some kind of matching logic which interprets what is stored and figures out which state machine state we should be in.

This con is to be expected because the purpose of this ticket is to decouple the storage layer from the state machine.

Incremental Solution

We could keep storing the state machine states in the storage layer (drop the idea of LedgerState) but still remove the Role trait and transition to an Actor trait. Instead of storing the ledger state in the Alice or Bob we'd just pass in the state to their actions method:

I don't think there's any use to this anymore.

impl Actor for Bob<Bitcoin, Ethereum, BitcoinQuantity, EtherQuantity> {
    /// Pretty much what we have now
    type ActionKind = alice::ActionKind;

    fn actions(&self, state: state_machine::SwapStates) -> Vec<Self::ActionKind> {
        match state {
            use self::SwapStates as SS;
            match *self {
                SS::Start(Start { ref role, .. }) => vec![
                    bob::ActionKind::Accept(role.accept_action()),
                    bob::ActionKind::Decline(role.decline_action()),
                ],
                SS::AlphaFunded(AlphaFunded { ref swap, .. }) => {
                    vec![bob::ActionKind::Fund(swap.fund_action())]
                }
                // ...
            }
        }
    }
}

This would solve problem (2) by itself instead of problem (1) together.

@LLFourn LLFourn added review and removed work-in-progress labels Jan 8, 2019

@D4nte

This comment has been minimized.

Copy link
Member Author

D4nte commented Jan 8, 2019

You are avoiding the creation of a new Refundable state by adding a bool to the Funded state. Instead of explicitly splitting into two state FundedNonRefundable and FundedRefundable you do it implicitly by adding a bool to the Funded state (ie, creating sub-states).

Also, the Refundable part may not be a state of the blockchain per say.
The state of the blockchain is "block X has been mined", if X = expiry then yes, it becomes refundable and you could consider it a state.

Now, let's say we want to restrict the BothFunded state to be only within a given timeframe for Alice. E.g., we do not want Alice to redeem the beta ledger if we are within 1 hour of the expiry of the beta HTLC.
One way would be to add a safely_redeemable boolean to the BothFunded state (for Alice).
But it does not seem correct as it does not describe a blockchain state but better an interpretation of the blockchain state.

Would it make more sense to add a property to this state such as blockchain_height and then use this property to generate the actions? This blockchain_height can then be used to generate (or not) the redeem and refund actions.

Let me know your thoughts.

@LLFourn

This comment has been minimized.

Copy link
Contributor

LLFourn commented Jan 8, 2019

The tickets I would create to realise this:

  1. Remove all the associated types from Role and store the keys and the secret inside Alice and Bob. For now we would leave the role in the state and the Actions trait implemented on SwapStates.

Then in actions we would ask Bob and Alice for the redeem and refund actions (like we ask Bob for the accpet and decline actions currently).

  1. Make all of the above changes

Refund feature: To get the refund feature we might not even need these changes because I think we can just use the system time. We have to transition away from Block height to timestamp because of the security issues with variable block times. To do it properly we would need these changes and then write to the storage layer at the earliest time we could possibly refund.

Early secret reveal: To do this I think we would need these changes. Once again we'd just set up a query which resolves as soon as the secret hits the mempool. When it does we'd write to storage that this has happened.

@LLFourn

This comment has been minimized.

Copy link
Contributor

LLFourn commented Jan 8, 2019

You are avoiding the creation of a new Refundable state by adding a bool to the Funded state. Instead of explicitly splitting into two state FundedNonRefundable and FundedRefundable you do it implicitly by adding a bool to the Funded state (ie, creating sub-states).

Yes. Keep in mind The LedgerState is not a state machine state. It's simply what's stored in the storage layer/database.

Also, the Refundable part may not be a state of the blockchain per say.
The state of the blockchain is "block X has been mined", if X = expiry then yes, it becomes refundable and you could consider it a state.

You would still need a to know when this has happened. The point about this change is that whenever this does happen you can just write it to the storage layer. You don't have to do it in the state machine.

Now, let's say we want to restrict the BothFunded state to be only within a given timeframe for Alice. E.g., we do not want Alice to redeem the beta ledger if we are within 1 hour of the expiry of the beta HTLC.
One way would be to add a safely_redeemable boolean to the BothFunded state (for Alice).
But it does not seem correct as it does not describe a blockchain state but better an interpretation of the blockchain state.

Yes. This is exactly the point of this change. So we can write flags like safely_redeemable to the storage layer without having to represent them as states in the state machine. (We'll see whether this is a good idea or not).

Would it make more sense to add a property to this state such as blockchain_height and then use this property to generate the actions? This blockchain_height can then be used to generate (or not) the redeem and refund actions.

Add them to where?
See my comments about not using block height anymore.

@LLFourn

This comment has been minimized.

Copy link
Contributor

LLFourn commented Jan 11, 2019

I think I'll leave this open until @thomaseizinger has had a look.

@LLFourn LLFourn assigned thomaseizinger and unassigned LLFourn Jan 11, 2019

@LLFourn

This comment has been minimized.

Copy link
Contributor

LLFourn commented Jan 13, 2019

After discussing with team it looks like this is the best way forward but we can put these changes on hold until we have a concrete issue with the current model.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment