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

Add pruning logic, implementation in message server. #5444

Merged
Merged
Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
036b0b0
Impl msg server, keeper logic.
DimitrisJim Dec 19, 2023
07df54f
Update naming used for start/end as per updates for keys.
DimitrisJim Dec 20, 2023
8787fdf
Self contained implementation logic in keeper.
DimitrisJim Dec 20, 2023
6b5ada0
Lint, test structure, error case checking.
DimitrisJim Dec 20, 2023
0c7d8e4
Test test teeeeest.
DimitrisJim Dec 20, 2023
cafb419
Rename keeper function, document a couple of additional things.
DimitrisJim Dec 20, 2023
e78cfd4
Test case: send packets after upgrade, no pruning.
DimitrisJim Dec 20, 2023
83ed801
Return number pruned, number left in response.
DimitrisJim Dec 20, 2023
a7d62dc
Update modules/core/04-channel/keeper/keeper_test.go
DimitrisJim Dec 20, 2023
d53d659
expSequenceStart -> expPruningSequenceStart
DimitrisJim Dec 20, 2023
84bb6a2
make: proto-format
DimitrisJim Dec 20, 2023
80a0440
rename per Aditya's suggestion (PruneStalePacketState)
DimitrisJim Dec 21, 2023
84a3b99
rename MsgPruneAcknowledgements to MsgPruneStalePacketState
crodriguezvega Dec 21, 2023
a63b27d
Revert "rename MsgPruneAcknowledgements to MsgPruneStalePacketState"
crodriguezvega Dec 21, 2023
089afaa
rename MsgPruneAcknowledgements to MsgPruneStalePacketData
crodriguezvega Dec 21, 2023
3a5a2ef
Merge branch '04-channel-upgrades' into jim/5414-msg-server-keeper-pr…
crodriguezvega Dec 21, 2023
dca600c
pr review
charleenfei Dec 21, 2023
3bdc4d6
chore: add total prefixes to response args and cleanup keeper funcs
damiannolan Dec 21, 2023
998e7ba
chore: fixing linter issues
chatton Dec 21, 2023
530c8ad
re-re-name back to PruneAcknowledgements
crodriguezvega Dec 21, 2023
90eda36
Merge branch '04-channel-upgrades' into jim/5414-msg-server-keeper-pr…
crodriguezvega Dec 21, 2023
bcf8535
remove calls to Has functions
crodriguezvega Dec 21, 2023
9b36dc8
Update modules/core/04-channel/keeper/keeper.go
crodriguezvega Dec 21, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions modules/apps/transfer/types/authz.pb.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

64 changes: 60 additions & 4 deletions modules/core/04-channel/keeper/keeper.go
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,18 @@ func (k Keeper) SetPacketReceipt(ctx sdk.Context, portID, channelID string, sequ
store.Set(host.PacketReceiptKey(portID, channelID, sequence), []byte{byte(1)})
}

// HasPacketReceipt returns true if the packet receipt exists
func (k Keeper) HasPacketReceipt(ctx sdk.Context, portID, channelID string, sequence uint64) bool {
store := ctx.KVStore(k.storeKey)
return store.Has(host.PacketReceiptKey(portID, channelID, sequence))
}

// deletePacketReceipt deletes a packet receipt from the store
func (k Keeper) deletePacketReceipt(ctx sdk.Context, portID, channelID string, sequence uint64) {
store := ctx.KVStore(k.storeKey)
store.Delete(host.PacketReceiptKey(portID, channelID, sequence))
}

// GetPacketCommitment gets the packet commitment hash from the store
func (k Keeper) GetPacketCommitment(ctx sdk.Context, portID, channelID string, sequence uint64) []byte {
store := ctx.KVStore(k.storeKey)
Expand Down Expand Up @@ -240,6 +252,12 @@ func (k Keeper) HasPacketAcknowledgement(ctx sdk.Context, portID, channelID stri
return store.Has(host.PacketAcknowledgementKey(portID, channelID, sequence))
}

// deletePacketAcknowledgement deletes the packet ack hash from the store
func (k Keeper) deletePacketAcknowledgement(ctx sdk.Context, portID, channelID string, sequence uint64) {
store := ctx.KVStore(k.storeKey)
store.Delete(host.PacketAcknowledgementKey(portID, channelID, sequence))
}

// IteratePacketSequence provides an iterator over all send, receive or ack sequences.
// For each sequence, cb will be called. If the cb returns true, the iterator
// will close and stop.
Expand Down Expand Up @@ -653,8 +671,46 @@ func (k Keeper) HasPruningSequenceStart(ctx sdk.Context, portID, channelID strin
return store.Has(host.PruningSequenceStartKey(portID, channelID))
}

// PruneAcknowledgements prunes packet acknowledgements from the store that have a sequence number less than or equal to the pruning sequence.
// The number of packet acknowledgements pruned is equal to limit. Pruning only occurs after a channel has been upgraded.
func (Keeper) PruneAcknowledgements(ctx sdk.Context, portID, channelID string, limit, pruningSequence uint64) error {
return nil
// PruneStalePacketState prunes packet acknowledgements and receipts that have a sequence number less than pruning sequence end.
// The number of packet acks/receipts pruned is equal to limit. Pruning can only occur after a channel has been upgraded.
crodriguezvega marked this conversation as resolved.
Show resolved Hide resolved
//
// Pruning sequence start keeps track of the packet ack/receipt that can be pruned next. When it reaches pruningSequenceEnd,
// pruning is complete.
func (k Keeper) PruneStalePacketState(ctx sdk.Context, portID, channelID string, limit uint64) (uint64, uint64, error) {
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)
Comment on lines +674 to +678
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

pruningSequenceEnd, found := k.GetPruningSequenceEnd(ctx, portID, channelID)
DimitrisJim marked this conversation as resolved.
Show resolved Hide resolved
if !found {
return 0, 0, errorsmod.Wrapf(types.ErrPruningSequenceEndNotFound, "port ID (%s) channel ID (%s)", portID, channelID)
}

start := pruningSequenceStart
end := pruningSequenceStart + limit
for ; start < end; start++ {
// stop pruning if pruningSequenceStart has reached pruningSequenceEnd, pruningSequenceEnd is
// set to be equal to the _next_ sequence to be sent by the counterparty.
if start >= pruningSequenceEnd {
break
}

if k.HasPacketAcknowledgement(ctx, portID, channelID, start) {
k.deletePacketAcknowledgement(ctx, portID, channelID, start)
damiannolan marked this conversation as resolved.
Show resolved Hide resolved
}

// NOTE: packet receipts are only relevant for unordered channels.
if k.HasPacketReceipt(ctx, portID, channelID, start) {
k.deletePacketReceipt(ctx, portID, channelID, start)
damiannolan marked this conversation as resolved.
Show resolved Hide resolved
}
}

// set pruning sequence start to the updated value
k.SetPruningSequenceStart(ctx, portID, channelID, start)

totalPruned := start - pruningSequenceStart
totalRemaining := pruningSequenceEnd - start

return totalPruned, totalRemaining, nil
}
Loading
Loading