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

[mithril] Fix bug in checkUpdate / Fix export for signer / Fix PARTY_ID for verification / Add mithril client download to non container docs #1761

Merged
merged 7 commits into from
May 4, 2024

Conversation

TrevorBenson
Copy link
Collaborator

@TrevorBenson TrevorBenson commented Apr 17, 2024

  1. Includes a syntax fix for syntactic bug in entrypoint.sh #1757
  2. Removes unecessary UPDATE_CHECK=N exports in the workflow now that it is a default value in the Dockerfile ENV / Image.
  3. Exports G_ACCOUNT before calls to ./guild-deploy.sh to ensure if images are built in a fork that the files come from the fork, not cardano-community.
  4. Sets default G_ACCOUNT and GUILD_DEPLOY_BRANCH so manual docker build commands do not require passing --build-arg's except to alter default values.
  5. Fixes a bug leftover in the update logic and abstracts common functions from mithril scripts into a new mithril.library sourced by the scripts, reduces maintenance overhead and code duplication.
  6. Prepares for [Mithril] - Add version check if update_check is set to 'Y' #1744 by getting the minimum versions for cardano node from mithrils new networks.json file.
  7. Implements changes suggested by @Fuma419 since branch node-8.10.0 will merge into alpha, but not node-8.9.0.
  8. Fixes a bug leftover in the update logic

1756 update bug

  • This changes the call to use $(basename "$0") for any instance of a mithril script calling checkUpdate for itself, should future proof against script renames.
    • Also runs checkUpdate for env and mithril.library.

1759 changes to mithril-client

  • The cardano-db is now the base command

    • snapshot is relocated inside cardano-db
    • download is relocated from snapshot directly into cardano-db
  • Implemented cleanup on crashes as well as user interupts. A new node should either have a full copy of the most recent snapshot, or be empty so cnode.sh has no corruption or incomplete files preventing a good sync.

  • This also changes mithril-signer.sh

    • Existing flags in mithril-signer.sh changed to have a better overlap in mithril scripts using similar OPTARGS (-u always skip updates, etc.).
    • New flags implemented that integrates the upstream mithril scripts to Verify signer registration and signer signatures.

Documentation has been updated to account for changes in the scripts usage, flags, commands and subcommands. Also the MITHRIL_DOWNLOAD variable is documented into the node-cli.md.

closes #1756
closes #1757
closes #1759

@TrevorBenson TrevorBenson marked this pull request as draft April 19, 2024 03:01
@TrevorBenson TrevorBenson self-assigned this Apr 19, 2024
@TrevorBenson TrevorBenson marked this pull request as ready for review April 19, 2024 06:43
@TrevorBenson
Copy link
Collaborator Author

Taking this back out of draft. After further testing the edge case I thought I may have found seems specific to the container setup, not the branch or the images it generates.

@rdlrt
Copy link
Contributor

rdlrt commented Apr 19, 2024

The only bit am unsure is what does the title refer to (cncli.sh)? That file is not touched in the PR - was it something you encountered when using docker (entrypoint.sh)?

@Scitz0
Copy link
Contributor

Scitz0 commented Apr 19, 2024

The only bit am unsure is what does the title refer to (cncli.sh)? That file is not touched in the PR - was it something you encountered when using docker (entrypoint.sh)?

I think it started small and grew, hence the title.
This was the referenced issue, calling update on cncli.sh in mithril-client.sh
https://github.com/cardano-community/guild-operators/pull/1761/files#diff-dcb7fafc553f752e887e28294b4c42ffcea644be175251669936690333f576c1L144

@TrevorBenson TrevorBenson marked this pull request as draft April 19, 2024 18:54
@TrevorBenson
Copy link
Collaborator Author

TrevorBenson commented Apr 19, 2024

I found an issue that go hidden by the final exit message being >> redirected into the log instead of | tee. The sourcing of the mithril.env doesn't export the vars for mithril signer, and it doesn't have flags to explicitely pass each value like mithril-client does, so it ends up missing CARDANO_CLI_PATH and other variables.

@rdlrt Since you approved already I wanted to bring commit eac10bf to your attention. This loops over each line in mithril.env and exports them for the signer. I also changed logging from >> to | tee -a so that its not hidden from the user in the log if an issue occurs. This should also make the journalctl contain all the signer log content for review

@TrevorBenson TrevorBenson marked this pull request as ready for review April 19, 2024 20:51
@rdlrt
Copy link
Contributor

rdlrt commented Apr 21, 2024

@rdlrt Since you approved already I wanted to bring commit eac10bf to your attention.

I am alright with that, mithril in general is in early/fluid state and expect a lot of changes (personally, I treat it as in alpha state) - it's primary use-case that brings value is very simplistic (ofcourse, quite useful for those synching nodes) , bring nodes to sync using archive instead of validating chain, should one opt to.

Accordingly, I am not too fussed with how the script is being designed atm, once it stabilises enough (especially caters for multiple versions and the releases / tags are not dates 🙂 ), we can improve the scripts to be a bit more aligned.

@TrevorBenson TrevorBenson force-pushed the issue/1756-mithril-script-updates branch 4 times, most recently from e6a085c to 6d9bfb5 Compare April 28, 2024 18:47
@TrevorBenson TrevorBenson requested a review from rdlrt April 28, 2024 19:12
@TrevorBenson
Copy link
Collaborator Author

TrevorBenson commented Apr 28, 2024

@Scitz0 Support for Sanchonet added to the current PR as discussed/requested.

  • guild-deploy.sh has been modified to accept CARDANO_NODE_VERSION from the environment when defined.
  • Sanchonet / Preview often relies on the pre-release binaries for either mithril, cardano node, or both.
    • Sanchonet branch
      • Created from and rebased continuously on alpha.
      • Uses the latest pre-release of cardano-node binaries.
      • Uses the unstable release of mithril binaries (as suggested from mithril).
    • Preview
      • Created from and rebased continuously on alpha.
      • Uses the latest full release of cardano-node binaries.
      • Uses the pre-release of mithril binaries (as suggested from mithril).
        • Except when "${release}-pre" == "${prerelease} ", then it defaults to using the full release binaries.
  • The Docker Image, docker_bin.yml, workflow has been modified to account for new testing branches.
    • Uses the CNVERSION from the files/docker/node/release-versions/cardano-node-prerelease.txt contained in the sanchonet branch.
    • Sanchonet uses configs from cardano-community, so may result in incompatible configs to pre-release node when breaking changes exist in the pre-release.
      • An alternate approach might be to get sanchonet configs from the book.world.dev.cardano.org environments. However I've left this open for discussion since breaking changes in configs are not a guarantee from a pre-release, just a posibility.
    • Preview uses the CNVERSION from the files/docker/node/release-versions/cardano-node-latest.txt contained in the preview branch.
  • A new Autoupdate testing branches autoupdate-testing.yml workflow has been added.
    • Fetches the cardano-node and mithril pre-release versions and updates them in the sanchonet and preview branches.
    • Fetches the mithril unstable version and updates it in the sanchonet branch.
      • Currently this is unstable and does not change between releases. Logic included in case mithril changes to a XXXX.Y-unstable naming convention similar to what is used in pre-release versions, in which case the workflow and CI builds should continue to function as expected.
    • Rebases both preview and sanchonet on the alpha branch.
    • If any changes occur throughout the workflow the branches are updated. If no change occurs throughout the entire worflow no update/push occurs.

The cnode.sh env script appears to have strong dependencies on the koios script support. The preview branch works and only uses the version in alpha. However, the sanchonet container using the suggested node and mithril binary versions results in an error on cnode.sh startup:

