Add key for storing counterparty next sequence send during WriteUpgradeOpen. #5460
Conversation
DimitrisJim
requested review from
colin-axner,
AdityaSripal and
damiannolan
as code owners
modules/core/24-host/packet_keys.go
Outdated
| KeyPacketAckPrefix = "acks" | ||
| KeyPacketReceiptPrefix = "receipts" | ||
| KeyPruningSequence = "pruningSequence" | ||
| KeyCounterpartyNextSeqSend = "counterpartyNextSequenceSend" |
There was a problem hiding this comment.
I don't particularly like the key name here, looking for better suggestions.
There was a problem hiding this comment.
PruningSequenceEnd?
There was a problem hiding this comment.
I think KeyCounterpartyNextSeqSend sounds good to me.
DimitrisJim
force-pushed
the
jim/set-key-for-max-pruning-limit
branch
from
91b3201
to
658c298
Compare
crodriguezvega
added
channel-upgradability
This comment was marked as outdated.
This comment was marked as outdated.
| @@ -553,6 +553,16 @@ func (k Keeper) WriteUpgradeOpenChannel(ctx sdk.Context, portID, channelID strin | |||
| k.SetNextSequenceAck(ctx, portID, channelID, upgrade.NextSequenceSend) | |||
| } | |||
|
|
|||
| // set the counterparty next sequence send in order to have upper bound to prune to | |||
| k.SetNextSequenceSend(ctx, portID, channelID, counterpartyUpgrade.NextSequenceSend) | |||
There was a problem hiding this comment.
ref aditya's comment in the other pr, the test can be updated to make sure these are set (pruning and next seq send)
There was a problem hiding this comment.
This should be k.SetCounterpartyNextSequenceSend, right?
There was a problem hiding this comment.
🤦♂️ the perils of similarly named things
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## charly/set_nextseqrecv #5460 +/- ##
==========================================================
+ Coverage 80.77% 80.82% +0.05%
==========================================================
Files 197 197
Lines 15133 15159 +26
==========================================================
+ Hits 12223 12253 +30
+ Misses 2443 2438 -5
- Partials 467 468 +1
|
There was a problem hiding this comment.
Nice and clean!
I'm unsure how I feel about the naming of the two different naming conventions for keys and getters/setters. Both sequences are used for pruning but they're naming doesn't lend them to be considered all that related imo!
The way I see it is:
PruningSequence: The current sequence (lower bound) at which all previous acks and receipts have already been deleted.CounterpartyNextSequenceSend: The current sequence (upper bound) for which it is safe to delete all previous acks and receipts.
| // First upgrade for this channel will set the pruning sequence to 1, the starting sequence for pruning. | ||
| // Subsequent upgrades will not modify the pruning sequence thereby allowing pruning to continue from the last | ||
| // pruned sequence. | ||
| if !k.HasPruningSequence(ctx, portID, channelID) { | ||
| k.SetPruningSequence(ctx, portID, channelID, 1) | ||
| } |
There was a problem hiding this comment.
Little bit unclear here as to when and where else the pruning sequence is mutated, exactly? Is it supposed to be in the pruning msg server handler?
its here, right? https://github.com/cosmos/ibc-go/pull/5444/files#diff-b225d8b78960f1ed19ccabcbdcda5ff708075790adac8463fbb0a83a8a63b78aR1145
There was a problem hiding this comment.
Yes! I would totally be up for this being set during first execution of a pruning message instead. The good thing of having it here is that we can see in 10 lines of code the initial bounds being set.
|
Following from the last comment on naming. We could also consider:
Any thoughts? cc. @colin-axner @charleenfei @chatton note: the field on the upgrade itself can remain as |
|
I'm pro the naming change @damiannolan suggests as well, these sequences are only stored after the upgrade completes for the sole purpose of pruning. So i don't think you have to keep the name the same as it is in the upgrade field. Store it under a pruning key that describes its particular purpose in the pruning logic |
There was a problem hiding this comment.
I'm in favour of PruningSequenceStart & PruningSequenceEnd suggested by @damiannolan to make things a little more explicit!
Otherwise everything else LGM
|
I am pro the naming changes as well as suggested by @damiannolan ~thanks! |
|
beautiful suggestion @damiannolan, very much in favor, |
DimitrisJim
force-pushed
the
jim/set-key-for-max-pruning-limit
branch
from
a3d5f27
to
ff2b170
Compare
| @@ -1476,32 +1476,53 @@ func (suite *KeeperTestSuite) TestWriteUpgradeOpenChannel_Ordering() { | |||
| path.EndpointB.ChannelConfig.ProposedUpgrade.Fields.Ordering = types.UNORDERED | |||
| }, | |||
| preUpgrade: func() { | |||
| ctx := suite.chainA.GetContext() | |||
There was a problem hiding this comment.
sorry for additional noise, couldn't help myself.
DimitrisJim
force-pushed
the
jim/set-key-for-max-pruning-limit
branch
from
989e0b1
to
7a14704
Compare
Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com>
Description
to be merged after #5438
This value is then used as the upper bound for pruning packet acks/receipts. Also did a rename of the previous path/key to
PruningSequenceinstead ofAcknowledgementPruningSequenceCommit Message / Changelog Entry
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.
docs/) or specification (x/<module>/spec/).godoccomments.Files changedin the Github PR explorer.Codecov Reportin the comment section below once CI passes.