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

fix: Use fixed length hex for pointer at FwdCapabilityKey (backport #11737) #12818

Merged
merged 4 commits into from
Aug 9, 2022

Conversation

yihuang
Copy link
Collaborator

@yihuang yihuang commented Aug 4, 2022

Description

ref: #11726


Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage
  • manually tested (if applicable)

CHANGELOG.md Outdated Show resolved Hide resolved
Comment on lines 42 to 47
// truncate the key to fixed length, keep backward compatible on common architectures.
key := fmt.Sprintf("%#010p", cap)
if len(key) > 10 {
key = key[len(key)-10:]
}
return []byte(fmt.Sprintf("%s/fwd/0x%s", module, key))
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this different from what we, recently, updated on main which is

	return []byte(fmt.Sprintf("%s/fwd/%#016p", module, cap))

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This one is not breaking for common architectures, by using the same length as before, but for mac m1, it truncate the high byte, to avoid hash mismatch.

Copy link
Contributor

Choose a reason for hiding this comment

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

But these keys are not in merkle state, so no change would be breaking AFAIK. As long as gas consumption is the same.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

exactly, the gas would be different if the encoded pointer has different length, the issue is here: #11726, it seems you have participated in it too.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ethermint feemarket will save the block gas used to merkle state ;D

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it works, but it's a breaking one, so can't backport to old versions directly.

Copy link
Contributor

@alexanderbez alexanderbez Aug 8, 2022

Choose a reason for hiding this comment

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

Its breaking because it charges a different gas compared to what it was before?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes

Copy link
Contributor

Choose a reason for hiding this comment

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

This seems ok given the circumstances, but it'd be nice to indicate in the code this is a temporary patch and should not be replicated. Also, I think the tests should use the two pointer values in the referenced issue

Nice work and thanks for opening up a non-breaking fix

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

added comments and backward compatibility test.

@colin-axner
Copy link
Contributor

this is related to #11726 correct?

@yihuang
Copy link
Collaborator Author

yihuang commented Aug 8, 2022

this is related to #11726 correct?

yes, a non-breaking backport to make mac m1 able to sync legacy chains.

@alexanderbez alexanderbez merged commit 09321d7 into cosmos:release/v0.45.x Aug 9, 2022
@alexanderbez
Copy link
Contributor

This will be included in the next 0.45 point release :)

cc @marbar3778

@yihuang yihuang deleted the backport-fwd-key-fix branch August 10, 2022 01:35
JeancarloBarrios pushed a commit to agoric-labs/cosmos-sdk that referenced this pull request Sep 28, 2024
…osmos#11737) (cosmos#12818)

* fix: Use fixed length hex for pointer at FwdCapabilityKey (backport cosmos#11737)

* Update CHANGELOG.md

* add comments and unit tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants