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

implement ecdsa sign, verify, sign_prehashed and verify_prehashed #2120

Merged
merged 7 commits into from
Mar 7, 2022

Conversation

melekes
Copy link
Contributor

@melekes melekes commented Mar 4, 2022

Closes #1968

Note this PR implements ecdsa_sign and introduces ecdsa_verify function, which are both out of the scope of the issue above.

@melekes melekes self-assigned this Mar 4, 2022
Cargo.toml Outdated Show resolved Hide resolved
src/executor/host.rs Outdated Show resolved Hide resolved
let (sig, ri) =
libsecp256k1::sign(&message, &expect_pointer_constant_size!(1, 32).into());

// NOTE: the function returns 2 slices: signature and recovery ID (AS A SLICE)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

should we return a single slice instead? signature (64 bytes) + 1 recovery byte

@melekes
Copy link
Contributor Author

melekes commented Mar 4, 2022

@tomaka can I also implement ecdsa verity (should be straightforward now that we have verify_prehashed)?

if we fail to parse secret key
@github-actions
Copy link
Contributor

github-actions bot commented Mar 4, 2022

twiggy diff report

Difference in .wasm size before and after this pull request.


 Delta Bytes │ Item
─────────────┼────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
      +66512 ┊ data[0]
      +46721 ┊ smoldot::executor::host::ReadyToRun::run_once::h588a43ce7bb9bec7
      +42088 ┊ smoldot::json_rpc::methods::MethodCall::from_defs::h08bdee9af59cef7f
      -42088 ┊ smoldot::json_rpc::methods::MethodCall::from_defs::h44ff5fc34961553a
      -39988 ┊ smoldot::executor::host::ReadyToRun::run_once::h36ac176a2ba9b38e
      +13035 ┊ <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll::h3a061a7a11693ceb
      -13035 ┊ <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll::h99900cb09ebfa9c0
      +12074 ┊ smoldot::network::service::ChainNetwork<TNow>::next_event::{{closure}}::h30560f19388ced0d
      -12074 ┊ smoldot::network::service::ChainNetwork<TNow>::next_event::{{closure}}::he4aaa26d26cbbfb2
      +11465 ┊ <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll::h66d64f78fd502255
      -11465 ┊ <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll::hfe120633e113f3f7
      +10525 ┊ <parity_wasm::elements::ops::Instruction as parity_wasm::elements::Deserialize>::deserialize::h0bbcef032d608a86
      -10525 ┊ <parity_wasm::elements::ops::Instruction as parity_wasm::elements::Deserialize>::deserialize::h5fe00cff0cc1bad7
      -10172 ┊ smoldot_light_base::Client<TChain,TPlat>::add_chain::h386d3178de44e3b1
      +10172 ┊ smoldot_light_base::Client<TChain,TPlat>::add_chain::hc7322fbe2c61cf84
      -10109 ┊ <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll::h00f08a3413b42ea4
      +10109 ┊ <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll::h33e133753a2f823a
       +9937 ┊ smoldot::libp2p::connection::established::substream::Substream<TNow,TRqUd,TNotifUd>::read_write2::h5b5f779dde123186
       -9937 ┊ smoldot::libp2p::connection::established::substream::Substream<TNow,TRqUd,TNotifUd>::read_write2::h7439ec84acda52b3
       -9384 ┊ smoldot::json_rpc::methods::Response::to_json_response::h6706eac6a04ee6f4
      +31263 ┊ ... and 25456 more.
     +101733 ┊ Σ [25476 Total Rows]

@tomaka
Copy link
Contributor

tomaka commented Mar 4, 2022

can I also implement ecdsa verity (should be straightforward now that we have verify_prehashed)?

Yeah go ahead

Copy link
Contributor

@tomaka tomaka 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, modulo avoiding the copy!

@tomaka
Copy link
Contributor

tomaka commented Mar 4, 2022

Note that I haven't properly checked if the behavior is the same as in Substrate, but it seems good.

Unfortunately none of the crypto functions in host.rs are tested because writing unit tests for that is a huge effort.

I know that a team from w3f is currently writing a test framework where they give runtimes to a client and check if said client behaves properly, but it's not ready yet.

@melekes
Copy link
Contributor Author

melekes commented Mar 4, 2022

Unfortunately none of the crypto functions in host.rs are tested because writing unit tests for that is a huge effort.

yeah, I figured

I know that a team from w3f is currently writing a test framework where they give runtimes to a client and check if said client behaves properly, but it's not ready yet.

this sounds like exactly what is needed!

@melekes melekes changed the title implement ecdsa sign, sign_prehashed and verify_prehashed implement ecdsa sign, verify, sign_prehashed and verify_prehashed Mar 4, 2022
@melekes melekes marked this pull request as ready for review March 4, 2022 14:46
@melekes melekes added the automerge Automatically merge pull request as soon as possible label Mar 7, 2022
@mergify mergify bot merged commit 00ba3c8 into main Mar 7, 2022
@mergify mergify bot deleted the anton/1968-backport-ecdsa_sign_prehashed-in-sp-io branch March 7, 2022 08:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Automatically merge pull request as soon as possible
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Backport "expose ecdsa_sign_prehashed in sp-io"
2 participants