Add pruning logic, implementation in message server. #5444

DimitrisJim · 2023-12-19T07:17:17Z

Description

closes: #3937

Commit Message / Changelog Entry

type: commit message

see the guidelines for commit messages. (view raw markdown for examples)

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

Targeted PR against the correct branch (see CONTRIBUTING.md).
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Code follows the module structure standards and Go style guide.
Wrote unit and integration tests.
Updated relevant documentation (docs/) or specification (x/<module>/spec/).
Added relevant godoc comments.
Provide a commit message to be used for the changelog entry in the PR description for review.
Re-reviewed Files changed in the Github PR explorer.
Review Codecov Report in the comment section below once CI passes.

crodriguezvega

I know this is still in draft, but left just a couple of comments. :)

crodriguezvega · 2023-12-19T08:41:27Z

modules/core/keeper/msg_server.go

+
+	// update pruning sequence in store
+	k.ChannelKeeper.SetAcknowledgementPruningSequence(ctx, msg.PortId, msg.ChannelId, sequence)
+
 	return &channeltypes.MsgPruneAcknowledgementsResponse{}, nil


Should we update the message to return the number of acks and receipts that were pruned?

we could log it instead? I have no strong preference, down to update it if people have one, it really depends on how and if others consume this information.

Let's do logging at least then.

ahah too late! I've changed this to return pruned, left. I think it made a lot of sense since we can work out this info.

crodriguezvega · 2023-12-19T08:41:53Z

modules/core/04-channel/keeper/keeper.go

+//
+// The pruningSequence keeps track of the packet acknowledgement that can be pruned next. When the pruning sequence reaches
+// the last send sequence, pruning is complete.
+func (k Keeper) PruneAcknowledgements(ctx sdk.Context, portID, channelID string, limit, pruningSequence uint64) uint64 {


Should the naming reflect that it also prunes packet receipts?

it most definitely should!

PruneStalePacketData? throwing ideas in the ether.

Note: should probably rename a bunch of things AckPruningSequence -> PruningSequence etc. Can do it in this PR but will increase noise.

PruneStalePacketData sounds good to me.

sounds good to me! Probably best to leave it as a slight follow up (to rename proto msg too) unless people don't mind the extended diff here.

modules/core/04-channel/keeper/keeper.go

damiannolan · 2023-12-20T11:24:09Z

Holding off on review until PRs are cascaded!

modules/core/04-channel/keeper/keeper.go

crodriguezvega

I think this is looking very good. Nice work, @charleenfei.

modules/core/04-channel/keeper/keeper_test.go

crodriguezvega · 2023-12-20T21:06:16Z

modules/core/keeper/msg_server.go

+
+	// update pruning sequence in store
+	k.ChannelKeeper.SetAcknowledgementPruningSequence(ctx, msg.PortId, msg.ChannelId, sequence)
+
 	return &channeltypes.MsgPruneAcknowledgementsResponse{}, nil


Let's do logging at least then.

modules/core/keeper/msg_server_test.go

modules/core/04-channel/keeper/keeper_test.go

modules/core/04-channel/keeper/keeper.go

Co-authored-by: Carlos Rodriguez <carlos@interchain.io>

DimitrisJim

I'm opening this PR for reviewz, I'm off from tomorrow so if people want to push nits/fixes on this, feel free to do so!

Oh, final note: the message should ideally be renamed, won't do it here since its already enough of a change and the name change diff would bring all sort of random files for additional review. Opening an issue for it now (#5473) but if people don't mind it being pushed here, please do!

proto/ibc/core/channel/v1/tx.proto

DimitrisJim · 2023-12-20T21:56:58Z

modules/core/04-channel/keeper/keeper_test.go

+}
+
+// sendMockPacket sends a packet from source to dest and acknowledges it on the source (completing the packet lifecycle).
+// Question(jim): find a nicer home for this?


so many people moved why not sendMockPackets? This and UgradeChannel seem like nice little helpers to have but they should do with a better home.

inb4: these already exist someplace and I missed em.

just realised I should've replied here instead of a new comment :D https://github.com/cosmos/ibc-go/pull/5444/files#r1434023684

