Conversation
Co-authored-by: Callum Waters <cmwaters19@gmail.com>
#1041 (comment) Most keys are now grouped by height with the exception of block hash's key (-1) and pending evidence (-2): ``` header: 11/0 block part: 11/1/0 block part 2: 11/1/1 ... commit: 11/2 seen commit: 11/3 ext commit: 11/4 validator set: 11/5 consensus params: 11/6 abci responses: 11/7 committed evidence: 11/8 ``` ``` block hash: -1/{hash} pending evidence: -2/11/{ev.Hash} ``` ``` light block: {chainID}/9/11 size: {chainID}/10 ``` Refs #1041
|
TODO:
|
except light client DB
| } | ||
|
|
||
| func validatorsKey(height int64) []byte { | ||
| // Since CometBFT requests block H and validators H+1 on every height, we |
There was a problem hiding this comment.
This is the most controversial change. But this is undoubtedly an improvement because the access pattern is clear (block H, parts H, validators H+1). Any thoughts?
There was a problem hiding this comment.
I think it should be fine although there are places where we fetch the validators info explicitly (like RPC status and pruning). But those are less frequent than block validation. Another frequent access pattern is loading the lastCommit (validators from height -1 ) which is called on every ApplyBlock
| } | ||
|
|
||
| func validatorsKey(height int64) []byte { | ||
| // Since CometBFT requests block H and validators H+1 on every height, we |
There was a problem hiding this comment.
I think it should be fine although there are places where we fetch the validators info explicitly (like RPC status and pruning). But those are less frequent than block validation. Another frequent access pattern is loading the lastCommit (validators from height -1 ) which is called on every ApplyBlock
| keys are now grouped by height and lexicographically sorted using the | ||
| [orderedcode](https://github.com/google/orderedcode) library. | ||
|
|
||
| To upgrade, run `cometbft migrate-db`. **Before doing so, it's highly |
There was a problem hiding this comment.
saw this pr on my feed, the title intrigued me. This step would prevent the sdk from adopting this change. This will cause downtime when users upgrade, there should be a way to make it lazy so users dont experience this downtime
There was a problem hiding this comment.
Would it be fine to live migrate the latest {N} blocks which hopefully happened quickly, and then migrate the rest in the background?
There was a problem hiding this comment.
im not sure there needs to be a migration at the point of upgrade, the node could be able to tell what is the old format vs new, then migrate in the background. Potentially causing 0 donwtime
There was a problem hiding this comment.
Hey Marko, Dave! Just dropping a note here that migration is on our mind and we're considering different approaches (making it optional / online migration / offline / partial migration). Getting input from operators also to weigh the pros and cons. Will follow-up here w/ progress -- #2057
|
Closing in favor of #1764 @jmalicevic: "Based on the experiments, this change performs worse than #1764. The only improvement compared to #1764 is in the IO stats, but for compaction and general impact on the blockchain performance when it is on, it underperforms." |
#1041 (comment)
Most keys are now grouped by height except for block hash's key (-1)
and pending evidence (-2):
Refs #1041
Fixes #1857
PR checklist
.changelog(we use unclog to manage our changelog)docs/orspec/) and code comments