Koios scripts have now been upgraded to support cardano-node 8.9.x ('8.10.1' found) / cardano-cli 8.20.x.x ('8.22.0.0' found).
Please update cardano-node binaries (ensure to read release notes and update various configs using guild-deploy (use appropriate options to download/install/overwrite parts you need) or use tagged branches for older node version (eg: ./<script>.sh -b node-8.1.2 to switch scripts to an older branch).

When asked about adding support for sanchonet I had not noticed the versionCheck for versions Koios is compatible with. My first thought is to add a environment variable which allows bypassing this check on sanchonet network. However I figured I would open this up for discussion in case there was a preference on the way to handle this for sanchonet.


@rdlrt This PR has additional content added around sanchonet, workflow changes, and modifications to guild-deploy.sh. As such I am clearing the current approval and requesting an additional review. Please let me know if you have any questions or suggestions.

Thanks

@rdlrt
Copy link
Contributor

rdlrt commented May 3, 2024

#@TrevorBenson - comments inline:

  • Sanchonet / Preview often relies on the pre-release binaries for either mithril, cardano node, or both.

    • Sanchonet branch
      • Created from and rebased continuously on alpha.
      • Uses the latest pre-release of cardano-node binaries.
      • Uses the unstable release of mithril binaries (as suggested from mithril).
    • Uses the CNVERSION from the files/docker/node/release-versions/cardano-node-prerelease.txt contained in the sanchonet branch.
    • Sanchonet uses configs from cardano-community, so may result in incompatible configs to pre-release node when breaking changes exist in the pre-release.

I dont think node for preview itself should rely on pre-release node binaries, it does not have fork dependency (where stable version doesnt work post a hard fork) atleast. Accordingly, I'd propose remove preview branch completely. I am OK with sanchonet branch and it's status above (as long as core scripts on alpha/master are not changed to add an error when there's a breaking change on test networks) 👍🏼 Accordingly, my comment to remove -pre reference in guild-deploy.sh

* An alternate approach might be to get sanchonet configs from the [book.world.dev.cardano.org environments](https://book.world.dev.cardano.org/environments.html). However I've left this open for discussion since breaking changes in configs are not a guarantee from a pre-release, just a posibility.

I am OK with using configs from book.play (it's quite confusing to have book.world vs book.play, but as per my understanding - more automated updates for pre-release go to book.play). The node binary itself is a bit of a playground atm, so I am not too worried about sanchonet nodes breaking between commits.
The only note is that using config from book.play will require adding config file manipulation to substitute logging and change some Tracers so that it doesnt break tools that read logs

  • Preview uses the CNVERSION from the files/docker/node/release-versions/cardano-node-latest.txt contained in the preview branch.

Preview should stick to stable node release (it is OK to use test/pre-release mithril versions), thus - should not require seperate branch.

The cnode.sh env script appears to have strong dependencies on the koios script support. The preview branch works and only uses the version in alpha. However, the sanchonet container using the suggested node and mithril binary versions results in an error on cnode.sh startup:

When asked about adding support for sanchonet I had not noticed the versionCheck for versions Koios is compatible with. My first thought is to add a environment variable which allows bypassing this check on sanchonet network. However I figured I would open this up for discussion in case there was a preference on the way to handle this for sanchonet.

@rdlrt This PR has additional content added around sanchonet, workflow changes, and modifications to guild-deploy.sh. As such I am clearing the current approval and requesting an additional review. Please let me know if you have any questions or suggestions.

versionCheck can be overridden by environment variable STRICT_VERSION_CHECK. Could be something for the workflow.

@rdlrt
Copy link
Contributor

rdlrt commented May 3, 2024

At a higher level, I'd recommend to keep the PR regarding sanchonet addition or workflow improvements seperate - mithril script changes could've been easier to merge =]
Becomes easier to review as well

@TrevorBenson TrevorBenson force-pushed the issue/1756-mithril-script-updates branch from 743ec11 to 444a316 Compare May 3, 2024 20:38
mithril-client binary command/subcommand changes replace snapshot with cardano-db #1759
@TrevorBenson TrevorBenson force-pushed the issue/1756-mithril-script-updates branch from 444a316 to 3fab72a Compare May 3, 2024 20:42
@TrevorBenson
Copy link
Collaborator Author

TrevorBenson commented May 3, 2024

At a higher level, I'd recommend to keep the PR regarding sanchonet addition or workflow improvements seperate - mithril script changes could've been easier to merge =] Becomes easier to review as well

The support for testing mithril and node pre-releases has been dropped, and will be opened in a separate PR.

@TrevorBenson
Copy link
Collaborator Author

versionCheck can be overridden by environment variable STRICT_VERSION_CHECK. Could be something for the workflow.

👍 I had overlooked the STRICT_VERSION_CHECK, but I tested this out yesterday on Sanchonet and it works perfectly.

@TrevorBenson
Copy link
Collaborator Author

I am OK with using configs from book.play (it's quite confusing to have book.world vs book.play, but as per my understanding - more automated updates for pre-release go to book.play). The node binary itself is a bit of a playground atm, so I am not too worried about sanchonet nodes breaking between commits. The only note is that using config from book.play will require adding config file manipulation to substitute logging and change some Tracers so that it doesnt break tools that read logs

I'll retool this for book.play and look for changes that would have been missed when comparing book.world to configurations in the repo.

@TrevorBenson
Copy link
Collaborator Author

TrevorBenson commented May 3, 2024

  • Preview uses the CNVERSION from the files/docker/node/release-versions/cardano-node-latest.txt contained in the preview branch.

Preview should stick to stable node release (it is OK to use test/pre-release mithril versions), thus - should not require seperate branch.

I'll split changes for handling pre-releases of cardano node and mithril binaries required in guild-deploy.sh out of alpha and into only sanchonet/preview. The workflow to automatically rebase these branches on changes to alpha should suffice to keep it functional until merge conflicts arise, in which case I will manually resolve them.


That said, the intent to run only a stable release on the Preview network doesn't quite match upstream suggestions. Both teams state that Preview is for testing pre-release (release candidates):

Creating a Docker Image from the preview (or sanchonet) branch should create a non public image. Per GitHub docs contributors to cardano-community/guild-operators won't even have access to the images. Someone with full control of the account would need to grant individual contributors access on the cardano community packages page. Or an SPO could build them in their own fork.

I believe this covers both use case :

  • Running a stable releases on Preview:
    • Using -b master, with stable/merged guild operators tools.
    • Using -b alpha, to test out the latest changes to guild operators tools
  • Running release candidates on Preview:
    • By using -b preview, which may result in non compatible combinations of cardano-node and cntools, cncli, gLiveView, but should be compatible for prerelease node and mithril client/signer.

We can discuss this further once a new PR is opened since the work is no longer inside this branch/PR.

@TrevorBenson TrevorBenson requested a review from rdlrt May 3, 2024 23:09
@TrevorBenson TrevorBenson changed the title [mithril] Fix bug in checkUpdate using cncli.sh [mithril] Fix bug in checkUpdate / Fix export for signer / Fix PARTY_ID for verification / Add mithril client download to non container docs May 3, 2024
@rdlrt
Copy link
Contributor

rdlrt commented May 3, 2024

That said, the intent to run only a stable release on the Preview network doesn't quite match upstream suggestions

While that maybe (I dont think I agree, some releases are specifically made available for sancho and later on said OK to run on preview) true for them, it does have a direct impact on scripts on this repository. The breaking changes added on releases need adjustments beyond download/run to be able to support. Having a higher version downloaded will automatically cause cascading effect of abundant requests to add support in scripts (which personally over the years I am not keen to - as the goal posts and comms can change quickly).

Another side-effect of having something ready is folks will reuse it preprod/mainnet too - we would not want to contribute to network issues. Sancho network is easy to reset if things go south, same cant be said for preview, as cardano does not have a comparable testnet with long enough history for any performance impact assessment on mainnet.

@TrevorBenson TrevorBenson merged commit e996aee into alpha May 4, 2024
3 checks passed
@TrevorBenson TrevorBenson deleted the issue/1756-mithril-script-updates branch May 4, 2024 15:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants