-
Notifications
You must be signed in to change notification settings - Fork 42
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
chore: Add dummy handler of MsgInjectedCheckpoint
#395
Conversation
chore: Add dumy handler of
MsgInjectedCheckpoint`MsgInjectedCheckpoint
MsgInjectedCheckpoint
MsgInjectedCheckpoint
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.
lgtm
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.
Looks good !
Though one note here, we can't backport it / deploy it on phase-2 testnet without additional work.
The checkpoint transactions which were previously marked as failed i.e had code different than 0
, now they will have code 0
, and transaction results influence hash of transactions which is in block headers. https://github.com/cometbft/cometbft/blob/v0.38.16/types/results.go#L47 shows that code is taken into computing tx hash in header.
This leaves us with two options:
- Schedule it for babylon v2.
- Do coordinated binary upgrade on testnet
Tbh given this is not critical issue, I would leave it for Babylon v2
@KonradStaniec |
Even though message is handled in This is not part of the This means the node with with this change will produce and expects different headers that are in the current testnet, which will lead to chain split. Actually , the easiest way to test this pr would be to try to sync the node build with this version with current testnet, this would verify:
|
@KonradStaniec Thanks for the explanation. It all makes sense. I agree that this pr is not a urgent fix can can leave it for |
Update after a sync test:
Based on the above, I'm not sure adding a dummy handler is a right way to go. Any ideas? @KonradStaniec @Lazar955 ? |
Ahhh true! to make it successful we would need to pass whole ante handler (gas and signatures). Thats a lot of custom work. Tbh given that the only downside of not having this improvement it the fact that our custom transaction shows as failed in explorer (but it is possible to decode it and get its data) I would
|
I agree with closing this PR. I wondered maybe we should raise an issue to Cosmos as I think SDK should have a common solution for this other than each project is doing their own way. Or do we want to consult Cosmos friends first? |
Closes #393