-
Notifications
You must be signed in to change notification settings - Fork 161
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.
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
feat: Add Consumer chain initiated slashing #28
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,59 @@ | ||
package keeper | ||
|
||
import ( | ||
sdk "github.com/cosmos/cosmos-sdk/types" | ||
slashingtypes "github.com/cosmos/cosmos-sdk/x/slashing/types" | ||
abci "github.com/tendermint/tendermint/abci/types" | ||
) | ||
|
||
var _ slashingtypes.SlashingHooks = Keeper{} | ||
|
||
// AfterValidatorDowntime gets the given validator address jailing time | ||
// in order either to add it the downtime jailing duration and initiated its slashing | ||
// on the provider chain or perfom a no-op | ||
func (k Keeper) AfterValidatorDowntime(ctx sdk.Context, consAddr sdk.ConsAddress, power int64) { | ||
// get validator signing info | ||
signInfo, _ := k.slashingKeeper.GetValidatorSigningInfo(ctx, consAddr) | ||
|
||
// do nothing if validator is jailed | ||
if ctx.BlockTime().UnixNano() < int64(signInfo.JailedUntil.UnixNano()) { | ||
return | ||
} | ||
|
||
// increase jail time | ||
signInfo.JailedUntil = ctx.BlockHeader().Time.Add(k.slashingKeeper.DowntimeJailDuration(ctx)) | ||
|
||
// reset the missed block counters | ||
signInfo.MissedBlocksCounter = 0 | ||
signInfo.IndexOffset = 0 | ||
k.slashingKeeper.ClearValidatorMissedBlockBitArray(ctx, consAddr) | ||
|
||
// update signing info | ||
k.slashingKeeper.SetValidatorSigningInfo(ctx, consAddr, signInfo) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this design will lead to confusion and bugs. Basically, My suggestion would be to leave only the call to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It’s a matter of opinion I guess. By adding the logic into the slashing module, it would introduce operations specific to consumer chains and not provider chains. I believe we prefer to have this kind of distinction implemented in the CCV module only. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I guess this approach make sense for V1 where we try to keep the changes to the rest of the SDK to the minimum. The alternative would be to have a global flag on the consumer chain indicating that the chain is a consumer chain, and add specific logic for that, but I agree, we don't need it for V1. BTW, in the slashing module we should make sure that this line doesn't actually return a validator (the staking module on the consumer chain may have validators with the same keys as the ones coming from the provider). @jtremback this could be possible, right? |
||
|
||
// send packet to initiate slashing on the provider chain | ||
k.SendPacket( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This send packet API is confusing. See the SendPacket api in parent keeper or transfer keeper. I think this should be something like `SendPacket(ctx, packet) |
||
ctx, | ||
abci.Validator{ | ||
Address: consAddr.Bytes(), | ||
Power: power, | ||
}, | ||
k.slashingKeeper.SlashFractionDowntime(ctx).TruncateInt64(), | ||
signInfo.JailedUntil.UnixNano(), | ||
) | ||
} | ||
|
||
// Hooks wrapper struct for ChildKeeper | ||
type Hooks struct { | ||
k Keeper | ||
} | ||
|
||
// Return the wrapper struct | ||
func (k Keeper) Hooks() Hooks { | ||
return Hooks{k} | ||
} | ||
|
||
// AfterValidatorDowntime implements the slashing hook interface | ||
func (h Hooks) AfterValidatorDowntime(ctx sdk.Context, consAddr sdk.ConsAddress, power int64) { | ||
h.k.AfterValidatorDowntime(ctx, consAddr, power) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -30,13 +30,14 @@ type Keeper struct { | |
portKeeper ccv.PortKeeper | ||
connectionKeeper ccv.ConnectionKeeper | ||
clientKeeper ccv.ClientKeeper | ||
slashingKeeper ccv.SlashingKeeper | ||
} | ||
|
||
// NewKeeper creates a new Child Keeper instance | ||
func NewKeeper( | ||
cdc codec.BinaryCodec, key sdk.StoreKey, paramSpace paramtypes.Subspace, scopedKeeper capabilitykeeper.ScopedKeeper, | ||
channelKeeper ccv.ChannelKeeper, portKeeper ccv.PortKeeper, | ||
connectionKeeper ccv.ConnectionKeeper, clientKeeper ccv.ClientKeeper, | ||
connectionKeeper ccv.ConnectionKeeper, clientKeeper ccv.ClientKeeper, slashingKeeper ccv.SlashingKeeper, | ||
) Keeper { | ||
// set KeyTable if it has not already been set | ||
if !paramSpace.HasKeyTable() { | ||
|
@@ -52,6 +53,7 @@ func NewKeeper( | |
portKeeper: portKeeper, | ||
connectionKeeper: connectionKeeper, | ||
clientKeeper: clientKeeper, | ||
slashingKeeper: slashingKeeper, | ||
} | ||
} | ||
|
||
|
@@ -314,3 +316,16 @@ func (k Keeper) VerifyParentChain(ctx sdk.Context, channelID string) error { | |
|
||
return nil | ||
} | ||
|
||
// GetLastUnbondingPacket returns the last unbounding packet stored in lexical order | ||
func (k Keeper) GetLastUnbondingPacket(ctx sdk.Context) ccv.ValidatorSetChangePacketData { | ||
ubdPacket := &channeltypes.Packet{} | ||
k.IterateUnbondingPacket(ctx, func(seq uint64, packet channeltypes.Packet) bool { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This may be very inefficient. Since the unbonding period is ~4weeks, the number of unbonding packets stored at any given time is ~345600 (given a validator set change packet every block, and a block every 7 seconds). Now that I think about it, I don't think it's a good idea to even store so many packets :) Nonetheless, here I suggest keeping a state w/ the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You should be able to iterate in reverse order and just return on first element. See store iteration methods |
||
*ubdPacket = packet | ||
return false | ||
}) | ||
var data ccv.ValidatorSetChangePacketData | ||
|
||
ccv.ModuleCdc.UnmarshalJSON(ubdPacket.GetData(), &data) | ||
return data | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,24 +3,27 @@ package keeper_test | |
import ( | ||
"fmt" | ||
"testing" | ||
|
||
ibctesting "github.com/cosmos/ibc-go/testing" | ||
"github.com/stretchr/testify/suite" | ||
"time" | ||
|
||
cryptocodec "github.com/cosmos/cosmos-sdk/crypto/codec" | ||
"github.com/cosmos/cosmos-sdk/crypto/keys/ed25519" | ||
sdk "github.com/cosmos/cosmos-sdk/types" | ||
slashingtypes "github.com/cosmos/cosmos-sdk/x/slashing/types" | ||
clienttypes "github.com/cosmos/ibc-go/modules/core/02-client/types" | ||
channeltypes "github.com/cosmos/ibc-go/modules/core/04-channel/types" | ||
commitmenttypes "github.com/cosmos/ibc-go/modules/core/23-commitment/types" | ||
ibctmtypes "github.com/cosmos/ibc-go/modules/light-clients/07-tendermint/types" | ||
ibctesting "github.com/cosmos/ibc-go/testing" | ||
"github.com/cosmos/interchain-security/app" | ||
"github.com/cosmos/interchain-security/testutil/simapp" | ||
"github.com/cosmos/interchain-security/x/ccv/child/types" | ||
childtypes "github.com/cosmos/interchain-security/x/ccv/child/types" | ||
parenttypes "github.com/cosmos/interchain-security/x/ccv/parent/types" | ||
ccv "github.com/cosmos/interchain-security/x/ccv/types" | ||
"github.com/stretchr/testify/suite" | ||
abci "github.com/tendermint/tendermint/abci/types" | ||
// slashing "github.com/cosmos/cosmos-sdk/x/slashing" | ||
// staking "github.com/cosmos/cosmos-sdk/x/staking" | ||
) | ||
|
||
func init() { | ||
|
@@ -361,3 +364,112 @@ func (suite *KeeperTestSuite) TestVerifyParentChain() { | |
func TestKeeperTestSuite(t *testing.T) { | ||
suite.Run(t, new(KeeperTestSuite)) | ||
} | ||
|
||
// TestValidatorDowntime tests that the slashing hooks | ||
// are registred and triggered when a validator has downtime | ||
func (suite *KeeperTestSuite) TestValidatorDowntime() { | ||
// initial setup | ||
app := suite.childChain.App.(*app.App) | ||
ctx := suite.childChain.GetContext() | ||
|
||
// create a validator pubkey and address | ||
pubkey := ed25519.GenPrivKey().PubKey() | ||
consAddr := sdk.ConsAddress(pubkey.Address()) | ||
|
||
// add the validator pubkey and signing info to the store | ||
app.SlashingKeeper.AddPubkey(ctx, pubkey) | ||
|
||
valInfo := slashingtypes.NewValidatorSigningInfo(consAddr, ctx.BlockHeight(), ctx.BlockHeight()-1, | ||
time.Time{}.UTC(), false, int64(0)) | ||
app.SlashingKeeper.SetValidatorSigningInfo(ctx, consAddr, valInfo) | ||
|
||
// Sign 1000 blocks | ||
valPower := int64(1) | ||
height := int64(0) | ||
for ; height < app.SlashingKeeper.SignedBlocksWindow(ctx); height++ { | ||
ctx = ctx.WithBlockHeight(height) | ||
app.SlashingKeeper.HandleValidatorSignature(ctx, pubkey.Address().Bytes(), valPower, true) | ||
} | ||
// Miss 500 blocks | ||
for ; height < app.SlashingKeeper.SignedBlocksWindow(ctx)+(app.SlashingKeeper.SignedBlocksWindow(ctx)-app.SlashingKeeper.MinSignedPerWindow(ctx))+1; height++ { | ||
ctx = ctx.WithBlockHeight(height) | ||
app.SlashingKeeper.HandleValidatorSignature(ctx, pubkey.Address().Bytes(), valPower, false) | ||
} | ||
|
||
signInfo, found := app.SlashingKeeper.GetValidatorSigningInfo(ctx, consAddr) | ||
suite.Require().True(found) | ||
suite.Require().NotZero(signInfo.JailedUntil) | ||
suite.Require().Zero(signInfo.MissedBlocksCounter) | ||
suite.Require().Zero(signInfo.IndexOffset) | ||
app.SlashingKeeper.IterateValidatorMissedBlockBitArray(ctx, consAddr, func(_ int64, missed bool) bool { | ||
suite.Require().False(missed) | ||
return false | ||
Comment on lines
+399
to
+406
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These tests should check that the slashing packet is actually sent |
||
}) | ||
} | ||
|
||
// TestAfterValidatorDowntimeHook tests the slashing hook implementation logic | ||
func (suite *KeeperTestSuite) TestAfterValidatorDowntimeHook() { | ||
app := suite.childChain.App.(*app.App) | ||
ctx := suite.ctx | ||
|
||
consAddr := sdk.ConsAddress(ed25519.GenPrivKey().PubKey().Bytes()).Bytes() | ||
|
||
now := time.Now() | ||
|
||
// Cover the cases when the validatir jailing duration | ||
// is either elapsed, null or still going | ||
testcases := []struct { | ||
jailedUntil time.Time | ||
expUpdate bool | ||
}{{ | ||
jailedUntil: now.Add(-1 * time.Hour), | ||
expUpdate: true, | ||
}, { | ||
jailedUntil: time.Time{}, // null | ||
expUpdate: true, | ||
}, { | ||
jailedUntil: now.Add(1 * time.Hour), | ||
expUpdate: false, | ||
}, | ||
} | ||
|
||
// synchronize the block time with the test cases | ||
ctx = ctx.WithBlockTime(now) | ||
valInfo := slashingtypes.NewValidatorSigningInfo(consAddr, int64(1), int64(1), | ||
time.Time{}.UTC(), false, int64(0)) | ||
|
||
for _, tc := range testcases { | ||
// set the current unjailing time | ||
valInfo.JailedUntil = tc.jailedUntil | ||
app.SlashingKeeper.SetValidatorSigningInfo(ctx, consAddr, valInfo) | ||
// execute the hook logic | ||
app.ChildKeeper.AfterValidatorDowntime( | ||
ctx, consAddr, int64(1)) | ||
// verify if we have the expected output | ||
signInfo, found := app.SlashingKeeper.GetValidatorSigningInfo(ctx, consAddr) | ||
suite.Require().True(found) | ||
suite.Require().True(tc.expUpdate == !(signInfo.JailedUntil.Equal(tc.jailedUntil))) | ||
Comment on lines
+449
to
+451
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ditto |
||
} | ||
|
||
suite.Require() | ||
} | ||
|
||
func (suite *KeeperTestSuite) TestGetLastUnboundingPacket() { | ||
app := suite.childChain.App.(*app.App) | ||
ctx := suite.childChain.GetContext() | ||
|
||
ubdPacket := app.ChildKeeper.GetLastUnbondingPacket(ctx) | ||
suite.Require().Zero(ubdPacket.ValsetUpdateId) | ||
for i := 0; i < 5; i++ { | ||
pd := ccv.NewValidatorSetChangePacketData( | ||
[]abci.ValidatorUpdate{}, | ||
uint64(i), | ||
) | ||
packet := channeltypes.NewPacket(pd.GetBytes(), uint64(i), "", "", "", "", | ||
clienttypes.NewHeight(1, 0), 0) | ||
app.ChildKeeper.SetUnbondingPacket(ctx, uint64(i), packet) | ||
} | ||
|
||
ubdPacket = app.ChildKeeper.GetLastUnbondingPacket(ctx) | ||
suite.Require().Equal(uint64(4), ubdPacket.ValsetUpdateId) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,6 +6,7 @@ import ( | |
|
||
sdk "github.com/cosmos/cosmos-sdk/types" | ||
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" | ||
clienttypes "github.com/cosmos/ibc-go/modules/core/02-client/types" | ||
channeltypes "github.com/cosmos/ibc-go/modules/core/04-channel/types" | ||
host "github.com/cosmos/ibc-go/modules/core/24-host" | ||
"github.com/cosmos/ibc-go/modules/core/exported" | ||
|
@@ -88,3 +89,57 @@ func (k Keeper) UnbondMaturePackets(ctx sdk.Context) error { | |
} | ||
return nil | ||
} | ||
|
||
// SendPacket sends a packet that initiates the given validator | ||
// slashing and jailing on the provider chain. | ||
func (k Keeper) SendPacket(ctx sdk.Context, val abci.Validator, slashFraction, jailedUntil int64) error { | ||
|
||
// check the setup | ||
channelID, ok := k.GetParentChannel(ctx) | ||
if !ok { | ||
return sdkerrors.Wrap(channeltypes.ErrChannelNotFound, "parent channel not set") | ||
} | ||
channel, ok := k.channelKeeper.GetChannel(ctx, types.PortID, channelID) | ||
if !ok { | ||
return sdkerrors.Wrapf(channeltypes.ErrChannelNotFound, "channel not found for channel ID: %s", channelID) | ||
} | ||
channelCap, ok := k.scopedKeeper.GetCapability(ctx, host.ChannelCapabilityPath(types.PortID, channelID)) | ||
if !ok { | ||
return sdkerrors.Wrap(channeltypes.ErrChannelCapabilityNotFound, "module does not own channel capability") | ||
} | ||
|
||
// get the next sequence | ||
sequence, found := k.channelKeeper.GetNextSequenceSend(ctx, types.PortID, channelID) | ||
if !found { | ||
return sdkerrors.Wrapf( | ||
channeltypes.ErrSequenceSendNotFound, | ||
"source port: %s, source channel: %s", types.PortID, channelID, | ||
) | ||
} | ||
|
||
// add the last ValsetUpdateId to the packet data so that the provider | ||
// can find the block height when the downtime happened | ||
valsetUpdateId := k.GetLastUnbondingPacket(ctx).ValsetUpdateId | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here we don't really need the packet, only the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, if this is the only packet sent from provider chain. This should be the same as NextSeqRecv and we don't need the ValsetUpdateId in the packet data. We should discuss when you get back. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
if valsetUpdateId == 0 { | ||
return sdkerrors.Wrapf(ccv.ErrInvalidChildState, "last valset update id not set") | ||
} | ||
packetData := ccv.NewValidatorDowtimePacketData(val, valsetUpdateId, slashFraction, jailedUntil) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Spelling error |
||
packetDataBytes := packetData.GetBytes() | ||
|
||
// send ValidatorDowntime infractions in IBC packet | ||
packet := channeltypes.NewPacket( | ||
packetDataBytes, sequence, | ||
types.PortID, channelID, | ||
channel.Counterparty.PortId, channel.Counterparty.ChannelId, | ||
clienttypes.Height{}, uint64(ccv.GetTimeoutTimestamp(ctx.BlockTime()).UnixNano()), | ||
) | ||
if err := k.channelKeeper.SendPacket(ctx, channelCap, packet); err != nil { | ||
return err | ||
} | ||
|
||
return nil | ||
} | ||
|
||
func (k Keeper) OnAcknowledgementPacket(ctx sdk.Context, packet channeltypes.Packet, ack channeltypes.Acknowledgement) error { | ||
return nil | ||
} |
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -351,10 +351,15 @@ func (am AppModule) OnRecvPacket( | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
// OnAcknowledgementPacket implements the IBCModule interface | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
func (am AppModule) OnAcknowledgementPacket( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
ctx sdk.Context, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
packet channeltypes.Packet, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
_ channeltypes.Packet, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
acknowledgement []byte, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
_ sdk.AccAddress, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
) (*sdk.Result, error) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
var ack channeltypes.Acknowledgement | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if err := ccv.ModuleCdc.UnmarshalJSON(acknowledgement, &ack); err != nil { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
fmt.Println(err) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return nil, sdkerrors.Wrapf(sdkerrors.ErrUnknownRequest, "cannot unmarshal parent packet acknowledgement: %v", err) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return nil, sdkerrors.Wrap(ccv.ErrInvalidChannelFlow, "cannot receive acknowledgement on child port, child chain does not send packet over channel.") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
352
to
364
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Currently, any ACK from the provider chain will results in error. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -1,11 +1,15 @@ | ||||||||||||||||||
package keeper | ||||||||||||||||||
|
||||||||||||||||||
import ( | ||||||||||||||||||
"fmt" | ||||||||||||||||||
"time" | ||||||||||||||||||
|
||||||||||||||||||
sdk "github.com/cosmos/cosmos-sdk/types" | ||||||||||||||||||
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" | ||||||||||||||||||
clienttypes "github.com/cosmos/ibc-go/modules/core/02-client/types" | ||||||||||||||||||
channeltypes "github.com/cosmos/ibc-go/modules/core/04-channel/types" | ||||||||||||||||||
host "github.com/cosmos/ibc-go/modules/core/24-host" | ||||||||||||||||||
"github.com/cosmos/ibc-go/modules/core/exported" | ||||||||||||||||||
"github.com/cosmos/interchain-security/x/ccv/parent/types" | ||||||||||||||||||
ccv "github.com/cosmos/interchain-security/x/ccv/types" | ||||||||||||||||||
abci "github.com/tendermint/tendermint/abci/types" | ||||||||||||||||||
|
@@ -23,6 +27,7 @@ func (k Keeper) SendPacket(ctx sdk.Context, chainID string, valUpdates []abci.Va | |||||||||||||||||
if !ok { | ||||||||||||||||||
return sdkerrors.Wrapf(channeltypes.ErrChannelNotFound, "channel not found for channel ID: %s", channelID) | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
channelCap, ok := k.scopedKeeper.GetCapability(ctx, host.ChannelCapabilityPath(types.PortID, channelID)) | ||||||||||||||||||
if !ok { | ||||||||||||||||||
return sdkerrors.Wrap(channeltypes.ErrChannelCapabilityNotFound, "module does not own channel capability") | ||||||||||||||||||
|
@@ -42,7 +47,7 @@ func (k Keeper) SendPacket(ctx sdk.Context, chainID string, valUpdates []abci.Va | |||||||||||||||||
packetDataBytes, sequence, | ||||||||||||||||||
types.PortID, channelID, | ||||||||||||||||||
channel.Counterparty.PortId, channel.Counterparty.ChannelId, | ||||||||||||||||||
clienttypes.Height{}, uint64(types.GetTimeoutTimestamp(ctx.BlockTime()).UnixNano()), | ||||||||||||||||||
clienttypes.Height{}, uint64(ccv.GetTimeoutTimestamp(ctx.BlockTime()).UnixNano()), | ||||||||||||||||||
) | ||||||||||||||||||
|
||||||||||||||||||
if err := k.channelKeeper.SendPacket(ctx, channelCap, packet); err != nil { | ||||||||||||||||||
|
@@ -113,7 +118,57 @@ func (k Keeper) EndBlockCallback(ctx sdk.Context) { | |||||||||||||||||
if len(valUpdates) != 0 { | ||||||||||||||||||
k.SendPacket(ctx, chainID, valUpdates, valUpdateID) | ||||||||||||||||||
k.IncrementValidatorSetUpdateId(ctx) // TODO: this needs to be moved out of this scope | ||||||||||||||||||
k.SetValsetUpdateBlockHeight(ctx, valUpdateID, uint64(ctx.BlockHeight())) | ||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This line (together with the line above) should be move after this scope (i.e., after the iteration over all consumer chains). Most likely they should also be reverted, i.e., the current block height should be mapped to the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Why? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
All the undelegations happening during this block are mapped with the current |
||||||||||||||||||
} | ||||||||||||||||||
return false | ||||||||||||||||||
}) | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
// OnRecvPacket slahes and jails the given validator in the packet data | ||||||||||||||||||
func (k Keeper) OnRecvPacket(ctx sdk.Context, packet channeltypes.Packet, data ccv.ValidatorDowntimePacketData) exported.Acknowledgement { | ||||||||||||||||||
if childChannel, ok := k.GetChannelToChain(ctx, packet.DestinationChannel); !ok && childChannel != packet.DestinationChannel { | ||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is pretty confusing to read, maybe break it onto several lines with named variable There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @AdityaSripal we are having a hard time understanding this boilerplate. This seems incorrect. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually maybe the issue is that @sainoe accidently ihnverted the ok ( There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||||||||||||||||||
ack := channeltypes.NewErrorAcknowledgement( | ||||||||||||||||||
fmt.Sprintf("packet sent on a channel %s other than the established child channel %s", packet.DestinationChannel, childChannel), | ||||||||||||||||||
) | ||||||||||||||||||
chanCap, _ := k.scopedKeeper.GetCapability(ctx, host.ChannelCapabilityPath(packet.DestinationPort, packet.DestinationChannel)) | ||||||||||||||||||
k.channelKeeper.ChanCloseInit(ctx, packet.DestinationPort, packet.DestinationChannel, chanCap) | ||||||||||||||||||
return &ack | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
// initiate slashing and jailing | ||||||||||||||||||
if err := k.HandleConsumerDowntime(ctx, data); err != nil { | ||||||||||||||||||
ack := channeltypes.NewErrorAcknowledgement(err.Error()) | ||||||||||||||||||
return &ack | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
return nil | ||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Here we should return an ACK. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it mandatory to send an ack? It seems like it can't be because we don't send an ack for 2 weeks in the core protocol, and around 345k packets could be received without an ack coming back. Sending it just means that we have more boilerplate on the other side. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From ICS 4:
This is why the consumer can receive around 345k packets w/o ACKs. However, no ACKs can be skipped (see below).
I guess the meaning here is a uni-directional channel, right? I think the packets sent by the consumer do not need to be ACKed.
However, to enable cleanup, we may need to send ACKs for all packets. |
||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
// HandleConsumerDowntime gets the validator and the downtime infraction height from the packet data | ||||||||||||||||||
// validator address and valset upate ID. Then it executes the slashing the and jailing accordingly. | ||||||||||||||||||
func (k Keeper) HandleConsumerDowntime(ctx sdk.Context, downtimeData ccv.ValidatorDowntimePacketData) error { | ||||||||||||||||||
// get the validator consensus address | ||||||||||||||||||
consAddr := sdk.ConsAddress(downtimeData.Validator.Address) | ||||||||||||||||||
mpoke marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||
|
||||||||||||||||||
// get the validator data | ||||||||||||||||||
val, found := k.stakingKeeper.GetValidatorByConsAddr(ctx, consAddr) | ||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are we sure that a validator from the consumer chain has the same consensus address as a validator from the provider chain? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The Key Delegation functionality is WIP. |
||||||||||||||||||
if !found { | ||||||||||||||||||
return fmt.Errorf("cannot find validator with address %s", consAddr.String()) | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @sainoe Patch this suggestion back into main |
||||||||||||||||||
// get the downtime block height from the valsetUpdateID | ||||||||||||||||||
blockHeight := k.GetValsetUpdateBlockHeight(ctx, downtimeData.ValsetUpdateId) | ||||||||||||||||||
if blockHeight == 0 { | ||||||||||||||||||
return fmt.Errorf("cannot find validator update id %d", downtimeData.ValsetUpdateId) | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
// slash and jail the validator | ||||||||||||||||||
k.stakingKeeper.Slash(ctx, consAddr, int64(blockHeight), downtimeData.Validator.Power, sdk.NewDec(1).QuoInt64(downtimeData.SlashFraction)) | ||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we want to allow the consumer chain to set the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes we do. It is assumed that the provider and consumer chains should agreed on these parameters off-chain during the proposal. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can the consumer change these values afterwards w/o asking the provider? That's something that we should avoid. This is related to one of the open issues described here. |
||||||||||||||||||
if !val.Jailed { | ||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||||
k.stakingKeeper.Jail(ctx, consAddr) | ||||||||||||||||||
} | ||||||||||||||||||
// TODO: check if the missed block bits and sign info need to be reseted | ||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We shouldn't reset if we want a validator to be also slashed for downtime on the provider, which I think we want. A validator can already be slashed multiple times for downtime on different consumer chains. |
||||||||||||||||||
k.slashingKeeper.JailUntil(ctx, consAddr, ctx.BlockHeader().Time.Add(time.Duration(downtimeData.JailTime))) | ||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as with the |
||||||||||||||||||
|
||||||||||||||||||
return nil | ||||||||||||||||||
} |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -21,6 +21,7 @@ import ( | |||||
channeltypes "github.com/cosmos/ibc-go/modules/core/04-channel/types" | ||||||
porttypes "github.com/cosmos/ibc-go/modules/core/05-port/types" | ||||||
host "github.com/cosmos/ibc-go/modules/core/24-host" | ||||||
"github.com/cosmos/ibc-go/modules/core/exported" | ||||||
ibcexported "github.com/cosmos/ibc-go/modules/core/exported" | ||||||
"github.com/cosmos/interchain-security/x/ccv/parent/keeper" | ||||||
"github.com/cosmos/interchain-security/x/ccv/parent/types" | ||||||
|
@@ -320,7 +321,28 @@ func (am AppModule) OnRecvPacket( | |||||
packet channeltypes.Packet, | ||||||
_ sdk.AccAddress, | ||||||
) ibcexported.Acknowledgement { | ||||||
return channeltypes.NewErrorAcknowledgement("cannot receive packet on parent chain") | ||||||
var ( | ||||||
ack exported.Acknowledgement | ||||||
data ccv.ValidatorDowntimePacketData | ||||||
) | ||||||
if err := ccv.ModuleCdc.UnmarshalJSON(packet.GetData(), &data); err != nil { | ||||||
errAck := channeltypes.NewErrorAcknowledgement(fmt.Sprintf("cannot unmarshal CCV packet data: %s", err.Error())) | ||||||
ack = &errAck | ||||||
} else { | ||||||
ack = am.keeper.OnRecvPacket(ctx, packet, data) | ||||||
} | ||||||
|
||||||
ctx.EventManager().EmitEvent( | ||||||
sdk.NewEvent( | ||||||
ccv.EventTypePacket, | ||||||
sdk.NewAttribute(sdk.AttributeKeyModule, types.ModuleName), | ||||||
sdk.NewAttribute(ccv.AttributeKeyAckSuccess, fmt.Sprintf("%t", ack != nil)), | ||||||
), | ||||||
) | ||||||
|
||||||
// NOTE: In successful case, acknowledgement will be written asynchronously | ||||||
// after unbonding period has elapsed. | ||||||
Comment on lines
+343
to
+344
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
This is not true. It's probably from the consumer chain. :) |
||||||
return ack | ||||||
} | ||||||
|
||||||
// OnAcknowledgementPacket implements the IBCModule interface | ||||||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -49,6 +49,9 @@ const ( | |||||
// ChildChainToUBDEsPrefix is for the index for looking up which unbonding delegation entries are waiting for a given | ||||||
// child chain to unbond | ||||||
UBDEIndexPrefix = "childchaintoubdes" | ||||||
|
||||||
//ValsetUpdateBlockHeightPrefix | ||||||
ValsetUpdateBlockHeightPrefix = "valsetupdateblockheigt" | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @sainoe patch this on main |
||||||
) | ||||||
|
||||||
var ( | ||||||
|
@@ -110,3 +113,9 @@ func UnbondingDelegationEntryKey(unbondingDelegationEntryID uint64) []byte { | |||||
|
||||||
return append([]byte(UnbondingDelegationEntryPrefix), bz...) | ||||||
} | ||||||
|
||||||
func ValsetUpdateBlockHeightKey(valsetUpdateId uint64) []byte { | ||||||
vuidBytes := make([]byte, 8) | ||||||
binary.BigEndian.PutUint64(vuidBytes, valsetUpdateId) | ||||||
return append([]byte(ValsetUpdateBlockHeightPrefix), vuidBytes...) | ||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -24,3 +24,37 @@ func (vsc ValidatorSetChangePacketData) GetBytes() []byte { | |
valUpdateBytes := ModuleCdc.MustMarshalJSON(&vsc) | ||
return valUpdateBytes | ||
} | ||
|
||
func NewValidatorDowtimePacketData(validator abci.Validator, valUpdateId uint64, slashFraction, jailTime int64) ValidatorDowntimePacketData { | ||
return ValidatorDowntimePacketData{ | ||
Validator: validator, | ||
SlashFraction: slashFraction, | ||
JailTime: jailTime, | ||
ValsetUpdateId: valUpdateId, | ||
} | ||
} | ||
|
||
func (vdt ValidatorDowntimePacketData) ValidateBasic() error { | ||
if len(vdt.Validator.Address) == 0 || vdt.Validator.Power == 0 { | ||
return sdkerrors.Wrap(ErrInvalidPacketData, "validator fields cannot be empty") | ||
} | ||
|
||
if vdt.JailTime <= 0 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should allow slash with 0 jail time right? |
||
return sdkerrors.Wrap(ErrInvalidPacketData, "jail duration must be positive") | ||
} | ||
|
||
if vdt.SlashFraction <= 0 { | ||
return sdkerrors.Wrap(ErrInvalidPacketData, "slash fraction must be positive") | ||
} | ||
|
||
if vdt.ValsetUpdateId == 0 { | ||
return sdkerrors.Wrap(ErrInvalidPacketData, "valset update id cannot be equal to zero") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @sainoe think about the 0/nil issue more and let me know if valsetupdate IDs need to start at 1 globally |
||
} | ||
|
||
return nil | ||
} | ||
|
||
func (vdt ValidatorDowntimePacketData) GetBytes() []byte { | ||
valDowntimeBytes := ModuleCdc.MustMarshalJSON(&vdt) | ||
return valDowntimeBytes | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you creating separate packet data type for each type of evidence?
My understanding of v1, was that we just send back a generic evidence packet with validator and slash fraction
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that Simon is generalizing this in his current work