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

Use mmr sizes in header to validate header weight #3395

Merged
merged 7 commits into from
Aug 19, 2020

Conversation

antiochp
Copy link
Member

@antiochp antiochp commented Jul 16, 2020

Resolves #3368.

We commit to output_mmr_size and kernel_mmr_size in the block header.
Given these we can approximate the full block weight (assuming 0 inputs).

This PR introduces a weight validation rule based on this when processing block headers.

My understanding is this is technically a softfork due to nodes fast-syncing headers (not full blocks) beyond the horizon? I believe @DavidBurkett stated this in keybase.

Otherwise we may accept a header as valid and then subsequently reject the full block as overweight (current behavior).

Took the opportunity to clean up weight to use u64 consistently and not an awkward usize.

cc @tromp @DavidBurkett.


Suggestion from @tromp - we can go one better than this.
This PR allows us to verify block weight per header.
We can also verify the global weight at any given header height based on the overall output MMR size.


DONE -

  • validate "global" weight limits when deserializing "untrusted" block header
  • validate per-header weight limits based on mmr sizes
  • validate per-header "must have at least 1 output and 1 kernel for block reward" based on mmr sizes

@antiochp antiochp changed the title [WIP] use mmr sizes in header to validate header weight Use mmr sizes in header to validate header weight Jul 18, 2020
@antiochp antiochp force-pushed the header_weight_validation branch 2 times, most recently from 59bbbd1 to 5f35cca Compare August 13, 2020 10:46
@antiochp antiochp requested a review from tromp August 18, 2020 12:37
chain/src/pipe.rs Outdated Show resolved Hide resolved
chain/src/pipe.rs Outdated Show resolved Hide resolved
set_pow(&mut b.header);

let mut vec = Vec::new();
ser::serialize_default(&mut vec, &b.header).expect("serialization failed");
Copy link
Contributor

Choose a reason for hiding this comment

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

why do you expect "serialization failed" ?

Copy link
Member Author

@antiochp antiochp Aug 18, 2020

Choose a reason for hiding this comment

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

Rust is confusing. This is saying we expect it not to be an error here.
expect("reason for error") is an alternative to an opaque unwrap()

https://doc.rust-lang.org/std/result/enum.Result.html#method.expect

let mut vec = Vec::new();
ser::serialize_default(&mut vec, &b.header).expect("serialization failed");
let res: Result<UntrustedBlockHeader, _> = ser::deserialize_default(&mut &vec[..]);
assert_eq!(res.err(), Some(ser::Error::CorruptedData));
Copy link
Contributor

Choose a reason for hiding this comment

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

so the transaction::Error::TooHeavy gets converted into a ser::Error::CorruptedData ?

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. We need to return a generic ser::Error error and in most cases we use CorruptedData for low level serialization issues.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually this is not happening because of transaction::Error::TooHeavy.
We have an additional validation check that happens during deserialization of a block header. This is where the "global weight" is used.

grin/core/src/core/block.rs

Lines 471 to 479 in 96e8bfa

// Validate global output and kernel MMR sizes against upper bounds based on block height.
let global_weight = TransactionBody::weight_as_block(
0,
header.output_mmr_count(),
header.kernel_mmr_count(),
);
if global_weight > global::max_block_weight() * (header.height + 1) {
return Err(ser::Error::CorruptedData);
}

This is a rough check that we can do very cheaply as part of deserialization.

We can also verify the global weight at any given header height based on the overall output MMR size.
That doesn't make sense in hindsight:-(

It's not perfect but it prevents somebody trying to broadcast a header with massively inflated MMR sizes.
And we can verify this without needing to read the previous header from the db.

We still do the per-header weight validation later to give us a more accurate validation.

@tromp
Copy link
Contributor

tromp commented Aug 18, 2020

We can also verify the global weight at any given header height based on the overall output MMR size.

That doesn't make sense in hindsight:-(

block specific TooHeavy error
@antiochp
Copy link
Member Author

We can also verify the global weight at any given header height based on the overall output MMR size.

That doesn't make sense in hindsight:-(

See comment above about CorruptedData error.
Still makes sense to verify this early (and cheaply).

@antiochp antiochp merged commit 1cff387 into mimblewimble:master Aug 19, 2020
@antiochp antiochp deleted the header_weight_validation branch August 19, 2020 08:41
@antiochp antiochp mentioned this pull request Sep 16, 2020
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.

Consider "max weight" consensus rule for block headers (not just full blocks)
2 participants