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.

Sign up for GitHub

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
+34 −2
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
69597f5
fix: Use fixed length hex for pointer at FwdCapabilityKey (backport #…
yihuang Aug 4, 2022
9f64c15
Update CHANGELOG.md
yihuang Aug 4, 2022
c0bc2c4
add comments and unit tests
yihuang Aug 9, 2022
7ddb667
Merge remote-tracking branch 'fork/backport-fwd-key-fix' into backpor…
yihuang Aug 9, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
* (x/mint) [#12384](https://github.com/cosmos/cosmos-sdk/pull/12384) Ensure `GoalBonded` must be positive when performing `x/mint` parameter validation.
* (simapp) [#12437](https://github.com/cosmos/cosmos-sdk/pull/12437) fix the non-determinstic behavior in simulations caused by `GenTx` and check
empty coins slice before it is used to create `banktype.MsgSend`.
* (x/capability) []() Use fixed length hex for pointer at FwdCapabilityKey.
yihuang marked this conversation as resolved.
Show resolved Hide resolved

## [v0.45.6](https://github.com/cosmos/cosmos-sdk/releases/tag/v0.45.6) - 2022-06-28

Expand Down
7 changes: 6 additions & 1 deletion x/capability/types/keys.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,12 @@ func RevCapabilityKey(module, name string) []byte {
// FwdCapabilityKey returns a forward lookup key for a given module and capability
// reference.
func FwdCapabilityKey(module string, cap *Capability) []byte {
return []byte(fmt.Sprintf("%s/fwd/%p", module, cap))
// 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 •
edited
Loading

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.

}

// IndexToKey returns bytes to be used as a key for a given capability index.
Expand Down
7 changes: 6 additions & 1 deletion x/capability/types/keys_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,12 @@ func TestRevCapabilityKey(t *testing.T) {

func TestFwdCapabilityKey(t *testing.T) {
cap := types.NewCapability(23)
expected := []byte(fmt.Sprintf("bank/fwd/%p", cap))
key := fmt.Sprintf("%#010p", cap)
if len(key) > 10 {
key = key[len(key)-10:]
}
require.Equal(t, 10, len(key))
expected := []byte(fmt.Sprintf("bank/fwd/0x%s", key))
require.Equal(t, expected, types.FwdCapabilityKey("bank", cap))
}

Expand Down