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

[Install script] Support multiple networks #941

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

okdas
Copy link
Member

@okdas okdas commented Nov 21, 2024

Summary

Allows to support multiple networks in the installation script.

Issue

Type of change

Select one or more from the following:

Testing

  • Documentation: make docusaurus_start; only needed if you make doc changes
  • Unit Tests: make go_develop_and_test
  • LocalNet E2E Tests: make test_e2e
  • DevNet E2E Tests: Add the devnet-test-e2e label to the PR.

Sanity Checklist

  • I have tested my changes using the available tooling
  • I have commented my code
  • I have performed a self-review of my own code; both comments & source code
  • I create and reference any new tickets, if applicable
  • I have left TODOs throughout the codebase, if applicable

@okdas okdas added the code health Cleans up some code label Nov 21, 2024
@okdas okdas added this to the Shannon Beta TestNet Launch milestone Nov 21, 2024
@okdas okdas self-assigned this Nov 21, 2024
sed -i -e "s|^seeds *=.*|seeds = \"$SEEDS\"|" \$HOME/.poktroll/config/config.toml
sed -i -e "s|^external_address *=.*|external_address = \"$EXTERNAL_IP:26656\"|" \$HOME/.poktroll/config/config.toml

Choose a reason for hiding this comment

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

[linter-name (fail-on-found)] reported by reviewdog 🐶
sed -i -e "s|^external_address =.|external_address = "$EXTERNAL_IP:26656"|" $HOME/.poktroll/config/config.toml

@okdas okdas marked this pull request as ready for review November 21, 2024 21:07
tools/installer/full-node.sh Outdated Show resolved Hide resolved
# Detect external IP address
EXTERNAL_IP=$(curl -s https://api.ipify.org)
print_color $YELLOW "Detected external IP address: $EXTERNAL_IP"
read -p "Is this your correct external IP address? (Y/n): " confirm_ip
Copy link
Member

Choose a reason for hiding this comment

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

Is anyone going to say no? Considering just commenting this out altogether

Copy link
Member Author

Choose a reason for hiding this comment

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

I suspect there might be cases when the discovered IP address is not correct and requires adjustment.

tools/installer/full-node.sh Outdated Show resolved Hide resolved
Copy link
Contributor

@bryanchriswhite bryanchriswhite left a comment

Choose a reason for hiding this comment

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

Very nice bashing sir! 😎 💥 Super easy to read. 🤩

Comment on lines +11 to +12
# DEV_NOTE: For testing purposes, you can change the branch name before merging to master.
GENESIS_BRANCH="master"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# DEV_NOTE: For testing purposes, you can change the branch name before merging to master.
GENESIS_BRANCH="master"
# DEV_NOTE: For testing purposes, you can change the branch name before merging to master.
POCKET_NETWORK_GENESIS_BRANCH="master"

read -p "Enter the chain-id (default: poktroll): " CHAIN_ID
CHAIN_ID=${CHAIN_ID:-"poktroll"}
# Update URLs to use the branch constant
BASE_URL="https://raw.githubusercontent.com/pokt-network/pocket-network-genesis/${GENESIS_BRANCH}/shannon/$NETWORK"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
BASE_URL="https://raw.githubusercontent.com/pokt-network/pocket-network-genesis/${GENESIS_BRANCH}/shannon/$NETWORK"
BASE_URL="https://raw.githubusercontent.com/pokt-network/pocket-network-genesis/${POCKET_NETWORK_GENESIS_BRANCH}/shannon/$NETWORK"

1) NETWORK="testnet-alpha" ;;
2) NETWORK="testnet-beta" ;;
3) NETWORK="mainnet" ;;
*) print_color $RED "Invalid choice. Exiting."; exit 1 ;;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
*) print_color $RED "Invalid choice. Exiting."; exit 1 ;;
*) print_color $RED "Invalid network choice. Exiting."; exit 1 ;;

*) print_color $RED "Invalid choice. Exiting."; exit 1 ;;
esac

print_color $GREEN "You have chosen to install the $NETWORK network."
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
print_color $GREEN "You have chosen to install the $NETWORK network."
print_color $GREEN "Installing the $NETWORK network."

if [ \$? -ne 0 ]; then
echo "Failed to download or extract binary"
exit 1
fi
chmod +x \$HOME/.poktroll/cosmovisor/genesis/bin/poktrolld
ln -sf \$HOME/.poktroll/cosmovisor/genesis/bin/poktrolld \$HOME/bin/poktrolld
Copy link
Contributor

Choose a reason for hiding this comment

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

Unrelated to these changes and likely out of scope but as this is the first time I'm reading this file, forgive my curiosity. 😅

Suggested change
ln -sf \$HOME/.poktroll/cosmovisor/genesis/bin/poktrolld \$HOME/bin/poktrolld
ln -sf \$HOME/.poktroll/cosmovisor/genesis/bin/poktrolld \$HOME/.local/bin/poktrolld

My understanding is that this aligns with the XDG user directory specification, which has wide adoption [1, 2].

I'm noticing that we seem to be creating this $HOME/bin directory. I'm wondering if it would be worth using an existing & conventional directory which is already included in the $PATH on most modern distros (except alpine, according to chatGPT). We could then remove the append to the $HOME/.profile:

-    echo 'export PATH=\$HOME/bin:\$PATH' >> \$HOME/.profile

[1] https://unix.stackexchange.com/questions/316765/which-distributions-have-home-local-bin-in-path
[2] https://askubuntu.com/questions/14535/whats-the-local-folder-for-in-my-home-directory

configure_ufw() {
if command -v ufw &> /dev/null; then
print_color $YELLOW "ufw is installed."
read -p "Do you want to open port 26656 for p2p communication? (Y/n): " open_port
Copy link
Contributor

Choose a reason for hiding this comment

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

Wdyt about printing a line about how to undo this in the future?

ufw allow 26656
print_color $GREEN "Port 26656 opened successfully."
else
print_color $YELLOW "Port 26656 not opened."
Copy link
Contributor

Choose a reason for hiding this comment

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

Does landing in this conditional branch really guarantee that the port is not open? If not, I would suggest we rephrase the message to be more precise (e.g. "Firewall rules not modified").

print_color $YELLOW "ufw is installed."
read -p "Do you want to open port 26656 for p2p communication? (Y/n): " open_port
if [[ $open_port =~ ^[Yy] ]]; then
ufw allow 26656
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need to check the existing rules and only add one if it's not already present; otherwise, it seems that duplicate rules accumulate. See this chatGPT convo.

print_color $YELLOW "Port 26656 not opened."
fi
else
print_color $YELLOW "ufw is not installed. Skipping port configuration."
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
print_color $YELLOW "ufw is not installed. Skipping port configuration."
print_color $YELLOW "ufw is not installed. Skipping firewall configuration."

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code health Cleans up some code
Projects
Status: 🏗 In progress
Development

Successfully merging this pull request may close these issues.

3 participants