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

Remove Message union, specifying SignedMessage or UnsignedMessage explicitly. #669

Merged
merged 1 commit into from
Dec 2, 2019

Conversation

anorth
Copy link
Member

@anorth anorth commented Nov 28, 2019

The deleted file was empty.

The reformat is from gofmt.

Copy link
Contributor

@jzimmerman jzimmerman left a comment

Choose a reason for hiding this comment

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

See inline question re: the Block struct -- I agree that in general we should use SignedMessage or UnsignedMessage rather than the union, whenever possible, but am not sure this is possible everywhere.

@@ -41,7 +41,7 @@ type BlockHeader struct {

type Block struct {
Header BlockHeader
Messages [msg.Message]
Messages [msg.SignedMessage]
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this match with what we expect on-chain? My impression was that the Block struct was the motivation for the union -- i.e., some of the messages are BLS signed and appear as UnsignedMessages in the list (with the signature aggregated), while some are Secp signed and appear as SignedMessages.

Copy link
Member Author

Choose a reason for hiding this comment

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

The Block type does not go on chain at all. This is an internal implementation detail that's in the spec only because we're trying to write implementation code in it.

But ok, I've added SECPMessages and BLSMessages separately, which is closer to what an implementation might actually use.

Copy link
Contributor

Choose a reason for hiding this comment

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

This particular definition of the Block struct may not go on-chain, but some version of it must. In particular, the MessageRoot and ReceiptRoot fields in BlockHeader are currently uninterpreted CIDs, which really should be modified to refer to the actual underlying algebraic datatype (respectively, [Message] and [MessageReceipt]). Then the question becomes what to do with that [Message].

In principle we could replace it with two separate arrays as you've done with SECPMessages and BLSMessages, but then we would lose the ordering of messages within the block (i.e., either all BLS messages must hit the VM before all Secp messages, or vice versa). This seems potentially problematic, and the on-chain union type a cleaner solution.

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, they must be replaced with two different arrays, the spec already had that at one point and both implementations do it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Edit: the spec already has that in the other BlockHeader definition (tho it's also wrong for different reasons). I intend to clean those up as part of block validation, for which this is pre-work.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, sounds good.

@anorth anorth requested a review from jzimmerman November 29, 2019 10:49
@anorth anorth force-pushed the anorth/msgunion branch 2 times, most recently from 7ef8eee to 0e87b0a Compare November 29, 2019 10:52
@jzimmerman jzimmerman mentioned this pull request Nov 29, 2019
@anorth
Copy link
Member Author

anorth commented Dec 2, 2019

FYI @jzimmerman I separated BLS and SECP messages in the interpreter's BlockMessages input structure, to aid your measuring their weight in #685

@anorth anorth merged commit 9b8212b into master Dec 2, 2019
@hugomrdias hugomrdias deleted the anorth/msgunion branch July 5, 2020 17:55
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.

2 participants