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

[WIP] Subxt-Core Crate #1411

Closed
wants to merge 47 commits into from
Closed

Conversation

tadeohepperle
Copy link
Contributor

relates to #1394

This PR does a few things:

  • make subxt-signer no-std compatible
  • create a new subxt-core crate that is no-std compatible by default and offers a subset of subxt's functionality.
  • use this subxt-core crate in subxt-signer and in subxt.

What is not working yet:

  • The critical_section feature of the once_cell crate used in subxt-signer results in linker errors when run in no-std.
  • Subxt compiles, but subxt examples are not all adjusted yet
  • Interface and imports need to be discucced and made nicer, some messiness has to be removed
  • Docs, especially book needs to be adjusted

fn deref(&self) -> &Self::Target {
static ONCE: once_cell::sync::OnceCell<Secp256k1<All>> = once_cell::sync::OnceCell::new();
ONCE.get_or_init(|| Secp256k1::new())
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When in an std context we could also add a line here to seed the Secp256k1 by some Rng.
Will not be available in WASM and no-std though.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd copy what Substrate does here to be on the safe side, which is to do:

#[cfg(feature = "std")]
let context = SECP256K1;
#[cfg(not(feature = "std"))]
let context = Secp256k1::signing_only();

In the sign/from_seed fns to select which context to use ie https://github.com/paritytech/polkadot-sdk/blob/d54412ce8bef113c7a515b3a08851e7d77553df5/substrate/primitives/core/src/ecdsa.rs#L390 and https://github.com/paritytech/polkadot-sdk/blob/d54412ce8bef113c7a515b3a08851e7d77553df5/substrate/primitives/core/src/ecdsa.rs#L460

Since only two places this context is used anyway, I think that's fine :)

@tadeohepperle
Copy link
Contributor Author

tadeohepperle commented Feb 7, 2024

I am having trouble getting no_std testing working properly.
I setup a crate testing/no-std-tests that pulls in subxt-metadata, subxt-signer and subxt-core. You can try to build it with either of these:

cargo run 
cargo build --target thumbv7em-none-eabi 
cargo build --target aarch64-unknown-none

With cargo run (the native default target), we get linker errors. The curious thing is that the linker errors on my linux machine are different from the erros James is getting on a macos target.

With the thumbv7em-none-eabi and aarch64-unknown-none targets the problem is, that parts of the rust core prelude (e.g. Some and None variants, Iterator trait, etc. are not found by the compiler).

@@ -91,8 +103,9 @@ impl Keypair {
/// keypair.sign(b"Hello world!");
/// ```
pub fn from_phrase(mnemonic: &bip39::Mnemonic, password: Option<&str>) -> Result<Self, Error> {
let big_seed = seed_from_entropy(&mnemonic.to_entropy(), password.unwrap_or(""))
.ok_or(Error::InvalidSeed)?;
let (arr, len) = mnemonic.to_entropy_array();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice catch to avoid an allocation anyway :)

@@ -192,31 +206,35 @@ pub fn verify<M: AsRef<[u8]>>(sig: &Signature, message: M, pubkey: &PublicKey) -
};
let message_hash = sp_core_hashing::blake2_256(message.as_ref());
let wrapped = Message::from_digest_slice(&message_hash).expect("Message is 32 bytes; qed");
signature.verify(&wrapped, &public).is_ok()
GlobalSecp256K1Context
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah and here you can use https://docs.rs/secp256k1/latest/secp256k1/struct.Secp256k1.html#method.verification_only when in no-std rather than signing_only, I think!


# The getrandom package is used via schnorrkel. We need to enable the JS
# feature on it if compiling for the web.
web = ["getrandom/js", "subxt?/web"]
native = ["subxt?/native"]
web = ["getrandom/js"]
Copy link
Collaborator

@jsdw jsdw Feb 8, 2024

Choose a reason for hiding this comment

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

Nice to be able to ditch the native feature flag!

Does everything work when "web" is enabled and "std" isn't, I wonder?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh that is a good question, I need to try that.

secrecy = { workspace = true }

# We only pull this in because `std::sync::OnceLock` is not available in no-std contexts.
once_cell = { workspace = true, default-features = false, features = ["alloc", "critical-section"] }
Copy link
Collaborator

Choose a reason for hiding this comment

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

critical-section uses this crate to provide the only-one-thread-at-a-time behaviour:

https://crates.io/crates/critical-section

This crate requires that you enable different features to support different no-std targets (perhaps this explains why you hit a compile error in no-std).

I'm not sure whether this is a good idea to use or not.

If we do use it, we should def enable critical-section/std when std feature is enabled though, so that it uses a standard mutex instead of anything strange in std cases. We'd also need to document to users to go read the critical-section docs (like once_cell does) because they would need to depend on critical-section themselves and add the relevant features for the targets they are trying to support.

But, all of this is just to enable the provided dev accounts at the end of the day, so perhaps the best route to take is revert to using std::sync::OnceLock, remove once_cell dep and just don't globally instantiate the dev accounts when in no-std mode. So we could alter the once_static macro thing to just use the OnceLock global when in std and recreate the account (run the $expr) every time when in no-std? (and then document on the macro that in no-std we run the expr every time)

subxt/src/lib.rs Outdated
Comment on lines 68 to 71
// pub use subxt_core::config;
// pub use subxt_core::config::{Config, PolkadotConfig, SubstrateConfig};
// pub use subxt_core::dynamic;
pub use subxt_core::*;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd prefer to be really explicit here so that it's obvious exactly what we're exporting :)

@@ -168,7 +170,8 @@ impl<CallData: EncodeAsFields> TxPayload for Payload<CallData> {
.map(|f| scale_encode::Field::new(f.ty.id, f.name.as_deref()));

self.call_data
.encode_as_fields_to(&mut fields, metadata.types(), out)?;
.encode_as_fields_to(&mut fields, metadata.types(), out)
.expect("The fields are valid types from the metadata, qed;");
Copy link
Collaborator

@jsdw jsdw Feb 14, 2024

Choose a reason for hiding this comment

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

There are various other reasons why the call data might not properly encode though I think; better to keep the ??

Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess in follow up PRs we can add more functionality; there's a bunch of TX stuff we could have in core too I think; most of the stuff around signing and encoding calls ready to send (just not the "online" bits eg actually sending them)

@@ -0,0 +1,3 @@
# Subxt-Core

This library provides core functionality using in `subxt` and `subxt-signer`. It should be no-std compatible.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think for now we should add a warning here and in the lib docs that this is not stable may still change significantly between releases, so better to rely on subxt where possible: there's a lot to look at and think about with this new crate, but at the same time it'd be good to feel like we can release it now and iterate on it without worrying too much about breaking things :)

@jsdw
Copy link
Collaborator

jsdw commented Feb 14, 2024

Interface and imports need to be discucced and made nicer, some messiness has to be removed

I'd be curious what you are currently thinking is messy and could be improved; perhaps we should have a walk through of this stuff at some point to get a better feeling for it all as there is a lot to think about in this PR :)

tadeohepperle and others added 17 commits February 14, 2024 10:42
…ory. (#1431)

* delete substrate and polkadot binaries

* use nodes should move instead of copy and remove the archive
* add codgen mod to the module tree

* port codegen tests to scale-typegen

* remove test flag
* no-std tests and porting of subxt-metadata

* update pipeline

* fix generate custom metadata test

* fix cargo run command

* adjust pipeline

* remove prelude from subxt-metadata

* revert autoformatting of Cargo.toml

* remove alloc::format! again, still causes linker errors

* add no-std-build for thumbv7em-none-eabi target

* remove std feature flag

* remove libc and add small readme with test instructions

* change ci for nightly no std
@tadeohepperle
Copy link
Contributor Author

I started a new branch, because it was easier than trying to solve all conflicts, correctly. To be continued here: #1466

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.

2 participants