-
Notifications
You must be signed in to change notification settings - Fork 1
Add state-sync to SSC start.sh injected script #37
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
base: main
Are you sure you want to change the base?
Conversation
| @@ -1,5 +1,5 @@ | |||
| # Controller config | |||
| controller_version: "0.10.2" | |||
| controller_version: "0.11.0-beta.2" | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Revert this. We will be using this playbook to deploy the new controller config before we upgrade. So, the controller_version is the last thing we want to change.
| {% if ssc_genesis_url is defined %} | ||
| - name: GENESIS_URL | ||
| value: "{{ ssc_genesis_url }}" | ||
| {% endif %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You already have GENESIS. If you don't like the name you can rename it. Also, since this is built for this codebase, you can safely remove the if. In the new structure every env has defaults for this.
| value: "info" | ||
| - name: PEERS | ||
| value: "{{ ssc_peers }}" | ||
| - name: PERSISTENT_PEERS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PEERS are already here. The same as previous comment. You can rename if you prefer, but no reason to duplicate.
ansible/roles/ssc/templates/start.sh
Outdated
| GENESIS_URL=${GENESIS_URL:-${GENESIS:-""}} | ||
| SNAPSHOT_TRUST_INTERVAL=${SNAPSHOT_TRUST_INTERVAL:-1000} | ||
| SYNC_RPC=${SYNC_RPC:-""} | ||
| PERSISTENT_PEERS=${PERSISTENT_PEERS:-${PEERS:-""}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- We don't nee two variables for the peers and two for the genesis
- We don't need defaults to empty string either
- I'd just keep the snapshot trust interval.
|
|
||
| # Check if we should start from state sync | ||
| ShouldStartFromStateSync() { | ||
| if [ -z "$SYNC_RPC" ]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall we also pass STATE_SYNC_ENABLED? So, we have a kill switch. Probably cleaner.
ansible/roles/ssc/templates/start.sh
Outdated
| echo "Unable to fetch trust block. Skipping state-sync config" | ||
| return | ||
| fi | ||
| TRUST_HASH=$(curl -s "$RPC_SERVER/block?height=$TRUST_HEIGHT" | jq -r '.result.block_id.hash' 2>/dev/null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to refetch, you have the json in $TRUST_BLOCK. Also, same jq problem. Maybe we can do this:
TRUST_HASH=$(
echo $TRUST_BLOCK \
| awk -F'"hash":"' '{print $2}' \
| awk -F'"' '{print $1}'
)
| # Configure state sync if enabled | ||
| if ShouldStartFromStateSync; then | ||
| ConfigureStartFromStateSync | ||
| fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We want to add the snapshot interval so everyone will serve snapshots by default via p2p. It can be conditional and defaulted to true in the inventory
ansible/roles/ssc/templates/start.sh
Outdated
| ConfigureStartFromStateSync() { | ||
| echo "Configuring state sync..." | ||
| RPC_SERVER=$(echo "$SYNC_RPC" | awk -F"," '{gsub("tcp","http",$1);print $1}') | ||
| CURRENT_BLOCK=$(curl -s "$RPC_SERVER/status" | jq -r '.result.sync_info.latest_block_height' 2>/dev/null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PROBLEM: the ssc container doesn't have jq installed. A bit ugly but we can use this:
CURRENT_BLOCK=$(
curl -s "$RPC_SERVER/status" \
| grep -o '"latest_block_height":"[^"]*"' \
| sed 's/.*:"//;s/"$//'
)
emanuelconunaemme
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can close this, my change is working
| exit 1 | ||
| fi | ||
|
|
||
| # Apply IBC compatibility fixes to existing genesis file (runs every time) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any docs on why we need this? Seems overly complicated for devnets where you usually start with a fresh genesis file.
| fi | ||
| fi | ||
|
|
||
| # Use awk if Python failed or is not available |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would we have a backup option with awk if we control what is deployed?
Is this AI-generated? Embedding python and awk code seems like something that's going to be a nightmare to maintain.
| name: grpc | ||
| - containerPort: 26660 | ||
| name: metrics | ||
| command: ["/bin/sh", "/scripts/start.sh"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we even need a custom start script? SSC already has one (meant for the controller) that can already be used. We just need to have a way to generate the genesis and provide peers.
Add state-sync to SSC start.sh injected script