Skip to content
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

Prune stale acknowledgements after successful channel upgrade #3937

Closed
3 tasks
colin-axner opened this issue Jun 22, 2023 · 4 comments · Fixed by #5444
Closed
3 tasks

Prune stale acknowledgements after successful channel upgrade #3937

colin-axner opened this issue Jun 22, 2023 · 4 comments · Fixed by #5444
Assignees
Labels
04-channel channel-upgradability Channel upgradability feature

Comments

@colin-axner
Copy link
Contributor

colin-axner commented Jun 22, 2023

Summary

Prune stale acknowledgements after successful channel upgrade.

Problem Definition

Acknowledgements currently live forever in IBC state despite no longer being necessary once a packet has completed its lifecycle. Overtime this can add up to quite a bit as outlined in #2869

Once a channel successfully upgrades, all old packets using previous channel parameters have been flushed and completed their lifecycle. The acknowledgement on the counterparty can then be safely removed.

Proposal

To avoid potential gas issues in block processing due to an unpredictable amount of acknowledgements to prune, I suggest the channel keeper store a mapping from the port/channel ID to the highest stale sequence. By stale sequence, I mean a sequence which refers to packets which have fully completed their lifecycle and thus all their information may be safely deleted.

The channel upgrade will already receive the counterparty latest sequence send, so on an upgrade it simply needs to store it:

// naming can be changed
k.setAcknowledgementPruningSequence(portID, channelID, counterpartyLatestSequenceSend)

A new msg type will need to be added which may prune any acknowledgement with a sequence <= to the highest stale packet sequence

message MsgPruneAcknowledgements {
  string port_id = 1;
  string channel_id    = 2;
  // optionally add a parameter to control the amount of acknowledgements to prune to avoid overrunning gas (we can alternatively account for this in the handler by checking gas left after each deletion)
}
// PruneAcknowledgements defines a rpc handler method for MsgCreateClient.
func (k Keeper) PruneAcknowledgements(goCtx context.Context, msg *clienttypes.MsgPruneAcknowledgements) (*channeltypes.MsgPruneAcknowledgementsResponse,
    ctx := sdk.UnwrapSDKContext(goCtx)

    pruningSequence, found := k.channelKeeper.GetAcknowledgementPruningSequence(ctx, msg.PortId, msg.ChannelId)
    if !found {
        return nil, err 
    }   

    // reverse iterate from earliest packet sequence, delete each acknowledgement until the acknowledgement sequence > pruningSequence 
    k.channelKeeper.pruneAcknowledegments(ctx, msg.PortId, msg.ChannelId, pruningSequence)
   

    return &channeltypes.MsgPruneAcknowledgementResponse{}, nil 
}

We can also add some result value to the MsgPruneAcknowledgementResponse to indicate if all possible acknowledgements were pruned, or if there are some left

Once the lowest acknowledgement sequence is greater than the pruning sequence, we can remove the pruning sequence mapping as well

Some quick pseudo code:

// TODO: add gas handling
func pruneAcknowledgements(ctx sdk.Context, portID, channelID string, pruningSequence uint64) {
    store := ctx.KVStore(k.storeKey)
    iterator := sdk.KVStorePrefixIterator(store, []byte(fmt.Sprintf("%s/%s", host.KeyPacketAckPrefix, host.ChannelPath(portID, channelID)))
    defer iterator.Close()

    for ; iterator.Valid(); iterator.Next() {
        iterKey := iterator.Key()
        seq := host.ParseSequence(iterKey)
        if seq > pruningSequence{
            break
        }   
        
        store.Delete(iterator.Key)
    }

    store.Delete(keyPruningSequence(portID, channelID))
}

For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged/assigned
@colin-axner colin-axner added 04-channel channel-upgradability Channel upgradability feature labels Jun 22, 2023
@colin-axner colin-axner added the needs discussion Issues that need discussion before they can be worked on label Jun 22, 2023
@DimitrisJim DimitrisJim self-assigned this Jul 5, 2023
@DimitrisJim
Copy link
Contributor

unsure what the timeline is on this but I'd like to take it on when the time is right ™️

@crodriguezvega
Copy link
Contributor

crodriguezvega commented Nov 20, 2023

Now that we removed LatestSequenceSend from Upgrade and we don't have this information available on the counterparty, do we need to rethink who to implement this feature? I guess we could add LatestSequenceEnd back if there is no other way, but would it be possible to use something like the following?

When chain A moves to OPEN (in openUpgradeHandshake) after completing the upgrade successfully we could get from the keys acks/ports/{portID}/channels/{channelID}/sequences/{sequence} the highest sequence number from all stored acknowledgements and store it as the acknowledgement pruning sequence. If the channel end on chain A has moved to OPEN and the upgrade has succeeded, then the counterparty (chain B) must have, at least, moved to FLUSHCOMPLETE, so so all packets sent from the counterparty chain should have completed the lifecycle.

@crodriguezvega crodriguezvega moved this from Todo to On hold in ibc-go Nov 27, 2023
@crodriguezvega crodriguezvega removed the needs discussion Issues that need discussion before they can be worked on label Dec 4, 2023
@crodriguezvega crodriguezvega moved this from On hold to Todo in ibc-go Dec 4, 2023
@DimitrisJim DimitrisJim moved this from Todo to In progress in ibc-go Dec 13, 2023
@DimitrisJim
Copy link
Contributor

DimitrisJim commented Dec 14, 2023

Gonna split this one up in a couple of PRs to make it faster to review.

  • protos/msgs.go impls tests/keeper + msg server stubs
  • setting ack pruning seq in upgrade open + relevant glue code (set/get in keeper/key in host etc)
  • impl of keeper/msg server + tests.

@colin-axner
Copy link
Contributor Author

woot woot this is done, see all linked issues

@github-project-automation github-project-automation bot moved this from In review to Done in ibc-go Dec 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
04-channel channel-upgradability Channel upgradability feature
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants