Skip to content

Allow sending transactions from an Ethereum address derived account id#8757

Merged
athei merged 23 commits intomasterfrom
at/eth_addr
Sep 29, 2025
Merged

Allow sending transactions from an Ethereum address derived account id#8757
athei merged 23 commits intomasterfrom
at/eth_addr

Conversation

@athei
Copy link
Copy Markdown
Member

@athei athei commented Jun 5, 2025

We always allowed signing transactions using an Bitcoin/Eth style SECP256k1 key. The account in this case is simply the blake2 hash of the public key.

This address derivation is problematic: It requires the public key in order to derive the account id. On Ethereum you simply can't know the public key of an address. This is why the mapping in pallet_revive is defined as address <-> account_id.

This PR adds a new signature variant that allows signing a transaction with an account id as origin that matches this mapping.

Why is this important?

Example1

A wallet contains an SECP256k1 key and wants to interact with native Polkadot APIs. It can sign the transaction using this key. However, without this change the origin of that transaction will be different than the one it would appear under if it had signed an Ethereum transaction.

Example2

A chain using an Ethereum style address (like Mythical) wants to send some tokens to one of their users account on AssetHub. How would they know what is the address of that user on AssetHub? With this change they can just pad the address with 0xEE and rely on the fact that the user can interact with AssetHub using their existing key.

Why a new variant?

We can't modify the existing variant. Otherwise the same signature would suddenly map to a different account making people lose access to their funds. Instead, we add a new variant that adds control over an additional account for the same signature.

A new KeccakSigner and KeccakSignature

After considering feedback by @moliholy I am convinced that we should use keccak instead of blake2b for this new MultiSignature variant. Reasoning is that this will make it much simpler for Ethereum tooling to generate such signatures. Since this signature is specifically created for Ethereum interop it just makes sense to also use keccak here.

To that end I made the ecdsa::{KeccakSigner, KeccakSignature} generic over their hash algorithm. Please note that I am using tags here and not the Hasher trait directly. This makes things more complicated but it was necessary: All Hasher implementations are in higher level crates and can't be directly referenced here. But I would have to reference it in order to make this a non breaking change. The Signer and Signature types behave exactly the same way as before.

@athei athei requested a review from bkchr June 5, 2025 12:07
@athei athei added the T17-primitives Changes to primitives that are not covered by any other label. label Jun 5, 2025
Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com>
@athei
Copy link
Copy Markdown
Member Author

athei commented Jun 5, 2025

/cmd prdoc --audience runtime_dev --bump major

@pgherveou
Copy link
Copy Markdown
Contributor

pgherveou commented Jun 6, 2025

This seems to be important for the foundry-forge project as well.

with paritytech/foundry-polkadot#130, the project is trying to upload the code on chain ahead of time so that we can interact with the code hash directly.

To do that, foundry will have to sign a pallet-revive::upload-code substrate transaction with an Ethererum private key.

cc @soul022

Copy link
Copy Markdown
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

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

Not a cryptographic expert, but looks reasonable.

