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

Mempool.MaxMsgBytes is in wrong position in config/toml.go #3868

Closed
bluele opened this issue Aug 2, 2019 · 5 comments · Fixed by #3869 or #3877
Closed

Mempool.MaxMsgBytes is in wrong position in config/toml.go #3868

bluele opened this issue Aug 2, 2019 · 5 comments · Fixed by #3869 or #3877
Assignees
Labels
T:bug Type Bug (Confirmed)

Comments

@bluele
Copy link
Contributor

bluele commented Aug 2, 2019

Previously, I fixed a related part at #3826, but during the release process of v 0.32.2, it seems that the degradation occurred due to the wrong commit merge.

tendermint/config/toml.go

Lines 280 to 306 in 4d7cd80

[mempool]
recheck = {{ .Mempool.Recheck }}
broadcast = {{ .Mempool.Broadcast }}
wal_dir = "{{ js .Mempool.WalPath }}"
# Maximum number of transactions in the mempool
size = {{ .Mempool.Size }}
# Limit the total size of all txs in the mempool.
# This only accounts for raw transactions (e.g. given 1MB transactions and
# max_txs_bytes=5MB, mempool will only accept 5 transactions).
max_txs_bytes = {{ .Mempool.MaxTxsBytes }}
# Size of the cache (used to filter transactions we saw earlier) in transactions
cache_size = {{ .Mempool.CacheSize }}
##### fast sync configuration options #####
[fastsync]
# Fast Sync version to use:
# 1) "v0" (default) - the legacy fast sync implementation
# 2) "v1" - refactor of v0 version for better testability
version = "{{ .FastSync.Version }}"
# Limit the size of TxMessage
max_msg_bytes = {{ .Mempool.MaxMsgBytes }}

Related commits are here: 4d7cd80#diff-092cdc48047eeb4c0bca311a2e1b8ae6

Tendermint version

v0.32.2

@melekes
Copy link
Contributor

melekes commented Aug 2, 2019

Oops.. thank you for spotting this.

Speaking of MaxMsgBytes, don't you think we should switch to MaxTxBytes? By specifiyng max_msg_bytes in config we are forcing a user to account for something he/she does not know (amino overhead). I believe we should calculate it ourselves and only require users to specify desired max_tx_bytes.

@melekes melekes self-assigned this Aug 2, 2019
melekes added a commit that referenced this issue Aug 2, 2019
It was incorrectly placed in fastsync section during
4d7cd80#diff-092cdc48047eeb4c0bca311a2e1b8ae6
merge.

Fixes #3868
@melekes melekes added the T:bug Type Bug (Confirmed) label Aug 2, 2019
@bluele
Copy link
Contributor Author

bluele commented Aug 2, 2019

Sure, it might be better.
However, the size limit of the data that mempool.Reactor can handle is max_tx_bytes plus amino overhead implicitly. Don't you mind that? (https://github.com/tendermint/tendermint/blob/master/mempool/reactor.go#L266)
If there is no problem here, I think it is a good decision.

bluele pushed a commit to bluele/tendermint that referenced this issue Aug 2, 2019
@melekes
Copy link
Contributor

melekes commented Aug 2, 2019

Don't you mind that? (https://github.com/tendermint/tendermint/blob/master/mempool/reactor.go#L266)

Can't we add aminoOverhead (MaxTxBytes + aminoOverhead)?

@bluele
Copy link
Contributor Author

bluele commented Aug 2, 2019

From my understanding, I think we can :)
Would you like me to create a new PR for that fix?

@melekes
Copy link
Contributor

melekes commented Aug 2, 2019

Go ahead!

melekes added a commit that referenced this issue Aug 2, 2019
* config: move max_msg_bytes into mempool section

It was incorrectly placed in fastsync section during
4d7cd80#diff-092cdc48047eeb4c0bca311a2e1b8ae6
merge.

Fixes #3868

* add changelog entry
melekes pushed a commit that referenced this issue Aug 5, 2019
…3877)

Fix #3868 (comment)

Commits:

* mempool: make `max_tx_bytes` configurable instead of `max_msg_bytes`

* update CHANGELOG_PENDING

* apply suggestions from code review
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T:bug Type Bug (Confirmed)
Projects
None yet
2 participants