Skip to content
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

Closed
wants to merge 3 commits into from

Conversation

gitferry
Copy link
Member

Closes #393

@gitferry gitferry changed the title chore: Add dumy handler of MsgInjectedCheckpoint` chore: Add dumy handler of MsgInjectedCheckpoint Jan 10, 2025
@gitferry gitferry changed the title chore: Add dumy handler of MsgInjectedCheckpoint chore: Add dummy handler of MsgInjectedCheckpoint Jan 10, 2025
@gitferry gitferry marked this pull request as ready for review January 10, 2025 09:59
Copy link
Member

@Lazar955 Lazar955 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Copy link
Collaborator

@KonradStaniec KonradStaniec left a 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:

  1. Schedule it for babylon v2.
  2. Do coordinated binary upgrade on testnet

Tbh given this is not critical issue, I would leave it for Babylon v2

@gitferry
Copy link
Member Author

@KonradStaniec MsgInjectedCheckpoint is currently been handled directly in PreBlocker. Why introducing this dummy handler would affect apphash?

@KonradStaniec
Copy link
Collaborator

@KonradStaniec MsgInjectedCheckpoint is currently been handled directly in PreBlocker. Why introducing this dummy handler would affect apphash?

Even though message is handled in PreBlocker, it is still transaction in the Block which means it gets executed during DeliverTx here - https://github.com/cosmos/cosmos-sdk/blob/8cc13ed2e2b4fca118b27cfaaaef71c2420aff06/baseapp/baseapp.go#L771. Thats why we see it as failed in explorers as its execution code is different than 0.

This is not part of the appHash but it is part of the LastResultsHash - https://github.com/cometbft/cometbft/blob/0d4d40f19dbf7e724b9a65b598b5431e733eee0c/types/block.go#L352

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:

  1. Whether there is risk of chain split or not
  2. Whether transactions which checkpoints are now considered successfull i.e have Code == 0

@gitferry
Copy link
Member Author

gitferry commented Jan 10, 2025

@KonradStaniec Thanks for the explanation. It all makes sense. I agree that this pr is not a urgent fix can can leave it for v2 but will test it out soon

@gitferry
Copy link
Member Author

Update after a sync test:

  1. Syncing to test-net-5 failed at height 361 with error preblocker: failed to extract injected checkpoint from the tx set: failed to decode injected vote extension tx: proto: wrong wireType = 2 for field Status: tx parse error [cosmos/[email protected]/x/auth/tx/decoder.go:49], which is expected because in this pr, we have introduced a new attribute placehold_address in the MsgInjectedCheckpoint
  2. I start a local testnet and start from scrach. At the height epoch * i + 1, got error 'out of gas in location: ReadFlat; gasWanted: 0, gasUsed: 1000: out of gas'. I think this is also expected as every tx in Cosmos should be charged with a basic gas as fee.

Based on the above, I'm not sure adding a dummy handler is a right way to go. Any ideas? @KonradStaniec @Lazar955 ?

@KonradStaniec
Copy link
Collaborator

I start a local testnet and start from scrach. At the height epoch * i + 1, got error 'out of gas in location: ReadFlat; gasWanted: 0, gasUsed: 1000: out of gas'. I think this is also expected as every tx in Cosmos should be charged with a basic gas as fee.

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

  • close the PR
  • document our findings somewhere. (Maybe ADR in pm repo, describing this findings that it is not trivial to make this custom tx successful ?)

@gitferry
Copy link
Member Author

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error in explorer: no message handler found for *types.MsgInjectedCheckpoint
3 participants