Finish implementing initial Simulation interface #26

gjtrowbridge · 2020-02-11T21:04:59Z

This commit implements all of the methods from the initial Simulation interface.

Notes:

Some logic is new, some logic was ported from notion-server/src/ethereum/mod.rs. In a future diff, notion-server/src/ethereum/mod.rs will be removed entirely, and the server logic in notion-server will instead point to eth2/simulation/.
Some reasons for this move / refactor:
- Libraries used in implementing eth2-specific types were not compatible with libraries used by notion-server (eg. Rocket).
- It didn't make sense to have the simulation and the server wrapping it be tightly coupled long-term, this approach is much more modular, and eg. will support using the simulation in other contexts also (eg. other wrappers could use the underlying Simulation logic, devs could use the Simulation logic directly without going through a wrapper, etc).

Next Steps:

Update notion-server to use the Simulation logic in the eth2/simulation repository.
Remove Simulation logic from notion-server.
Verify that notion-server is working and satisfies all current user needs.
Write an SDK library that can be imported into 3rd party projects and which abstracts out communication with notion-server. This will allow other projects to easily communicate with notion-server without needing to "manually" write HTTP requests.
Continue updating the notion-server, SDK, and Simulation interfaces as-necessary as user needs change.

SamWilsn · 2020-02-12T21:25:17Z

eth2/simulation/src/simulation.rs

+        a: args::CreateExecutionEnvironment,
+    ) -> Result<EEIndex> {
+        // Create EE struct from args
+        let vl_wasm_code =


Is the vl_ prefix a convention from the lighthouse repository?

I think I'd prefer to see just wasm_code, and use the shorthand assignment in the ExecutionEnvironment initialization.

I agree, I'll just use wasm_code.

SamWilsn · 2020-02-12T21:29:20Z

eth2/simulation/src/lib.rs

+#[derive(Debug, Snafu)]
+pub enum Error {
+    #[snafu(display("{} exceeds max allowable length", what))]
+    MaxLengthExceeded { what: String },


Is MaxLengthExceeded always triggered from a ssz_types::Error?

Either way, I'd add a source field here, to give more context on the error.

Correct, currently MaxLengthExceeded is only triggered from a ssz_types::Error. However, I could envision it being useful for other types of errors in the future.

Though, unless I'm misuderstanding Snafu, I believe that 1) I can only use context()?; if I add a source field (unless I'm calling .context() on a function returning an Option), and 2) this error type will ONLY ever be able to be used for ssz_types::Error if I do that approach, which could be OK but if so I'd want to change the name from MaxLengthExceeded to something like SszTypeError or similar.

One last note: my original implementation did use Snafu's canonical .context(...)?; pattern, along with a source field. However, ssz_types::Error doesn't actually implement std::error::Error, which means that ssz_types::Error can't be used as a source error. Obviously, I can update the ssz_types::Error type to implement std:error::Error, but I opted not to do this immediately, since I anticipate that ssz_types will eventually be a crate instead of "owned code" in this repo. That said, prior to pushing this as a crate, I'm sure the ssz_types authors would make the Error type compliant with the std::error::Error trait.

TL;DR: I don't feel too strongly here, as I see it, there are 2 paths forward:

Leave it as-is, and update in the future if we want the granularity / lower-level visibility into the ssz_types error.

Change to have this use a source field now, which would entail:

Updating the ssz_types::Error field to implement the std:error:Error trait

Likely changing the name of this wrapper error from MaxLengthExceeded to something like SszTypeError, and have the display implementation of the wrapper just show the underlying display implementation of the ssz error (which I would have just added in the first bullet point). This way the error could wrap any of the ssz error types that may come back.

Either way works, and #2 is fairly easy to do. #2 has the advantage of the lower-level visibility, and would make it easier to give more info like what the max length was when it was exceeded. #1 is obviously already in use, and has the advantage of in my opinion being simpler and more readable.

My preference is #1 for now unless/until we have a compelling need for #2, but if you or others feel strongly #2 is easy enough to implement.

SamWilsn · 2020-02-12T21:30:54Z

eth2/simulation/src/simulation.rs

