Skip to content

Conversation

@jjeangal
Copy link
Member

@jjeangal jjeangal commented Apr 7, 2025

Closes this and that Asana Task

This pull request includes several updates to test out Nitro version 3.5.2 and nitro contracts 2.1.3.

This PR:

  • Updates the Espresso node Docker image to use the matching release (release-goldendoodle)
  • Updates Nitro contracts version to cff556bc886a4383a3936dce206ceef0b019fd62 (v2.1.3)
  • Updates Nitro contracts branch to v2.1.3-alpha (tag)
  • Adds a readiness check for the sequencer HTTP service in smoke tests to ensure proper initialization before test execution

This PR does not:

  • Modify the core functionality of the test node
  • Change the underlying architecture or deployment process
  • Update any other dependencies or configurations

Key places to review:

  • docker-compose.yaml: Changes to the Espresso node image configuration
  • test-node.bash: Updates to Nitro contracts version and branch
  • smoke-test.bash: New sequencer HTTP readiness check implementation

How to test this PR:

  • Run the smoke test script: ./smoke-test.bash
  • Test should exit with message: Number of confirmed staking nodes: 1

@jjeangal jjeangal requested a review from lukeiannucci April 7, 2025 18:09

# Wait for the sequencer HTTP to be ready
echo "Waiting for sequencer HTTP to be ready..."
while ! curl -sf http://localhost:8547 > /dev/null; do

Choose a reason for hiding this comment

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

do we want some time limit on this or will this always eventually be ready?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question, it should normally be ready at some point but nothing wrong in adding an extra check. I'm just not sure what should be the higher bound.

Choose a reason for hiding this comment

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

it might not be necessary, i was just curious

Copy link
Member

@zacshowa zacshowa Apr 29, 2025

Choose a reason for hiding this comment

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

It won't be necessary for CI but I think for the sake of not keeping things running longer than they need to we could fail and exit the script after a certain number of iterations to impose a time limit. However, we can do it in another PR.

Something like this maybe.

Suggested change
while ! curl -sf http://localhost:8547 > /dev/null; do
max_iterations=12 # Multiplied by the sleep value in the loop to calculate how long the sequencer has to start.
iterations=0
while ! curl -sf http://localhost:8547 > /dev/null; do
echo "Sequencer HTTP is not ready yet. Retrying in 5 seconds..."
sleep 5
if [$iterations -eq $max_iterations]; then
fail "Could not connect to Sequencer HTTP server after $max_iterations attempts."
fi
((iterations +=1))
done

Where fail is some function like this from the migration test:

# This is a utility function for exiting with with error strings
fail(){
    echo "$*" 1>&2; exit 1;
}

Copy link

@Sneh1999 Sneh1999 left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @jjeangal 🎉

Copy link

@lukeiannucci lukeiannucci left a comment

Choose a reason for hiding this comment

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

nice! 🐐

Copy link
Member

@zacshowa zacshowa left a comment

Choose a reason for hiding this comment

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

This lgtm for the update. We can improve the smoke test, but I think we can worry about it later. Also, thanks for the great PR description!


# Wait for the sequencer HTTP to be ready
echo "Waiting for sequencer HTTP to be ready..."
while ! curl -sf http://localhost:8547 > /dev/null; do
Copy link
Member

@zacshowa zacshowa Apr 29, 2025

Choose a reason for hiding this comment

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

It won't be necessary for CI but I think for the sake of not keeping things running longer than they need to we could fail and exit the script after a certain number of iterations to impose a time limit. However, we can do it in another PR.

Something like this maybe.

Suggested change
while ! curl -sf http://localhost:8547 > /dev/null; do
max_iterations=12 # Multiplied by the sleep value in the loop to calculate how long the sequencer has to start.
iterations=0
while ! curl -sf http://localhost:8547 > /dev/null; do
echo "Sequencer HTTP is not ready yet. Retrying in 5 seconds..."
sleep 5
if [$iterations -eq $max_iterations]; then
fail "Could not connect to Sequencer HTTP server after $max_iterations attempts."
fi
((iterations +=1))
done

Where fail is some function like this from the migration test:

# This is a utility function for exiting with with error strings
fail(){
    echo "$*" 1>&2; exit 1;
}

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.

5 participants