feat(mempool): backport Lanes and DOG from upstream #5
Conversation
…etbft#3659) Required for cometbft#3658 --- - [ ] Tests written/updated - [ ] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments --------- Co-authored-by: Andy Nogueira <[email protected]>
<!-- Please add a reference to the issue that this PR addresses and indicate which files are most critical to review. If it fully addresses a particular issue, please include "Closes #XXX" (where "XXX" is the issue number). If this PR is non-trivial/large/complex, please ensure that you have either created an issue that the team's had a chance to respond to, or had some discussion with the team prior to submitting substantial pull requests. The team can be reached via GitHub Discussions or the Cosmos Network Discord server in the #cometbft channel. GitHub Discussions is preferred over Discord as it allows us to keep track of conversations topically. https://github.com/cometbft/cometbft/discussions If the work in this PR is not aligned with the team's current priorities, please be advised that it may take some time before it is merged - especially if it has not yet been discussed with the team. See the project board for the team's current priorities: https://github.com/orgs/cometbft/projects/1 --> --- - [ ] Tests written/updated - [ ] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments - [ ] Title follows the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec --------- Co-authored-by: Anton Kaliaev <[email protected]>
…#3700) Fixes cometbft#3694 This PR replaces: ``` type CListMempool struct { txsBytes atomic.Int64 // total size of mempool, in bytes ... txs *clist.CList txsMap sync.Map ... } ``` by ``` type CListMempool struct { ... txsMtx cmtsync.RWMutex txs *clist.CList // concurrent linked-list of valid txs txsMap map[types.TxKey]*clist.CElement // for quick access to txs txsBytes int64 // total size of mempool, in bytes ... } ``` The new `txsMtx` mutex is used every time the variables `txs`, `txsMap`, and `txsBytes` are accessed. --- - [ ] Tests written/updated - [x] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments - [ ] Title follows the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec --------- Co-authored-by: Andy Nogueira <[email protected]> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
…4117) Closes cometbft#4120 - [Allow different lanes to the same priority](cometbft@7360365) - [Limits on lane capacities](cometbft@2de8f07)
…4229) There are reports of some test failing because of non-existing lanes (eg. https://github.com/cometbft/cometbft/actions/runs/11121278617/job/30900042983) This PR always initialises mempools in tests with lane information fetched from the app.
As per the title. --- - [ ] Tests written/updated - [ ] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments --------- Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Closes cometbft#4138 Formatted file: [docs/guides/app-dev/mempool-lanes.md](https://github.com/cometbft/cometbft/blob/hvanz/docs-guides-lanes-4138/docs/guides/app-dev/mempool-lanes.md) --------- Co-authored-by: Jasmina Malicevic <[email protected]> Co-authored-by: Anton Kaliaev <[email protected]> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: Sergio Mena <[email protected]>
Contributes to cometbft#4318 The DOG spec is based on Flood's spec (cometbft#4477) [README.md](https://github.com/cometbft/cometbft/tree/hvanz/flood-spec-4318/spec/mempool/gossip/) --------- Co-authored-by: Andy Nogueira <[email protected]> Co-authored-by: Daniel <[email protected]> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Solves cometbft#4318 Based on cometbft#4476 [README.md](https://github.com/cometbft/cometbft/tree/hvanz/dog-spec-4318/spec/mempool/gossip) --------- Co-authored-by: Andy Nogueira <[email protected]> Co-authored-by: Daniel <[email protected]> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
…ometbft#4555) Closes cometbft#4558 The bulk of the work is in one file: `mempool/reactor.go`. Each commit is a self-contained addition to the code: - [add new proto messages HaveTx and ResetRoute](cometbft@e236427) - [add config](cometbft@bca343a) - [add MempoolControlChannel](cometbft@cb65e15) - [add GetSenders method to Mempool interface, and Senders to Entry interface](cometbft@d73f263) - [add router to mempool reactor](cometbft@fb38f14) - [add redundancy controller to mempool reactor](cometbft@f91be43) - [add metrics DisabledRoutes and Redundancy](cometbft@28b14f1) - [add changelog file](cometbft@4677722) --- - [X] Tests written/updated - [X] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [X] Updated relevant documentation (`docs/` or `spec/`) and code comments --------- Co-authored-by: Jasmina Malicevic <[email protected]> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
…t#4646) A mistake in the tests got merged with the PR merging the DOG protocol into main. `TestDOGDisabledRoutes` was put asleep for 100s instead of 100ms. --- #### PR checklist - [ ] Tests written/updated - [ ] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments
The test should be able now to catch the error introduced by cometbft#3657. ---- To see how it would break, please checkout the branch `cason/test-nobroadcastsender`, then: ``` $ cd mempool $ go test -v -run TestReactorNoBroadcastToSender ``` And wait it to fail, after 2 minutes. You are going to see the busy-loop around, logs entries starting with `Skipping tx`, forever for the same transaction. Important: the test unit only works in the traditional mempool design, without lanes. We should consider adding a variant of it that should also pass when lanes are considered. --- - [x] Tests written/updated - [ ] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments --------- Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
LGTM! |
| // NewPersistentApplication creates a new application using the goleveldb database engine. | ||
| func NewPersistentApplication(dbDir string) *Application { | ||
| // newDB creates a DB engine for persisting the application state. | ||
| func newDB(dbDir string) *dbm.PebbleDB { |
There was a problem hiding this comment.
leveldb is still supported in v1? Pebble is just defult in newly generated configs?
Btw do you have any benchmarks or anything that you could share regarding decision to move to pebble as default storage.
There was a problem hiding this comment.
Only pebble is supported but it's still possible to use any database: https://github.com/cometbft/cometbft/blob/main/UPGRADING.md#db-changes
Some of the reasons for this decision are here (from 2023) and more recently here.
There was a problem hiding this comment.
@hvanz Does that mean we only need to use the goleveldb build flag with v1, without requiring any custom implementation (e.g., DB interface modifications, DefaultDBProvider overrides, etc.)?
We’re currently evaluating Pebble, but migrating would be a major effort. We’d prefer to avoid mixing implementations for now.
There was a problem hiding this comment.
Sorry, ignore my previous message. Those changes are not in v1.0.1. You would only need to set db_backend = "goleveldb". See here: https://github.com/InjectiveLabs/cometbft/blob/hvanz/v1.0.1%2Blanes%2Bdog/docs/references/config/config.toml.md#db_backend
There was a problem hiding this comment.
Also there is a document we wrote as Upgrading guidelines to v1.0 which might have some other helpful insights.
There was a problem hiding this comment.
leveldb is still supported in v1? Pebble is just defult in newly generated configs? Btw do you have any benchmarks or anything that you could share regarding decision to move to pebble as default storage.
@nmarcetic We tested Pebble when exploring changing the database key layout and pruning + compaction.
We had reports from other teams about Pebble's superiors performance compared to goleveldb. More than that golevelDB is not maintained anymore.
In our local tests, our app was not really storage bound but the numbers we got from multiple runs indicated Pebble was slightly better:
https://github.com/cometbft/cometbft/blob/main/docs/references/storage/README.md#pebble vs . https://github.com/cometbft/cometbft/blob/main/docs/references/storage/README.md#local-1node .
The dealbreaker was the fact that Pebble is able to actually compact data without us intructing it to.
Namely, for golevelDB , in v1.x you now have an explicit call to db.Compact when you prune. This will now actually delete the files on disk (unlike before where pruning was not doing this). Pebble does not need that call.
There was a problem hiding this comment.
Thanks @jmalicevic @hvanz
Btw we also have the same results from benchmarking Leveldb vs. PebbleDB: lower CPU, RAM, and IO utilization and faster read/write (mostly reads due to improved caching).
I cherry-picked the relevant commits for Lanes and DOG from
cometbft/cometbft'smainbranch. These commits were mostly unchanged.When it's ready to be merged, remember to do "Rebase and Merge"!