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

Kernel sum and MMR sizes in block header #1163

Merged
merged 14 commits into from
Jun 21, 2018
Merged

Kernel sum and MMR sizes in block header #1163

merged 14 commits into from
Jun 21, 2018

Conversation

ignopeverell
Copy link
Contributor

@ignopeverell ignopeverell commented Jun 12, 2018

Adds the kernel sum total and output and kernel MMR sizes to block headers. This allows validation of all intermediate kernel roots during fast sync. Also allows getting rid of the BlockSum index.

Fixes #1162, #1107, and #1165.

@ignopeverell ignopeverell added the consensus breaking Use for issues or PRs that will break consensus and force a hard fork label Jun 12, 2018
@ignopeverell ignopeverell added this to the Beta / testnet3 milestone Jun 12, 2018
Blocks are now summed and validated based on their own totals and
not the totals since genesis. This allows to get rid of BlockSum
and simplified the setting of a new block's roots, kernel sum and
MMR sizes. Fixes #1163
@ignopeverell ignopeverell changed the title [WIP] Kernel sum and MMR sizes in block header Kernel sum and MMR sizes in block header Jun 15, 2018
Copy link
Member

@antiochp antiochp left a comment

Choose a reason for hiding this comment

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

Looks great. A lot simpler now in addition to fixing the fast sync vulnerability.

@@ -125,6 +127,13 @@ pub struct BlockHeader {
/// We can derive the kernel offset sum for *this* block from
/// the total kernel offset of the previous block header.
pub total_kernel_offset: BlindingFactor,
/// Total accumulated sum of kernel commitments since genesis block.
/// Should always equal the UTXO commitment sum minus supply.
pub total_kernel_sum: Commitment,

This comment was marked as spam.

@@ -576,24 +596,17 @@ impl Block {

// now sum the kernel_offsets up to give us
// an aggregate offset for the entire block
let total_kernel_offset = {
kernel_offsets.push(prev.total_kernel_offset);
let total_kernel_offset = committed::sum_kernel_offsets(kernel_offsets, vec![])?;

This comment was marked as spam.

)?;
// take the kernel offset for this block (block offset minus previous) and
// verify outputs and kernel sums
let block_kernel_offset = if self.header.total_kernel_offset() == prev_kernel_offset.clone()

This comment was marked as spam.

@antiochp
Copy link
Member

Maybe done as a separate PR but I think we can get rid of BlockMarker here also as we can derive the "markers" from the MMR sizes at each block?

@antiochp
Copy link
Member

Also I think the changes specific to fixing #1165 can be backported to master as they don't depend on the header changes?

@ignopeverell
Copy link
Contributor Author

You're right on both counts. And yes, I think removing BlockMarker should be done in another PR.

@ignopeverell ignopeverell changed the base branch from master to milestone/testnet3 June 21, 2018 00:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
consensus breaking Use for issues or PRs that will break consensus and force a hard fork
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants