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

Implemented ECDSA recover function. #914

Merged
merged 17 commits into from
Oct 14, 2021

Conversation

xgreenx
Copy link
Collaborator

@xgreenx xgreenx commented Sep 10, 2021

Added support of seal_ecdsa_recover from the paritytech/substrate#9686.
This function allows to recover the ecdsa compressed public key from the signature and message hash. It will return ECDSAPublicKey which has two methods to_eth_address and to_account_id.

Added method `to_eth_address` and `to_account_id`.
Added tests.
@cmichi
Copy link
Collaborator

cmichi commented Sep 10, 2021

@xgreenx Thanks for the PR! Could you add the two words ECDSA and Ethereum here? That's why the spellcheck CI stage fails.

The other stage seems to be a Rust nightly change to the way the compiler displays error messages again. We'll sort it out in another PR.

Copy link
Collaborator

@cmichi cmichi 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 overall, I've got some comments.

.expect("Unable to parse the compressed ECDSA public key");
let uncompressed = pub_key.serialize();

// Hash the uncompressed public key without first byte by Keccak256 algorithm.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you add the reasoning why the first byte is skipped to the comment?

@@ -604,3 +604,38 @@ where
instance.hash_encoded::<H, T>(input, output)
})
}

/// Recovers the compressed ECDSA public key for given `signature` and `message_hash`, and stores the result in `output`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
/// Recovers the compressed ECDSA public key for given `signature` and `message_hash`, and stores the result in `output`.
/// Recovers the compressed ECDSA public key for given `signature` and `message_hash`,
/// and stores the result in `output`.

@@ -140,6 +140,14 @@ pub trait EnvBackend {
H: CryptoHash,
T: scale::Encode;

/// Recovers the compressed ECDSA public key for given `signature` and `message_hash`, and stores the result in `output`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
/// Recovers the compressed ECDSA public key for given `signature` and `message_hash`, and stores the result in `output`.
/// Recovers the compressed ECDSA public key for given `signature` and `message_hash`,
/// and stores the result in `output`.

@@ -1013,4 +1017,62 @@ where
ink_env::hash_encoded::<H, V>(value, &mut output);
output
}

/// Recovers the compressed ECDSA public key for given `signature` and `message_hash`, and stores the result in `output`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
/// Recovers the compressed ECDSA public key for given `signature` and `message_hash`, and stores the result in `output`.
/// Recovers the compressed ECDSA public key for given `signature` and `message_hash`,
/// and stores the result in `output`.

signature[64]
};
let message = Message::parse(message_hash);
let signature = Signature::parse_slice(&signature[0..64]).unwrap();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you replace the unwrap()'s in the PR with expect(…)? We try to avoid unwrap's when possible.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Even better is unwrap_or_else(|error| panic!("<error description>: {}", error)) which contains the root error message alongside a local error description provided by us.

crates/engine/src/ext.rs Show resolved Hide resolved
Signature,
};

let recovery_byte = if signature[64] > 27 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Something that I'm missing is an explanation of this magic number. How about creating a const with a mnemonic name and adding a comment to that const?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please turn the 27 into a const with proper documentation of where it comes. Ideally a link.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added the comment before the if section. It is more clear than a comment near a constant. This value is used in several crates, so I can't create one global constant. Define it in every crate seems overkill and better to leave the comment because 27 is used in one place.

];

assert_eq!(pub_key.to_account_id(), EXPECTED_ACCOUNT_ID.into());
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you add a test for the failure case, asserting EcdsaRecoverFailed?


/// The ECDSA compressed public key.
#[derive(Debug, Copy, Clone)]
pub struct ECDSAPublicKey(pub [u8; 33]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a reason why you've put this into traits.rs? From what I see there is no trait logic and my intuition would have been to put it into env_access.rs.

crates/lang/src/traits.rs Outdated Show resolved Hide resolved
}

/// Address of Ethereum account
pub type EthereumAddress = [u8; 20];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you make it a pub struct EthereumAddress([u8; 20]); with Deref and DerefMut implemented for it? So basically analog to what you did for ECDSAPublicKey.

Copy link
Collaborator

@cmichi cmichi left a comment

Choose a reason for hiding this comment

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

Thank you!

@cmichi
Copy link
Collaborator

cmichi commented Sep 17, 2021

@xgreenx Could you run cargo fmt --all? That's why the CI currently fails.

@xgreenx
Copy link
Collaborator Author

xgreenx commented Sep 17, 2021

Now gitlab fmt fails=)

result
}

pub fn to_account_id(&self) -> <DefaultEnvironment as Environment>::AccountId {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be better if we did not only support the DefaultEnvironment if that's possible.

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 can be only DefaultEnvironment because it hashes the ECDSA public key and the output is 32 bytes. I copied this logic from the substrate(it is how substrate generates AccountId from the ECDSA).

So the user can't use a custom type of AccountId here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If that's the case I'd rather remove to_account_id and instead provide a way to extract the [u8; 32] so that users can themselves create an AccountId from them if necessary.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The user already has the ability to get an inner array(it is public).
The idea of this method is to simplify work with default AccountId. Because without this implementation the user must check the source code of substrate and find the way how to convert ECDSA public key into AccountId.

Copy link
Collaborator

Choose a reason for hiding this comment

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

First of all this API is missing proper documentation.
Second I find it very confusing for users that this only works for the default environment.
It would be an improvement if it worked for any environment that uses the default environment account ID instead but that would still lead to confusion.
I'd personally prefer an API that returns something that can easily turned into the default account ID instead, e.g. [u8; 32] byte array.

crates/eth_compatibility/src/lib.rs Outdated Show resolved Hide resolved
Comment on lines 1077 to 1082
let result = ink_env::ecdsa_recover(signature, message_hash, &mut output);

match result.is_ok() {
true => Ok(ECDSAPublicKey { 0: output }),
false => Err(Error::EcdsaRecoverFailed),
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This could be rewritten using Result::map and Result::map_err.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
let result = ink_env::ecdsa_recover(signature, message_hash, &mut output);
match result.is_ok() {
true => Ok(ECDSAPublicKey { 0: output }),
false => Err(Error::EcdsaRecoverFailed),
}
ink_env::ecdsa_recover(signature, message_hash, &mut output)
.map(|_| ECDSAPublicKey { 0: output })
.map_err(|_| Error::EcdsaRecoverFailed)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe borrow checker makes problems due to &mut output. Not checked, yet.

USed symbolic links instead files.
@codecov-commenter
Copy link

codecov-commenter commented Sep 17, 2021

Codecov Report

Merging #914 (35b5fcc) into master (6e7941a) will decrease coverage by 0.44%.
The diff coverage is 59.21%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #914      +/-   ##
==========================================
- Coverage   83.00%   82.56%   -0.45%     
==========================================
  Files         190      191       +1     
  Lines        8160     8234      +74     
==========================================
+ Hits         6773     6798      +25     
- Misses       1387     1436      +49     
Impacted Files Coverage Δ
crates/engine/src/ext.rs 61.18% <0.00%> (-6.70%) ⬇️
crates/env/src/backend.rs 0.00% <ø> (ø)
crates/lang/src/env_access.rs 27.27% <ø> (ø)
...tes/env/src/engine/experimental_off_chain/impls.rs 66.29% <68.75%> (+0.53%) ⬆️
crates/env/src/engine/off_chain/impls.rs 54.83% <73.33%> (+3.55%) ⬆️
crates/eth_compatibility/src/lib.rs 74.07% <74.07%> (ø)
crates/env/src/api.rs 47.05% <100.00%> (+3.30%) ⬆️
crates/primitives/src/key_ptr.rs 75.00% <0.00%> (-25.00%) ⬇️
crates/primitives/src/key.rs 65.00% <0.00%> (-17.50%) ⬇️
...ates/storage/src/collections/hashmap/fuzz_tests.rs 95.74% <0.00%> (-4.26%) ⬇️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6e7941a...35b5fcc. Read the comment docs.

@Robbepop
Copy link
Collaborator

What is missing from this PR still to get a final review and merge it?

@xgreenx
Copy link
Collaborator Author

xgreenx commented Sep 21, 2021

If you agree with this, then the change is ready=)

@@ -195,6 +195,43 @@ impl EnvBackend for EnvInstance {
self.hash_bytes::<H>(&encoded[..], output)
}

fn ecdsa_recover(
Copy link
Collaborator

Choose a reason for hiding this comment

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

I hope we can get rid of the huge code dupe once we eliminate the old off-chain engine.

crates/eth_compatibility/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@Robbepop Robbepop left a comment

Choose a reason for hiding this comment

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

sorry for be belayed review. found some things. rest looks good. if fixed we can merge.


/// The ECDSA compressed public key.
#[derive(Debug, Copy, Clone)]
pub struct ECDSAPublicKey(pub [u8; 33]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
pub struct ECDSAPublicKey(pub [u8; 33]);
pub struct ECDSAPublicKey([u8; 33]);

Copy link
Collaborator

Choose a reason for hiding this comment

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

is there a good reason why this should be pub?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The substrate had something like this structure and the field was public. I did the same=)
I've reworked it to be private and implemented the From<[u8; 33]> trait.

crates/eth_compatibility/src/lib.rs Show resolved Hide resolved
Comment on lines 34 to 48
impl core::ops::Deref for ECDSAPublicKey {
type Target = [u8; 33];

#[inline]
fn deref(&self) -> &Self::Target {
&self.0
}
}

impl core::ops::DerefMut for ECDSAPublicKey {
#[inline]
fn deref_mut(&mut self) -> &mut Self::Target {
&mut self.0
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Deref and DerefMut should be used with great care and usually should only be applied on reference-like or pointer-like types.
Instead I'd propose to implement AsRef<[u8; 33]> and AsMut<[u8; 33]>.
This leads the same purpose but is way less intrusive.

Comment on lines 54 to 68
impl core::ops::Deref for EthereumAddress {
type Target = [u8; 20];

#[inline]
fn deref(&self) -> &Self::Target {
&self.0
}
}

impl core::ops::DerefMut for EthereumAddress {
#[inline]
fn deref_mut(&mut self) -> &mut Self::Target {
&mut self.0
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Deref and DerefMut should be used with great care and usually should only be applied on reference-like or pointer-like types.
Instead I'd propose to implement AsRef<[u8; 33]> and AsMut<[u8; 33]>.
This leads the same purpose but is way less intrusive.

crates/eth_compatibility/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@Robbepop Robbepop left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks a lot for the PR and sorry for taking so long.

@cmichi cmichi merged commit 284e4e5 into use-ink:master Oct 14, 2021
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.

4 participants