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.

Sign up for GitHub

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

Jump to bottom

Implement TipIndex #113

Merged
merged 10 commits into from
Dec 21, 2019
Merged

Implement TipIndex #113

merged 10 commits into from
Dec 21, 2019

Conversation

dutterbutter
Copy link
Contributor

@dutterbutter dutterbutter commented Dec 19, 2019 •
edited
Loading

A couple of important notes:

You will see in the unit tests I am re-using the setup in tipset.rs to construct a tipset, again I explored a few approaches in order to just import setup() to avoid redundancy but could not get it to work since it's within mod tests in tipset.rs and pulling it out of mod tests would require pulling all dependant functions with it which is too messy. I also tried making these test setup functions into its own crate test_utils however, I ran into circular dependencies.

Changes made in this PR:

  • Restructured blockchain component into isolated crates
  • Added store struct with relevant fields
  • Added TipIndex structures with appropriate methods
  • Added unit tests for each of those methods
  • Added errors mod for chain crate

TODO:

  • Implement a ChainReader trait that makes use of these methods that can be shared across components for easy lookups

Closes #109

@dutterbutter
initial tip index structure
c761fbb
@dutterbutter
resolved conflicts
4e2c466
@dutterbutter
Restructured Blockchain Component
f8b55a9 
* Restructured blockchain component into isolated crates
* Added TipIndex methods
* Initial store structures
@dutterbutter
Implemented TipIndex and Tests
594c81e 
* Added store struct with relevant fields
* Added TipIndex structures with appropiate methods
* Added unit tests for each of those methods
* Added errors mod for chain crate
@dutterbutter
Merge branch 'master' into dustin/tip-index
d36bb47
@dutterbutter dutterbutter added Blockchain Priority: 3 - Medium Nice-to-have, does not impede core functionality labels Dec 19, 2019
austinabell
austinabell suggested changes Dec 19, 2019
View reviewed changes
Copy link
Contributor

@austinabell austinabell left a comment

Choose a reason for hiding this comment

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

breaking up the review, will look into the logical changes in another

blockchain/Cargo.toml Outdated Show resolved Hide resolved
blockchain/blocks/Cargo.toml Outdated Show resolved Hide resolved
blockchain/blocks/Cargo.toml Outdated Show resolved Hide resolved
blockchain/blocks/Cargo.toml Outdated Show resolved Hide resolved
blockchain/chain/Cargo.toml Outdated Show resolved Hide resolved
blockchain/chain/src/store/tip_index.rs Outdated Show resolved Hide resolved
blockchain/chain/src/store/tip_index.rs Outdated Show resolved Hide resolved
blockchain/chain_sync/Cargo.toml Outdated Show resolved Hide resolved
blockchain/chain_sync/Cargo.toml Outdated Show resolved Hide resolved
node/network/src/lib.rs Outdated Show resolved Hide resolved
austinabell
austinabell suggested changes Dec 19, 2019
View reviewed changes
Copy link
Contributor

@austinabell austinabell left a comment

Choose a reason for hiding this comment

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

As some of the changes I suggested are a little more nuanced and repeated, I pushed some of the changes so it may be easier to follow. Feel free to copy if you agree with the changes (branch austin/tipisuggestions) dustin/tip-index...austin/tipisuggestions

blockchain/blocks/src/tipset.rs Outdated Show resolved Hide resolved
blockchain/chain/src/store/tip_index.rs Outdated Show resolved Hide resolved
blockchain/chain/src/store/tip_index.rs Outdated Show resolved Hide resolved
blockchain/chain/src/store/tip_index.rs Outdated Show resolved Hide resolved
blockchain/chain/src/store/tip_index.rs Outdated Show resolved Hide resolved
blockchain/chain/src/store/tip_index.rs Outdated Show resolved Hide resolved
blockchain/chain/src/store/tip_index.rs Outdated Show resolved Hide resolved
blockchain/chain/src/store/tip_index.rs Outdated Show resolved Hide resolved
blockchain/chain/src/store/tip_index.rs Outdated Show resolved Hide resolved
@dutterbutter
Copy link
Contributor Author

dutterbutter commented Dec 19, 2019 •
edited
Loading

@austinabell I have made the requested changes and reviewed your branch....thank you, much appreciated!!

The two points:

  1. ...better way of indexing the metadata than having two hash maps, duplicating all metadata

I completely agree and I tried implementing a very generic approach as described in the PR dialogue. I will try to refactor to improve the redundancy today but open to suggestions/direction on how to accomplish.

  1. Test setup redundancy

Again agree and I made a comment about it in the PR dialogue on the different approach I looked at. Again I will try to refactor to improve but open to suggestions/direction on how to accomplish.

@austinabell
Copy link
Contributor

  1. ...better way of indexing the metadata than having two hash maps, duplicating all metadata

I completely agree and I tried implementing a very generic approach as described in the PR dialogue. I will try to refactor to improve the redundancy today but open to suggestions/direction on how to accomplish.

The issue is that the HashTable cannot have multiple values for the key type, it must be one. What can be done is just making the key a u64 (what the hash resolves to anyway) and just use that to remove the need for putting and getting using the seperate hashmaps.

I implemented on my flight because I got bored: dustin/tip-index...austin/tipindexsingle
This is functionally better but is also in my opinion more readable and extensible (I really like dynamic pointers). Let me know what you think, and feel free to push the commits or copy the changes if you agree.

@dutterbutter dutterbutter mentioned this pull request Dec 20, 2019
Closed
4 tasks
austinabell
austinabell approved these changes Dec 20, 2019
View reviewed changes
blockchain/chain/src/store/tip_index.rs Outdated Show resolved Hide resolved
austinabell
austinabell reviewed Dec 20, 2019
View reviewed changes
blockchain/chain/src/store/chain_store.rs
use cid::Cid;
use network::service::NetworkMessage;

pub struct _Store {
Copy link
Contributor

@austinabell austinabell Dec 20, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

oh and you don't need to underscore prefix the struct, only the unused fields, this does nothing

@dutterbutter @austinabell
Update blockchain/chain/src/store/tip_index.rs
8ea9df8 
Co-Authored-By: Austin Abell <austinabell8@gmail.com>
@dutterbutter dutterbutter merged commit e4363f1 into master Dec 21, 2019
@dutterbutter dutterbutter deleted the dustin/tip-index branch December 21, 2019 00:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@austinabell austinabell austinabell approved these changes

@ec2 ec2 ec2 approved these changes

@ansermino ansermino Awaiting requested review from ansermino

@GregTheGreek GregTheGreek Awaiting requested review from GregTheGreek

Assignees
No one assigned
Labels
Priority: 3 - Medium Nice-to-have, does not impede core functionality
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement TipIndex Tracker
3 participants
@dutterbutter @austinabell @ec2