Implement BlockManager in Rust by boydjohnson · Pull Request #1637 · hyperledger-archives/sawtooth-core

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

+}
+
+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

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


Probably would name this blockstore_name

Cool, Thanks!

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

+        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

+            .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

+    }
+
+    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

+            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

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


Please add a docstring explaining why this struct exists.

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

+                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

+
+                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

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


This should probably take the predecessor that was missing.

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

+
+#[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

+}
+
+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

+
+#[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

+
+        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

+    }
+
+    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

+                        "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

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


This clone seems avoidable

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

+                } 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

+                !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

+                        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

+        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

+
+        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 <bjohnson@bitwise.io>

Signed-off-by: Boyd Johnson <bjohnson@bitwise.io>

The `get` method returns an iterator over the blocks that are being asked for. Signed-off-by: Boyd Johnson <bjohnson@bitwise.io>

The `ref_block` puts an external hold on the particular block that the BlockManager already has. Signed-off-by: Boyd Johnson <bjohnson@bitwise.io>

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 <bjohnson@bitwise.io>

Signed-off-by: Boyd Johnson <bjohnson@bitwise.io>

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 <bjohnson@bitwise.io>

Signed-off-by: Boyd Johnson <bjohnson@bitwise.io>

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 <bjohnson@bitwise.io>

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 <bjohnson@bitwise.io>

Signed-off-by: Boyd Johnson <bjohnson@bitwise.io>

boydjohnson self-assigned this May 11, 2018

boydjohnson requested review from aludvik, nick-drozd and peterschwarz 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 agunde406, aludvik, delventhalz, jsmitchell, peterschwarz and vaporos 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 <bjohnson@bitwise.io>

Add beginning BlockManager implementation with put method

6742abc

Signed-off-by: Boyd Johnson <bjohnson@bitwise.io>

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 <bjohnson@bitwise.io>

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 <bjohnson@bitwise.io>

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 <bjohnson@bitwise.io>

Add test of ref_block and unref_block in the BlockManager

46a3cc6

Signed-off-by: Boyd Johnson <bjohnson@bitwise.io>

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 <bjohnson@bitwise.io>

Implement add_store method on BlockManager

2137ceb

Signed-off-by: Boyd Johnson <bjohnson@bitwise.io>

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 <bjohnson@bitwise.io>

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 <bjohnson@bitwise.io>

Add test for persisting blocks in the BlockManager

62ebbc9

Signed-off-by: Boyd Johnson <bjohnson@bitwise.io>

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?;

Uh oh!

Conversation

boydjohnson commented May 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aludvik May 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

boydjohnson commented May 11, 2018 •

edited

Loading

aludvik May 14, 2018 •

edited

Loading