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

WIP: Create 0000-combine-mmr-roots.md #14

Closed
wants to merge 1 commit into from

Conversation

@DavidBurkett
Copy link

commented Jul 22, 2019

Still very much WIP, but submitting a PR to get initial thoughts/feedback.

@lehnberg lehnberg added the node dev label Jul 22, 2019

@lehnberg

This comment has been minimized.

Copy link
Contributor

commented Jul 22, 2019

@DavidBurkett

This comment has been minimized.

Copy link
Author

commented Jul 22, 2019

Initial input from @tromp in keybase that I need to address:

tromp 7:11 AM
i see two downsides
to this proposal
one is the increase in code complexity needed to deal with the merging
the other is the increase in size of merkle proofs

dburkett 7:14 AM
By merging, are you talking about the fact that we'll still have the old headers to deal with?

tromp 7:14 AM
the other is the increase in size of merkle proofs
How do you foresee merkle proofs being used in the future?

tromp 7:14 AM
no, merging the 3 MMR roots into one

dburkett 7:15 AM
Ahh, yea, but that code is relatively isolated. I agree it's a complexity increase, but probably not significant imho

tromp 7:16 AM
i can't foresee where merkle proofs will be used, but i expect they will have some use to light clients
@antiochp

This comment has been minimized.

Copy link
Member

commented Jul 23, 2019

(profiling I've done indicates header lookups are the biggest cause of Grin performance woes).

99% certain this is primarily from our PoW validation where we look back over the past 60 (?) blocks.
But LMDB is very fast and will keep this data mmap'd.
From earlier investigation it wasn't the db lookups that were the bottleneck it was the repeated deserialization of these 60 headers.

Are there any scenarios where we would need to maintain more than those 60 headers in memory?

@antiochp

This comment has been minimized.

Copy link
Member

commented Jul 23, 2019

One use case for keeping the kernel MMR root separate -

To prevent history being rewritten we need to validate the kernel MMR root against every block height.

We can do this for kernels as they are never pruned - we can rewind the kernel MMR to reflect the state at any prior block.
We cannot do this for the output and rangeproof MMRs due to pruning of spent outputs. There is no way to recreate the output or rangeproof MMR root for a block beyond the horizon (7 days of full history).

If we only stored a single aggregate MMR root (across all 3 MMRs) in each header we would be unable to verify kernel MMR roots for historical blocks.

@DavidBurkett

This comment has been minimized.

Copy link
Author

commented Jul 23, 2019

(profiling I've done indicates header lookups are the biggest cause of Grin performance woes).

99% certain this is primarily from our PoW validation where we look back over the past 60 (?) blocks.
But LMDB is very fast and will keep this data mmap'd.
From earlier investigation it wasn't the db lookups that were the bottleneck it was the repeated deserialization of these 60 headers.

Correct, but the deserialization is only really avoidable if you keep the objects in memory.

Are there any scenarios where we would need to maintain more than those 60 headers in memory?

It's very helpful for many of the node APIs that the wallet uses, for example grin-wallet check does a ton of header lookups.

To prevent history being rewritten we need to validate the kernel MMR root against every block height.

I never really understood why we validate every kernel root in the past. History can't be rewritten without new PoW. Besides, if someone has the hash power to reorg the chain beyond horizon, we've got much bigger issues.

@antiochp

This comment has been minimized.

Copy link
Member

commented Jul 23, 2019

I never really understood why we validate every kernel root in the past. History can't be rewritten without new PoW.

Context and reasoning is explained here - mimblewimble/grin#1162

History can't be rewritten without new PoW.

During fast sync a malicious peer could rewrite all the kernels with an entirely different set of kernels as long as the overall root matched (and hence without affecting PoW).

We need to verify -

  • all historical kernels
  • and that the set of kernels evolved correctly over time (that these really are the historical kernels)
@DavidBurkett

This comment has been minimized.

Copy link
Author

commented Jul 23, 2019

Context and reasoning is explained here - mimblewimble/grin#1162

Makes sense. Basically, if a miner had more than 50% hash power, they could mine 2000 blocks in private and completely rewrite the entire history, unlike in bitcoin, where they would need to remine every block to rewrite the entire history. I'll have to think this RFC through some more with this context in mind.

During fast sync a malicious peer could rewrite all the kernels with an entirely different set of kernels as long as the overall root matched (and hence without affecting PoW).

This doesn't make sense. You can't find 2 different sets of kernels that give the exact same root. If you could, your hash function would be broken. mimblewimble/grin#1162 is a valid issue, but unlike what you're suggesting here, it does affect PoW. To pull it off requires mining thousands of malicious blocks.

@antiochp

This comment has been minimized.

Copy link
Member

commented Jul 23, 2019

This doesn't make sense.

Yeah - that was wrong. The issue is in terms of a long fork (beyond the fast sync horizon) rewriting history, not a node silently rewriting history...

@DavidBurkett

This comment has been minimized.

Copy link
Author

commented Jul 29, 2019

I think this RFC was a bad idea, based on the reasoning @antiochp linked to. Although in practice, I suspect the vulnerability pointed out in mimblewimble/grin#1162 would be too impractical to exploit, it's not a risk I'm willing to ignore.

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