+    ) -> Result<EEIndex> {
+        // Create EE struct from args
+        let vl_wasm_code =
+            VariableList::new(a.wasm_code).map_err(|_| Error::MaxLengthExceeded {


You should be able to use snafu's ResultExt::context method for these types of conversions.

Same comment as above -- interested to hear your thoughts on my breakdown of the two options. As mentioned, I have a slight preference for this because it is simpler, uses std library methods, and IMO is more readable.

SamWilsn · 2020-02-12T21:35:52Z

eth2/simulation/src/simulation.rs

+            })?;
+
+        // For each shard, add the initial state to the shard
+        for shard_state in self.store.current_beacon_state.shard_states.iter_mut() {


I think I'd like to see a little more encapsulation here. Instead of reaching through multiple levels of struct's internal fields, create some helper methods. Should reduce the amount of duplicate code (especially error conversion.)

Thanks for the comment! I don't think I agree for this particular case, though it's possible I'm misunderstanding your point here. What kind of helper function do you envision?

My thoughts:

I don't expect any code duplication from this sort of thing -- chaining properties like this seems normal and readable, and current_beacon_state will always have shard_states, etc.

Not sure what you mean by error conversion -- there's no error conversion here. Let me know what I'm missing.

I could see this being applicable in other contexts, is there another line or example that you think could benefit from helper methods?

SamWilsn · 2020-02-12T21:40:55Z

eth2/simulation/src/simulation.rs

+            let wasm_code: Vec<u8> = execution_environment.wasm_code.clone().into();
+            let data: Vec<u8> = transaction.data.clone().into();


You should be able to omit the clone calls here, and the conversions to Vec. VariableList<u8> implements Deref<Target=[u8]>.

Wow, very clever. Thanks for the catch, will update to this.

SamWilsn · 2020-02-12T21:45:27Z

eth2/types/src/slot_epoch_root.rs

    pub fn new(slot: u64) -> Self {
        Self(slot)
    }
 }

+/// Convert from a hex string to a Root
+impl TryFrom<&str> for Root {


FromStr might be a more idiomatic approach, but I don't think this is incorrect.

SamWilsn · 2020-02-12T21:46:50Z

eth2/types/src/slot_epoch_root.rs

+
+    fn try_from(s: &str) -> Result<Self, Self::Error> {
+        let vec = Vec::from_hex(s).map_err(|e| {
+            return format!("cannot convert string to hex: {:?}", e);


Converting an Error into a String loses a lot of contextual information. Generally it'd be better to either return the original error type, or convert it to this crate's error type if you don't want to leak dependencies.

Thanks for the callout!

This may be me being fairly new to Rust, but I'm not sure I totally agree here. Certainly I agree that returning a string will make it so the underlying Error struct defined by the hex crate won't get passed along in the return value of try_from. That said, I think all the relevant context will be there, since the debug print will just print out the underlying error from the hex crate and include it in the try_from error message.

I don't feel strongly here at all, but to me it doesn't seem compelling to have the extra logic to either 1) define an Error type for the eth2/types library or 2) just return the underlying error from hex. Seems cleaner, simpler, and just as effective to have it as written.

Interested in reading your response on this :)

SamWilsn · 2020-02-12T21:47:36Z

eth2/types/src/slot_epoch_root.rs

+    }
+}
+
+impl EEIndex {


EeIndex would be more standard.

Awesome, thanks, will change!

villanuevawill · 2020-02-13T00:42:48Z

eth2/simulation/src/simulation.rs

+            .get(ee_index)
+            .unwrap();
+        assert_eq!(ee_state, &expected_post_state);
+    }


thanks for all the tests - helps see the interface.

jrhea · 2020-02-13T16:47:57Z

This is fantastic. The code is super easy to follow and the tests demonstrate how the interface will be used. The simulation interface is definitely functional, but it would be convenient if I didn't necessarily have to know about SSZ and other spec types in order to interact with it.

For example, when calling create_shard_block() I have to know about the ssz type VariableList in order to create a ShardTransaction. You could make this easier by either changing the CreatShardBlock struct to this:

    pub struct CreateShardBlock {
        pub shard: Shard,
        pub shard_transactions: Vec<u8>,
    }

or providing a helper method called create_shard_transaction() that accepts some bytes and an index.

gjtrowbridge · 2020-03-12T21:28:05Z

Closing this because all of this is included in #27

gjtrowbridge added 15 commits February 4, 2020 11:14

Update types to reflect changes in the spec.

e184b27

Save progress...

c429ffe

Clean up error handling a bit, start filling in methods.

0dd70f8

More stuff in create_shard_block...

fdb5a01

Finish shard block func.

694a77b

Fill in get_ee.

fab29b9

Fill in get ee state method.

e0d59b8

Implement get_shard_block.

49ef476

Implement get_shard_state.

0dc693e

Implement tests for create, get ee and for simulation.new.

00730d0

Add test for running transactions.

c8e2668

Finish tests.

3048eac

Add some comments.

79a7a69

More small changes.

59d497f

Run cargo fmt

b14447b

gjtrowbridge requested review from villanuevawill, adietrichs, jrhea, lightclient and SamWilsn February 11, 2020 21:05

SamWilsn requested changes Feb 12, 2020

View reviewed changes

Address review comments.

084755a

gjtrowbridge requested a review from SamWilsn February 12, 2020 23:27

Change from vl_wasm_code to wasm_code.

42371d3

villanuevawill reviewed Feb 13, 2020

View reviewed changes

gjtrowbridge closed this Mar 12, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Finish implementing initial Simulation interface #26

Finish implementing initial Simulation interface #26

gjtrowbridge commented Feb 11, 2020 •

edited

Loading

SamWilsn Feb 12, 2020

gjtrowbridge Feb 12, 2020

SamWilsn Feb 12, 2020

gjtrowbridge Feb 12, 2020 •

edited

Loading

SamWilsn Feb 12, 2020

gjtrowbridge Feb 12, 2020

SamWilsn Feb 12, 2020

gjtrowbridge Feb 12, 2020

SamWilsn Feb 12, 2020

gjtrowbridge Feb 12, 2020

SamWilsn Feb 12, 2020

SamWilsn Feb 12, 2020

gjtrowbridge Feb 12, 2020

SamWilsn Feb 12, 2020

gjtrowbridge Feb 12, 2020

villanuevawill Feb 13, 2020

jrhea Feb 13, 2020

jrhea commented Feb 13, 2020

gjtrowbridge commented Mar 12, 2020

		let wasm_code: Vec<u8> = execution_environment.wasm_code.clone().into();
		let data: Vec<u8> = transaction.data.clone().into();

Finish implementing initial Simulation interface #26

Finish implementing initial Simulation interface #26

Conversation

gjtrowbridge commented Feb 11, 2020 • edited Loading

Notes:

Next Steps:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gjtrowbridge Feb 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrhea commented Feb 13, 2020

gjtrowbridge commented Mar 12, 2020

gjtrowbridge commented Feb 11, 2020 •

edited

Loading

gjtrowbridge Feb 12, 2020 •

edited

Loading