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

Variable Size Kernels #21

Merged
merged 13 commits into from Sep 11, 2019

Conversation

@antiochp
Copy link
Member

commented Aug 15, 2019

Rendered link to RFC document
Tracking issue: mimblewimble/grin#3038

This is a pre-requisite for "relative kernels" as we need to be able to introduce new kernel feature variants, with additional associated data.

The proposal is to support kernels of variable size, with each kernel variant supporting additional data specific to the variant. Height locked kernels have an associated lock height for example.

@antiochp antiochp added the node dev label Aug 15, 2019

@antiochp antiochp self-assigned this Aug 15, 2019

@tromp

This comment was marked as resolved.

Copy link

commented Aug 15, 2019

thx for the write-up, Antioch!
there's a typo in "benfit"; also rather than give byte savings over 1M kernels, iI suggest saying it's about a 7% space savings per plain kernel (105/113 bytes).

@lehnberg

This comment was marked as resolved.

Copy link
Contributor

commented Aug 21, 2019

Just took a first pass at this. I actually understood the proposal, which says a lot about the quality of your writing @antiochp - nice job. Reads clear and concise.

A modest suggestion would be to write the RFC as if it's already been accepted and is current documentation, i.e.:

"The proposal is to have variant specific data follow the feature byte" would become something more like "Variant specific data follow the feature byte"; and

"The initial feature byte would determine the number of subsequent bytes to read for variant specific data." becomes "The initial feature byte determines the number..."

Similarly, protocol version 1 should be referred to as a previous version, and the "proposed" version 2 should be referred to as the actual current version, if that makes sense. So that it's not a proposal, but the status quo.

(These are not exhaustive examples)

Rationale being that if the RFC is rejected, then the text will not exist in any case, but if it gets merged, then it will be in the repo and act as an instruction essentially, until the point it gets superseded by some other RFC.

@antiochp

This comment was marked as resolved.

Copy link
Member Author

commented Aug 21, 2019

@lehnberg That makes a lot of sense - I'll rework as suggested.

@antiochp antiochp marked this pull request as ready for review Aug 22, 2019

@antiochp

This comment was marked as resolved.

Copy link
Member Author

commented Aug 22, 2019

Still got one remaining section to complete (txhashset fast sync support). But promoting this to a full PR.

@antiochp antiochp changed the title [WIP] Variable Size Kernels Variable Size Kernels Aug 22, 2019

@lehnberg

This comment has been minimized.

Copy link
Contributor

commented Aug 27, 2019

In accordance with our RFC process, as of today this can be considered in Final Comment Period with a disposition of merge

@lehnberg

This comment has been minimized.

Copy link
Contributor

commented Sep 9, 2019

The Final Comment Period is due to expire for this RFC tomorrow. The proposal will then be merged and considered accepted unless there is a strong case made against it.

@jaspervdm

This comment has been minimized.

Copy link
Member

commented Sep 9, 2019

This might be a stupid question, but do block headers commit to the full kernels that are contained in the block?

  • If yes: does this versioning affect this? I.e. if i convert the kernels inside a block from protocol v1 to v2, will it invalidate the PoW?
  • If no: what does it commit to, the excess only? Wouldn't that make kernels malleable to some extent? Maybe this is not a big deal because creating a kernel with the same public excess but different features requires knowledge of the excess, so only the original creators could do it.
@DavidBurkett

This comment has been minimized.

Copy link

commented Sep 9, 2019

This might be a stupid question, but do block headers commit to the full kernels that are contained in the block?

  • If yes: does this versioning affect this? I.e. if i convert the kernels inside a block from protocol v1 to v2, will it invalidate the PoW?
  • If no: what does it commit to, the excess only? Wouldn't that make kernels malleable to some extent? Maybe this is not a big deal because creating a kernel with the same public excess but different features requires knowledge of the excess, so only the original creators could do it.

No, block headers just commit to the excess, MMR roots, and MMR sizes. Kernels aren't malleable because changing them would change the MMR root

@antiochp

This comment has been minimized.

Copy link
Member Author

commented Sep 9, 2019

do block headers commit to the full kernels that are contained in the block?

Headers commit to the kernel MMR root. Which in turn commits to the hashes (and positions) of all the kernels contained in the kernel MMR.

So headers don't commit to the full kernels contained in the block directly.
But they do commit to all kernel hashes in the kernel MMR in its entirety (which includes the kernels in the block in question).

tl;dr not the full kernels in the block, but the kernel hashes in all blocks

Kernel hashes currently are based on the full kernels (even for 0 fee on coinbase kernels etc). And we need to be careful to preserve this for historical data even when moving to future protocol versions.

@quentinlesceller
Copy link
Member

left a comment

👍. Minors formatting and typo comments.

text/0000-variable-size-kernels.md Outdated Show resolved Hide resolved
text/0000-variable-size-kernels.md Outdated Show resolved Hide resolved
text/0000-variable-size-kernels.md Outdated Show resolved Hide resolved
text/0000-variable-size-kernels.md Outdated Show resolved Hide resolved
text/0000-variable-size-kernels.md Outdated Show resolved Hide resolved
text/0000-variable-size-kernels.md Outdated Show resolved Hide resolved
@lehnberg lehnberg referenced this pull request Sep 11, 2019
0 of 3 tasks complete
lehnberg added 3 commits Sep 11, 2019
RFC#0005
Add tracking issue, PR, assign RFC number
Minor formatting
Missed two tweaks in previous commit.

@lehnberg lehnberg merged commit a4aea53 into mimblewimble:master Sep 11, 2019

@lehnberg

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2019

🎉 Wohooo! This RFC has now been merged! 🤸‍♀️
Tracking issue: mimblewimble/grin#3038

@antiochp antiochp deleted the antiochp:variable_size_kernels branch Sep 11, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.