fix: correct ica wiring and update pfm fixes for v8 upgrade

[yaruwangway]

During the course of simulated upgrade testing with exported genesis from mainnet it was discovered that the node would panic after upgrading when attempting to prune two different stores, icacontroller and packetforwardmiddleware (PFM).

Update: The fix for ica controller is to remove this as it was accidentally leftover as an artifact from the original v8 release with v0.46 of the sdk. For PFM, the fix includes a corrected wiring in Gaia, and several updates to PFM itself due to an incorrectly named module and incorrect implementation of an interface necessary to pass to the TransferKeeper.

[codecov]

Codecov Report

Merging #2008 (3fb8b0f) into main (a4d5b52) will increase coverage by 0.11%.
The diff coverage is 91.83%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2008      +/-   ##
==========================================
+ Coverage   79.08%   79.20%   +0.11%     
==========================================
  Files          23       23              
  Lines        1568     1577       +9     
==========================================
+ Hits         1240     1249       +9     
  Misses        274      274              
  Partials       54       54              

[damiannolan]

Hey @yaruwangway, @glnro as part of this PR or another can you remove the ICAControllerKeeper from AppKeepers as well as the store key in keepers/keys.go.

I was going to make one myself but I don't have push access rights on this repo it seems.
I think it would be worth retesting the upgrade scenario you were having issues with after making these changes. I recall there was errors regarding the icacontroller store during some store pruning. I'm not sure at what point you were trying to do pruning but the icacontroller store wouldn't have previously existed so may explain why its erroring trying to delete previous iavl versions. The error you're seeing is bubbling up from here, right?

[glnro]

Hey @yaruwangway, @glnro as part of this PR or another can you remove the ICAControllerKeeper from AppKeepers as well as the store key in keepers/keys.go.

I was going to make one myself but I don't have push access rights on this repo it seems. I think it would be worth retesting the upgrade scenario you were having issues with after making these changes. I recall there was errors regarding the icacontroller store during some store pruning. I'm not sure at what point you were trying to do pruning but the icacontroller store wouldn't have previously existed so may explain why its erroring trying to delete previous iavl versions. The error you're seeing is bubbling up from here, right?

Yes, that's where the error is bubbling up. We're running an upgrade test with this test branch and then will assess.

[glnro]

Hey @yaruwangway, @glnro as part of this PR or another can you remove the ICAControllerKeeper from AppKeepers as well as the store key in keepers/keys.go.
I was going to make one myself but I don't have push access rights on this repo it seems. I think it would be worth retesting the upgrade scenario you were having issues with after making these changes. I recall there was errors regarding the icacontroller store during some store pruning. I'm not sure at what point you were trying to do pruning but the icacontroller store wouldn't have previously existed so may explain why its erroring trying to delete previous iavl versions. The error you're seeing is bubbling up from here, right?

Yes, that's where the error is bubbling up. We're running an upgrade test with this test branch and then will assess.

@damiannolan the fix of removing the ICACOntrollerKeeper as well as the store key seems to have worked in the stateful upgrade test we ran. Now the sdk panics when it tries to prune the packetforwardmiddleware store. Not ideal but still at least we can close this up.

[faddat]

interesting!

[faddat]

hey, FYI I've PR'd into this branch:

#2012

Basically just handles the ibc-go v4 jump sooner.

[damiannolan]

This looks like its not going to work as expected because the RouterKeeper is initialised below on line 314.
At this point it looks like we're going to be passing an empty struct.

[yaruwangway]

yeah, TransferKeeper need routerKeeper, routerKeeper need transferKeeper to initialize, one must be empty

[glnro]

@damiannolan Strangelove have added a setter on RouterKeeper for the TransferKeeper.

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

[glnro]

@Mergifyio backport release/v8.0.x

[mergify]

backport release/v8.0.x

✅ Backports have been created

• #2024 fix: correct ica wiring and update pfm fixes for v8 upgrade (backport #2008) has been created for branch release/v8.0.x