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

Add clarifiication on uncle headers #158

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Add clarifiication on uncle headers #158

wants to merge 4 commits into from

Conversation

acolytec3
Copy link
Contributor

Reverts a previous accidental commit to master and then adds clarifying comment defining uncle headers using the devp2p definition for list_of_uncle_headers found here

works for all current transaction types.

Note 2: The `list_of_uncle_headers` refers to the array of uncle headers [defined in the devp2p spec](https://github.com/ethereum/devp2p/blob/master/caps/eth.md#block-encoding-and-validity).
Copy link
Contributor

Choose a reason for hiding this comment

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

Using devp2p as the reference is not wrong, but also not helpful, maybe? It's really the yellow paper spec that defines it. That format is consensus-relevant, because the uncles hash found in the block header is the hash of rlp.encode(list_of_uncle_headers). That's the primary reason for the choice. I would have a hard time using "devp2p did it this way" as a primary justification for any of our design choices :P

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, gotcha, sorry. I remember that in your PR and I'm good with it. I just wanted to explain what list_of_uncle_headers actually was since it's not clearly defined that it's an array of byte arrays where each byte array is the raw bytes representation of the uncle header.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I revised my reference to point to the yellow paper and then further define in hopefully clear language how the uncles should be represented.

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool looks good, thanks for adding!

@@ -209,7 +209,7 @@ encoded_uncles = rlp.encode(list_of_uncle_headers)
Note 1: The type-specific encoding might be different in future transaction types, but this encoding
works for all current transaction types.

Note 2: The `list_of_uncle_headers` refers to the array of uncle headers [defined in the devp2p spec](https://github.com/ethereum/devp2p/blob/master/caps/eth.md#block-encoding-and-validity).
Note 2: The `list_of_uncle_headers` refers to the list of uncle headers [defined in the Ethereum yellow paper](https://github.com/ethereum/yellowpaper/blob/30782852fef61f61a247dafbdad814ae7e00fb39/Paper.tex#L399) and is represented as an array of byte arrays where each byte array is the raw bytes representing an uncle header.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you rather need to say here that it is an RLP encoded list of block headers (from the uncle blocks) that results in a byte string/array.

E.g.:

Suggested change
Note 2: The `list_of_uncle_headers` refers to the list of uncle headers [defined in the Ethereum yellow paper](https://github.com/ethereum/yellowpaper/blob/30782852fef61f61a247dafbdad814ae7e00fb39/Paper.tex#L399) and is represented as an array of byte arrays where each byte array is the raw bytes representing an uncle header.
Note 2: The `list_of_uncle_headers` refers to the list of uncle headers [defined in the Ethereum yellow paper](https://github.com/ethereum/yellowpaper/blob/30782852fef61f61a247dafbdad814ae7e00fb39/Paper.tex#L399) and is represented RLP encoded list of block headers, where each block header is the header from an uncle block.

Also, I think a bit more relevant yellow paper link is this line: https://github.com/ethereum/yellowpaper/blob/30782852fef61f61a247dafbdad814ae7e00fb39/Paper.tex#L414 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd be hesitant to say it's an RLP encoded list since that's kind of what the larger content: rlp(list_of_uncle_headers) already says and I'm talking specifically about what's inside the parentheses. I wouldn't want implementers to think we mean our actual code should be rlp.encode(rlp.encode(list_of_uncle_headers)). Otherwise, I'm good with the above.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Right, forgot about that. Then I'd suggest dropping everything from the and onwards. I mostly wanted to point out that the byte array wording is not really correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went with @carver's suggestion. Any last comments before we merge this?

history-network.md Outdated Show resolved Hide resolved
Co-authored-by: Jason Carver <[email protected]>
@pipermerriam
Copy link
Member

What should happen with this PR?

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.

4 participants