fix: remove changing unnecessary parameters in single-node.sh #1386

rach-id · 2023-02-16T09:42:12Z

Overview

Checklist

New and updated code has appropriate documentation
New and updated code has new and/or updated testing
Required CI checks are passing
Visual proof for any user facing features like CLI or documentation updates
Linked issues closed with keywords

rach-id · 2023-02-16T09:42:51Z

scripts/single-node.sh

-sed -i'.bak' 's/index_all_keys = false/index_all_keys = true/g' ~/.celestia-app/config/config.toml
-sed -i'.bak' 's/mode = "full"/mode = "validator"/g' ~/.celestia-app/config/config.toml
+sed -i'.bak' 's#"tcp://localhost:26657"#"tcp://0.0.0.0:26657"#g' ~/.celestia-app/config/client.toml
+sed -i'.bak' 's/timeout_commit = "10s"/timeout_commit = "1s"/g' ~/.celestia-app/config/config.toml


[note for reviewers]
the defaults are now 10s

forgot to mention that #1386 (comment) changing these won't do anything. The only way to spin up a faster chain is programmatically.

I see, thanks 👍

so we can just delete these sed commands?

What about the config files that still contain those parameters, probably they should be deleted to avoid any confusion for validators, no?

so we can just delete these sed commands?

0c8806d

scripts/single-node.sh

rach-id · 2023-02-16T09:43:39Z

scripts/single-node.sh

@@ -20,11 +20,9 @@ celestia-appd collect-gentxs
 # Set proper defaults and change ports
 # If you encounter: `sed: -I or -i may not be used with stdin` on MacOS you can mitigate by installing gnu-sed
 # https://gist.github.com/andre3k1/e3a1a7133fded5de5a9ee99c87c6fa0d?permalink_comment_id=3082272#gistcomment-3082272
-sed -i'.bak' 's#"tcp://127.0.0.1:26657"#"tcp://0.0.0.0:26657"#g' ~/.celestia-app/config/config.toml


[note for reviewers]
this was moved to the client.toml file

when was this moved? it appears to be on line 91 of v0.12.0-rc7

Apparently sometime between v0.12.0-rc7 and latest master. I checked the history and it seems that only this 990c720 touches on similar stuff. Want me to investigate?

I think there are two separate fields being discussed here. With a fresh celestia-appd init foo, I observe:

config.toml

# TCP or UNIX socket address for the RPC server to listen on laddr = "tcp://127.0.0.1:26657"

client.toml

# <host>:<port> to Tendermint RPC interface for this chain node = "tcp://localhost:26657"

I compiled main and reintiated, and for me I'm still seeing the line on 91 of config.toml

edit: sorry github didn't update fast enough and I didn't see the above comment before posting mine

@rootulp Yes, just checked, we have both. Which refer to what? I guess they're the same no?

The client.toml contains the default configuration for running commands, cosmos/cosmos-sdk#8953.
So, I will revert the change: 10b99a6

I think line 23 still needs to be reverted to

sed -i'.bak' 's#"tcp://127.0.0.1:26657"#"tcp://0.0.0.0:26657"#g' ~/.celestia-app/config/config.toml

Good catch, messed the address

rach-id · 2023-02-16T09:58:24Z

The issue that I am seeing is that even after updating the timeout_commit and timeout_propose in config.toml to 1ms, the blocks are still being created using the default block time 10s. Are we hard-coding the 10s and not allowing to change?

Should I investigate or this is expected behaviour?

rach-id · 2023-02-16T11:32:22Z

Also, by default, we have this:

[tx_index]

# What indexer to use for transactions
#
# The application will set which txs to index. In some cases a node operator will be able
# to decide which txs to index based on configuration set in the application.
#
# Options:
#   1) "null"
#   2) "kv" (default) - the simplest possible indexer, backed by key-value storage (defaults to levelDB; see DBBackend).
#               - When "kv" is chosen "tx.height" and "tx.hash" will always be indexed.
#   3) "psql" - the indexer services backed by PostgreSQL.
# When "kv" or "psql" is chosen "tx.height" and "tx.hash" will always be indexed.
indexer = "null"

Why is the indexer disabled by default even if the docs state "kv" (default) -... ?

evan-forbes

one question

evan-forbes · 2023-02-16T13:01:00Z

scripts/single-node.sh

@@ -20,11 +20,9 @@ celestia-appd collect-gentxs
 # Set proper defaults and change ports
 # If you encounter: `sed: -I or -i may not be used with stdin` on MacOS you can mitigate by installing gnu-sed
 # https://gist.github.com/andre3k1/e3a1a7133fded5de5a9ee99c87c6fa0d?permalink_comment_id=3082272#gistcomment-3082272
-sed -i'.bak' 's#"tcp://127.0.0.1:26657"#"tcp://0.0.0.0:26657"#g' ~/.celestia-app/config/config.toml


when was this moved? it appears to be on line 91 of v0.12.0-rc7

evan-forbes · 2023-02-16T13:43:30Z

Are we hard-coding the 10s and not allowing to change?

yeah, unfortunately. if we use the cli, then consensus timeouts are overridden. Ideally we make then params and not configs.

evan-forbes · 2023-02-16T13:44:31Z

Why is the indexer disabled by default even if the docs state "kv" (default) -... ?

that's another good point, we changed the defaults here in the app, but those docs are in tendermint

rach-id · 2023-02-16T14:22:01Z

that's another good point, we changed the defaults here in the app, but those docs are in tendermint

should we do the change? or no need?

evan-forbes · 2023-02-16T14:30:40Z

should we do the change? or no need?

probably just a low priority change in tendermint

rootulp

I don't understand what the bug is or how this PR addresses it. @sweexordious do you mind creating an issue with repro steps and expected behavior?

rootulp · 2023-02-16T16:04:27Z

scripts/single-node.sh

@@ -20,11 +20,9 @@ celestia-appd collect-gentxs
 # Set proper defaults and change ports
 # If you encounter: `sed: -I or -i may not be used with stdin` on MacOS you can mitigate by installing gnu-sed
 # https://gist.github.com/andre3k1/e3a1a7133fded5de5a9ee99c87c6fa0d?permalink_comment_id=3082272#gistcomment-3082272
-sed -i'.bak' 's#"tcp://127.0.0.1:26657"#"tcp://0.0.0.0:26657"#g' ~/.celestia-app/config/config.toml


I think there are two separate fields being discussed here. With a fresh celestia-appd init foo, I observe:

config.toml

# TCP or UNIX socket address for the RPC server to listen on laddr = "tcp://127.0.0.1:26657"

client.toml

# <host>:<port> to Tendermint RPC interface for this chain node = "tcp://localhost:26657"

scripts/single-node.sh

rach-id · 2023-02-16T21:12:12Z

I don't understand what the bug is or how this PR addresses it. @sweexordious do you mind creating an issue with repro steps and expected behavior?

I added the bug label because I thought there is a bug as timeout_commit was not picked up, before I knew it's expected because those values are hardcoded.

I will update the title of the PR to something more meaningful now as the discussion can be switched to correcting the single-node.sh script to have correct commands (and delete commands that are not making any change to the resulting chain)

rach-id · 2023-02-17T11:33:12Z

PR updated, the new purpose of this PR is to remove these commands that are not making any change:

sed -i'.bak' 's/timeout_commit = "25s"/timeout_commit = "1s"/g' ~/.celestia-app/config/config.toml
sed -i'.bak' 's/timeout_propose = "3s"/timeout_propose = "1s"/g' ~/.celestia-app/config/config.toml
sed -i'.bak' 's/index_all_keys = false/index_all_keys = true/g' ~/.celestia-app/config/config.toml
sed -i'.bak' 's/mode = "full"/mode = "validator"/g' ~/.celestia-app/config/config.toml

I guess all the conversations are now resolved. Let me know if I should address something else

rootulp · 2023-02-17T13:58:15Z

scripts/single-node.sh

@@ -20,11 +20,9 @@ celestia-appd collect-gentxs
 # Set proper defaults and change ports
 # If you encounter: `sed: -I or -i may not be used with stdin` on MacOS you can mitigate by installing gnu-sed
 # https://gist.github.com/andre3k1/e3a1a7133fded5de5a9ee99c87c6fa0d?permalink_comment_id=3082272#gistcomment-3082272
-sed -i'.bak' 's#"tcp://127.0.0.1:26657"#"tcp://0.0.0.0:26657"#g' ~/.celestia-app/config/config.toml


I think line 23 still needs to be reverted to

sed -i'.bak' 's#"tcp://127.0.0.1:26657"#"tcp://0.0.0.0:26657"#g' ~/.celestia-app/config/config.toml

scripts/single-node.sh

fix: update the overwritten parameters in single-node.sh

15580c9

rach-id added the bug Something isn't working label Feb 16, 2023

rach-id self-assigned this Feb 16, 2023

rach-id requested a review from evan-forbes as a code owner February 16, 2023 09:42

MSevey requested review from a team and cmwaters and removed request for a team February 16, 2023 09:42

rach-id commented Feb 16, 2023

View reviewed changes

evan-forbes reviewed Feb 16, 2023

View reviewed changes

rach-id mentioned this pull request Feb 16, 2023

Update docs for tx indexing celestiaorg/celestia-core#967

Open

fix: remove unecessary seds

0c8806d

MSevey requested a review from a team February 16, 2023 14:37

rootulp reviewed Feb 16, 2023

View reviewed changes

rach-id removed the bug Something isn't working label Feb 16, 2023

rach-id changed the title ~~fix: update the overwritten parameters in single-node.sh~~ fix: update single-node.sh for latest changes Feb 16, 2023

fix: use config.toml instead of client.toml

10b99a6

MSevey requested a review from a team February 17, 2023 11:29

rach-id changed the title ~~fix: update single-node.sh for latest changes~~ fix: remove changing unnecessary parameters in single-node.sh Feb 17, 2023

rach-id requested review from rootulp and evan-forbes February 17, 2023 11:33

evan-forbes previously approved these changes Feb 17, 2023

View reviewed changes

rootulp requested changes Feb 17, 2023

View reviewed changes

rach-id commented Feb 17, 2023

View reviewed changes

scripts/single-node.sh Outdated Show resolved Hide resolved

rach-id dismissed evan-forbes’s stale review via ecdfb35 February 17, 2023 14:37

Update scripts/single-node.sh

ecdfb35

MSevey requested a review from a team February 17, 2023 14:37

rach-id requested a review from rootulp February 17, 2023 14:37

rootulp approved these changes Feb 17, 2023

View reviewed changes

rach-id requested a review from evan-forbes February 17, 2023 21:34

evan-forbes approved these changes Feb 20, 2023

View reviewed changes

rach-id merged commit 0e27c05 into celestiaorg:main Feb 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: remove changing unnecessary parameters in single-node.sh #1386

fix: remove changing unnecessary parameters in single-node.sh #1386

rach-id commented Feb 16, 2023

rach-id Feb 16, 2023 •

edited

Loading

evan-forbes Feb 16, 2023

rach-id Feb 16, 2023

rach-id Feb 16, 2023

evan-forbes Feb 16, 2023

rach-id Feb 16, 2023

rach-id Feb 16, 2023

rach-id Feb 16, 2023 •

edited

Loading

evan-forbes Feb 16, 2023

rach-id Feb 16, 2023 •

edited

Loading

rootulp Feb 16, 2023

evan-forbes Feb 16, 2023 •

edited

Loading

rach-id Feb 16, 2023

rach-id Feb 17, 2023

rootulp Feb 17, 2023

rach-id Feb 17, 2023

rach-id commented Feb 16, 2023 •

edited

Loading

rach-id commented Feb 16, 2023 •

edited

Loading

evan-forbes left a comment

evan-forbes Feb 16, 2023

evan-forbes commented Feb 16, 2023

evan-forbes commented Feb 16, 2023

rach-id commented Feb 16, 2023

evan-forbes commented Feb 16, 2023

rootulp left a comment

rootulp Feb 16, 2023

rach-id commented Feb 16, 2023

rach-id commented Feb 17, 2023

rootulp Feb 17, 2023

fix: remove changing unnecessary parameters in single-node.sh #1386

fix: remove changing unnecessary parameters in single-node.sh #1386

Conversation

rach-id commented Feb 16, 2023

Overview

Checklist

rach-id Feb 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rach-id Feb 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rach-id Feb 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evan-forbes Feb 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rach-id commented Feb 16, 2023 • edited Loading

rach-id commented Feb 16, 2023 • edited Loading

evan-forbes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evan-forbes commented Feb 16, 2023

evan-forbes commented Feb 16, 2023

rach-id commented Feb 16, 2023

evan-forbes commented Feb 16, 2023

rootulp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rach-id commented Feb 16, 2023

rach-id commented Feb 17, 2023

Choose a reason for hiding this comment

rach-id Feb 16, 2023 •

edited

Loading

rach-id Feb 16, 2023 •

edited

Loading

rach-id Feb 16, 2023 •

edited

Loading

evan-forbes Feb 16, 2023 •

edited

Loading

rach-id commented Feb 16, 2023 •

edited

Loading

rach-id commented Feb 16, 2023 •

edited

Loading