Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Add an optional verifier to crowdloan #2248

Merged
merged 14 commits into from
Feb 26, 2021

Conversation

xlc
Copy link
Contributor

@xlc xlc commented Jan 11, 2021

This will allow crowd loan owner to enforce any custom policy offchain for contributors by requiring a valid signature before accept a contribution.

TODOs:

  • docs
  • test
  • think about replay attack

Replaces #2230

@shawntabrizi shawntabrizi added B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels Jan 12, 2021
@xlc
Copy link
Contributor Author

xlc commented Jan 26, 2021

I've update the code so it benchmark with the signature case, so it will overestimate for non signature case but I think that's fine.

But because crowdloan is not added to any runtime so I can't run the benchmark code to verify if my change actually works.

@xlc
Copy link
Contributor Author

xlc commented Jan 26, 2021

Actually my benchmark code doesn't work because the signing API is not available in wasm unless full_crypto feature is enabled. Is there any workaround or we just need to enable full_crypto when benchmarking? @shawntabrizi

@bkchr
Copy link
Member

bkchr commented Feb 1, 2021

Actually my benchmark code doesn't work because the signing API is not available in wasm unless full_crypto feature is enabled. Is there any workaround or we just need to enable full_crypto when benchmarking? @shawntabrizi

What signing api do you need?

@xlc
Copy link
Contributor Author

xlc commented Feb 1, 2021

https://github.com/paritytech/substrate/blob/26f3d6826a181674257b8e9fc708aa04b49dc73c/primitives/core/src/ed25519.rs#L424-L425

So I can sign something and get it verified in the benchmarked code

@bkchr
Copy link
Member

bkchr commented Feb 1, 2021

@xlc
Copy link
Contributor Author

xlc commented Feb 1, 2021

How do I inject key into the crypto store?

@bkchr
Copy link
Member

bkchr commented Feb 2, 2021

@xlc
Copy link
Contributor Author

xlc commented Feb 9, 2021

@bkchr CI failed with error:

---- crowdloan::benchmarking::tests::test_benchmarks stdout ----
783thread 'crowdloan::benchmarking::tests::test_benchmarks' panicked at 'No `keystore` associated for the current context!', /usr/local/cargo/git/checkouts/substrate-7e08433d4c370a21/09ba69f/primitives/io/src/lib.rs:459:14
784stack backtrace:
785   0: rust_begin_unwind
786             at /rustc/e1884a8e3c3e813aada8254edfa120e85bf5ffca/library/std/src/panicking.rs:495:5
787   1: core::panicking::panic_fmt
788             at /rustc/e1884a8e3c3e813aada8254edfa120e85bf5ffca/library/core/src/panicking.rs:92:14
789   2: core::option::expect_failed
790             at /rustc/e1884a8e3c3e813aada8254edfa120e85bf5ffca/library/core/src/option.rs:1260:5
791   3: <&mut dyn sp_externalities::Externalities as sp_io::crypto::Crypto>::ed25519_generate_version_1
792   4: std::thread::local::LocalKey<T>::with
793   5: tracing::span::Span::in_scope
794   6: sp_io::crypto::ed25519_generate_version_1
795   7: sp_io::crypto::ed25519_generate
796   8: polkadot_runtime_common::crowdloan::benchmarking::create_fund
797   9: <polkadot_runtime_common::crowdloan::benchmarking::SelectedBenchmark as frame_benchmarking::utils::BenchmarkingSetup<T>>::instance
798  10: polkadot_runtime_common::crowdloan::benchmarking::test_benchmark_create::{{closure}}
799  11: std::thread::local::LocalKey<T>::with
800  12: polkadot_runtime_common::crowdloan::benchmarking::tests::test_benchmarks
801  13: core::ops::function::FnOnce::call_once
802             at /rustc/e1884a8e3c3e813aada8254edfa120e85bf5ffca/library/core/src/ops/function.rs:227:5

@xlc
Copy link
Contributor Author

xlc commented Feb 23, 2021

I have the keystone working in test but not sure if it will work in runtime benchmark.

Also another issue is in the real runtime, we will likely be using MultiSignature but TestSignature is used in mock.

But there is no appropriate interface to create signature so I am using let sig = T::ContributionSignature::decode(&mut &sig.encode()[..]).unwrap(); but it only works for MultiSignature, not TestSignature.

Maybe we need to add some new benchmarking only method to trait Verify? @shawntabrizi

@xlc
Copy link
Contributor Author

xlc commented Feb 23, 2021

Updated to only support ed25519 to make it simpler

@xlc
Copy link
Contributor Author

xlc commented Feb 23, 2021

This is ready for review

@gavofyork gavofyork added the A0-please_review Pull request needs code review. label Feb 25, 2021
@gavofyork
Copy link
Member

@shawntabrizi you ok with this?

@shawntabrizi
Copy link
Member

@xlc ive made it support MultiSigner and MultiSignature for ed25519, sr25519, and edcsa

@bkchr bkchr merged commit 01657e9 into paritytech:master Feb 26, 2021
@xlc xlc deleted the crowdloan-permission-2 branch February 26, 2021 08:43
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants