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

Apply Blocks and refactor #374

Merged
merged 10 commits into from
Apr 26, 2020
Merged

Apply Blocks and refactor #374

merged 10 commits into from
Apr 26, 2020

Conversation

austinabell
Copy link
Contributor

Summary of changes
Changes introduced in this pull request:

  • Implements apply blocks method from syncer. Apply Blocks from StateManager #363
    • Main logic is moved to inside of the VM since applying the tipset messages made sense to not be attached to the syncer
  • Setup buffered blockstore
    • As to not commit all changes to the underlying store (especially in the case of a failure) and will be expanded in future to limit writes based on what's connected to the root

Refactor and cleanup:

  • Replaced state tree trait with just the structure itself
    • There wasn't a great reason why this was the way it is other than future proofing and there is no need to swap out implementation for testing
    • There was a bunch of changes in state tree, so I opened issue detailing potential changes Update StateTree to not flush on snapshot #373
  • Refactored the conversions between Tipset and FullTipset
    • Isn't ideal the way it's setup now, so I'm hoping to make a dedicated pass through since this will be used a lot and still has unnecessary clones
  • Update and fix how peer heads are managed (was a bug previously to only use the tipset passed in the function and the tipset was never connected to the respective peer)
    • Still not in an ideal place, but since this will be replaced with GraphSync I won't refactor it too much (only enough to test)

Reference issue to close (if applicable)

Closes
#363

Other information and links

self.blocks
}
// TODO: conversions from full to regular tipset should not return a result
// and should be validated on creation instead
Copy link
Contributor

Choose a reason for hiding this comment

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

What validations should be checked? The tipset constructor has validation checks included but curious what other validation would be required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Tipset does yeah, but the FullTipset is sometimes constructed without validation (why the conversion to Tipset is error prone

blockchain/state_manager/src/lib.rs Show resolved Hide resolved
vm/interpreter/src/lib.rs Outdated Show resolved Hide resolved
use std::error::Error as StdError;

/// Wrapper around `BlockStore` to limit and have control over when values are written.
/// This type is not threadsafe and can only be used in syncronous contexts.
Copy link
Member

Choose a reason for hiding this comment

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

TYPO: synchronous


impl<'bs, BS> BufferedBlockStore<'bs, BS>
where
BS: BlockStore + Store,
Copy link
Member

Choose a reason for hiding this comment

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

Curious what are the consequences of removing the requirement of Store on BS, since BlockStore inherets the Store trait... I removed it and it works.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

functionally the same, but yeah should be more succinct, I'll fix

Copy link
Member

Choose a reason for hiding this comment

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

Cool. Just wanted to make sure i was on the same page. But yeah, removing is best

@austinabell austinabell merged commit 5355acd into master Apr 26, 2020
@austinabell austinabell deleted the austin/sync/applyblocks branch April 26, 2020 18:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants