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

no-std implementation #167

Open
wants to merge 37 commits into
base: main
Choose a base branch
from

Conversation

dharjeezy
Copy link

@dharjeezy dharjeezy commented Dec 27, 2022

Implement alloc crate
Implement hashbrown crate
Made use of milagro crate for the BLS scheme.

closes #166

started implementing display for errors
@dharjeezy dharjeezy marked this pull request as draft December 27, 2022 21:39
@Wizdave97
Copy link

Looking good.

Cargo.toml Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
src/clock.rs Outdated Show resolved Hide resolved
@dharjeezy dharjeezy requested a review from Wizdave97 January 5, 2023 11:54
@dharjeezy dharjeezy marked this pull request as ready for review January 5, 2023 23:18
@dharjeezy dharjeezy changed the title started implementing no-std support no-std implementation Jan 5, 2023
@dharjeezy
Copy link
Author

@ralexstokes can you please, help review this. Thanks

Copy link
Owner

@ralexstokes ralexstokes 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 the delay on this and thanks for opening the PR!

there are a few high-level things to note before moving further:

  1. use of blst or some alternative
  2. note that you edited some code that falls under the spec code gen you can read about here: https://github.com/ralexstokes/ethereum-consensus#notes and the point is that we shouldn't be edited those files manually

otherwise I left some comments on the PR

@@ -202,8 +201,9 @@ pub fn process_deposit<

let public_key = &deposit.data.public_key;
let amount = deposit.data.amount;
let cloned_validators = state.validators.clone();
Copy link
Owner

Choose a reason for hiding this comment

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

can we avoid a full clone here?

Copy link
Author

Choose a reason for hiding this comment

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

For this, if i don't clone, mutable borrow occurs when i try to iterate over the validators and put them into a HashSet

@@ -315,9 +315,9 @@ pub fn process_sync_aggregate<
let proposer_reward =
participant_reward * PROPOSER_WEIGHT / (WEIGHT_DENOMINATOR - PROPOSER_WEIGHT);

let cloned_validators = state.validators.clone();
Copy link
Owner

Choose a reason for hiding this comment

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

same -- why do we need to change this in the first place?

Copy link
Author

Choose a reason for hiding this comment

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

This is due to the iteration and collecting into the Hashbrown's Hashset crate

src/altair/state_transition/block_processing.rs Outdated Show resolved Hide resolved
Comment on lines 120 to 125
#[cfg(not(feature = "serde"))]
let i_bytes: [u8; 4] = (i.saturating_div(32)).to_le_bytes();

#[cfg(feature = "serde")]
let i_bytes: [u8; 8] = (i.saturating_div(32)).to_le_bytes();

Copy link
Owner

Choose a reason for hiding this comment

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

what is happening here?

Copy link
Author

@dharjeezy dharjeezy Jan 19, 2023

Choose a reason for hiding this comment

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

So, I am trying to make this repo compile to target --target=wasm32-unknown-unknown
I get this error when i try to compile this section of code to that target

 let i_bytes: [u8; 8] = (i/32).to_le_bytes();
    |                      -------   ^^^^^^^^^^^^^^^^^^^^ expected an array with a fixed size of 8 elements, found one with 4 elements
    |                      |
    |                      expected due to this

Hence the reason why i used the feature tag to separate the std and no-std implementation

src/crypto.rs Outdated
use crate::primitives::Bytes32;
#[cfg(feature = "serde")]
use crate::serde::{try_bytes_from_hex_str, HexError};
use crate::ssz::ByteVector;
use blst::{min_pk as bls_impl, BLST_ERROR};
use error_chain::ChainedError;
use milagro_bls::{AggregatePublicKey, AggregateSignature, AmclError};
Copy link
Owner

Choose a reason for hiding this comment

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

this may be its own sidequest but I would strongly prefer to use blst

do you know why it is not no-std compatible?

Copy link
Author

@dharjeezy dharjeezy Jan 19, 2023

Choose a reason for hiding this comment

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

According to the blst crate, it is not no-std compatible hence the usage of milagro_bls crate which is no-std compatible

Choose a reason for hiding this comment

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

blst crate is only supports compilation to wasm32-unknown-emscripten because of it's dependence on some FFIs, but wasm32-unknown-unknown is the target we'll need in the future.
More details here rustwasm/team#291

@@ -364,8 +364,9 @@ pub fn process_deposit<

let public_key = &deposit.data.public_key;
let amount = deposit.data.amount;
let cloned_validators = state.validators.clone();
Copy link
Owner

Choose a reason for hiding this comment

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

same question here

Copy link
Author

Choose a reason for hiding this comment

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

For this, if i don't clone, mutable borrow occurs when i try to iterate over the validators and put them into a HashSet

pub const DEPOSIT_CONTRACT_TREE_DEPTH: usize = 2usize.pow(5);
pub const DEPOSIT_CONTRACT_TREE_DEPTH: usize = 2usize.saturating_pow(5);
Copy link
Owner

