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: unfork cometbft, cosmos-sdk, ibc-go #16

Merged
merged 40 commits into from
Dec 5, 2024

Conversation

hoank101
Copy link

@hoank101 hoank101 commented Nov 2, 2024

Closes #2
Closes #1
The default of FifoMempool size is 5000
if need modify it, please config

// mempool flags
FlagMempoolMaxTxs = "mempool.max-txs"

@hoank101 hoank101 marked this pull request as draft November 2, 2024 03:47
@TropicalDog17 TropicalDog17 changed the title feat: unfork cosmossdk and cometbft feat: unfork cometbft Nov 4, 2024
custom/auth/ante/fee_tax.go Show resolved Hide resolved
app/app.go Outdated Show resolved Hide resolved
app/mempool/default.go Outdated Show resolved Hide resolved
app/mempool/mempool.go Outdated Show resolved Hide resolved
app/mempool/mempool.go Outdated Show resolved Hide resolved
app/mempool/default.go Outdated Show resolved Hide resolved
@hoank101 hoank101 marked this pull request as ready for review November 23, 2024 05:17
@hoank101 hoank101 requested a review from StrathCole November 23, 2024 10:32
app/mempool/mempool_fifo.go Outdated Show resolved Hide resolved
app/mempool/mempool_fifo.go Outdated Show resolved Hide resolved
app/app.go Outdated Show resolved Hide resolved
app/mempool/mempool_fifo.go Outdated Show resolved Hide resolved
app/mempool/mempool_fifo.go Outdated Show resolved Hide resolved
@@ -118,7 +118,10 @@ func computeTax(ctx sdk.Context, tk TreasuryKeeper, principal sdk.Coins, simulat
return taxes
}

func isOracleTx(msgs []sdk.Msg) bool {
func IsOracleTx(msgs []sdk.Msg) bool {

Choose a reason for hiding this comment

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

Remember to put this function to some other file as it is now used not only in fee calculation.

@@ -9,7 +11,7 @@ require (
github.com/CosmWasm/wasmd v0.46.0
github.com/CosmWasm/wasmvm v1.5.5
github.com/cometbft/cometbft v0.37.4
github.com/cometbft/cometbft-db v0.8.0
github.com/cometbft/cometbft-db v0.11.0

Choose a reason for hiding this comment

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

If you change to cometbft this also allows compiling in pebbledb. Maybe you can include that build option to the make file. See https://github.com/NibiruChain/nibiru/pull/1818/files

Copy link
Author

Choose a reason for hiding this comment

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

already rollback

Copy link
Author

Choose a reason for hiding this comment

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

ah yeah, we upgrade cometbft to v0.37.13, will upgrade the make file

go.mod Outdated Show resolved Hide resolved
hoank101 and others added 4 commits November 24, 2024 18:11
* feat: unfork cachesize setting

* feat: handleBlockHeightMiddleware

* feat: add hook for validator power limit

* perf: change hooks ordering

* revert toolchain

* bump to sdk 47.14

* comments for option
go.mod Outdated
@@ -1,4 +1,6 @@
go 1.20
go 1.22.7

Choose a reason for hiding this comment

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

Be careful with bumping go requirements. This has to be very explicitly communicated during tests and deployment, because many validators and nodes run on 1.20

Copy link
Author

Choose a reason for hiding this comment

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

yeah, i think we can also update Go to the latest version (include security fixes and performance improve)

Choose a reason for hiding this comment

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

I have nothing against this, but we need to make that 100% clear to all validators in the release notes and upgrade instructions.

func (mp *FifoMempool) Insert(_ context.Context, tx sdk.Tx) error {
mp.updateMtx.RLock()
defer mp.updateMtx.RUnlock()
totalTxs := mp.txs.Len() + mp.txsOracle.Len()

Choose a reason for hiding this comment

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

I just re-checked the current mempool. It seems that oracle txs are not calculated towards the size.

Check:
https://github.com/classic-terra/cometbft/blob/v0.37.4-terra1/mempool/v0/clist_mempool.go#L147
Which only uses mem.txs and not mem.oracleTxs

And on size check:
https://github.com/classic-terra/cometbft/blob/v0.37.4-terra1/mempool/v0/clist_mempool.go#L228
which calls https://github.com/classic-terra/cometbft/blob/v0.37.4-terra1/mempool/v0/clist_mempool.go#L384
only the size is checked which ignores oracle txs.

Copy link
Author

@hoank101 hoank101 Nov 25, 2024

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

I am sorry, you're right. I overlooked that part. 👍

@hoank101 hoank101 changed the title feat: unfork cometbft feat: unfork cometbft, cosmos-sdk, ibc-go Nov 28, 2024
@kien6034 kien6034 self-requested a review December 5, 2024 16:14
@kien6034 kien6034 merged commit 68438e7 into develop Dec 5, 2024
21 checks passed
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.

5 participants