Consumer handles expired client to provider

[mpoke]

Partial fix for #435 (see #448 for the other part). The consumer collects the IBC packets that cannot be sent to the provider due to the client being expired. Once the client is upgraded, these packets are sent in the next block.

[mpoke]

@sainoe This PR removes the SlashRequests from the genesis state. Instead, we need to add the pending data packets, but it's a bit more involved. See #264 (comment) for more details.

[MSalopek]

This is an edgecase also handled in #448.

I'm wondering if this should invoke a panic?

We could simply delete packets that were sent so we don't resend them when a new client is established?

[mpoke]

This should never happen. The client cannot expired in the middle of a block as the time doesn't change. Also, it cannot expire in the middle of a loop as the SDK is sequential. I guess we could remove this check to save space :)

[jtremback]

The code structure here is very hard to follow IMO. As one example, the call to SendPendingDataPackets on line 89 of relay.go can return false for its clientExpired return value, even if the client is expired, if the queue is empty. This is then checked again lower down in the function, leading to a pretty circuitous logic path. There are other examples as well.

I believe that all of this could be made much simpler if the functions SendVSCMaturedPackets and SendSlashPacket would only ever append to the PendingDataPacket queue. Then, another endblock method would deal with sending everything in the PendingDataPacket queue and dealing with expired clients etc.

[mpoke]

I believe that all of this could be made much simpler if the functions SendVSCMaturedPackets and SendSlashPacket would only ever append to the PendingDataPacket queue. Then, another endblock method would deal with sending everything in the PendingDataPacket queue and dealing with expired clients etc.

I agree this would simplify the logic. The only downside is that we'll do a read and write to the store for every packet (even when the client is not expired).

[sainoe]

Maybe I missed sthg but sending the pending packet in Endblock won't work due to the channel order property. For instance, in a block h1 the channel isn't established so all packets are appended to the queue. Then in the next block h2 the channel is ready and a SlashPacket for downtime infraction is sent in BeginBlock. In this case the packets order isn't respected.

[mpoke]

Maybe I missed sthg but sending the pending packet in Endblock won't work due to the channel order property. For instance, in a block h1 the channel isn't established so all packets are appended to the queue. Then in the next block h2 the channel is ready and a SlashPacket for downtime infraction is sent in BeginBlock. In this case the packets order isn't respected.

The idea is that all packets to be send are added to the queue and then in EndBlock all the packets in the queue are sent. The order is maintain by the queue. In your example, the SlashPacket for downtime infraction is appended at the end of the queue in the BeginBlock of h2.

[sainoe]

Okay I got it! Thanks for the correction.

[danwt]

I did read through and I like the semantic changes but I agree with other comments that it could do with some substantial refactoring. I'd accept a merge as long as it was followed up with refactoring immediately afterwards.

Nit: I think DataPackets as a term is a bit weird: all packets have data so it's not informative. Just Packets is fine

[danwt]

By the way, will the changes here affect the fact that currently pending consumer slash requests are sent in reverse order?

• Document why pending consumer initiated slash requests are sent in reverse order to provider #368

[mpoke]

By the way, will the changes here affect the fact that currently pending consumer slash requests are sent in reverse order?

• Document why pending consumer initiated slash requests are sent in reverse order to provider #368

Yes. That was just a stupid optimization. Was not necessary for safety.

[MSalopek]

Merged into marius/435-client-expired (#448) as advised by the rest of the team.

[MSalopek]

Closing in favor of #448 which has all changes in a single PR.