Notary #31

ChosunOne · 2018-04-14T00:37:02Z

I feel that the notary branch is ready to be integrated into the develop branch. Major improvements include the following:

SMC Listener integration into the main binary
Thread refactor
Skeletal Notary integration
Full unit test coverage of implemented features

There are still a lot of questions I have about functions related to the notary, such as how a notary selects a proposal to vote on and what information is needed in the vote message, but it is at the point where merging this will allow more parallel development of the rest of the project.

…ed trees, and have a proper test for storing collations in the notary.

# Conflicts: # README.md # src/lib.rs # src/notary.rs # src/smc_listener.rs

…drops into notary

…_drops into notary # Conflicts: # README.md # src/notary.rs

…diamond_drops into notary" This reverts commit 606e1a7, reversing changes made to 0435f49.

fixed typo

…mports into test module.

…nts" * Rearranged order of imported files, and struct attributes for consistency * Add `#[derive(Debug)]` to enums to allow use with `println!` with `{:?}` * Add meaningful message to `panic!` calls * Fix indentation of `notary.run();` * Change variable names used in collation/header.rs tests to be more readable and consistent with implementation * Add `id` attribute to `Notary` and `Proposer` structs for identification when debugging the client, such as knowing what notary id we are storing a collation into * Add `println!` for debugging received messages and current thread in SMC listener * Add `generate_notary()` function to remove code duplication in notary tests * Update SMC registration function to be notary or proposer specific, and to return primative boolean value with meaningful success or error logs * Change `get_selected_notaries()` function to accept `shard_id` argument of `ethereum_types::U256` instead of `usize` for consistency withe the rest of the code * Add `register_proposer_address()` since there will be both a notary registry and a proposer registry in the SMC Contract according to the Phase 1 Sharding spec * Change notary test function names to be more explicit

Reference: https://rust-lang-nursery.github.io/rust-cookbook/logging.html#logging

…minal

jamesray1 · 2018-04-16T04:10:05Z

src/collation/header.rs

