Implement BlockManager in Rust #1637

boydjohnson · 2018-05-11T19:54:05Z

This work is based off of the ideas in this RFC pull request: hyperledger-archives/sawtooth-rfcs#5.

~~The persist method is not yet implemented, but I wanted to get eyes on the work so far.~~

peterschwarz · 2018-05-11T21:32:48Z

validator/src/journal/block_store.rs

+}
+
+struct GetBlockIterator<'a> {
+    blockstore: &'a InMemoryBlockStore,


This probably shouldn't be tied to InMemoryBlockStore, or should be called InMemoryBlockStoreIterator

I can probably do &'a BlockStore, because all we care about is that it is a BlockStore.

peterschwarz · 2018-05-11T21:33:20Z

validator/src/journal/block_manager.rs

+}
+
+struct Anchor {
+    pub blockstore: String,


Probably would name this blockstore_name

Cool, Thanks!

peterschwarz · 2018-05-11T21:36:52Z

validator/src/journal/block_manager.rs

+        left.block_num as i64 - right.block_num as i64
+    }
+
+    fn walk_back_1_from_block(&self, block_id: String) -> Option<&'a Block> {


get_previous_block seems like a better name

Thanks, Good suggestion.

peterschwarz · 2018-05-11T21:37:19Z

validator/src/journal/block_manager.rs

+            .unwrap_or(None)
+    }
+
+    fn walk_back_n_from_block(&self, block_id: String, n: u64) -> Option<&'a Block> {


Likewise, get_nth_previous_block

Thanks, good suggestion.

peterschwarz · 2018-05-11T21:42:46Z

validator/src/journal/block_store.rs

+    }
+
+    fn iter(&self) -> Box<Iterator<Item = Block>> {
+        unimplemented!()


Doesn't look like you're using the Iterator below.

The iterator below is used in get.

Is this going to be implemented, or is it intentionally unimplemented?

I didn't need it for any of the work I had done so far, so I didn't implement it. Can you talk about iterating through a blockstore that wouldn't be like branch or branch_diff where you are starting from some known tip?

aludvik · 2018-05-14T15:23:56Z

validator/src/journal/block_manager.rs

+            blockstore_name: blockstore_name.to_string(),
+            external_ref_count,
+            internal_ref_count,
+            block_id: block_id.to_string(),


Per previous style discussions, prefer into() for string conversions like this.

aludvik · 2018-05-14T15:44:23Z

validator/src/journal/block_manager.rs

+}
+
+#[derive(Default)]
+struct Anchors {


Please add a docstring explaining why this struct exists.

aludvik · 2018-05-14T15:56:55Z

validator/src/journal/block_manager.rs

+                let predecessor_of_head_in_any_blockstore = self.blockstore_by_name.values().any(
+                    |blockstore| blockstore.get(vec![head.previous_block_id.clone()]).count() > 0,
+                );
+                let head_is_genesis = head.previous_block_id == NULL_BLOCK_IDENTIFIER;


I wouldn't use blockchain terminology here, just think of it as a tree, so this would be the root. Also, I think it should be possible to avoid doing root-node-specific checks by making the root nodes anchors.

I'm not sure it is possible to do without the NULL_BLOCK_IDENTIFIER check. If the first put is C 3, D 4 and with C pointing to an unknown block B is it ok or is it an error?

I am thinking that when you create the block manager, rather than having anchors be empty, you could initialize it with an anchor to a block with id = NULL_BLOCK_IDENTIFIER. But there might be some other reason not to do this.

aludvik · 2018-05-14T15:57:45Z

validator/src/journal/block_manager.rs

+
+                if !(predecessor_of_head_in_memory || predecessor_of_head_in_any_blockstore
+                    || head_is_genesis)
+                {


This check should probably be refactored behind a contains function.

aludvik · 2018-05-14T16:00:00Z

validator/src/journal/block_manager.rs

+#[derive(Debug, PartialEq)]
+pub enum BlockManagerError {
+    MissingPredecessor(String),
+    MissingPredecessorInBranch,


This should probably take the predecessor that was missing.

aludvik · 2018-05-14T16:00:22Z

validator/src/journal/block_manager.rs

+
+#[derive(Debug, PartialEq)]
+pub enum BlockManagerError {
+    MissingPredecessor(String),


Similar to below, this should add a clarifying phrase about where the predecessor was expected.

aludvik · 2018-05-14T17:50:09Z

validator/src/journal/block_manager.rs

+}
+
+impl<'a> BranchIterator<'a> {
+    pub fn new(block_manager: &'a BlockManager, first_block_id: String) -> Self {


I think that when you create this, it should ref the first_block_id and then when it is dropped, it should unref the first_block_id. That way you can guarantee the iterator is always valid.

Ah, that makes sense, so that blocks aren't being dropped while the Iterator is iterating.

@aludvik That will change the get method from &self to &mut self. does that seem ok?

aludvik · 2018-05-14T19:01:28Z

validator/src/journal/block_manager.rs

+
+#[derive(Default)]
+pub struct BlockManager {
+    block_by_block_id: HashMap<String, (Block, i64, i64)>,


I would use a struct here instead of a tuple so it is clearer which i64 is for internal/external.

Can we use a u64 here with checked subtraction to catch when we try to unref below 0? https://doc.rust-lang.org/std/primitive.u64.html#method.checked_sub

aludvik · 2018-05-14T19:07:21Z

validator/src/journal/block_manager.rs

+
+        let (external_ref_count, internal_ref_count, block_id) = ref_count_block_id_result?;
+
+        if external_ref_count <= 0 && internal_ref_count <= 0 {


I think this should be split up into checking whether the count is less than 0, which should be a panic, and checking if the count is 0, which should cause a purge.

aludvik · 2018-05-14T19:13:28Z

validator/src/journal/block_store.rs

+    }
+
+    fn iter(&self) -> Box<Iterator<Item = Block>> {
+        unimplemented!()


Is this going to be implemented, or is it intentionally unimplemented?

aludvik · 2018-05-14T19:17:37Z

validator/src/journal/block_manager.rs

+                        "During Put, missing predecessor of block {}: {}",
+                        head.header_signature, head.previous_block_id
+                    )));
+                } else {


This doesn't need to be else, since you are returning above.

aludvik · 2018-05-14T19:18:43Z

validator/src/journal/block_manager.rs

+                                        ),
+                                    ));
+                                }
+                                previous = Some(block.header_signature.clone());


