-
Notifications
You must be signed in to change notification settings - Fork 179
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
Conversation
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. |
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. |
I found an issue that go hidden by the final exit message being @rdlrt Since you approved already I wanted to bring commit eac10bf to your attention. This loops over each line in |
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. |
e6a085c
to
6d9bfb5
Compare
@Scitz0 Support for Sanchonet added to the current PR as discussed/requested.
The
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 Thanks |
#@TrevorBenson - comments inline:
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
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.
Preview should stick to stable node release (it is OK to use test/pre-release mithril versions), thus - should not require seperate branch.
|
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 =] |
743ec11
to
444a316
Compare
… image ENV variables
mithril-client binary command/subcommand changes replace snapshot with cardano-db #1759
use tee for logging to not hide issues/errors
444a316
to
3fab72a
Compare
The support for testing mithril and node pre-releases has been dropped, and will be opened in a separate PR. |
👍 I had overlooked the |
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. |
I'll split changes for handling pre-releases of cardano node and mithril binaries required in 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 I believe this covers both use case :
We can discuss this further once a new PR is opened since the work is no longer inside this branch/PR. |
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. |
./guild-deploy.sh
to ensure if images are built in a fork that the files come from the fork, not cardano-community.docker build
commands do not require passing--build-arg
's except to alter default values.networks.json
file.1756 update bug
$(basename "$0")
for any instance of a mithril script calling checkUpdate for itself, should future proof against script renames.env
andmithril.library
.1759 changes to mithril-client
The cardano-db is now the base command
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
mithril-signer.sh
changed to have a better overlap in mithril scripts using similar OPTARGS (-u always skip updates, etc.).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