Skip to content

Commit

Permalink
Attempting to use sync_pmmr instead of header_pmmr during kernel vali…
Browse files Browse the repository at this point in the history 
…dation.
  • Loading branch information
David Burkett committed Nov 24, 2018
1 parent 07aacf6 commit 14a8852
Showing 1 changed file with 8 additions and 2 deletions.
10 changes: 8 additions & 2 deletions chain/src/txhashset/txhashset.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -765,6 +765,7 @@ impl<'a> HeaderExtension<'a> {
pub struct Extension<'a> {
header: BlockHeader,

sync_pmmr: DBPMMR<'a, BlockHeader, HashOnlyMMRBackend>,
header_pmmr: DBPMMR<'a, BlockHeader, HashOnlyMMRBackend>,
output_pmmr: PMMR<'a, OutputIdentifier, PMMRBackend<OutputIdentifier>>,
rproof_pmmr: PMMR<'a, RangeProof, PMMRBackend<RangeProof>>,
Expand Down Expand Up @@ -818,6 +819,10 @@ impl<'a> Extension<'a> {
fn new(trees: &'a mut TxHashSet, batch: &'a Batch, header: BlockHeader) -> Extension<'a> {
Extension {
header,
sync_pmmr: DBPMMR::at(
&mut trees.sync_pmmr_h.backend,
trees.sync_pmmr_h.last_pos,
),
header_pmmr: DBPMMR::at(
&mut trees.header_pmmr_h.backend,
trees.header_pmmr_h.last_pos,
Expand Down Expand Up @@ -1044,10 +1049,11 @@ impl<'a> Extension<'a> {
let next_header_pmmr_index = pmmr::insertion_to_pmmr_index(last_block_header.height + 1);
debug!(
"next_header_for_kernel_root_validation: height {}, index {}",
last_block_header.height + 1, next_header_pmmr_index
last_block_header.height + 1,
next_header_pmmr_index
);

if let Some(next_header_hash) = self.header_pmmr.get_hash(next_header_pmmr_index) {
if let Some(next_header_hash) = self.sync_pmmr.get_hash(next_header_pmmr_index) {
let next_header_to_validate = self.batch.get_block_header(&next_header_hash)?;

if next_header_to_validate.prev_hash != last_block_header.hash() {
Expand Down

2 comments on commit 14a8852

@antiochp
Copy link
Member

@antiochp antiochp commented on 14a8852 Nov 26, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Edit: sorry - this comment was specific to the "use sync MMR for validating kernels"... I think I replied to something in github and didn't realize this would appear as a generic comment on the PR...

What's the rationale for this?
The reason we kept the sync MMR out of the full extension is it doesn't necessarily track the same data.
The sync head will track headers on a possible fork.
The header MMR itself though will track headers (for full blocks) on the (what we believe to be) main chain.

If we're validating kernels I think the header MMR is more applicable.

Adding the sync MMR to the extension gets complicated and potentially error prone as the heads of the various MMRs may not always align.

@DavidBurkett
Copy link
Contributor

Choose a reason for hiding this comment

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

Very long story. The not-very-quick TL;DR is I was trying to get a header by height of the header 'chain' (ie. the one 'header_head' points to), not the fully validated block 'chain' (the one 'head' points to). Since we only populate the store with block height indices after we've fully validated the block 'chain', I had no quick way of doing this. I attempted to use sync_pmmr (since header_pmmr isn't populated until the chain is fully validated, I believe) because I erroneously thought the leaves were the hashes of the headers, and I could figure out heights that way, but I quickly realized the leaf hashes also include the index when hashing. So the next commit I reverted these changes.

In order to get around the need for knowing block heights, the Kernels message had to be changed to include the block hashes that the kernels belong to (https://github.com/mimblewimble/grin/blob/d468f595c829339e9526a76de40c0b8c88af4d13/p2p/src/msg.rs).

Please sign in to comment.