This clone seems avoidable

aludvik · 2018-05-14T19:20:48Z

validator/src/journal/block_manager.rs

+                } else {
+                    let mut previous = None;
+
+                    for block in tail {


This internal branch integrity check feels like a good candidate for refactoring into a separate private helper function with a descriptive name so it is easier to tell what is going on.

aludvik · 2018-05-14T19:27:26Z

validator/src/journal/block_manager.rs

+                !self.block_by_block_id.contains_key(&block.header_signature)
+                    && !self.blockstore_by_name.values().any(|blockstore| {
+                        blockstore.get(vec![block.header_signature.clone()]).count() > 0
+                    })


Can you just use self.contains() here?

aludvik · 2018-05-14T19:29:29Z

validator/src/journal/block_manager.rs

+                        blockstore.get(vec![block.header_signature.clone()]).count() > 0
+                    })
+            })
+            .collect();


Can you avoid the extra vector allocation here by chaining a for_each() here instead? The you can just do if let Some(last_block) = branch.last() after?

I wouldn't do branch.last() because branch has been consumed and it is logically a different thing from blocks_not_added_yet.

aludvik · 2018-05-14T19:29:43Z

validator/src/journal/block_manager.rs

+        if let Some((last_block, blocks)) = blocks_not_added_yet.split_last() {
+            self.block_by_block_id.insert(
+                last_block.header_signature.clone(),
+                (last_block.clone(), 0, 0),


I think this needs an external reference count of 1 to start.

Just so I understand, the blocks that go in with an internal reference count of 0 as well as ones that have an internal reference count of 1 would all have an external reference count of 1 to start.

aludvik · 2018-05-15T18:50:38Z

validator/src/journal/block_manager.rs

+
+        let mut optional_new_tip = None;
+
+        let (external_ref_count, internal_ref_count, block_id) = ref_count_block_id_result?;


Is there a reason to separate these lines instead of just doing: let (external_ref_count, internal_ref_count, block_id) = self.lower_tip_blocks_refcount(tip)?;?

The BlockStore trait is only added so that the full BlockManager interface can be shown. A blockstore implementation will be delayed until there is work on persist and add_store for the BlockManager. Signed-off-by: Boyd Johnson <[email protected]>

Signed-off-by: Boyd Johnson <[email protected]>

The `get` method returns an iterator over the blocks that are being asked for. Signed-off-by: Boyd Johnson <[email protected]>

The `ref_block` puts an external hold on the particular block that the BlockManager already has. Signed-off-by: Boyd Johnson <[email protected]>

This method considers blocks in order from some branch tip and removes all blocks up to any block that has an external hold or a ref-count of 2+. Signed-off-by: Boyd Johnson <[email protected]>

Signed-off-by: Boyd Johnson <[email protected]>

The test for this method only tests blocks that are not in a blockstore. More tests will be added when persist and add_store are added. Signed-off-by: Boyd Johnson <[email protected]>

Signed-off-by: Boyd Johnson <[email protected]>

The branch_diff gives a difference between two chains. C D block_manager.branch_diff tip<- C, exclude <- D \/ will give the single block in the branch C. B | A Signed-off-by: Boyd Johnson <[email protected]>

The `persist` method inserts blocks into a named blockstore and removes the other fork from that blockstore. Anchors for blocks with a non-zero reference count are created. Signed-off-by: Boyd Johnson <[email protected]>

Signed-off-by: Boyd Johnson <[email protected]>

boydjohnson self-assigned this May 11, 2018

boydjohnson requested review from peterschwarz, aludvik and nick-drozd May 11, 2018 19:54

boydjohnson changed the title ~~Implementation of BlockManager in Rust~~ Implement BlockManager in Rust May 11, 2018

peterschwarz suggested changes May 11, 2018

View reviewed changes

aludvik suggested changes May 14, 2018

View reviewed changes

boydjohnson requested review from peterschwarz, aludvik, vaporos, jsmitchell, delventhalz and agunde406 May 15, 2018 14:43

aludvik reviewed May 15, 2018

View reviewed changes

aludvik approved these changes May 15, 2018

View reviewed changes

Boyd Johnson added 11 commits May 15, 2018 15:19

Add journal mod and BlockStore trait

478a9e4

The BlockStore trait is only added so that the full BlockManager interface can be shown. A blockstore implementation will be delayed until there is work on persist and add_store for the BlockManager. Signed-off-by: Boyd Johnson <[email protected]>

Add beginning BlockManager implementation with put method

6742abc

Signed-off-by: Boyd Johnson <[email protected]>

Implement get method on BlockManager

8b6d885

The `get` method returns an iterator over the blocks that are being asked for. Signed-off-by: Boyd Johnson <[email protected]>

Implement ref_block method on BlockManager

d0555c0

The `ref_block` puts an external hold on the particular block that the BlockManager already has. Signed-off-by: Boyd Johnson <[email protected]>

Implement unref_block in BlockManager

85362f9

This method considers blocks in order from some branch tip and removes all blocks up to any block that has an external hold or a ref-count of 2+. Signed-off-by: Boyd Johnson <[email protected]>

Add test of ref_block and unref_block in the BlockManager

46a3cc6

Signed-off-by: Boyd Johnson <[email protected]>

Add branch method to BlockManager

78bb75e

The test for this method only tests blocks that are not in a blockstore. More tests will be added when persist and add_store are added. Signed-off-by: Boyd Johnson <[email protected]>

Implement add_store method on BlockManager

2137ceb

Signed-off-by: Boyd Johnson <[email protected]>

Implement branch_diff in BlockManager

dae500c

The branch_diff gives a difference between two chains. C D block_manager.branch_diff tip<- C, exclude <- D \/ will give the single block in the branch C. B | A Signed-off-by: Boyd Johnson <[email protected]>

Implement persist on the BlockManager

82f83de

The `persist` method inserts blocks into a named blockstore and removes the other fork from that blockstore. Anchors for blocks with a non-zero reference count are created. Signed-off-by: Boyd Johnson <[email protected]>

Add test for persisting blocks in the BlockManager

62ebbc9

Signed-off-by: Boyd Johnson <[email protected]>

peterschwarz approved these changes May 15, 2018

View reviewed changes

boydjohnson merged commit 715b72e into hyperledger-archives:master May 16, 2018


		let (external_ref_count, internal_ref_count, block_id) = ref_count_block_id_result?;

		if external_ref_count <= 0 && internal_ref_count <= 0 {


		let mut optional_new_tip = None;

		let (external_ref_count, internal_ref_count, block_id) = ref_count_block_id_result?;

Implement BlockManager in Rust #1637

Implement BlockManager in Rust #1637

Conversation

boydjohnson commented May 11, 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

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

aludvik May 14, 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

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

Choose a reason for hiding this comment

boydjohnson commented May 11, 2018 •

edited

Loading

aludvik May 14, 2018 •

edited

Loading