Choose a reason for hiding this comment

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

seems unnecessary? if there is a compatibility issue with pow then we can just write 32usize here

Comment on lines -34 to +35
pub const DEPOSIT_DATA_LIST_BOUND: usize = 2usize.pow(DEPOSIT_CONTRACT_TREE_DEPTH as u32);
pub const DEPOSIT_DATA_LIST_BOUND: usize =
2usize.saturating_pow(DEPOSIT_CONTRACT_TREE_DEPTH as u32);
Copy link
Owner

Choose a reason for hiding this comment

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

if we can't just use pow then we can just write out the integer as usize

Copy link
Author

@dharjeezy dharjeezy Jan 19, 2023

Choose a reason for hiding this comment

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

Yh! so this is due to the compilation target wasm32-unknown-unknown
But if i do 2^32 which is 4294967296, I am going to end up with an overflowing literal

Comment on lines -27 to +31
pub const VALIDATOR_REGISTRY_LIMIT: usize = 2usize.pow(40);
pub const VALIDATOR_REGISTRY_LIMIT: usize = 2usize.saturating_pow(40);
pub const BASE_REWARD_FACTOR: u64 = 64;
pub const WHISTLEBLOWER_REWARD_QUOTIENT: u64 = 512;
pub const PROPOSER_REWARD_QUOTIENT: u64 = 8;
pub const INACTIVITY_PENALTY_QUOTIENT: u64 = 2u64.pow(26);
pub const INACTIVITY_PENALTY_QUOTIENT: u64 = 2u64.saturating_pow(26);
Copy link
Owner

Choose a reason for hiding this comment

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

not sure why we changed these

Copy link
Author

@dharjeezy dharjeezy Jan 19, 2023

Choose a reason for hiding this comment

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

Yh! so this is due to the compilation target wasm32-unknown-unknown

src/prelude.rs Outdated
@@ -0,0 +1,47 @@
mod core {
Copy link
Owner

Choose a reason for hiding this comment

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

for consistency's sake, I'd prefer to follow the pattern from ralexstokes/ssz-rs#25

i.e. a mod lib with this inside and then we can use crate::lib::*; elsewhere

Copy link
Author

Choose a reason for hiding this comment

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

I feel this prelude looks cleaner

Copy link
Author

Choose a reason for hiding this comment

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

Gone ahead to do as adviced. @ralexstokes

…ensus into dami/no-std-support

� Conflicts:
�	src/altair/state_transition/block_processing.rs
�	src/altair/state_transition/epoch_processing.rs
�	src/state_transition/error.rs
@dharjeezy dharjeezy requested review from ralexstokes and removed request for Wizdave97 January 19, 2023 14:58
dharjeezy and others added 9 commits January 22, 2023 00:23
use lib for no-std importss
…ensus into dami/no-std-support

� Conflicts:
�	Cargo.toml
�	src/altair/genesis.rs
�	src/altair/helpers.rs
�	src/altair/state_transition/block_processing.rs
�	src/altair/state_transition/epoch_processing.rs
�	src/bellatrix/genesis.rs
�	src/crypto.rs
�	src/lib.rs
�	src/phase0/genesis.rs
@@ -117,7 +117,12 @@ pub fn compute_proposer_index<
loop {
let shuffled_index = compute_shuffled_index(i % total, total, seed, context)?;
let candidate_index = indices[shuffled_index];
#[cfg(not(feature = "serde"))]
let i_bytes: [u8; 4] = (i / 32).to_le_bytes();

Choose a reason for hiding this comment

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

I'm curious what is the purpose of this? It seems to be broken at the moment if the "serde" feature is not enabled

Choose a reason for hiding this comment

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

@dharjeezy why is there a serde feature flag here?

Copy link
Author

Choose a reason for hiding this comment

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

So, our target compilation is wasm32-unknown-unknown
I had to include the feature flag because of that else it errors out

@Wizdave97 @willemolding

Choose a reason for hiding this comment

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

Collecting into a dynamic type like a Vec might work better in this case.

Copy link

@willemolding willemolding Feb 25, 2023

Choose a reason for hiding this comment

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

Maybe better to use #[cfg(target_arch = "wasm32")] if it is a wasm specific thing? As it stands it errors out building for x86 with serde disabled

Choose a reason for hiding this comment

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

Oh wait an even better solution is to remove the intermediate variable entirely e.g.

hash_input[32..].copy_from_slice(&((i / 32) as u64).to_le_bytes());

This will build either way regardless of the size of a u64 on the given target

dharjeezy and others added 6 commits February 25, 2023 11:39
…ensus into dami/no-std-support

� Conflicts:
�	Cargo.toml
�	src/bytes.rs
�	src/crypto.rs
�	src/serde.rs
�	src/ssz/byte_list.rs
�	src/ssz/byte_vector.rs
�	src/state_transition/error.rs
std related clock features
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.

Support no-std
5 participants