feat(hard_fork): part2 (Delyayed ack callback) #1355
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
omritoptix
requested changes
Oct 30, 2024
| logger.Info("reverting IBC rollapp packets", "rollappID", rollappID) | ||
|
|
||
| // Get all the pending packets | ||
| rollappPendingPackets := k.ListRollappPackets(ctx, types.PendingByRollappIDFromHeight(rollappID, height)) |
There was a problem hiding this comment.
probably wont fix for now but I think possible DOS if a lot of packets and fraud is e.g few days ago.
There was a problem hiding this comment.
pls open research topic
x/delayedack/keeper/fraud.go
Outdated
| logger := ctx.Logger().With("module", "DelayedAckMiddleware") | ||
| logger.Info("reverting IBC rollapp packets", "rollappID", rollappID) | ||
|
|
||
| // Get all the pending packets |
There was a problem hiding this comment.
Suggested change
| // Get all the pending packets | |
| // Get all the pending packets from fork height inclusive |
x/delayedack/keeper/fraud.go
Outdated
| } else { | ||
| // for incoming packets, we need to reset the packet receipt | ||
| ibcPacket := rollappPacket.Packet | ||
| k.channelKeeper.SetPacketReceipt(ctx, ibcPacket.GetDestPort(), ibcPacket.GetDestChannel(), ibcPacket.GetSequence()) |
There was a problem hiding this comment.
I don't see how this is deleting the packet reciept?
from the ADR
// remove receipt
ibcPacket := rollappPacket.Packet
receiptKey := host.PacketReceiptKey(ibcPacket.GetDestPort(), ibcPacket.GetDestChannel(), ibcPacket.GetSequence())
channelKeeperStore.Delete(receiptKey)| } | ||
|
|
||
| // Get all pending rollapp packets until the latest finalized height | ||
| return k.ListRollappPackets(ctx, types.PendingByRollappIDByMaxHeight(rollappID, latestFinalizedHeight)), latestFinalizedHeight, nil |
There was a problem hiding this comment.
seems like we're filitering by max latest finalized height instead from finalized height forwad.
There was a problem hiding this comment.
this code haven't changed. copied from x/delayedack/keeper/finalize.go
it's not used for fraud, it used by the query GetPendingPacketsByReceiver
I'll rename for clarity
mtsitrin
merged commit b08a106
into
mtsitrin/937-rollapp-hard-fork-hub-side
3 of 5 checks passed
omritoptix
reviewed
Nov 1, 2024
Comment on lines
+29
to
35
| // refund all pending outgoing packets | ||
| if rollappPacket.Type == commontypes.RollappPacket_ON_ACK || rollappPacket.Type == commontypes.RollappPacket_ON_TIMEOUT { | ||
| // refund all pending outgoing packets | ||
| // we don't have access directly to `refundPacketToken` function, so we'll use the `OnTimeoutPacket` function | ||
| err := ibc.OnTimeoutPacket(ctx, *rollappPacket.Packet, rollappPacket.Relayer) | ||
| if err != nil { | ||
| logger.Error("failed to refund reverted packet", append(logContext, "error", err.Error())...) | ||
| } |
There was a problem hiding this comment.
should be changed to follow the ADR:
- timeout - only deleting the rollapp packet
- on_ack - to delete packet and re-create the commitment
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
RevertedstatusHardForkx/delayedackinvariants as it's broken (since lazy finalization)