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

feat!: handle registering evm addresses in QGB module #2169

Merged
merged 35 commits into from
Aug 17, 2023

Conversation

cmwaters
Copy link
Contributor

@cmwaters cmwaters commented Jul 26, 2023

Addresses: #1698

@MSevey MSevey requested a review from a team July 26, 2023 12:01
@cmwaters cmwaters self-assigned this Jul 26, 2023
@cmwaters cmwaters marked this pull request as draft July 26, 2023 12:05
@cmwaters cmwaters changed the title chore: port across custom staking module from cosmos-sdk fork feat!: handle registering evm addresses in QGB module Aug 2, 2023
@cmwaters cmwaters added this to the Post-mainnet milestone Aug 2, 2023
proto/celestia/qgb/v1/tx.proto Outdated Show resolved Hide resolved
test/util/common.go Outdated Show resolved Hide resolved
x/qgb/client/tx.go Show resolved Hide resolved
x/qgb/client/tx.go Outdated Show resolved Hide resolved
x/qgb/client/tx.go Show resolved Hide resolved
x/qgb/keeper/keeper_valset.go Outdated Show resolved Hide resolved
@cmwaters cmwaters mentioned this pull request Aug 15, 2023
5 tasks
@evan-forbes evan-forbes added the consensus breaking modifies block validity rules in a way that will break consensus unless all nodes update their rules label Aug 17, 2023
// should always have an associated EVM address. Fortunately we can
// safely recover from this by deriving the default again.
ctx.Logger().Error("validator does not have an evm address set")
evmAddress = types.DefaultEVMAddress(val).String()
Copy link
Member

Choose a reason for hiding this comment

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

If we create a new EVM address here, this is not persisting it in store. Does it make sense to loop for correct default addresses in the hook?
Also, does it make sense to have a safer mechanism to derive the default EVM address? Something like hashing the operator address + something deterministic and specific to the current block?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, does it make sense to have a safer mechanism to derive the default EVM address? Something like hashing the operator address + something deterministic and specific to the current block?

I assume the validator address is unique thus it's safe to derive the EVM address from? Is there a reason why you would like to add more variables?

If we create a new EVM address here, this is not persisting it in store. Does it make sense to loop for correct default addresses in the hook?

Hmm so in theory this should never happen but perhaps you're right that it's better that if there is a developer error that we still persist the new default EVM address

Copy link
Member

Choose a reason for hiding this comment

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

I assume the validator address is unique thus it's safe to derive the EVM address from? Is there a reason why you would like to add more variables?

the validator address is unique but as was discussed yesterday, some other validator can derive your validator EVM address and use it for themselves. Especially if the validator reuses keys

Copy link
Member

Choose a reason for hiding this comment

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

I guess it makes sense to open an issue to keep note that we can have a squatting issue and we need to investigate it when we have time

x/qgb/keeper/hooks.go Outdated Show resolved Hide resolved
@cmwaters
Copy link
Contributor Author

@MSevey MSevey requested a review from a team August 17, 2023 10:05
rach-id
rach-id previously approved these changes Aug 17, 2023
Copy link
Member

@rach-id rach-id left a comment

Choose a reason for hiding this comment

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

🎉 🎉 Awesome stuff 👍 Thanks a lot for your patience with all the feedback

x/qgb/keeper/keeper_valset.go Outdated Show resolved Hide resolved
@MSevey MSevey requested a review from a team August 17, 2023 10:54
@MSevey MSevey requested a review from a team August 17, 2023 13:50
@cmwaters cmwaters merged commit 27f684f into main Aug 17, 2023
25 of 26 checks passed
@cmwaters cmwaters deleted the cal/migrate-staking branch August 17, 2023 14:19
cmwaters added a commit that referenced this pull request Aug 17, 2023
Bumps to the latest version of the SDK. Note that this deactivates the
use of the EVM address. To fulfill that functionality we will need to
reintroduce a mapping in
#2169
cmwaters added a commit that referenced this pull request Aug 17, 2023
Addresses: #1698

---------

Co-authored-by: CHAMI Rachid <[email protected]>
Co-authored-by: Evan Forbes <[email protected]>
cmwaters added a commit that referenced this pull request Aug 17, 2023
Updates the sdk and cometbft version, reverts to using the vanilla staking implementation moving the mapping between the validators and their evm addresses to the QGB module as a separate msg
0xchainlover pushed a commit to celestia-org/celestia-app that referenced this pull request Aug 1, 2024
Bumps to the latest version of the SDK. Note that this deactivates the
use of the EVM address. To fulfill that functionality we will need to
reintroduce a mapping in
celestiaorg/celestia-app#2169
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
consensus breaking modifies block validity rules in a way that will break consensus unless all nodes update their rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants