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.

Sign up for GitHub

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

imp: remove LatestSequenceSend #5108

Merged
merged 7 commits into from
Nov 14, 2023
Merged

imp: remove LatestSequenceSend #5108

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ea49842
del: latestsequencesend field
Nov 13, 2023
ef965f1
fix: comment
sangier Nov 13, 2023
3c58665
gofumpt
Nov 14, 2023
c16932e
del: tests
sangier Nov 14, 2023
c23e09a
del leftover tests part
sangier Nov 14, 2023
386e07f
adressing comments
sangier Nov 14, 2023
452c4fc
addressing comment
sangier Nov 14, 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
1 change: 0 additions & 1 deletion e2e/tests/core/02-client/client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,6 @@ func (s *ClientTestSuite) TestScheduleIBCUpgrade_Succeeds() {
})

t.Run("ensure legacy proposal does not succeed", func(t *testing.T) {

authority, err := s.QueryModuleAccountAddress(ctx, govtypes.ModuleName, chainA)
s.Require().NoError(err)
s.Require().NotNil(authority)
Expand Down
18 changes: 9 additions & 9 deletions e2e/testsuite/grpc_query.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,15 +85,15 @@ func (s *E2ETestSuite) InitGRPCClients(chain *cosmos.CosmosChain) {
FeeQueryClient: feetypes.NewQueryClient(grpcConn),
ICAControllerQueryClient: controllertypes.NewQueryClient(grpcConn),
ICAHostQueryClient: hosttypes.NewQueryClient(grpcConn),
BankQueryClient: banktypes.NewQueryClient(grpcConn),
GovQueryClient: govtypesv1beta1.NewQueryClient(grpcConn),
GovQueryClientV1: govtypesv1.NewQueryClient(grpcConn),
GroupsQueryClient: grouptypes.NewQueryClient(grpcConn),
ParamsQueryClient: paramsproposaltypes.NewQueryClient(grpcConn),
AuthQueryClient: authtypes.NewQueryClient(grpcConn),
AuthZQueryClient: authz.NewQueryClient(grpcConn),
ConsensusServiceClient: cmtservice.NewServiceClient(grpcConn),
UpgradeQueryClient: upgradetypes.NewQueryClient(grpcConn),
BankQueryClient: banktypes.NewQueryClient(grpcConn),
GovQueryClient: govtypesv1beta1.NewQueryClient(grpcConn),
GovQueryClientV1: govtypesv1.NewQueryClient(grpcConn),
GroupsQueryClient: grouptypes.NewQueryClient(grpcConn),
ParamsQueryClient: paramsproposaltypes.NewQueryClient(grpcConn),
AuthQueryClient: authtypes.NewQueryClient(grpcConn),
AuthZQueryClient: authz.NewQueryClient(grpcConn),
ConsensusServiceClient: cmtservice.NewServiceClient(grpcConn),
UpgradeQueryClient: upgradetypes.NewQueryClient(grpcConn),
Comment on lines +88 to +96
Copy link
Contributor

Choose a reason for hiding this comment

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

seems like something weird is going on with linting 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

I pushed this change after I run gofumpt in the branch.

}
}

Expand Down
18 changes: 0 additions & 18 deletions modules/core/04-channel/keeper/packet.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,24 +132,6 @@ func (k Keeper) RecvPacket(
return errorsmod.Wrapf(types.ErrInvalidChannelState, "expected channel state to be one of [%s, %s], but got %s", types.OPEN, types.FLUSHING, channel.State)
}

// in the case of the channel being in FLUSHING we need to ensure that the the counterparty last sequence send
// is less than or equal to the packet sequence.
if channel.State == types.FLUSHING {
counterpartyUpgrade, found := k.GetCounterpartyUpgrade(ctx, packet.GetDestPort(), packet.GetDestChannel())
if !found {
return errorsmod.Wrapf(types.ErrUpgradeNotFound, "counterparty upgrade not found for channel: %s", packet.GetDestChannel())
}

// only error if the counterparty latest sequence send is set (> 0)
counterpartyLatestSequenceSend := counterpartyUpgrade.LatestSequenceSend
if counterpartyLatestSequenceSend != 0 && packet.GetSequence() > counterpartyLatestSequenceSend {
return errorsmod.Wrapf(
types.ErrInvalidPacket,
"failed to receive packet, cannot flush packet at sequence greater than counterparty last sequence send (%d) > (%d)", packet.GetSequence(), counterpartyLatestSequenceSend,
)
}
}

// Authenticate capability to ensure caller has authority to receive packet on this channel
capName := host.ChannelCapabilityPath(packet.GetDestPort(), packet.GetDestChannel())
if !k.scopedKeeper.AuthenticateCapability(ctx, chanCap, capName) {
Expand Down
76 changes: 0 additions & 76 deletions modules/core/04-channel/keeper/packet_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -335,82 +335,6 @@ func (suite *KeeperTestSuite) TestRecvPacket() {
},
nil,
},
{
"success with channel in flushing state",
func() {
suite.coordinator.Setup(path)
sequence, err := path.EndpointA.SendPacket(defaultTimeoutHeight, disabledTimeoutTimestamp, ibctesting.MockPacketData)
suite.Require().NoError(err)
packet = types.NewPacket(ibctesting.MockPacketData, sequence, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, defaultTimeoutHeight, disabledTimeoutTimestamp)
channelCap = suite.chainB.GetChannelCapability(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID)

channel := path.EndpointB.GetChannel()
channel.State = types.FLUSHING
path.EndpointB.SetChannel(channel)

// set last packet sent sequence to sequence + 1
counterpartyUpgrade := types.Upgrade{LatestSequenceSend: sequence + 1}
path.EndpointB.SetChannelCounterpartyUpgrade(counterpartyUpgrade)
Comment on lines -351 to -353
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can keep this tests case, but just remove this part.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Re-added the test removing the highlighted lines!

},
nil,
},
{
"success with an counterparty latest sequence send set to 0",
func() {
suite.coordinator.Setup(path)
sequence, err := path.EndpointA.SendPacket(defaultTimeoutHeight, disabledTimeoutTimestamp, ibctesting.MockPacketData)
suite.Require().NoError(err)

packet = types.NewPacket(ibctesting.MockPacketData, sequence, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, defaultTimeoutHeight, disabledTimeoutTimestamp)
channelCap = suite.chainB.GetChannelCapability(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID)

channel := path.EndpointB.GetChannel()
channel.State = types.FLUSHING
path.EndpointB.SetChannel(channel)

// set last packet sent sequence to zero.
counterpartyUpgrade := types.Upgrade{LatestSequenceSend: 0}
path.EndpointB.SetChannelCounterpartyUpgrade(counterpartyUpgrade)
},
nil,
},
{
"failure while upgrading channel, counterparty upgrade not found",
func() {
suite.coordinator.Setup(path)
sequence, err := path.EndpointA.SendPacket(defaultTimeoutHeight, disabledTimeoutTimestamp, ibctesting.MockPacketData)
suite.Require().NoError(err)
packet = types.NewPacket(ibctesting.MockPacketData, sequence, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, defaultTimeoutHeight, disabledTimeoutTimestamp)
channelCap = suite.chainB.GetChannelCapability(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID)

channel := path.EndpointB.GetChannel()
channel.State = types.FLUSHING
path.EndpointB.SetChannel(channel)
},
types.ErrUpgradeNotFound,
},
{
"failure while upgrading channel, packet sequence > counterparty last send sequence",
func() {
suite.coordinator.Setup(path)
// send 2 packets so that when LatestSequenceSend is set to sequence - 1, it is not 0.
_, err := path.EndpointA.SendPacket(defaultTimeoutHeight, disabledTimeoutTimestamp, ibctesting.MockPacketData)
suite.Require().NoError(err)
sequence, err := path.EndpointA.SendPacket(defaultTimeoutHeight, disabledTimeoutTimestamp, ibctesting.MockPacketData)
suite.Require().NoError(err)
packet = types.NewPacket(ibctesting.MockPacketData, sequence, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, defaultTimeoutHeight, disabledTimeoutTimestamp)
channelCap = suite.chainB.GetChannelCapability(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID)

channel := path.EndpointB.GetChannel()
channel.State = types.FLUSHING
path.EndpointB.SetChannel(channel)

// set last packet sent sequence to sequence - 1
counterpartyUpgrade := types.Upgrade{LatestSequenceSend: sequence - 1}
path.EndpointB.SetChannelCounterpartyUpgrade(counterpartyUpgrade)
},
types.ErrInvalidPacket,
},
{
"failure while upgrading channel, channel in flush complete state",
func() {
Expand Down
6 changes: 0 additions & 6 deletions modules/core/04-channel/keeper/upgrade.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -763,12 +763,6 @@ func (k Keeper) startFlushing(ctx sdk.Context, portID, channelID string, upgrade
channel.State = types.FLUSHING
k.SetChannel(ctx, portID, channelID, channel)

nextSequenceSend, found := k.GetNextSequenceSend(ctx, portID, channelID)
if !found {
return errorsmod.Wrapf(types.ErrSequenceSendNotFound, "port ID (%s) channel ID (%s)", portID, channelID)
}

upgrade.LatestSequenceSend = nextSequenceSend - 1
upgrade.Timeout = k.getAbsoluteUpgradeTimeout(ctx)
k.SetUpgrade(ctx, portID, channelID, *upgrade)

Expand Down
17 changes: 0 additions & 17 deletions modules/core/04-channel/keeper/upgrade_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,10 +301,6 @@ func (suite *KeeperTestSuite) TestChanUpgradeTry() {
suite.Require().NoError(err)
suite.Require().NotEmpty(upgrade)
suite.Require().Equal(proposedUpgrade.Fields, upgrade.Fields)

nextSequenceSend, found := path.EndpointB.Chain.GetSimApp().IBCKeeper.ChannelKeeper.GetNextSequenceSend(path.EndpointB.Chain.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID)
suite.Require().True(found)
suite.Require().Equal(nextSequenceSend-1, upgrade.LatestSequenceSend)
} else {
suite.assertUpgradeError(err, tc.expError)
}
Expand Down Expand Up @@ -489,14 +485,6 @@ func (suite *KeeperTestSuite) TestChanUpgradeAck() {
},
commitmenttypes.ErrInvalidProof,
},
{
"fails due to proof verification failure, counterparty update has unexpected sequence",
func() {
// Decrementing LatestSequenceSend is sufficient to cause the proof to fail.
counterpartyUpgrade.LatestSequenceSend--
},
commitmenttypes.ErrInvalidProof,
},
{
"fails due to mismatch in upgrade ordering",
func() {
Expand Down Expand Up @@ -1757,12 +1745,7 @@ func (suite *KeeperTestSuite) TestStartFlush() {
suite.assertUpgradeError(err, tc.expError)
} else {
channel := path.EndpointB.GetChannel()

nextSequenceSend, ok := suite.chainB.GetSimApp().IBCKeeper.ChannelKeeper.GetNextSequenceSend(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID)
suite.Require().True(ok)

suite.Require().Equal(types.FLUSHING, channel.State)
suite.Require().Equal(nextSequenceSend-1, upgrade.LatestSequenceSend)

expectedTimeoutTimestamp := types.DefaultTimeout.Timestamp + uint64(suite.chainB.GetContext().BlockTime().UnixNano())
suite.Require().Equal(expectedTimeoutTimestamp, upgrade.Timeout.Timestamp)
Expand Down
5 changes: 2 additions & 3 deletions modules/core/04-channel/types/upgrade.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,8 @@ import (
// NewUpgrade creates a new Upgrade instance.
func NewUpgrade(upgradeFields UpgradeFields, timeout Timeout, latestPacketSent uint64) Upgrade {
return Upgrade{
Fields: upgradeFields,
Timeout: timeout,
LatestSequenceSend: latestPacketSent,
Fields: upgradeFields,
Timeout: timeout,
}
}

Expand Down
87 changes: 28 additions & 59 deletions modules/core/04-channel/types/upgrade.pb.go
View file
Open in desktop

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

8 changes: 3 additions & 5 deletions proto/ibc/core/channel/v1/upgrade.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,12 @@ import "ibc/core/channel/v1/channel.proto";

// Upgrade is a verifiable type which contains the relevant information
// for an attempted upgrade. It provides the proposed changes to the channel
// end, the timeout for this upgrade attempt and the latest packet sequence sent
// to allow the counterparty to block sends after the upgrade has started.
// end and the timeout for this upgrade attempt.
message Upgrade {
option (gogoproto.goproto_getters) = false;

UpgradeFields fields = 1 [(gogoproto.nullable) = false];
Timeout timeout = 2 [(gogoproto.nullable) = false];
uint64 latest_sequence_send = 3;
UpgradeFields fields = 1 [(gogoproto.nullable) = false];
Timeout timeout = 2 [(gogoproto.nullable) = false];
}

// UpgradeFields are the fields in a channel end which may be changed
Expand Down
3 changes: 1 addition & 2 deletions testing/endpoint.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -873,8 +873,7 @@ func (endpoint *Endpoint) GetProposedUpgrade() channeltypes.Upgrade {
ConnectionHops: []string{endpoint.ConnectionID},
Version: endpoint.ChannelConfig.Version,
},
Timeout: channeltypes.NewTimeout(endpoint.Counterparty.Chain.GetTimeoutHeight(), 0),
LatestSequenceSend: 0,
Timeout: channeltypes.NewTimeout(endpoint.Counterparty.Chain.GetTimeoutHeight(), 0),
}

override := endpoint.ChannelConfig.ProposedUpgrade
Expand Down
Loading