fix: avoid race conditions in ics27 handshakes (backport #2682) #2698
Conversation
![mergify[bot]](https://avatars.githubusercontent.com/in/10562?s=60&v=4){kind=small}
* wip adding conditional to msg server and go apis, adding tests * cleanup * cleanup middleware enabled code * adding additional test case for reopening channel via msg server * Update modules/apps/27-interchain-accounts/controller/keeper/keeper.go Co-authored-by: Cian Hatton <cian@interchain.io> * updating error msgs and test case assertion * updating InitGenesis to set middleware disabled Co-authored-by: Cian Hatton <cian@interchain.io> (cherry picked from commit c9b8064)
mergify
bot
requested review from
seantking,
colin-axner,
AdityaSripal and
damiannolan
as code owners
| store.Set(icatypes.KeyIsMiddlewareEnabled(portID, connectionID), icatypes.MiddlewareEnabled) | ||
| } | ||
|
|
||
| // SetMiddlewareDisabled stores a flag to indicate that the underlying application callbacks should be disabled for the given port and connection identifier pair |
There was a problem hiding this comment.
Sorry, but I didn't have time to review the original PR, so I am just mentioning this as a suggestion that could be taken up in a follow up PR if you think it's useful. It's more of a code hygiene thing, anyway...
Instead of having two separate functions (SetMiddlewareEnabled and SetMiddlewareDisabled), would it be possible instead to have just one (e.g. SetMiddlewareStatus) and pass as third argument either icatypes.MiddlewareEnabledoricatypes.MiddlewareDisabled? The key could be also icatypes.KeyIsMiddlewareStatusinsteadicatypes.KeyIsMiddlewareEnabled`.
There was a problem hiding this comment.
I thought about this and originally when I was working on the issue I had one IsMiddlewareEnabled() func which retruned a true/false boolean value.
The problem with this is you need to handle more than just two cases cases. Middleware is either enabled, disabled or hasn't been set. In the case of it not being set it would be the first time a handshake is initiated for that connection/owner tuple and we need to allow it proceed these checks.
I think with your suggestion of SetMiddlewareStatus you'd also need a CheckMiddlewareStatus(enabled/disabled) which takes an arg and returns true/false in the appropriate cases. I'm sure there's a few different approachs you could take here. I think the current code and logic is pretty readable when you consider all the scenarios.
This is an automatic backport of pull request #2682 done by Mergify.
Mergify commands and options
More conditions and actions can be found in the documentation.
You can also trigger Mergify actions by commenting on this pull request:
@Mergifyio refreshwill re-evaluate the rules@Mergifyio rebasewill rebase this PR on its base branch@Mergifyio updatewill merge the base branch into this PR@Mergifyio backport <destination>will backport this PR on<destination>branchAdditionally, on Mergify dashboard you can:
Finally, you can contact us on https://mergify.com