sdk/state: when channel closes at stale state the channel does not update to closed state

[leighmcculloch]

When testing with the example console application manually and running through the scenario where a participant closes using stale data using #302, I saw that the channel of both participants update to the ClosingWithStaleState after the declaration transaction is ingested, but it did not change state when the close transaction was ingested.

> declarecloseidx 2
> ingesting cursor: 147592256163840 tx: 48e0bfaaa2fac9bb64b134bb7a3309445d9460d6cedca775e279b2d8debe0507
state before: 1
state after: 3
writing event: 3

> closeidx 2
> ingesting cursor: 147794119626752 tx: 1e5e89232907ea2d75ad5b0fa19b26b848ed9016c99307ace7a65283c3c0b134
state before: 3
state after: 3

State 3 is the ClosingWithStaleState. I expected to see state 4 which is the Closed state to occur after the close transaction was ingested, which was the 1e5e89 transaction.

[acharb]

this is because the state depends on the sequence num, which never reaches the latest declTx or closeTx seqNums.

@leighmcculloch I think the best way to handle is adding a flag like c.channelClosed that gets set to true in IngestTx if there isn't both signers on the initiator Escrow. And then from that flag we can better interpret the channel state. thoughts / is there a simpler solution I'm not thinking of?

🤔 we can also add the state: StateClosedWithOutdatedState (or StateClosedPrematurely). I think it adds more info for the user at the expense of some added complexity in the logic (can't just say StateClosed if c.channelClosed)

[leighmcculloch]

I think what we can do is, if the channel is open, and seq num is less than latest closing, we could mod the seq to find out whether it is the closing/closed tx. Theoretically we could do that for the latest closing/closed too, but I think when you were working on that logic I suggested you use the stored value inside the transactions and check for exact equality. Maybe that was short-sighted of me, because we don't have all txns stored for earlier revisions to do the same thing. I think we could change to the approach I think you had suggested previously, checking what type of tx the seq would be.

Wdyt?

🤔 we can also add the state: StateClosedWithOutdatedState (or StateClosedPrematurely).

+1. I think this is worthwhile.

[acharb]

sgtm 👍 seems simpler. My concern with mod initially was what if EI after closing on an earlier closeTx then bumped its sequence +1, then the channel would ingest and say channel is back open.

But since we're blocking IngestTx if the channel is closed, shouldn't be an issue.

[leighmcculloch]

Good point. Having IngestTx not ingest after that point becomes more critical. 👍🏻

Something else that this reinforces is that while #312 and #319 made it safe for IngestTx to ingest transactions for the two escrow accounts out-of-order with each other, it is still critical that for a single escrow account that transactions are still seen in order.

For example, this ingestion order is supported where ER's txs are ingested before EI's.

EI tx=1
ER tx=3
ER tx=4
EI tx=2
EI tx=5

But, this ingestion order is not supported where ER's txs are ingested out of order.

EI tx=1
ER tx=4
ER tx=3
EI tx=2
EI tx=5

[acharb]

But, this ingestion order is not supported where ER's txs are ingested out of order.
EI tx=1
ER tx=4
ER tx=3
EI tx=2
EI tx=5

why isn't this supported? If we see an older transaction we're ignoring it atm, is that what you mean by not supported?

[leighmcculloch]

You're correct we ignore transactions that would shift a seq num backwards, and we ignore older txs when updating balances, but it is still not safe to ingest txs for a single account out of order, only that txs across two accounts can be, even if they are shared.

This isn't supported because we'd miss states. For example, if I close a channel with an old state and then continue to transact on it, if you ingested those txs out of order you might see the channel close at the latest state, and then see the early close tx later.

[acharb]

This isn't supported because we'd miss states. For example, if I close a channel with an old state and then continue to transact on it, if you ingested those txs out of order you might see the channel close at the latest state, and then see the early close tx later.

🤔 good point, I hadn't thought about that

[leighmcculloch]

I think I thought #312 and #319 would make it okay to ingest all transactions out of order, but when I got into it, not so much. But that's okay, we still get a big win out of #312 and #319 because we can now do:

• sdk/horizon: make transaction streaming stream only the escrow accounts #320

which means we no longer need to stream all transations on the whole network to ingest. 🎉