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

Construct Collation and Merklize Chunks into chunkRoot #133

Merged
merged 36 commits into from
Jun 14, 2018

Conversation

roveneliah
Copy link
Contributor

This is a PR referencing #125 to construct collation instances from serialized blobs as well as merklizing the blobs to get the chunkRoot.

Details

  • deserialize body into tx list and pass list into NewCollation function
  • created wrapper Chunks in order to use types.DeriveSha function
  • merklize chunks in SaveBody

Copy link
Member

@terencechain terencechain left a comment

Choose a reason for hiding this comment

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

Couple fixes on the comments. Are we covering these changes in the tests?



// Chunks is a wrapper around a chunk array to implement DerivableList,
// which allows us to Merklize the chunks into the chunkRoot
Copy link
Member

Choose a reason for hiding this comment

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

missing a period after chunkRoot

col := NewCollation(header, body, nil)
txs, err := Deserialize(body)
if err != nil {
return nil, fmt.Errorf("cannot deserialize body", err)
Copy link
Member

Choose a reason for hiding this comment

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

"cannot deserialize body:"

@@ -78,7 +79,11 @@ func (s *Shard) CollationByHash(headerHash *common.Hash) (*Collation, error) {
}
// TODO: deserializes the body into a txs slice instead of using
Copy link
Member

Choose a reason for hiding this comment

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

Take off the todo

@terencechain
Copy link
Member

don't forget to add package name to the commit message

@terencechain
Copy link
Member

lint and tests failed, we should look into that

Copy link
Member

@prestonvanloon prestonvanloon left a comment

Choose a reason for hiding this comment

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

Tests?

// right now we will just take the raw keccak256 of the body until #92 is merged.
chunkRoot := common.BytesToHash(body)
// check if body is empty and throw error.
if body == nil {
Copy link
Member

Choose a reason for hiding this comment

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

This only checks if the body is nil, did you also want to check len(body) == 0? I think that is the true condition for an empty body.

@rauljordan rauljordan changed the title Construct collation and merklize chunks into chunkRoot Construct Collation and Merklize Chunks into chunkRoot May 24, 2018
@prestonvanloon prestonvanloon added this to the Ruby milestone May 25, 2018
@rauljordan
Copy link
Contributor

Any update on this @elihanover? Do you have a timeframe for finishing the PR?

@roveneliah
Copy link
Contributor Author

Yes, focusing back on this tomorrow

@roveneliah
Copy link
Contributor Author

// CalculateChunkRoot updates the collation header's chunk root based on the body.
func (c *Collation) CalculateChunkRoot() {
	// TODO: this needs to be based on blob serialization.
	// For proof of custody we need to split chunks (body) into chunk + salt and
	// take the merkle root of that.
	chunkRoot := common.BytesToHash(c.body)
	c.header.data.ChunkRoot = &chunkRoot
}

Could someone point me to any resources on "salt"? Not sure how that fits in.

@nisdas
Copy link
Member

nisdas commented May 28, 2018

I think salt is referring to Proof of Custody. You can take a look at #112 for context on it

@terencechain
Copy link
Member

Here is another good thread on Proof of Custody:
https://ethresear.ch/t/enforcing-windback-validity-and-availability-and-a-proof-of-custody/949

Utilize Proof of Custody and construct chunk root with salt may be out of the scope for this PR. You are more than welcome to skip that and open another PR down the line

return fmt.Errorf("body is empty")
}

chunks := Chunks(body) // wrapper allowing us to merklizing the chunks
Copy link
Member

Choose a reason for hiding this comment

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

you forgot a period after the comment

// right now we will just take the raw keccak256 of the body until #92 is merged.
chunkRoot := common.BytesToHash(body)
// check if body is empty and throw error.
if body == nil || len(body) == 0 { // is this check strict enough?
Copy link
Member

Choose a reason for hiding this comment

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

the check looks good to me

// nil as the third arg to MakeCollation.
col := NewCollation(header, body, nil)
txs, err := DeserializeBlobToTx(body)
Copy link
Member

Choose a reason for hiding this comment

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

Why does this have to do with construct collation and merklize chunks into chunk root?

@roveneliah
Copy link
Contributor Author

@terenc3t just let me know. Either way, I'll work on that either today or this weekend.

@roveneliah
Copy link
Contributor Author

@rauljordan thanks for the feedback. I'll make changes today.


c.CalculateChunkRoot()
salt := []byte{1, 0x9f}
fmt.Printf("%x\n", c.header.data.ChunkRoot)
Copy link
Member

Choose a reason for hiding this comment

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

Please remove all of these print statements from this test

Copy link
Contributor

@rauljordan rauljordan left a comment

Choose a reason for hiding this comment

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

Very nice, thank you for doing this @elihanover!

@terencechain
Copy link
Member

LGTM. Thanks for the PR @elihanover!

// right now we will just take the raw keccak256 of the body until #92 is merged.
chunkRoot := common.BytesToHash(body)
if len(body) == 0 {
return fmt.Errorf("body is empty")
Copy link
Member

Choose a reason for hiding this comment

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

Please do not use fmt if not formatting. Use errors.New

@roveneliah
Copy link
Contributor Author

Pulled the most recent code and ran the whisperv6 test. It worked for me, so not sure why it's failing with Travis..

@terencechain
Copy link
Member

Restarted Travis. It's flaky

}

// ConvertBackToTx converts raw blobs back to their original transactions.
func ConvertBackToTx(rawBlobs []utils.RawBlob) ([]*types.Transaction, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

There's an identical function, convertRawBlobToTx below

Copy link
Member

Choose a reason for hiding this comment

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

Good catch. Let's delete one of them

prestonvanloon pushed a commit that referenced this pull request Jul 22, 2018
Construct Collation and Merklize Chunks into chunkRoot
prestonvanloon pushed a commit that referenced this pull request Jul 22, 2018
Construct Collation and Merklize Chunks into chunkRoot

Former-commit-id: 1c70ba6
prestonvanloon pushed a commit that referenced this pull request Jul 22, 2018
Construct Collation and Merklize Chunks into chunkRoot

Former-commit-id: d4ed64c3d3802f1bfc330d6d4c7ee18109cd3f85 [formerly 1c70ba6]
Former-commit-id: 1ab7be7
Redidacove pushed a commit to Redidacove/prysm that referenced this pull request Aug 13, 2024
* Init Week0

* Added week 0 updates

* republish hackmd public link

* Week 1 and name updates

* EPF Week 2 Updates

---------

Co-authored-by: Mário Havel <61149543+taxmeifyoucan@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants