dymensionxyz / dymension

Dymension Hub
https://dymension.xyz
Other
363 stars 327 forks source link

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

Open zale144 opened 5 days ago

zale144 commented 5 days ago

Description


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:

---;

After reviewer approval:

danwt commented 3 days ago

Closes what issue?

zale144 commented 3 days ago

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 commented 2 days ago

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 commented 2 days ago

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.