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

fast sync is insecure #1162

Closed
tromp opened this issue Jun 12, 2018 · 5 comments
Closed

fast sync is insecure #1162

tromp opened this issue Jun 12, 2018 · 5 comments
Labels
bug consensus breaking Use for issues or PRs that will break consensus and force a hard fork

Comments

@tromp
Copy link
Contributor

tromp commented Jun 12, 2018

In the current fast sync approach, we take the MMR commitments in the horizon header as the indisputable truth (as secured by the cumulative pow difficulty). But an attacker could have branched off from the previous block (or the one before that) and even mined 2000 blocks on top, to make up arbitrary MMRs at horizon,
which are presented to the syncer along with the horizon header. The syncer merely checks that the MMRs
have the correct commitment, but doesn't check their past evolution.

To be certain of the kernel MMR that the horizon cumulative diff is committed to, the syncer needs to go through all intermediate kernel MMR sizes at each height, compute their commitment, and check that this matches the one in the header at that height. Here it might help to have #kernels as a separate header field.

@ignopeverell ignopeverell added bug consensus breaking Use for issues or PRs that will break consensus and force a hard fork labels Jun 12, 2018
@ignopeverell ignopeverell added this to the Beta / testnet3 milestone Jun 12, 2018
@ignopeverell
Copy link
Contributor

Fix is to:

  • Add the kernel MMR size in each header.
  • On fast sync, rewind incrementally to each header MMR sizes, from 1 to current, and check that for each of these the root matches.

@antiochp
Copy link
Member

If we are considering adding the output and kernel sums on the header (#1107) do we add both kernel MMR size and the corresponding output MMR size for consistency?

@ignopeverell
Copy link
Contributor

I was still thinking of going minimal and only adding the size for 8 bytes. Kernel sums can be computed from that. Not sure about output MMR size, what do you think?

@ignopeverell
Copy link
Contributor

Mmmh forgot about #1107, which makes sense as well. Okay, let's add those 33+8+8 bytes...

@antiochp
Copy link
Member

Resolved in #1163 - we now rewind and check the kernel root against the root in the block header at each block height.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug consensus breaking Use for issues or PRs that will break consensus and force a hard fork
Projects
None yet
Development

No branches or pull requests

3 participants