-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
ibc-transfer packet acknowledgements are broken #423
Comments
@michaelfig sorry for the confusion but #416 had a bug. #421 fixed this. Do you still see this error on master? (It's the same as the reported bug so I suspect it is fixed) |
We could reduce the number to 1000. This would give an hour for connection handshake to complete. 100 might still be a little low, connection handshakes would have to be completed within 10 minutes of being initiated |
I'm noticing some indeterminacy in the tests (though it has been like this for a while). I suspect there is some very subtle bug. Let me know if you continue to run into issues using master. I will cut a v0.8.0 when I think the code has stabilized a little. |
I think I know the issue, will try to post a fix. This is also something that can be remedied using auto updates (which has now been merged) see #424 there may be remaining bugs but this should help Edit: #425 is needed to stabilize the relayer. This was an existing problem, the recent refactors just helped reveal it to me |
I thought |
Please tag me when you'd like me to test again. Current master (81dd43c):
|
consensus states are objects stored on chain. Historical entries is not used, but each time we need to prove something on chain, we need to do so against a specific consensus state. Much of the code in this codebase separates construction of an update message and construction of the proof. This is problematic since we need the proof to be constructed against the update message. #425 would fix this but #424 might fix some issues in the short term |
A word of warning, retries for packet relaying will likely fail. I fixed it for handshakes but relaying retries will require a deeper refactor. Simply rerunning the command should work |
Awesome! @michaelfig that's great to hear. Closing this for now since #425 should hopefully tackle the major issues remaining |
@colin-axner writes:
I tested #416 between my cosmos-sdk v0.41.0 chains (with
--pruning=nothing
), waiting until thegenesis.app_state.staking.params.historical_entries
value of 100 was long past, and I got a lot of consensus state failures:IOW, the packet was relayed correctly but the ack was not returned. This is between two cosmos-sdk
application/ibc-transfer
instances, listening on port transfer. This phenomenon replicates what I found in my testing of dynamic IBC.BTW, After this is sorted out, should we also encourage Gaia to default to the (old)
100
value for historical_entries instead of10000
?https://github.com/cosmos/gaia/blob/7756f45641f6ee8cfabf83bfac85b688bae3713f/app/migrate.go#L164
The text was updated successfully, but these errors were encountered: