ICS3: Remove ability for multiple TRY msgs to succeed for single INIT msg #1723
Conversation
AdityaSripal
requested review from
crodriguezvega,
seantking,
charleenfei,
damiannolan and
chatton
as code owners
| @@ -242,6 +247,10 @@ func (k Keeper) ConnOpenAck( | |||
| connection.Counterparty.ConnectionId = counterpartyConnectionID | |||
| k.SetConnection(ctx, connectionID, connection) | |||
|
|
|||
| // delete generatedConnectionID mapping now that handshake attempt is successful | |||
| // we no longer need to store it for redundancy protection | |||
| k.DeleteGeneratedConnectionID(ctx, connection.ClientId, counterpartyConnectionID) | |||
There was a problem hiding this comment.
Not sure if I should be deleting in ACK and CONFIRM. No longer strictly necessary for the state machine but may be useful as a query even after handshake is complete
There was a problem hiding this comment.
Shouldn't we only need to delete in the ConnOpenConfirm handler?
There was a problem hiding this comment.
Well, theoretically we could have a crossing hello; where ACK gets called on both ends
There was a problem hiding this comment.
In this case does CONFIRM not get called on both ends too?
There was a problem hiding this comment.
nope because both sides go to OPEN on ACK, there's no need for CONFIRM
AdityaSripal
changed the title
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #1723 +/- ##
==========================================
+ Coverage 78.54% 78.57% +0.02%
==========================================
Files 181 181
Lines 12580 12604 +24
==========================================
+ Hits 9881 9903 +22
- Misses 2262 2264 +2
Partials 437 437
|
| func (k Keeper) SetExistingConnectionID(ctx sdk.Context, clientID, counterpartyConnectionID, connectionID string) { | ||
| store := ctx.KVStore(k.storeKey) | ||
| store.Set([]byte(types.ExistingConnectionIDKey(clientID, counterpartyConnectionID)), []byte(connectionID)) | ||
| } | ||
|
|
||
| // GetExistingConnectionID returns the generated connectionID for a given clientID and counterparty connectionID | ||
| func (k Keeper) GetExistingConnectionID(ctx sdk.Context, clientID, counterpartyConnectionID string) string { | ||
| store := ctx.KVStore(k.storeKey) | ||
| return string(store.Get([]byte(types.ExistingConnectionIDKey(clientID, counterpartyConnectionID)))) | ||
| } | ||
|
|
||
| // DeleteExistingConnectionID removes the ExistingConnectionID mapping from the store | ||
| func (k Keeper) DeleteExistingConnectionID(ctx sdk.Context, clientID, counterpartyConnectionID string) { | ||
| store := ctx.KVStore(k.storeKey) | ||
| store.Delete([]byte(types.ExistingConnectionIDKey(clientID, counterpartyConnectionID))) | ||
| } |
There was a problem hiding this comment.
requires genesis export/import logic. Maybe you could derive the mapping from the existing genesis fields?
| // delete existingConnectionID mapping now that handshake attempt is successful | ||
| // we no longer need to store it for redundancy protection | ||
| k.DeleteExistingConnectionID(ctx, connection.ClientId, counterpartyConnectionID) | ||
|
|
There was a problem hiding this comment.
This is unsafe? After the connection handshakes succeeds, in the tendermint client case, if the consensus height used to do proof init has not expired, there will still be a past state with a valid init proof which is not associated with a specific connection (on the try side). Unfortunate, but I believe the storage costs should be minimal? I don't think there is any in-protocol pruning we could do here without adding additional logic to the client layer to tell us when a past proof has expired
|
Closing pr for now (to reduce noise) as we await for more information on the upstream issue. We can reopen when necessary |
Description
closes: #1430 for connections. A separate PR will do the same logic for channels once the approach here is approved
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/) or specification (x/<module>/spec/)godoccomments.Unreleasedsection inCHANGELOG.mdFiles changedin the Github PR explorerCodecov Reportin the comment section below once CI passes