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: --syncmode is empty string #156

Merged
merged 1 commit into from
Aug 29, 2024
Merged

Conversation

quertc
Copy link
Contributor

@quertc quertc commented Jul 26, 2024

OP_GETH_SYNCMODE in .env file is set but is empty string. Condition is never met.

Or instead of this PR, you can add to .env:

# Uncomment the following line to force op-geth to use --syncmode=full
# OP_GETH__SYNCMODE=full

OP_GETH_SYNCMODE in .env file is set but is empty string. Condition is never met.
@Chomtana
Copy link
Collaborator

Thank you. Will look into +x things

@supershania
Copy link

supershania commented Aug 23, 2024

As a side note, all scripts have the same problem where ${VARIABLE+x} is used. The meaning of using ${OP_GETH__SYNCMODE+x} is:

  • ${OP_GETH__SYNCMODE+x}: This syntax checks if the variable OP_GETH__SYNCMODE is set.
  • If OP_GETH__SYNCMODE is set (regardless of whether it has a value or is empty), ${OP_GETH__SYNCMODE+x} will expand to the string x.
  • If OP_GETH__SYNCMODE is not set, the expression will expand to an empty string.

When the .env file is injected into the docker environment, all variables are set as the value or empty string.

...
env_file:
      - ./envs/${NETWORK_NAME}/op-geth.env
      - .env
...

I made a test below:

# Assuming the .env file is injected as bellow
NODE_TYPE=full
OP_GETH__SYNCMODE=

echo "OP_GETH__SYNCMODE+x=${OP_GETH__SYNCMODE+x};"

# The following code comes from scripts/start-op-geth.sh
# Determine syncmode based on NODE_TYPE
if [ -z "${OP_GETH__SYNCMODE+x}" ]; then
  if [ "$NODE_TYPE" = "full" ]; then
    export OP_GETH__SYNCMODE="snap"
    OP_GETH__SYNCMODE="snap"
  else
    export OP_GETH__SYNCMODE="full"
    OP_GETH__SYNCMODE="full"
  fi
fi

echo "OP_GETH__SYNCMODE=${OP_GETH__SYNCMODE};"

and the output is:

OP_GETH__SYNCMODE+x=x;
OP_GETH__SYNCMODE=;

fixed as the commit:

# Assuming the .env file is injected as bellow
NODE_TYPE=full
OP_GETH__SYNCMODE=

echo "OP_GETH__SYNCMODE+x=${OP_GETH__SYNCMODE+x};"

# The following code comes from scripts/start-op-geth.sh in this commit
# Determine syncmode based on NODE_TYPE
if [ -z "${OP_GETH__SYNCMODE}" ]; then  # not to use +x here
  if [ "$NODE_TYPE" = "full" ]; then
    export OP_GETH__SYNCMODE="snap"
    OP_GETH__SYNCMODE="snap"
  else
    export OP_GETH__SYNCMODE="full"
    OP_GETH__SYNCMODE="full"
  fi
fi

echo "OP_GETH__SYNCMODE=${OP_GETH__SYNCMODE};"

we will get the right result:

OP_GETH__SYNCMODE+x=x;
OP_GETH__SYNCMODE=snap;

@supershania
Copy link

OP_GETH_SYNCMODE in .env file is set but is empty string. Condition is never met.

Or instead of this PR, you can add to .env:

# Uncomment the following line to force op-geth to use --syncmode=full
# OP_GETH__SYNCMODE=full

I think it's unsuitable to set OP_GETH__SYNCMODE to "full" in .env forcibly.

@Chomtana
Copy link
Collaborator

The root cause is

set -eou

-u option make command error if the variable is unset. Removing ou option allow us to avoid +x

I think it's unsuitable to set OP_GETH__SYNCMODE to "full" in .env forcibly.

This shouldn't be set by default, and it will choose the most appropriate sync mode automatically.

@Chomtana Chomtana mentioned this pull request Aug 29, 2024
@Chomtana Chomtana merged commit cdb75a4 into smartcontracts:main Aug 29, 2024
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.

3 participants