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

Fetch params from ipfs if possible #2597

Merged
merged 2 commits into from Sep 21, 2017

Conversation

Projects
None yet
6 participants
@kpcyrd
Contributor

kpcyrd commented Aug 27, 2017

This patch allows fetch-params to download the params from ipfs, if it's installed. This reduces the need for a central server and ensures the params are available if the official server ceases to exist.

For now, the default is still wget. If wget exits with an error, the script automatically tries ipfs. To use ipfs instead of wget:

ZC_DISABLE_WGET=1 ./zcutil/fetch-params.sh
@str4d

Concept ACK! I'd like to see some refactoring :)

Show outdated Hide outdated zcutil/fetch-params.sh

@str4d str4d added this to 1.0.12: Cleanup / Fixes in Release planning Aug 28, 2017

@str4d str4d added this to the 1.0.12 milestone Sep 4, 2017

@str4d str4d requested a review from arcalinea Sep 4, 2017

Show outdated Hide outdated zcutil/fetch-params.sh
@kpcyrd

This comment has been minimized.

Show comment
Hide comment
@kpcyrd

kpcyrd Sep 5, 2017

Contributor

Please let me know if I missed something. :)

Since zcash seems to be into rust, is there interest to have those scripts translated? I can see why shell might be preferable, but I could offer porting them if you give me some instructions on how you would like to have it integrated into the repo. (I'd prefer to handle this in a followup pull request though).

Contributor

kpcyrd commented Sep 5, 2017

Please let me know if I missed something. :)

