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

Increase default recv_rate and send_rate #4213

Closed
rootulp opened this issue Jan 13, 2025 · 6 comments · Fixed by celestiaorg/cosmos-sdk#425 or #4220
Closed

Increase default recv_rate and send_rate #4213

rootulp opened this issue Jan 13, 2025 · 6 comments · Fixed by celestiaorg/cosmos-sdk#425 or #4220
Assignees

Comments

@rootulp
Copy link
Collaborator

rootulp commented Jan 13, 2025

Context

celestia-app/Makefile

Lines 337 to 348 in 2e16394

configure-v3:
@echo "Using config file at: $(CONFIG_FILE)"
@if [ "$$(uname)" = "Darwin" ]; then \
sed -i '' "s/^recv_rate = .*/recv_rate = $(SEND_RECV_RATE)/" $(CONFIG_FILE); \
sed -i '' "s/^send_rate = .*/send_rate = $(SEND_RECV_RATE)/" $(CONFIG_FILE); \
sed -i '' "s/ttl-num-blocks = .*/ttl-num-blocks = 12/" $(CONFIG_FILE); \
else \
sed -i "s/^recv_rate = .*/recv_rate = $(SEND_RECV_RATE)/" $(CONFIG_FILE); \
sed -i "s/^send_rate = .*/send_rate = $(SEND_RECV_RATE)/" $(CONFIG_FILE); \
sed -i "s/ttl-num-blocks = .*/ttl-num-blocks = 12/" $(CONFIG_FILE); \
fi
.PHONY: configure-v3
modified the send and receive rates in config file

Problem

The defaults for those fields are still low.

Proposal

  1. Increase the defaults so that the configure-v3 command isn't necessary if someone downloads a new binary and generates a new config.
  2. Look at the ttl-num-blocks field and see if that default also needs to be bumped
@rootulp rootulp self-assigned this Jan 13, 2025
@rootulp
Copy link
Collaborator Author

rootulp commented Jan 13, 2025

On the latest v3.x binary, a freshly generated config:

# Rate at which packets can be sent, in bytes/second
send_rate = 5120000

# Rate at which packets can be received, in bytes/second
recv_rate = 5120000

# Note, if ttl-duration is also defined, a transaction will be removed if it
# has existed in the mempool at least ttl-num-blocks number of blocks or if
# it's insertion time into the mempool is beyond ttl-duration.
ttl-num-blocks = 12

And after running the configure-v3 command, they look like this:

# Rate at which packets can be sent, in bytes/second
send_rate = 10485760  

# Rate at which packets can be received, in bytes/second
recv_rate = 10485760  

# Note, if ttl-duration is also defined, a transaction will be removed if it
# has existed in the mempool at least ttl-num-blocks number of blocks or if
# it's insertion time into the mempool is beyond ttl-duration.
ttl-num-blocks = 12

so the TTL num blocks default does not need to be modified. But the send and recv rates do.

@rootulp
Copy link
Collaborator Author

rootulp commented Jan 13, 2025

Looks like the rates should already be overridden to 10 MiB but these lines don't work as expected

const mebibyte = 1048576
cfg.P2P.SendRate = 10 * mebibyte
cfg.P2P.RecvRate = 10 * mebibyte

@rootulp
Copy link
Collaborator Author

rootulp commented Jan 13, 2025

Looks like inside celestia-core the config is based on serverCtx.Config which has overridden values for other default_overrides like Mempool.TTLDuration but not for P2P.SendRate so digging into why

@rootulp
Copy link
Collaborator Author

rootulp commented Jan 13, 2025

The default overrides that we configured are getting overridden by Cosmos SDK https://github.com/celestiaorg/cosmos-sdk/blame/d2163cecdef80591e86d0e5c8c7e6b8f70e2d0ad/server/util.go#L216-L217

@evan-forbes
Copy link
Member

sorry didn't mean to close, I think it auto closed with the merging of #celelstiaorg/cosmos-sdk/425

@rootulp
Copy link
Collaborator Author

rootulp commented Jan 14, 2025

Yea no worries

mergify bot pushed a commit that referenced this issue Jan 14, 2025
rootulp added a commit that referenced this issue Jan 14, 2025
Closes #4213 by
upgrading to
https://github.com/celestiaorg/cosmos-sdk/releases/tag/v1.26.1-sdk-v0.46.16
which contains the fix.<hr>This is an automatic backport of pull request
#4220 done by [Mergify](https://mergify.com).

Co-authored-by: Rootul P <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants