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

Implement kernel and output features as enums #2312

Merged
merged 3 commits into from
Jan 8, 2019

Conversation

antiochp
Copy link
Member

@antiochp antiochp commented Jan 8, 2019

Longer term we may want to consider being able to maintain the various feature specific data (lock heights for a height locked kernel, fee for coinbase kernel etc.) on the enum itself.
But to do this we would need to implement per enum type serialization and we would no longer be able to represent the enum as a simple u8.

This PR gets us 90% of the way there in terms of simplifying how we deal with features.

TODO -

  • test serialization against floonet (nothing should change)
    • fast sync
    • broadcast (blocks and txs)

Copy link
Contributor

@tromp tromp left a comment

Choose a reason for hiding this comment

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

It is critical that we reject features outside the current range 0-2,
so we need to maintain the fallback clause
let valid_features = match features {
...
_ => false,

@antiochp
Copy link
Member Author

antiochp commented Jan 8, 2019

@tromp Rust enums matching must be exhaustive (it won't compile otherwise). We don't need a fallback here because we have covered all the possibilities.

In fact if we try and add a fallback clause here then the rust compiler will complain -

warning: unreachable pattern                                                                                                                             
    --> core/src/core/transaction.rs:1382:3                                                                                                              
     |                                                                                                                                                   
1382 |         _ => false,                                                                                                                               
     |         ^                                                                                                                                         
     |                                                                                                                                                   
     = note: #[warn(unreachable_patterns)] on by default   

It is now impossible to construct a TxKernel with a KernelFeatures outside of the range 0-2.

@tromp
Copy link
Contributor

tromp commented Jan 8, 2019

Then what happens if the deserializer reads a value 3? Is that guaranteed to result in error?

@antiochp
Copy link
Member Author

antiochp commented Jan 8, 2019

Then what happens if the deserializer reads a value 3? Is that guaranteed to result in error?

Yes we will catch this during deserialization here -

https://github.com/mimblewimble/grin/pull/2312/files#diff-404c77cf79ebda67cdbd58c1c99daa8cR63

KernelFeatures::from_u8(reader.read_u8()?).ok_or(ser::Error::CorruptedData)?;

Edit: let me add a test to demonstrate/verify this, so we have something to refer to in the code.

@tromp
Copy link
Contributor

tromp commented Jan 8, 2019

So the KernelFeatures::from_u8 throws an error given a value of 3?
I see you already added a test for that, so we're good...
Thanks!

@antiochp antiochp merged commit 27801f6 into mimblewimble:master Jan 8, 2019
@antiochp antiochp deleted the features_as_enums branch January 8, 2019 16:07
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.

2 participants