+}
+
+// A crude way of converting the ethereum_types::U256 to a u8 byte array to be hashed.  Suggestions to improve this are desired. 
+fn u256_to_bytes32(u256: ethereum_types::U256) -> [u8; 32] {


I think we should use https://docs.rs/ethereum-types/0.3.1/ethereum_types/struct.U256.html#method.as_u32.

u32 is only 32 bits (4 bytes), while a u256 is 256 bits (32 bytes), the function turns the u256 into a raw 32-byte array, which is 256 bits.

Ah, my mistake.

jamesray1 · 2018-04-16T04:27:54Z

src/lib.rs

+            }
+            match notary_thread.handle.unwrap().join() {
+                Ok(x) => { println!("Successful notary thread join {:?}", x); () },
+                Err(e) => { panic!("Failed notary thread join {:?}", e); }


You could probably avoid duplication in these above two functions by having one that passes proposer or notary as a string to these outputs as well as log them.

jamesray1 · 2018-04-16T04:32:46Z

src/notary.rs

+    /// manager_listener: mpsc::Receiver<client_thread::Command>
+    /// 
+    /// The smc_listener allows the Notary to receive messages from the SMC Listener, and the manager_listener allows the thread to receive commands from outside the thread.
+    pub fn new(smc_listener: mpsc::Receiver<message::Message>, manager_listener: mpsc::Receiver<client_thread::Command>) -> Notary {


Let's break these lines up as they are too long.

jamesray1 · 2018-04-16T04:40:51Z

src/notary.rs

-    fn store_proposal(&self, collation: collation::collation::Collation) {}
+    fn store_proposal(&mut self, proposal: collation::Collation) {
+        self.proposal_vectors.entry(self.shard_id).or_insert(vec![]);
+        let vector = self.proposal_vectors.get_mut(&self.shard_id).unwrap();


We should avoid using unwrap and handle the error, although of course the Collation.body is undefined (not in a common spec), so that can be done once we have it.

The only error case I can think of is when the shard id isn't in the hashmap, which is addressed in the preceding line by putting an empty vector if the shard_id doesn't have an entry.

jamesray1 · 2018-04-16T04:44:18Z

src/notary.rs

+    use collation::body;
+
+    fn generate_genesis_collation() -> collation::Collation {
+        let g_shard_id = ethereum_types::U256::from_dec_str("0").unwrap();


This will need to vary according to the actual shard ID, which could be 1 to 99.

I'll make this function a little more flexible

jamesray1 · 2018-04-16T04:48:16Z

src/notary.rs

+        notary.store_collation(genesis_collation);
+        notary.store_collation(first_collation);
+
+        // Check that the operations succeded


jamesray1 · 2018-04-16T04:54:07Z

src/smc_listener.rs

+    fn it_registers() {
+        let (tx, rx) = mpsc::channel();
+        let smc = SMCListener::new(tx);
+        let addr_bytes: [u8; 20] = [0x22, 0xFF, 0x31, 0x10, 0xA2, 0x82, 0xc1, 0x19, 0x77, 0x36, 0xb3, 0xfC, 0xe3, 0x4a, 0xD4, 0xFc, 0x5e, 0xEe, 0x75, 0xc8];


Again, let's break these long lines.

jamesray1 · 2018-04-16T04:57:04Z

tests/cli_test.rs

@@ -1,103 +0,0 @@
-extern crate diamond_drops;


I still think we should have integration tests in /tests/ and unit tests in /src/.

I agree, the tests there were actually just unit tests of the parse_cli_args function.

… instead of u256_to_bytes32

…unction so tests pass

Reference: https://github.com/daboross/fern/blob/master/examples/pretty-colored.rs

jamesray1

Reduce line lengths to 100 characters max.

jamesray1 · 2018-04-16T08:55:50Z

src/cli/args.rs

@@ -6,7 +6,7 @@ enum ConfigType {
    Nil
 }

-pub fn parse_args(args: Vec<String>) -> Result<config::Config, &'static str> {
+pub fn parse_cli_args(args: Vec<String>) -> Result<config::Config, &'static str> {


Can you add docstrings for all public functions?

Yeah will do

…ions.

feat: Refactor notary. Detect when running cargo test to conditionally run thread termination code. Setup logger

Notary

Josiah Evans and others added 30 commits April 9, 2018 18:00

added to the tree implementation.

7255b98

removed premature tests, added calculation of header hash.

7515c71

updated header to reflect most recent sharding spec

8afa56c

included index tree and renamed collator to notary

bfabcff

updated notary functions to be in line with new minimum sharding spec.

84d0596

updated the smc listener for the new sharding spec

76194c0

modified collators to store collation vectors instead of trees, remov…

2a8d518

…ed trees, and have a proper test for storing collations in the notary.

added store collation functionality to the notary.

c78c457

Merge remote-tracking branch 'origin/develop' into notary

962d9ed

Merge branch 'basic_cli' into notary

86e71ad

# Conflicts: # README.md # src/lib.rs # src/notary.rs # src/smc_listener.rs

updated launch.json

e364fe2

updated message name and improved u256_to_bytes32

2583d55

ignore tests that are stubs or depend on the SMC.

86bfe73

Add src/bin, bin/bin.rs

d380e1d

add docstring

0435f49

Merge branch 'notary' of https://github.com/Drops-of-Diamond/diamond_…

606e1a7

…drops into notary

Merge branch 'develop' of https://github.com/Drops-of-Diamond/diamond…

e3f5d0f

…_drops into notary # Conflicts: # README.md # src/notary.rs

Added ClientThread for lib.rs refactor

f0afe08

Revert "Merge branch 'notary' of https://github.com/Drops-of-Diamond/…

1bb9d91

…diamond_drops into notary" This reverts commit 606e1a7, reversing changes made to 0435f49.

Update client_thread.rs

b7ed59e

fixed typo

stop tracking ml.dot

4b49bf6

add ignore tags to unfinished tests.

44e0649

Refactored lib by adding the ClientThread struct.

265d3c8

moved cli_tests as unit tests to args.rs

cbac665

Fixed panic when running both proposer and notary.

69cf597

Connect SMCListener to lib.rs, refactor ClientThread run, move test i…

ecb9693

…mports into test module.

added store_proposal to notary, abstracted some test functionality

8a72004

Suppress some compiler warnings in tests

8d518c9

reposition docstring

842d042

updated docstring

ddde92e

ltfschoen added 5 commits April 15, 2018 11:15

docs: Setup Log crate

f94d253

feat: Add Timestamp to Logs

e71e58c

Reference: https://rust-lang-nursery.github.io/rust-cookbook/logging.html#logging

feat: Add Fern Crate to save log to gitignored file and output to ter…

2733ead

…minal

refactor: Switch entirely to Fern logger to stdout and log file

1c44cea

jamesray1 reviewed Apr 16, 2018

View reviewed changes

jamesray1 and others added 4 commits April 16, 2018 15:38

Add indentations for address arrays, and ethereum_types::U256::as_u32…

361dfc3

… instead of u256_to_bytes32

fix: Revert changes in previous commit by restoring u256_to_bytes32 f…

db0cf58

…unction so tests pass

feat: Add Fern colours to the log level text shown in stdout

6ff0d02

feat: Add Fern logger pretty colours to stdout

fff7116

Reference: https://github.com/daboross/fern/blob/master/examples/pretty-colored.rs

jamesray1 suggested changes Apr 16, 2018

View reviewed changes

Josiah Evans added 6 commits April 16, 2018 21:23

Broke up long lines and added some functionality to notary test funct…

cc2a930

…ions.

Added docstrings

32e90db

added docstrings

32d4d98

Merge branch 'notary' into notary

b913550

Merge pull request #32 from ltfschoen/notary

929614e

feat: Refactor notary. Detect when running cargo test to conditionally run thread termination code. Setup logger

Shortened line and removed unused import in main

7273e95

jamesray1 merged commit fae4b10 into develop Apr 17, 2018

jamesray1 deleted the notary branch April 17, 2018 01:55

jamesray1 restored the notary branch April 17, 2018 09:25

jamesray1 deleted the notary branch April 17, 2018 09:49

AyushyaChitransh pushed a commit to AyushyaChitransh/diamond_drops that referenced this pull request May 9, 2018

Merge pull request Drops-of-Diamond#31 from Drops-of-Diamond/notary

de08e6a

Notary

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Notary #31

Notary #31

ChosunOne commented Apr 14, 2018

jamesray1 Apr 16, 2018

ChosunOne Apr 16, 2018 •

edited

Loading

jamesray1 Apr 17, 2018

jamesray1 Apr 16, 2018

jamesray1 Apr 16, 2018

jamesray1 Apr 16, 2018 •

edited

Loading

ChosunOne Apr 16, 2018

jamesray1 Apr 16, 2018

ChosunOne Apr 16, 2018

jamesray1 Apr 16, 2018

jamesray1 Apr 16, 2018 •

edited

Loading

jamesray1 Apr 16, 2018

ChosunOne Apr 16, 2018

jamesray1 left a comment

jamesray1 Apr 16, 2018

ChosunOne Apr 16, 2018

Notary #31

Notary #31

Conversation

ChosunOne commented Apr 14, 2018

Choose a reason for hiding this comment

ChosunOne Apr 16, 2018 • 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

jamesray1 Apr 16, 2018 • 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

jamesray1 Apr 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesray1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChosunOne Apr 16, 2018 •

edited

Loading

jamesray1 Apr 16, 2018 •

edited

Loading

jamesray1 Apr 16, 2018 •

edited

Loading