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

feat: p2p: allow overriding bootstrap nodes with environmemnt variable #12292

Merged

Conversation

hanabi1224
Copy link
Contributor

@hanabi1224 hanabi1224 commented Jul 24, 2024

Related Issues

ChainSafe/forest#4576
ChainSafe/forest#4580
ChainSafe/forest#4581
#12289

Proposed Changes

Allow overriding bootstrap nodes at runtime with network specific environment variables

Additional Info

Checklist

Before you mark the PR ready for review, please make sure that:

  • Commits have a clear commit message.
  • PR title is in the form of of <PR type>: <area>: <change being made>
    • example: fix: mempool: Introduce a cache for valid signatures
    • PR type: fix, feat, build, chore, ci, docs, perf, refactor, revert, style, test
    • area, e.g. api, chain, state, mempool, multisig, networking, paych, proving, sealing, wallet, deps
  • Update CHANGELOG.md or signal that this change does not need it.
    • If the PR affects users (e.g., new feature, bug fix, system requirements change), update the CHANGELOG.md and add details to the UNRELEASED section.
    • If the change does not require a CHANGELOG.md entry, do one of the following:
      • Add [skip changelog] to the PR title
      • Add the label skip/changelog to the PR
  • New features have usage guidelines and / or documentation updates in
  • Tests exist for new functionality or change in behavior
  • CI is green

@hanabi1224 hanabi1224 marked this pull request as ready for review July 24, 2024 12:32
@rvagg
Copy link
Member

rvagg commented Jul 25, 2024

Is there a good reason to make new env vars for the different network types? Wouldn't a single LOTUS_P2P_BOOTSTRAPPERS suffice for all of them?

@hanabi1224
Copy link
Contributor Author

hanabi1224 commented Jul 25, 2024

Is there a good reason to make new env vars for the different network types? Wouldn't a single LOTUS_P2P_BOOTSTRAPPERS suffice for all of them?

Hi @rvagg I was assuming a developer could run different networks on the same machine(not necessarily at the same time), maybe devnet with LOTUS_P2P_BOOTSTRAPPERS_DEVNET then calibnet, in this case if it's a shared variable, they will always need to change the value whenever they switch the network. Does that make sense?

@rvagg
Copy link
Member

rvagg commented Jul 25, 2024

Not really, this seems like the kind of env var you'd want to localise to the instance itself, not set in the global environment, so it gets targeted to whatever instance you're running. We also don't have a precedent of doing this, I don't believe there's ever been a need (git grep .Getenv.*LOTUS | awk -F'"' '{print $2}' | sort | uniq). It might make sense if you could have two network types in the same lotus instance, but you can't, you have one path through the config generation, in fact, it's specific to a compiled binary.

Unless you have a specific workflow in mind how you'd consume this that would make it particularly awkward to switch for the different network types?

@hanabi1224
Copy link
Contributor Author

@rvagg changed to using a single LOTUS_P2P_BOOTSTRAPPERS

@rvagg rvagg merged commit 9c9bb95 into filecoin-project:master Jul 26, 2024
82 checks passed
@rvagg rvagg mentioned this pull request Jul 29, 2024
@hanabi1224 hanabi1224 deleted the hm/override-bootstrappers-env-var branch August 16, 2024 07:36
ribasushi pushed a commit to ribasushi/ci-abusing-lotus-fork that referenced this pull request Aug 20, 2024
ribasushi pushed a commit to ribasushi/ci-abusing-lotus-fork that referenced this pull request Aug 20, 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.

2 participants