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

Add sr25519_verify() #1840

Merged
merged 36 commits into from
Aug 9, 2023
Merged

Conversation

goastler
Copy link
Contributor

@goastler goastler commented Jul 6, 2023

This is an extension of another PR which attempted to add sr25519 signature verification to ink.

Several points were made in that PR but not all were addressed. The original author has not resolved those issues as they do not have the time, so I have instead.

I have fixed the panics which were being thrown due to invalid public keys or signatures.

I've added tests for each signature verification outcome:

  • valid
  • invalid public key
  • invalid signature
  • invalid message
    and put these in the integrations-tests folder (hopefully that's the right place?)

At the moment, the context parameter of sr25519 is fixed to "substrate". This is set in the substrate repo, so I've mirrored it here. It probably should be a parameter rather than being hard-coded, but that would require a change on the substrate end and ink end, so should be left as a job for after this PR.

I've also applied clippy and rust fmt recommendations. Also added docs.

kziemianek and others added 21 commits April 19, 2023 14:22
public key byte[] and/or signature byte[] may not be constructed into a PublicKey or Signature if it's in the wrong format. These errors originate from the schnorrkel library and need to be caught, otherwise it'll panic and propagate up to smart contracts in ink.
there is no need to make the context directly, we can delegate that to the schnorrkel library
Docs + example + notes about context set to "substrate"
Helpful for debugging why the error has occurred, as there is only ever one of 3 reasons: invalid public key, invalid message or invalid signature
Tested invalid public key, invalid signature, invalid message and valid case
@goastler goastler marked this pull request as ready for review July 6, 2023 13:39
Copy link
Collaborator

@xgreenx xgreenx 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, the PR is almost ready=) There are small [nit]s and a question to @athei and @cmichi regarding our strategy for unstable functionality

crates/ink/ir/src/ir/trait_def/item/mod.rs Outdated Show resolved Hide resolved
crates/ink/ir/src/ir/item_mod.rs Outdated Show resolved Hide resolved
integration-tests/sr25519-verification/Cargo.toml Outdated Show resolved Hide resolved
@@ -146,6 +146,15 @@ mod sys {
output_ptr: Ptr32Mut<[u8]>,
) -> ReturnCode;

/// **WARNING**: this function is from the [unstable interface](https://github.com/paritytech/substrate/tree/master/frame/contracts#unstable-interfaces),
/// which is unsafe and normally is not available on production chains.
pub fn sr25519_verify(
Copy link
Collaborator

Choose a reason for hiding this comment

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

@athei @cmichi What do you think about adding a new unstable feature to the ink_env crate?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep . I think we can feature-gate the function with the sr25519-verify verify until it gets stabilized

Copy link
Contributor

Choose a reason for hiding this comment

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

Features are a last resort and should only be used when absolutely necessary (i.e not here). We don't need a feature here. A clear doc that this is an unstable functionality is enough. If someone ignores that hint nothing bad will happen. The contract will simply not deploy on a production chain.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@goastler Could you propagate this warning to the public method then, please(available for users)? Because this comment is useful only for people who dive deep into the ink! codebase.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure thing :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Co-authored-by: Green Baneling <XgreenX9999@gmail.com>
@SkymanOne
Copy link
Contributor

Please merge master into your branch to fix CI

@goastler
Copy link
Contributor Author

Please merge master into your branch to fix CI

done

@goastler
Copy link
Contributor Author

Providing @athei @xgreenx @SkymanOne and @cmichi are happy, this is ready to merge

Copy link
Contributor

@SkymanOne SkymanOne left a comment

Choose a reason for hiding this comment

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

Overall, looks very good, just few quick changes to be made.

CHANGELOG.md Outdated Show resolved Hide resolved
}

#[ink(message)]
pub fn noop(&self) {}
Copy link
Contributor

Choose a reason for hiding this comment

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

I presume since you must have a message in the ink! contacts, but you can't use sr25519_verify on the substrate-contracts-node, you left the function blank. Can you please either document it somehow, or double check that you can't use sr25519_verify with the substrate-contracts-node?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're correct, I left it blank due to the >0 function in an ink contract requirement. I wanted to provide a demonstration of the sr25519_verify function in the unit tests for a contract which would be the same usage as in production for a contract, thus making a good example. I can move it to run directly against substrate-contracts-node but this would drop the example.

In the meantime, I've added a comment documenting this

Copy link
Contributor

@SkymanOne SkymanOne left a comment

Choose a reason for hiding this comment

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

Looks better now. Happy to get it merged

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.

5 participants