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

Refactor Blockheader #169

Merged
merged 12 commits into from
Jan 16, 2020
Merged

Refactor Blockheader #169

merged 12 commits into from
Jan 16, 2020

Conversation

austinabell
Copy link
Contributor

  • Implements part of Implement Block CBOR Encoding #141 (but doesn't match lotus implementation, can be tracked now by CBOR Encoding #143 to match the undocumented format)
  • Fixes Mutable Cid Method should not be mutable #156 with a refactor that changes how the BlockHeader is built, a few points about the refactor:
    • BlockHeaderBuilder now defaults everything, and there is an option to build() (no validation or cache generation used for testing or specific generation) or build_and_validate() (TODO for validating header and then generates bytes and Cid cache)
    • All getters changed to passing back references to the data instead of cloning. The clones most of the time are redundant when reading and this gives more control to only clone when needed. This change also removes the need for having all fields public (leads to unintended problems when not used correctly)
    • If there is a need to mutate any of the BlockHeader data in the future (I don't see a need for it now) then simply the cache of encoded bytes and cid must be regenerated (I set up a function update_cache to do exactly this)
    • These changes allow for more control over testing with headers as you only need to specify what you are testing and do not need to validate the headers when not necessary for test
  • Implements cbor serialization and deserialization for subtypes needed and added TODO comments referencing types which CBOR encoding format has not been checked against non-documented format (reference at top)

Some of this can be opinionated, so please give opinions on how you think this should look to future proof our usage of it and not require more refactors.

@@ -64,54 +66,50 @@ impl Tipset {
if i > 0 {
// Skip redundant check
// check parent cids are equal
if !headers[i].parents.equals(headers[0].parents.clone()) {
if headers[i].parents() != headers[0].parents() {
Copy link
Contributor

Choose a reason for hiding this comment

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

This always compares header[0] with header[0] as parents() returns the cids for header[0]. The equals method made a full comparison.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

my mistake, should have verified when I switched

Copy link
Contributor Author

Choose a reason for hiding this comment

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

note: equals right now does the same as an equality check. Is there a plan to change this in the future? Right now it requires all cids to be in same order

Copy link
Contributor

Choose a reason for hiding this comment

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

Not that I am aware, it just needs to ensure that the parent cids are equal

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but does the ordering matter? if not then the traditional compare would be sufficient, if not then this equals function doesn't do what you think it does

blockchain/blocks/src/block.rs Outdated Show resolved Hide resolved
@austinabell austinabell merged commit 468846f into master Jan 16, 2020
@austinabell austinabell deleted the austin/bheader/serialization branch January 16, 2020 22:42
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.

Mutable Cid Method should not be mutable
3 participants