Comment on lines +497 to +499
impl CryptoType for KeccakSignature {
type Pair = KeccakPair;
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

For the public key it will report the wrong Pair. Not actually sure where we use this stuff and if that is that important for the public key.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yeah I think it should return the correct Pair. Will need to make the Public generic, too.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Made Public generic in order to fix that. Not sure if really necessary but let's be safe here.

@josepot
Copy link
Copy Markdown
Contributor

josepot commented Jul 14, 2025

I guess that would be possible, too. But I think adding a new variant is easier to understand than adding more complexity to the logic which verifies the signature to account for one signature type having multiple possible addresses.

Actually, I just realized that there is no need to actually check against both options... Instead, simply check if the last 12 bytes of the account-id are padded with 0xee, if they are then use the Eth-keccak-variant, if they aren't then use the traditional method.

I think that could be preferable to having the following enum:

pub enum MultiSignature {
	/// An Ed25519 signature.
	Ed25519(ed25519::Signature),
	/// An Sr25519 signature.
	Sr25519(sr25519::Signature),
	/// An ECDSA/SECP256k1 signature.
	Ecdsa(ecdsa::Signature),
	/// An ECDSA/SECP256k1 signature but with a different address derivation.
	Eth(ecdsa::KeccakSignature),
}

TBH

EDIT:

Just to be clear: it's not that I actually have a super-strong opinion about this. I can see the tradeoffs for both options... To be fair: both feel a bit hacky, but in different ways. I just found it a bit surprising that this option was never discussed/considered, that's all.

@athei
Copy link
Copy Markdown
Member Author

athei commented Aug 4, 2025

It just seems more straightforward in theory. In the actual code you would need to touch the unchecked extrinsic implementation. While this is neatly self contained.

@patriciobcs patriciobcs moved this from In progress to Audited in Security Audit (PRs) - Parity Security Sep 26, 2025
@athei athei added this pull request to the merge queue Sep 29, 2025
@athei
Copy link
Copy Markdown
Member Author

athei commented Sep 29, 2025

Nothing severe was found in the audit. Going ahead.

@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 29, 2025
@paritytech-workflow-stopper
Copy link
Copy Markdown

All GitHub workflows were cancelled due to failure one of the required jobs.
Failed workflow url: https://github.com/paritytech/polkadot-sdk/actions/runs/18092686244
Failed job name: build-subkey

@athei athei added this pull request to the merge queue Sep 29, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 29, 2025
@athei athei added this pull request to the merge queue Sep 29, 2025
Merged via the queue into master with commit 50372ea Sep 29, 2025
255 of 259 checks passed
@athei athei deleted the at/eth_addr branch September 29, 2025 15:13
bee344 pushed a commit that referenced this pull request Oct 7, 2025
#8757)

We always allowed signing transactions using an Bitcoin/Eth style
SECP256k1 key. The account in this case is simply the blake2 hash of the
public key.

This address derivation is problematic: It requires the public key in
order to derive the account id. On Ethereum you simply can't know the
public key of an address. This is why the mapping in pallet_revive is
defined as `address <-> account_id`.

This PR adds a new signature variant that allows signing a transaction
with an account id as origin that matches this mapping.

## Why is this important?

### Example1 
A wallet contains an SECP256k1 key and wants to interact with native
Polkadot APIs. It can sign the transaction using this key. However,
without this change the origin of that transaction will be different
than the one it would appear under if it had signed an Ethereum
transaction.

### Example2
A chain using an Ethereum style address (like Mythical) wants to send
some tokens to one of their users account on AssetHub. How would they
know what is the address of that user on AssetHub? With this change they
can just pad the address with `0xEE` and rely on the fact that the user
can interact with AssetHub using their existing key.

## Why a new variant?
We can't modify the existing variant. Otherwise the same signature would
suddenly map to a different account making people lose access to their
funds. Instead, we add a new variant that adds control over an
additional account for the same signature.

## A new `KeccakSigner` and `KeccakSignature`

After considering feedback by @moliholy I am convinced that we should
use keccak instead of blake2b for this new `MultiSignature` variant.
Reasoning is that this will make it much simpler for Ethereum tooling to
generate such signatures. Since this signature is specifically created
for Ethereum interop it just makes sense to also use keccak here.

To that end I made the `ecdsa::{KeccakSigner, KeccakSignature}` generic
over their hash algorithm. Please note that I am using tags here and not
the `Hasher` trait directly. This makes things more complicated but it
was necessary: All Hasher implementations are in higher level crates and
can't be directly referenced here. But I would have to reference it in
order to make this a non breaking change. The `Signer` and `Signature`
types behave exactly the same way as before.

---------

Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com>
Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
alvicsam pushed a commit that referenced this pull request Oct 17, 2025
#8757)

We always allowed signing transactions using an Bitcoin/Eth style
SECP256k1 key. The account in this case is simply the blake2 hash of the
public key.

This address derivation is problematic: It requires the public key in
order to derive the account id. On Ethereum you simply can't know the
public key of an address. This is why the mapping in pallet_revive is
defined as `address <-> account_id`.

This PR adds a new signature variant that allows signing a transaction
with an account id as origin that matches this mapping.

## Why is this important?

### Example1 
A wallet contains an SECP256k1 key and wants to interact with native
Polkadot APIs. It can sign the transaction using this key. However,
without this change the origin of that transaction will be different
than the one it would appear under if it had signed an Ethereum
transaction.

### Example2
A chain using an Ethereum style address (like Mythical) wants to send
some tokens to one of their users account on AssetHub. How would they
know what is the address of that user on AssetHub? With this change they
can just pad the address with `0xEE` and rely on the fact that the user
can interact with AssetHub using their existing key.

## Why a new variant?
We can't modify the existing variant. Otherwise the same signature would
suddenly map to a different account making people lose access to their
funds. Instead, we add a new variant that adds control over an
additional account for the same signature.

## A new `KeccakSigner` and `KeccakSignature`

After considering feedback by @moliholy I am convinced that we should
use keccak instead of blake2b for this new `MultiSignature` variant.
Reasoning is that this will make it much simpler for Ethereum tooling to
generate such signatures. Since this signature is specifically created
for Ethereum interop it just makes sense to also use keccak here.

To that end I made the `ecdsa::{KeccakSigner, KeccakSignature}` generic
over their hash algorithm. Please note that I am using tags here and not
the `Hasher` trait directly. This makes things more complicated but it
was necessary: All Hasher implementations are in higher level crates and
can't be directly referenced here. But I would have to reference it in
order to make this a non breaking change. The `Signer` and `Signature`
types behave exactly the same way as before.

---------

Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com>
Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@louismerlin louismerlin moved this from Backlog to Scheduled in Security Audit (PRs) - SRLabs Oct 23, 2025
@redzsina redzsina moved this from Scheduled to Backlog in Security Audit (PRs) - SRLabs Oct 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

T17-primitives Changes to primitives that are not covered by any other label.

Projects

Status: Backlog
Status: Done

Development

Successfully merging this pull request may close these issues.