Can also look into moving this to a more generalized testing func in the testing pkg at some point! :)

modules/core/04-channel/keeper/keeper.go

This reverts commit 84a3b99.

…une-acks

chatton

LGTM, I absolutely love the test coverage, really nice job! Had just a few small suggestions for tweaks in the tests but nothing major

modules/core/keeper/msg_server_test.go

proto/ibc/core/channel/v1/tx.proto

modules/core/04-channel/keeper/keeper_test.go

chatton · 2023-12-21T10:22:11Z

modules/core/04-channel/keeper/keeper_test.go

+			nil,
+		},
+		{
+			"success: stale packet state pruned, two upgrades",


nice consideration!

modules/core/04-channel/keeper/keeper_test.go

damiannolan

Just leaving two initial comments on the proto api.

Will review fully after lunch and I can also help push any follow up commits to the branch!

proto/ibc/core/channel/v1/tx.proto

AdityaSripal

Approving as I only have a minor nit on optimistically deleting

Ok with any of the proposed names. Either the current, or Damian's suggestions. Let's come to final consensus and change once so we don't keep changing on every new opinion 😛

modules/core/04-channel/keeper/keeper.go

modules/core/04-channel/keeper/keeper_test.go

…une-acks

damiannolan

LGTM!! 🚀 🎉

Awesome work @DimitrisJim and amazing work on the testing, looks super thorough!
Thanks to everyone for their commits and reviews too! ❤️

damiannolan · 2023-12-21T13:03:59Z

modules/core/04-channel/keeper/keeper_test.go

+}
+
+// UpgradeChannel performs a channel upgrade given a specific set of upgrade fields.
+// Question(jim): setup.coordinator.UpgradeChannel() wen?


damiannolan · 2023-12-21T13:04:34Z

modules/core/04-channel/keeper/keeper_test.go

+}
+
+// sendMockPacket sends a packet from source to dest and acknowledges it on the source (completing the packet lifecycle).
+// Question(jim): find a nicer home for this?


Can also look into moving this to a more generalized testing func in the testing pkg at some point! :)

colin-axner

Really top notch work @DimitrisJim! 👑 Thank you so much!!

modules/core/04-channel/keeper/keeper.go

colin-axner · 2023-12-21T12:53:27Z

modules/core/keeper/msg_server_test.go

+
+			if tc.expErr == nil {
+				suite.Require().NoError(err)
+				suite.Require().NotNil(resp)


super nit suggestion: we could assert the response values are as expected by sending a packet and asserting 1 packet receive is pruned

colin-axner · 2023-12-21T13:07:46Z

modules/core/04-channel/keeper/keeper_test.go

+					suite.chainA.GetContext(),
+					path.EndpointA.ChannelConfig.PortID,
+					path.EndpointA.ChannelID,
+					5,


Suggested change

5,

17,

should we be attempting to prune post upgrade packet sends by setting it to a higher limit? To show that it safely returns after all possible have been pruned?

colin-axner · 2023-12-21T13:09:58Z

modules/core/04-channel/keeper/keeper.go

+	if !k.HasPruningSequenceStart(ctx, portID, channelID) {
+		return 0, 0, errorsmod.Wrapf(types.ErrPruningSequenceStartNotFound, "port ID (%s) channel ID (%s)", portID, channelID)
+	}
+
+	pruningSequenceStart := k.GetPruningSequenceStart(ctx, portID, channelID)


I find it odd that the get func here doesn't return a found bool. But I guess it is fine

I agree, I found I had the same reaction too!

But apparently the argument is saving gas with a Has check? :D lol

Isn't this increasing gas? I suspect the majority of calls to this function to have the sequence set, which results in extra gas costs

I think it would increase gas in the successful case of it being set. And save gas in the scenario of it not being set

Yea! Looking at the gas config earlier though, this should be miniscule enough to not warrant it, we can totally switch it up to have get return val, found as usual, my bad in adding it this way.

colin-axner · 2023-12-21T13:11:34Z

modules/core/04-channel/keeper/keeper_test.go

+		}
+	)
+
+	testCases := []struct {


could we add a test case where a middle sequence doesn't have an ack stored (because a timeout occurred)?

Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com>

codecov-commenter · 2023-12-21T13:22:28Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (6c0db8d) 80.82% compared to head (9b36dc8) 80.90%.

Additional details and impacted files

@@                   Coverage Diff                   @@
##           04-channel-upgrades    #5444      +/-   ##
=======================================================
+ Coverage                80.82%   80.90%   +0.07%     
=======================================================
  Files                      197      197              
  Lines                    15195    15234      +39     
=======================================================
+ Hits                     12282    12325      +43     
+ Misses                    2441     2437       -4     
  Partials                   472      472

Files	Coverage Δ
modules/core/04-channel/keeper/keeper.go	`90.04% <100.00%> (+1.30%)`	⬆️
modules/core/keeper/msg_server.go	`62.76% <100.00%> (+0.64%)`	⬆️

damiannolan · 2023-12-21T13:25:09Z

Opened #5476 for testing improvements. Agree with @colin-axner's points!

DimitrisJim added the channel-upgradability Channel upgradability feature label Dec 19, 2023

crodriguezvega reviewed Dec 19, 2023

View reviewed changes

DimitrisJim force-pushed the jim/5414-msg-server-keeper-prune-acks branch from 2b8147f to 449cc8b Compare December 19, 2023 11:55

crodriguezvega added the priority PRs that need prompt reviews label Dec 19, 2023

crodriguezvega reviewed Dec 20, 2023

View reviewed changes

modules/core/04-channel/keeper/keeper.go Outdated Show resolved Hide resolved

DimitrisJim force-pushed the jim/5414-msg-server-keeper-prune-acks branch from 449cc8b to b9acc51 Compare December 20, 2023 08:33

DimitrisJim force-pushed the jim/5414-msg-server-keeper-prune-acks branch 2 times, most recently from 56edec6 to 98f10d1 Compare December 20, 2023 14:40

AdityaSripal reviewed Dec 20, 2023

View reviewed changes

modules/core/04-channel/keeper/keeper.go Outdated Show resolved Hide resolved

colin-axner reviewed Dec 20, 2023

View reviewed changes

modules/core/04-channel/keeper/keeper.go Outdated Show resolved Hide resolved

DimitrisJim force-pushed the jim/5414-msg-server-keeper-prune-acks branch from a99fa8f to de6ada4 Compare December 20, 2023 15:35

DimitrisJim commented Dec 20, 2023

View reviewed changes

modules/core/04-channel/keeper/keeper.go Show resolved Hide resolved

DimitrisJim added 3 commits December 20, 2023 18:46

Impl msg server, keeper logic.

036b0b0

Update naming used for start/end as per updates for keys.

07df54f

Self contained implementation logic in keeper.

8787fdf

DimitrisJim force-pushed the jim/5414-msg-server-keeper-prune-acks branch from de6ada4 to 5e646ba Compare December 20, 2023 17:14

Lint, test structure, error case checking.

6b5ada0

DimitrisJim force-pushed the jim/5414-msg-server-keeper-prune-acks branch from 5e646ba to 6b5ada0 Compare December 20, 2023 17:22

DimitrisJim added 3 commits December 20, 2023 22:51

Test test teeeeest.

0c7d8e4

Rename keeper function, document a couple of additional things.

cafb419

Test case: send packets after upgrade, no pruning.

e78cfd4

crodriguezvega reviewed Dec 20, 2023

View reviewed changes

DimitrisJim and others added 3 commits December 20, 2023 23:46

Return number pruned, number left in response.

83ed801

Update modules/core/04-channel/keeper/keeper_test.go

a7d62dc

Co-authored-by: Carlos Rodriguez <carlos@interchain.io>

expSequenceStart -> expPruningSequenceStart

d53d659

DimitrisJim marked this pull request as ready for review December 20, 2023 21:52

DimitrisJim requested a review from damiannolan as a code owner December 20, 2023 21:52

make: proto-format

84bb6a2

DimitrisJim commented Dec 20, 2023

View reviewed changes

DimitrisJim mentioned this pull request Dec 20, 2023

Rename MsgPruneAcknowledgements #5473

Closed

AdityaSripal reviewed Dec 20, 2023

View reviewed changes

modules/core/04-channel/keeper/keeper.go Outdated Show resolved Hide resolved

rename per Aditya's suggestion (PruneStalePacketState)

80a0440

DimitrisJim linked an issue Dec 21, 2023 that may be closed by this pull request

Prune stale acknowledgements after successful channel upgrade #3937

Closed

3 tasks

rename MsgPruneAcknowledgements to MsgPruneStalePacketState

84a3b99

DimitrisJim requested review from chatton and srdtrk as code owners December 21, 2023 09:17

crodriguezvega added 3 commits December 21, 2023 10:18

Revert "rename MsgPruneAcknowledgements to MsgPruneStalePacketState"

a63b27d

This reverts commit 84a3b99.

rename MsgPruneAcknowledgements to MsgPruneStalePacketData

089afaa

Merge branch '04-channel-upgrades' into jim/5414-msg-server-keeper-pr…

3a5a2ef

…une-acks

chatton approved these changes Dec 21, 2023

View reviewed changes

pr review

dca600c

damiannolan reviewed Dec 21, 2023

View reviewed changes

proto/ibc/core/channel/v1/tx.proto Outdated Show resolved Hide resolved

proto/ibc/core/channel/v1/tx.proto Outdated Show resolved Hide resolved

AdityaSripal approved these changes Dec 21, 2023

View reviewed changes

modules/core/04-channel/keeper/keeper.go Outdated Show resolved Hide resolved

modules/core/04-channel/keeper/keeper.go Outdated Show resolved Hide resolved

chore: add total prefixes to response args and cleanup keeper funcs

3bdc4d6

damiannolan reviewed Dec 21, 2023

View reviewed changes

modules/core/04-channel/keeper/keeper_test.go Outdated Show resolved Hide resolved

chatton and others added 4 commits December 21, 2023 12:57

chore: fixing linter issues

998e7ba

re-re-name back to PruneAcknowledgements

530c8ad

Merge branch '04-channel-upgrades' into jim/5414-msg-server-keeper-pr…

90eda36

…une-acks

remove calls to Has functions

bcf8535

damiannolan approved these changes Dec 21, 2023

View reviewed changes

colin-axner approved these changes Dec 21, 2023

View reviewed changes

Update modules/core/04-channel/keeper/keeper.go

9b36dc8

Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com>

damiannolan mentioned this pull request Dec 21, 2023

Testing improvements for PruneAcknowledgements rpc #5476

Closed

6 tasks

crodriguezvega merged commit 01c3699 into 04-channel-upgrades Dec 21, 2023
56 checks passed

crodriguezvega deleted the jim/5414-msg-server-keeper-prune-acks branch December 21, 2023 13:39

colin-axner mentioned this pull request Dec 21, 2023

Add message server, keeper implementation for PruneAcknowledgements #5414

Closed

3 tasks

crodriguezvega mentioned this pull request Dec 21, 2023

fix: return bool in GetPruningSequenceStart #5488

Merged

9 tasks

Add pruning logic, implementation in message server. #5444

Add pruning logic, implementation in message server. #5444

Conversation

DimitrisJim commented Dec 19, 2023 • edited Loading

Description

Commit Message / Changelog Entry

crodriguezvega left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DimitrisJim Dec 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damiannolan commented Dec 20, 2023

crodriguezvega left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DimitrisJim left a comment • edited Loading

Choose a reason for hiding this comment

DimitrisJim Dec 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chatton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damiannolan left a comment

Choose a reason for hiding this comment

AdityaSripal left a comment

Choose a reason for hiding this comment

damiannolan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colin-axner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Dec 21, 2023 • edited Loading

Codecov Report

damiannolan commented Dec 21, 2023

DimitrisJim commented Dec 19, 2023 •

edited

Loading

DimitrisJim Dec 19, 2023 •

edited

Loading

DimitrisJim left a comment •

edited

Loading

DimitrisJim Dec 20, 2023 •

edited

Loading

codecov-commenter commented Dec 21, 2023 •

edited

Loading