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

feat: add support for genesis download #2841

Closed
wants to merge 5 commits into from

Conversation

zivkovicmilos
Copy link
Member

Description

This PR introduces a genesis download subcommand, for downloading official genesis.json artifacts using the gnoland binary.

Contributors' checklist...
  • Added new tests, or not needed, or not feasible
  • Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory
  • Updated the official documentation or not needed
  • No breaking changes were made, or a BREAKING CHANGE: xxx message was included in the description
  • Added references to related issues and PRs
  • Provided any useful hints for running manual tests
  • Added new benchmarks to generated graphs, if any. More info here.

@zivkovicmilos zivkovicmilos added 🌱 feature New update to Gno 📦 ⛰️ gno.land Issues or PRs gno.land package related labels Sep 24, 2024
@zivkovicmilos zivkovicmilos self-assigned this Sep 24, 2024
Copy link

codecov bot commented Sep 24, 2024

Codecov Report

Attention: Patch coverage is 68.11594% with 22 lines in your changes missing coverage. Please review.

Project coverage is 60.96%. Comparing base (912a5db) to head (e3fb205).
Report is 9 commits behind head on master.

Files with missing lines Patch % Lines
gno.land/cmd/gnoland/genesis_download.go 67.64% 11 Missing and 11 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2841      +/-   ##
==========================================
- Coverage   60.97%   60.96%   -0.01%     
==========================================
  Files         564      565       +1     
  Lines       75273    75342      +69     
==========================================
+ Hits        45897    45935      +38     
- Misses      26008    26028      +20     
- Partials     3368     3379      +11     
Flag Coverage Δ
contribs/gnodev 60.65% <ø> (-0.82%) ⬇️
contribs/gnofaucet 15.31% <ø> (+0.85%) ⬆️
gno.land 67.93% <68.11%> (+<0.01%) ⬆️
gnovm 65.78% <ø> (ø)
misc/genstd 80.54% <ø> (ø)
misc/logos 20.23% <ø> (ø)
tm2 62.09% <ø> (-0.04%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@zivkovicmilos zivkovicmilos marked this pull request as ready for review September 25, 2024 16:17
@zivkovicmilos zivkovicmilos requested a review from a team as a code owner September 25, 2024 16:17
Comment on lines 24 to 30
const test4ID = "test4"

const deploymentPathFormat = "https://raw.githubusercontent.com/gnolang/gno/refs/heads/master/misc/deployments/%s.gno.land/genesis.json"

var genesisSHAMap = map[string]string{
test4ID: "beb781dffc09b96e3114fb7439fa85c4fe8ea796f64ec0cd3801a6b518ab023c",
}
Copy link
Member

Choose a reason for hiding this comment

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

This should be automated; or at least we should test the verification (ie. each source should map the sha sum, this should be an explicit test)

Also the link to genesis.json shouldn't be master, but a permalink like a commit. I think we can avoid making this dynamic with the %s, but instead have a struct with Source string; Checksum string

Copy link
Member Author

Choose a reason for hiding this comment

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

I adopted the metadata structure you were suggesting:

e3fb205

Our unit tests already cover actually fetching the genesis from the source and verifying it


defer resp.Body.Close()

f, err := os.Create(
Copy link
Member

Choose a reason for hiding this comment

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

os.CreateTemp?

Copy link
Member Author

Choose a reason for hiding this comment

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

Why would we use it instead of os.Create?

@albttx
Copy link
Member

albttx commented Sep 27, 2024

I don't understand why you're downloading the Genesis file from github, the genesis file is downloadable from any RPCs,

Btw if you check a cosmoshub rpc they have an extra endpoint /genesis_chunked?chunk=_, genesis file could be pretty huge sometime, (some chains hardfork, and get all previous state as txs in genesis)

https://rpc.test4.gno.land/genesis

@moul
Copy link
Member

moul commented Sep 27, 2024

Please wait for comments on this issue: #2824

I believe we should clean up gnoland by removing the genesis subcommand and moving the commands to a contribs binary.

@zivkovicmilos
Copy link
Member Author

I don't understand why you're downloading the Genesis file from github, the genesis file is downloadable from any RPCs,

A live RPC is not really the most reliable backup and restore mechanism, as 100% of our deployments are proof of that

@zivkovicmilos
Copy link
Member Author

zivkovicmilos commented Oct 9, 2024

Please wait for comments on this issue: #2824

I believe we should clean up gnoland by removing the genesis subcommand and moving the commands to a contribs binary.

@moul

We can always move this feature along with the command suite, I don't think it adds additional bloat we should worry about.

The primary use case I see is people who want to easily spin up their RPC / whatever cluster, with the existing gnoland binary on the machine, it would be easy peasy to not require a manual file by the node operator to provide

cc @sw360cab @r3v4s @mazzy89 what do you think about this sugar command?

@mazzy89
Copy link
Contributor

mazzy89 commented Oct 9, 2024

I do not see much benefit of having this command around. In our deployment model the genesis is passed as a json file inside a k8s ConfigMap and mounted in a volume of the container pod. Can't imagine any very useful place to have this command in our current flow.

@mazzy89
Copy link
Contributor

mazzy89 commented Oct 9, 2024

Actually looking better at our deployment flow, we pull the genesis at bootstrap time in an init container using curl. A simply curl command to pull it from Github or S3. I mean this command might be useful but nothing that a curl command does not already do.

@mazzy89
Copy link
Contributor

mazzy89 commented Oct 9, 2024

for the sake of completeness. how it looks our init container

  fetch-genesis:
    Container ID:  containerd://562154f389aad709a9a7c66b171e8889f5cfc1e82e7eb6601c90645264320ae9
    Image:         curlimages/curl:latest
    Image ID:      docker.io/curlimages/curl@sha256:8addc281f0ea517409209f76832b6ddc2cabc3264feb1ebbec2a2521ffad24e4
    Port:          <none>
    Host Port:     <none>
    Command:
      sh
      -c
      if [ ! -f /gnoland-data/genesis.json ]; then
        curl -o /gnoland-data/genesis.json "https://raw.githubusercontent.com/gnolang/gno/v0.2.0/misc/deployments/test4.gno.land/genesis.json"
      fi

@r3v4s
Copy link
Contributor

r3v4s commented Oct 10, 2024

@zivkovicmilos

I don't think it adds additional bloat we should worry about.

Agreed, this pr isn't really spaghetti

what do you think about this sugar command?

I really don't have specific opinion for this, as @mazzy89 said we can achieve same result with curl/wget.

However maybe we can use this command as internally for bootstrap script from our side. Or it can be used inside some docker image that doesn't support curl/wget by default.

@sw360cab
Copy link
Contributor

Actually looking better at our deployment flow, we pull the genesis at bootstrap time in an init container using curl. A simply curl command to pull it from Github or S3. I mean this command might be useful but nothing that a curl command does not already do.

IMO it can be useful if the goal is concentrating all the useful setup commands within the gnoland image.
From a DevOps POV this means referring always to this image, without leveraging other handy images, or requiring other extra tools within the image itself.

On the other hand, probably this subsubcommand is satisfying only a very small subset of use cases, the price of maintaining it can become higher that its real usefulness.

@zivkovicmilos
Copy link
Member Author

Closing this PR after comments from the ops crew 🙏

It's not a feature that should suck out maintain-time in the future :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 ⛰️ gno.land Issues or PRs gno.land package related 🌱 feature New update to Gno
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

7 participants