Since zcash seems to be into rust, is there interest to have those scripts translated? I can see why shell might be preferable, but I could offer porting them if you give me some instructions on how you would like to have it integrated into the repo. (I'd prefer to handle this in a followup pull request though).

@str4d

Looking good! One small comment in-line.

Also, how about adding a message after the loop to check if the file succeeded? If there were no available methods (or all usable ones failed), users are told something like "Hey, try installing one of these programs and then run this again!"

Show outdated Hide outdated zcutil/fetch-params.sh
@str4d

This comment has been minimized.

Show comment
Hide comment
@str4d

str4d Sep 5, 2017

Contributor

Since zcash seems to be into rust, is there interest to have those scripts translated? I can see why shell might be preferable, but I could offer porting them if you give me some instructions on how you would like to have it integrated into the repo. (I'd prefer to handle this in a followup pull request though).

Interesting idea. Are you envisaging that we build a binary for downloading parameters instead of using a script? It would certainly be more portable, and it would also be a self-contained chunk of code which would make integration easier. We are still figuring out how exactly we will integrate Rust into Zcash, but a separate binary like this would likely be done in a different way to rewriting or extending internal zcashd code. You should open an issue for discussing this!

Contributor

str4d commented Sep 5, 2017

Since zcash seems to be into rust, is there interest to have those scripts translated? I can see why shell might be preferable, but I could offer porting them if you give me some instructions on how you would like to have it integrated into the repo. (I'd prefer to handle this in a followup pull request though).

Interesting idea. Are you envisaging that we build a binary for downloading parameters instead of using a script? It would certainly be more portable, and it would also be a self-contained chunk of code which would make integration easier. We are still figuring out how exactly we will integrate Rust into Zcash, but a separate binary like this would likely be done in a different way to rewriting or extending internal zcashd code. You should open an issue for discussing this!

@arcalinea

This comment has been minimized.

Show comment
Hide comment
@arcalinea

arcalinea Sep 5, 2017

Contributor

Successfully ran this and downloaded the zcash params from ipfs, great work!

Some notes:

  1. On first try, forgot to start my ipfs daemon, and got Error: merkledag not found (not a very informative error) before falling back on wget.
  2. On second try, got Error: Failed to get block for QmS8Ae5pM1PcPJJkWd5uTkDuSzhRiTYAczz67g1tr3oVEM: open /home/arcalinea/.ipfs/blocks/CIQDQ/CIQDQPAKGUFHCRNXONQROR7XC2RVBOZHPBJOKHNTR22WMG3FZ4ZVDJQ.data: too many open files before falling back on wget. Apparently this was a bug with older versions of ipfs, worked after I updated.
  3. On third try, after stopping the daemon and updating, got Error: api not running
  4. After re-starting the daemon again, success:
Retrieving (ipfs): /ipfs/QmZKKx7Xup7LiAtFRhYsE1M7waXcv9ir9eCECyXAFGxhEo/sprout-proving.key
Saving file(s) to /home/arcalinea/.zcash-params/sprout-proving.key.dl
 87.00 MB / 868.22 MB [============>---------------------------------------------------------------------------------------------]  10.02% 53s

Could be nice to add a line at the top reminding users to update and start their ipfs daemon if they intend to download from ipfs, since it's not clear that this script is going to try to do that.

We should also document this option in the 1.0 User Guide when we release.

ACK

Contributor

arcalinea commented Sep 5, 2017

Successfully ran this and downloaded the zcash params from ipfs, great work!

Some notes:

  1. On first try, forgot to start my ipfs daemon, and got Error: merkledag not found (not a very informative error) before falling back on wget.
  2. On second try, got Error: Failed to get block for QmS8Ae5pM1PcPJJkWd5uTkDuSzhRiTYAczz67g1tr3oVEM: open /home/arcalinea/.ipfs/blocks/CIQDQ/CIQDQPAKGUFHCRNXONQROR7XC2RVBOZHPBJOKHNTR22WMG3FZ4ZVDJQ.data: too many open files before falling back on wget. Apparently this was a bug with older versions of ipfs, worked after I updated.
  3. On third try, after stopping the daemon and updating, got Error: api not running
  4. After re-starting the daemon again, success:
Retrieving (ipfs): /ipfs/QmZKKx7Xup7LiAtFRhYsE1M7waXcv9ir9eCECyXAFGxhEo/sprout-proving.key
Saving file(s) to /home/arcalinea/.zcash-params/sprout-proving.key.dl
 87.00 MB / 868.22 MB [============>---------------------------------------------------------------------------------------------]  10.02% 53s

Could be nice to add a line at the top reminding users to update and start their ipfs daemon if they intend to download from ipfs, since it's not clear that this script is going to try to do that.

We should also document this option in the 1.0 User Guide when we release.

ACK

@str4d

str4d approved these changes Sep 6, 2017

All my comments have been addressed. ACK!

I'll hold off merging this for now, in case any of @arcalinea's non-blocking comments get addressed.

@kpcyrd

This comment has been minimized.

Show comment
Hide comment
@kpcyrd

kpcyrd Sep 6, 2017

Contributor

If there were no available methods (or all usable ones failed), users are told something like "Hey, try installing one of these programs and then run this again!"

I noticed this as well in tests, I've added an error message if every method failed.

I've tried to clean up the output a little bit by adding extra space, I'm not sure if it's the best way to do it, I'd revert that commit if there are any issues (multiline with cat might be considered bad style, I'm not sure how portable echo -e is, but it might be the better solution). :)

@arcalinea I'm still thinking about what's the best way to implement this. A running daemon is required for most cases, but it's still able to complete successfully if ipfs already has the blocks in its cache.

Are there any opinions about using colors in this script? A yellow warning if the daemon isn't running might be a good solution, but it would greatly increase complexity if it should only print that if stderr is not a tty. Do you have a prefer solution how it should behave in this case and what it should print?

Contributor

kpcyrd commented Sep 6, 2017

If there were no available methods (or all usable ones failed), users are told something like "Hey, try installing one of these programs and then run this again!"

I noticed this as well in tests, I've added an error message if every method failed.

I've tried to clean up the output a little bit by adding extra space, I'm not sure if it's the best way to do it, I'd revert that commit if there are any issues (multiline with cat might be considered bad style, I'm not sure how portable echo -e is, but it might be the better solution). :)

@arcalinea I'm still thinking about what's the best way to implement this. A running daemon is required for most cases, but it's still able to complete successfully if ipfs already has the blocks in its cache.

Are there any opinions about using colors in this script? A yellow warning if the daemon isn't running might be a good solution, but it would greatly increase complexity if it should only print that if stderr is not a tty. Do you have a prefer solution how it should behave in this case and what it should print?

@arcalinea

This comment has been minimized.

Show comment
Hide comment
@arcalinea

arcalinea Sep 14, 2017

Contributor

@kpcyrd I don't think colors are necessary, I was thinking of just adding a line at the top to the effect of:
"...If the files are already present and have the correct sha256sum, no networking is used. If downloading through ipfs, make sure the daemon is running."

But this is a non-blocking comment

Contributor

arcalinea commented Sep 14, 2017

@kpcyrd I don't think colors are necessary, I was thinking of just adding a line at the top to the effect of:
"...If the files are already present and have the correct sha256sum, no networking is used. If downloading through ipfs, make sure the daemon is running."

But this is a non-blocking comment

@whyrusleeping

This comment has been minimized.

Show comment
Hide comment
@whyrusleeping

whyrusleeping Sep 14, 2017

It might be best to add this as a well documented flag option for now (keep the default as pulling from s3) so that we can get some real users trying it out before forcing it as the default. Another potentially useful thing would be to have it try doing an http download from $IPFS_GATEWAY that defaults to ipfs.io but allows the user to override it with their own gateway. This mirrors fairly closely our "ipfs migration strategy", where the first step is to make sure that all your downloads, even if they are through http, reference hashes of content directly, then the second step is to actually fetch them (via ipfs) in a way that verifies their integrity continuously (e.g. ipfs get /ipfs/QmFooBar)

whyrusleeping commented Sep 14, 2017

It might be best to add this as a well documented flag option for now (keep the default as pulling from s3) so that we can get some real users trying it out before forcing it as the default. Another potentially useful thing would be to have it try doing an http download from $IPFS_GATEWAY that defaults to ipfs.io but allows the user to override it with their own gateway. This mirrors fairly closely our "ipfs migration strategy", where the first step is to make sure that all your downloads, even if they are through http, reference hashes of content directly, then the second step is to actually fetch them (via ipfs) in a way that verifies their integrity continuously (e.g. ipfs get /ipfs/QmFooBar)

@whyrusleeping

This comment has been minimized.

Show comment
Hide comment
@whyrusleeping

whyrusleeping commented Sep 14, 2017

I'd also like to just leave this here: https://news.ycombinator.com/item?id=15251469

Show outdated Hide outdated zcutil/fetch-params.sh
@str4d

NACK. Failed to run on my local machine that doesn't have IPFS installed.

I'd also like to see the commits squashed together.

Show outdated Hide outdated zcutil/fetch-params.sh
@str4d

This comment has been minimized.

Show comment
Hide comment
@str4d

str4d Sep 20, 2017

Contributor

@kpcyrd do you have time to address the above issues? If not, I will address them myself in around 10-12 hours.

Contributor

str4d commented Sep 20, 2017

@kpcyrd do you have time to address the above issues? If not, I will address them myself in around 10-12 hours.

@kpcyrd

This comment has been minimized.

Show comment
Hide comment
@kpcyrd

kpcyrd Sep 21, 2017

Contributor

@str4d I've addressed the crash, not sure how I missed that one. :) I've also changed the order to prefer wget over ipfs, I was waiting for some more opinions on this because this greatly reduces the chance that my feature gets any use, but I get the reasoning behind this.

Thanks for reviewing this everybody.

Contributor

kpcyrd commented Sep 21, 2017

@str4d I've addressed the crash, not sure how I missed that one. :) I've also changed the order to prefer wget over ipfs, I was waiting for some more opinions on this because this greatly reduces the chance that my feature gets any use, but I get the reasoning behind this.

Thanks for reviewing this everybody.

@str4d

str4d approved these changes Sep 21, 2017

@str4d

This comment has been minimized.

Show comment
Hide comment
@str4d

str4d Sep 21, 2017

Contributor

Excellent! When 1.0.12 is released, we will update the 1.0 User Guide to specify how ipfs users can try this out.

@zkbot r+

Contributor

str4d commented Sep 21, 2017

Excellent! When 1.0.12 is released, we will update the 1.0 User Guide to specify how ipfs users can try this out.

@zkbot r+

@zkbot

This comment has been minimized.

Show comment
Hide comment
@zkbot

zkbot Sep 21, 2017

Contributor

📌 Commit 076e177 has been approved by str4d

Contributor

zkbot commented Sep 21, 2017

📌 Commit 076e177 has been approved by str4d

@zkbot

This comment has been minimized.

Show comment
Hide comment
@zkbot

zkbot Sep 21, 2017

Contributor

⌛️ Testing commit 076e177 with merge 430a247...

Contributor

zkbot commented Sep 21, 2017

⌛️ Testing commit 076e177 with merge 430a247...

zkbot added a commit that referenced this pull request Sep 21, 2017

Auto merge of #2597 - kpcyrd:fetch-ipfs, r=str4d
Fetch params from ipfs if possible

This patch allows fetch-params to download the params from [ipfs], if it's installed. This reduces the need for a central server and ensures the params are available if the official server ceases to exist.

For now, the default is still wget. If wget exits with an error, the script automatically tries ipfs. To use ipfs instead of wget:

    ZC_DISABLE_WGET=1 ./zcutil/fetch-params.sh

[ipfs]: https://github.com/ipfs/go-ipfs
@zkbot

This comment has been minimized.

Show comment
Hide comment
@zkbot

zkbot Sep 21, 2017

Contributor

💔 Test failed - pr-merge

Contributor

zkbot commented Sep 21, 2017

💔 Test failed - pr-merge

@str4d

This comment has been minimized.

Show comment
Hide comment
@str4d

str4d Sep 21, 2017

Contributor

Another transient RPC test failure on the Debian 8 builder. It would be nice to figure out what causes these...

@zkbot retry

Contributor

str4d commented Sep 21, 2017

Another transient RPC test failure on the Debian 8 builder. It would be nice to figure out what causes these...

@zkbot retry

@zkbot

This comment has been minimized.

Show comment
Hide comment
@zkbot

zkbot Sep 21, 2017

Contributor

⌛️ Testing commit 076e177 with merge 1a9960c...

Contributor

zkbot commented Sep 21, 2017

⌛️ Testing commit 076e177 with merge 1a9960c...

zkbot added a commit that referenced this pull request Sep 21, 2017

Auto merge of #2597 - kpcyrd:fetch-ipfs, r=str4d
Fetch params from ipfs if possible

This patch allows fetch-params to download the params from [ipfs], if it's installed. This reduces the need for a central server and ensures the params are available if the official server ceases to exist.

For now, the default is still wget. If wget exits with an error, the script automatically tries ipfs. To use ipfs instead of wget:

    ZC_DISABLE_WGET=1 ./zcutil/fetch-params.sh

[ipfs]: https://github.com/ipfs/go-ipfs
@zkbot

This comment has been minimized.

Show comment
Hide comment
@zkbot

zkbot Sep 21, 2017

Contributor

💔 Test failed - pr-merge

Contributor

zkbot commented Sep 21, 2017

💔 Test failed - pr-merge

@str4d

This comment has been minimized.

Show comment
Hide comment
@str4d

str4d Sep 21, 2017

Contributor

@zkbot retry

Contributor

str4d commented Sep 21, 2017

@zkbot retry

@zkbot

This comment has been minimized.

Show comment
Hide comment
@zkbot

zkbot Sep 21, 2017

Contributor

⌛️ Testing commit 076e177 with merge 0eebdef...

Contributor

zkbot commented Sep 21, 2017

⌛️ Testing commit 076e177 with merge 0eebdef...

zkbot added a commit that referenced this pull request Sep 21, 2017

Auto merge of #2597 - kpcyrd:fetch-ipfs, r=str4d
Fetch params from ipfs if possible

This patch allows fetch-params to download the params from [ipfs], if it's installed. This reduces the need for a central server and ensures the params are available if the official server ceases to exist.

For now, the default is still wget. If wget exits with an error, the script automatically tries ipfs. To use ipfs instead of wget:

    ZC_DISABLE_WGET=1 ./zcutil/fetch-params.sh

[ipfs]: https://github.com/ipfs/go-ipfs
@kpcyrd

This comment has been minimized.

Show comment
Hide comment
@kpcyrd

kpcyrd Sep 21, 2017

Contributor

would it help if I rebase to the current master?

Contributor

kpcyrd commented Sep 21, 2017

would it help if I rebase to the current master?

@zkbot

This comment has been minimized.

Show comment
Hide comment
@zkbot

zkbot Sep 21, 2017

Contributor

☀️ Test successful - pr-merge
Approved by: str4d
Pushing 0eebdef to master...

Contributor

zkbot commented Sep 21, 2017

☀️ Test successful - pr-merge
Approved by: str4d
Pushing 0eebdef to master...

@zkbot zkbot merged commit 076e177 into zcash:master Sep 21, 2017

1 check passed

homu Test successful
Details

@kpcyrd kpcyrd deleted the kpcyrd:fetch-ipfs branch Sep 21, 2017

@str4d str4d removed this from 1.0.12: Cleanup / Fixes in Release planning Sep 28, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment