feat(delayedack): paginate rollapp packets when deleting them #972

zale144 · 2024-07-02T12:22:14Z

Description

When deleting rollapp packets each epoch end, instead of fetching all of them at once, we apply batching now, to prevent unbounded memory usage.
The upgrade handlers are refactored so that they use an interface for the upgrades themselves, keepers are extracted to be in a separate package.

Closes #320

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow-up issues.

PR review checkboxes:

I have...

SDK Checklist

Full security checklist here

----;

For Reviewer:

Confirmed the correct type prefix in the PR title
Reviewers assigned
Confirmed all author checklist items have been addressed

---;

After reviewer approval:

In case the PR targets the main branch, PR should not be squash merge in order to keep meaningful git history.
In case the PR targets a release branch, PR must be rebased.

danwt · 2024-07-04T10:20:44Z

Closes what issue?

danwt

Biggest open question I have is, there are several calls to ListRollappPackets in our code and after this PR not all of them are batched with a limit

So why can't memory problems occur at one of those places?

app/keepers/keepers.go

app/upgrades/types.go

danwt · 2024-07-04T10:30:11Z

app/upgrades/types.go

+// Upgrade defines a struct containing necessary fields that a SoftwareUpgradeProposal
+// must have written, in order for the state migration to go smoothly.
+// An upgrade must implement this struct, and then set it in the app.go.
+// The app.go will then define the handler.


obviously it defines a struct, dont need to say that

app/upgrades/types.go

danwt · 2024-07-04T10:39:19Z

app/upgrades/v4/upgrade_test.go

