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

[DNM] kernel data file download #2743

Closed
wants to merge 11 commits into from

Conversation

antiochp
Copy link
Member

@antiochp antiochp commented Apr 10, 2019

Adds support for "kernel file download" via a new protocol msg pair (KernelDataRequest and KernelDataResponse).

Still very much WIP but given we are planning to support "variable length" kernel MMR and we need some way of supporting this in a backward compatible way it may end up being convenient to split out the kernel sync from the output/rangeproof txhashset state sync at the same time.

The kernel MMR consists of the following file -

pmmr_data.bin
pmmr_hash.bin
pmmr_size.bin

Note the hash file and the size file can be (re)built locally from the underlying data file.
So for a full kernel sync we only need to send the data file across.

pmmr_data.bin

Can test this via API endpoint -

curl -X POST 127.0.0.1:13413/v1/kerneldownload

This PR introduces the following -

  • build a temp file from the underlying kernel MMR backend data file
  • introduce a new KernelDataResponse msg type with a single "kernel data file" as attachment
  • version support on KernelDataResponse
    • version = 0 (fixed size)
    • version = 1 (proposed variable size, not yet implemented)
  • api endpoint for testing purposes so we can trigger a "kernel download" from another peer

TODO -

  • handle a KernelDataResponse writing the attachment to a temp file on receiving end
  • and then presumably doing something useful with it
  • rebuilding a kernel MMR and validating it for example

@ignopeverell
Copy link
Contributor

Just making @DavidBurkett aware of this.

@DavidBurkett
Copy link
Contributor

Yep, following closely. I actually still have the code to download kernels in parallel from different peers, if anyone wants to go that route. If we're looking to go with a bittorrent-like download though, then there would be no point.

@0xmichalis
Copy link
Contributor

0xmichalis commented Apr 12, 2019

@antiochp please make sure the new protocol messages get documented in https://github.com/mimblewimble/docs/wiki/P2P-Protocol once this PR is merged.

/// Note: Both versions will produce identical hash values (and corresponding MMR root).
/// Only the "data" serialization differs.
///
pub version: u8,
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure this will be very useful. If the peer that requested the kernel data can't support v1, then it shouldn't ever receive v1 kernel data. It should be up to the requester to ask which version it needs.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, if we implement the protocol versioning scheme I suggested here: mimblewimble/grin-pm#102 (comment) , we won't even need a version byte at all.

@antiochp
Copy link
Member Author

Went down the slippery slope of integrating part of this with the txhashset validation logic and it got out of hand (500 lines of code changed).
Going to park this for a bit and revisit it later and see if I can pull a smaller chunk out into a separate PR...

@antiochp antiochp changed the title [WIP] kernel data file download [DNM] kernel data file download Apr 19, 2019
@antiochp
Copy link
Member Author

Split out the initial piece of work (the kernel download request/response msg) in to #2765.

@yeastplume yeastplume added this to the 2.x.x milestone Jun 6, 2019
@antiochp
Copy link
Member Author

Closing for now. Will revisit again at some point.

@antiochp antiochp closed this Sep 23, 2019
@antiochp antiochp deleted the kernel_download branch September 23, 2019 11:09
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.

5 participants