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

Fix: Remove logic that trimmed the last byte from transaction ID in publishReferenceScripts function (Demo Setup) #1782

Merged
merged 3 commits into from
Jan 13, 2025

Conversation

f0rm4l1ty
Copy link
Contributor

I reported this in Discord, but this is a fix for the Hydra Head "Getting started" demo located here https://hydra.family/head-protocol/docs/getting-started

Running the ./seed-devnet.sh command would invoke a function called publishReferenceScripts that saves a transaction id to the .env file named HYDRA_SCRIPTS_TX_ID. This function included a piped command: head -c -1 which removed the last byte from the outputted transaction id.

This must have been utilized in a previous version, however now it incorrectly trims the last byte from the transaction id, causing it to be invalid (incorrect length). Running the docker compose up -d hydra-node-{1,2,3} would cause an error like so, and cause the containers to crash and keep restarting:

option --hydra-scripts-tx-id: RawBytesHexErrorBase16DecodeFail "0e2114c7a09c171ce8a8a5ba282fa2a47056a1c6415d4aaecbbb44093cdf2e7" "invalid bytestring size"

I have confirmed that removing this specific command fixes the issue and the Hydra nodes successfully start up.


  • CHANGELOG updated or not needed
  • Documentation updated or not needed
  • Haddocks updated or not needed
  • No new TODOs introduced or explained herafter

…ublishReferenceScripts function

I reported this in Discord, but this is a fix for the Hydra Head "Getting started" demo located here https://hydra.family/head-protocol/docs/getting-started

Running the `./seed-devnet.sh` command would invoke a function called `publishReferenceScripts` that saves a transaction id to the `.env` file named `HYDRA_SCRIPTS_TX_ID`. This function included a command: `head -c -1` which removed the last byte from outputted transaction id.

This must have been utilized in a previous version, however now it incorrectly removes the last byte from the transaction id, causing it to be invalid (incorrect length). Running the `docker compose up -d hydra-node-{1,2,3}` would cause an error like so and cause the containers to crash and keep restarting:
```
option --hydra-scripts-tx-id: RawBytesHexErrorBase16DecodeFail "0e2114c7a09c171ce8a8a5ba282fa2a47056a1c6415d4aaecbbb44093cdf2e7" "invalid bytestring size"
```

I have confirmed that removing this specific command fixes the issue and the Hydra nodes successfully start up.
@noonio
Copy link
Contributor

noonio commented Jan 13, 2025

Thanks for looking into this @f0rm4l1ty !

Can you tell me what the output of this command is, on your computer?

echo "a\nb\nc" | tr '\n' ','

For me, on NixOS, I get a trailing comma. I'm assuming on your system you don't get that?

-- Edit: Disregard, this isn't the problem.

@noonio
Copy link
Contributor

noonio commented Jan 13, 2025

Ah, I think I see the problem @f0rm4l1ty ; in fact it's because that code has been changed in preparation for an upcoming release of 0.20.0, where we need the extra-byte removal ( 🥲 ) as there are multiple script ids printed on newlines.

When you run it via the demo, you don't have that version (it's using 0.19.0) so it just incorrectly removes the final byte.

I think perhaps the best idea for now is to accept this PR, which reverts it to 0.19.0 behaviour, and then we will make this change again when the demo uses 0.20.0.

Thanks for finding this!

@noonio noonio self-requested a review January 13, 2025 12:15
Copy link
Contributor

@noonio noonio left a comment

Choose a reason for hiding this comment

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

Approving; the code here was updated for 0.20.0 of hydra-node (which prints a list of scripts instead of a single one).

So we can just remove it entirely.

Thanks!

@noonio noonio enabled auto-merge January 13, 2025 12:23
@noonio noonio disabled auto-merge January 13, 2025 13:31
@noonio noonio merged commit 5748b3c into cardano-scaling:master Jan 13, 2025
15 of 26 checks passed
github-merge-queue bot pushed a commit that referenced this pull request Jan 13, 2025
…ished script addresses (#1784)

The recent PR #1782 removed behaviour that the _network tests_ relied on
(as they use the latest code from this repo) but _broke_ the demo.

I think the essential lesson is at least a few things:

1. We need to test the demo in CI
2. One script doing two things is confusing
@f0rm4l1ty f0rm4l1ty deleted the demo-fix branch January 14, 2025 05:26
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.

2 participants