+		expPass     bool
+	}{
+		{
+			"Test that upgrade does not panic and sets correct parameters",


dont need 'test that'

In general you dont need to repeat contextually obvious info all the time

x/delayedack/keeper/rollapp_packet.go

danwt · 2024-07-04T10:54:10Z

x/delayedack/keeper/rollapp_packet_test.go

@@ -135,6 +135,10 @@ func (suite *DelayedAckTestSuite) TestListRollappPackets() {
 	revertedPackets := keeper.ListRollappPackets(ctx, types.ByStatus(commontypes.Status_REVERTED))
 	suite.Require().Equal(expectRevertedLength, len(revertedPackets))

+	expectRevertedLengthLimit := 4
+	revertedPacketsLimit := keeper.ListRollappPackets(ctx, types.ByStatus(commontypes.Status_REVERTED).Take(4))


4 hardcoded in Take()

danwt · 2024-07-04T10:55:24Z

x/delayedack/types/rollapp_packets_list_filter.go

+func (f RollappPacketListFilter) Take(limit int) RollappPacketListFilter {
+	f.Limit = limit
+	return f
+}


beaut ❤️

zale144 · 2024-07-04T13:52:00Z

Biggest open question I have is, there are several calls to ListRollappPackets in our code and after this PR not all of them are batched with a limit

So why can't memory problems occur at one of those places?

The scope of this PR is to address the potential DoS when deleting the finalized and reverted packets

danwt · 2024-07-05T08:52:00Z

The task is really to fix the DOS, the one in after epoch is just an example. If you think it should be broken down into multiple PRs then create issues to fix the other places. I don't see why adding a limit to after epoch solves the problem when the grpc queries and FinalizeRollappPackets still do everything unlimited?

Also, some of the naming is still sloppy IMO and the keepers/ module is unwarranted

zale144 · 2024-07-05T09:13:33Z

The task is really to fix the DOS, the one in after epoch is just an example. If you think it should be broken down into multiple PRs then create issues to fix the other places. I don't see why adding a limit to after epoch solves the problem when the grpc queries and FinalizeRollappPackets still do everything unlimited?

Also, some of the naming is still sloppy IMO and the keepers/ module is unwarranted

The FinalizeRollappPackets issue is tracked separately and will be addressed in another PR. For this one, it's just preventing out of bonds memory use.

mtsitrin · 2024-07-08T12:14:11Z

x/delayedack/keeper/hooks.go

+
+		for _, packet := range toDeletePackets {
+			// copy 'packet' not needed since Go 1.22
+			_ = osmoutils.ApplyFuncIfNoError(ctx, func(ctx sdk.Context) error {


let's log any case of deletion error

mtsitrin · 2024-07-08T12:26:25Z

x/delayedack/types/params.go

 )

 const (
-	defaultEpochIdentifier = "hour"
+	defaultEpochIdentifier       = "hour"
+	defaultDeletePacketBatchSize = 1000


The default valuie should be much higher IMO
the goal of this value is to be PROTECTION against memory exhasution, not optimization tuning

mtsitrin · 2024-07-08T12:28:32Z

x/delayedack/keeper/hooks.go

+		e.Logger(ctx).Debug("Deleting rollapp packets", "num_packets", len(toDeletePackets))
+
+		for _, packet := range toDeletePackets {
+			// copy 'packet' not needed since Go 1.22


comment is irrelevant

mtsitrin · 2024-07-08T13:23:30Z

@zale144 as discussed

pls split the deletion across multiple iterations
document the assumptions we discussed

@danwt u right regarding the finalization and queries

finalization will be handled in different issue (https://github.com/dymensionxyz/research/issues/327)
grpc queries are expected to be blocked and not publicly facing (@omritoptix )

mtsitrin · 2024-07-08T13:24:51Z

app/app.go

-
-	v3upgrade "github.com/dymensionxyz/dymension/v3/app/upgrades/v3"
-)
+	/* ---------------------------- upgrade handlers ---------------------------- */)


this seperator is misplaced

… constant

Paginate rollapp packets when deleting them

4411241

zale144 self-assigned this Jul 2, 2024

zale144 requested a review from a team as a code owner July 2, 2024 12:22

github-actions bot added the dym-internal label Jul 2, 2024

zale144 added 2 commits July 2, 2024 15:53

Refactor upgrade handlers, add upgrades and changelog entry

12cc9c9

Upgrade version fix

5a5b126

zale144 mentioned this pull request Jul 3, 2024

chore(app): move initialization logic outside of the app and into keepers package #975

Merged

29 tasks

danwt requested changes Jul 4, 2024

View reviewed changes

PR fix

d565784

zale144 force-pushed the zale144/paginate-delete-rollapp-packets branch from afe1b39 to d565784 Compare July 4, 2024 14:14

zale144 requested a review from danwt July 4, 2024 14:14

mtsitrin reviewed Jul 8, 2024

View reviewed changes

Set hard limit on packets deleted per epoch, maintain batch size as a…

ee5bfce

… constant

mtsitrin approved these changes Jul 9, 2024

View reviewed changes

mtsitrin merged commit 1b11625 into main Jul 9, 2024
73 of 89 checks passed

mtsitrin deleted the zale144/paginate-delete-rollapp-packets branch July 9, 2024 11:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(delayedack): paginate rollapp packets when deleting them #972

feat(delayedack): paginate rollapp packets when deleting them #972

zale144 commented Jul 2, 2024 •

edited

Loading

danwt commented Jul 4, 2024

danwt left a comment

danwt Jul 4, 2024

danwt Jul 4, 2024

danwt Jul 4, 2024

danwt Jul 4, 2024

zale144 commented Jul 4, 2024

danwt commented Jul 5, 2024

zale144 commented Jul 5, 2024

mtsitrin Jul 8, 2024

mtsitrin Jul 8, 2024

mtsitrin Jul 8, 2024

mtsitrin commented Jul 8, 2024

mtsitrin Jul 8, 2024

feat(delayedack): paginate rollapp packets when deleting them #972

feat(delayedack): paginate rollapp packets when deleting them #972

Conversation

zale144 commented Jul 2, 2024 • edited Loading

Description

danwt commented Jul 4, 2024

danwt 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

zale144 commented Jul 4, 2024

danwt commented Jul 5, 2024

zale144 commented Jul 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtsitrin commented Jul 8, 2024

Choose a reason for hiding this comment

zale144 commented Jul 2, 2024 •

edited

Loading