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.

Sign up for GitHub

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

[Awaiting Mainnet25] Clean up database prefixes #5134

Merged
merged 2 commits into from
Jan 30, 2024
Merged

[Awaiting Mainnet25] Clean up database prefixes #5134

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
54ea60d
clean up db prefixes
jordanschalm Dec 12, 2023
1b6ca62
Merge branch 'master' into jordan/fix-dupe-db-code-prefix
jordanschalm Jan 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions storage/badger/operation/prefix.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,18 +36,18 @@ const (
codeSealedRootHeight = 27 // the height of the highest sealed block contained in the root snapshot

// codes for single entity storage
// 31 was used for identities before epochs
codeHeader = 30
_ = 31 // DEPRECATED: 31 was used for identities before epochs
codeGuarantee = 32
codeSeal = 33
codeTransaction = 34
codeCollection = 35
codeExecutionResult = 36
codeExecutionReceiptMeta = 36
codeResultApproval = 37
codeChunk = 38
codeExecutionReceiptMeta = 39 // NOTE: prior to Mainnet25, this erroneously had the same value as codeExecutionResult (36)
Comment on lines +40 to +48
Copy link
Member

Choose a reason for hiding this comment

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

I like that you clearly assign the Deprecated values.

In the long-term, what is our strategy of reusing those values? I think we desire to re-use them, otherwise we would unnecessarily bloat the badgers key space. In other words, I am assuming that we agree that it is fine to assign previously deprecated prefix values (such as 31) for new purposes.

I would suggest we follow this policy here:

  • prefix 31 is not used
  • codeExecutionReceiptMeta requires a new prefix value.
  • so codeExecutionReceiptMeta could use the prefix 31
Suggested change
_ = 31 // DEPRECATED: 31 was used for identities before epochs
codeGuarantee = 32
codeSeal = 33
codeTransaction = 34
codeCollection = 35
codeExecutionResult = 36
codeExecutionReceiptMeta = 36
codeResultApproval = 37
codeChunk = 38
codeExecutionReceiptMeta = 39 // NOTE: prior to Mainnet25, this erroneously had the same value as codeExecutionResult (36)
codeHeader = 30
codeExecutionReceiptMeta = 31 // NOTE: prior to Mainnet25, this erroneously had the same value as codeExecutionResult (36)
codeGuarantee = 32
codeSeal = 33
codeTransaction = 34
codeCollection = 35
codeExecutionResult = 36
codeResultApproval = 37
codeChunk = 38
codeProtocolState = 39

Copy link
Member Author

Choose a reason for hiding this comment

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

Personally I don't think we should re-use key prefixes until we need to, which could very easily be years away.

There is a benefit to re-using key prefixes only once we are running out of keyspace.

By contrast, although it's possible re-using key prefixes is harmless, I think there is a real possibility it has an adverse impact on some users -- for example the Flow Diver team has recently been indexing data from old sporks. I can imagine how a practice of re-using rather than retiring key prefixes could make that sort of thing more challenging.

Copy link
Member

Choose a reason for hiding this comment

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

👍


// codes for indexing single identifier by identifier/integeter
// codes for indexing single identifier by identifier/integer
codeHeightToBlock = 40 // index mapping height to block ID
codeBlockIDToLatestSealID = 41 // index mapping a block its last payload seal
codeClusterBlockToRefBlock = 42 // index cluster block ID to reference block ID
Expand All @@ -56,8 +56,8 @@ const (
codeBlockIDToQuorumCertificate = 45 // index of quorum certificates by block ID

// codes for indexing multiple identifiers by identifier
// NOTE: 51 was used for identity indexes before epochs
codeBlockChildren = 50 // index mapping block ID to children blocks
_ = 51 // DEPRECATED: 51 was used for identity indexes before epochs
AlexHentschel marked this conversation as resolved.
Show resolved Hide resolved
codePayloadGuarantees = 52 // index mapping block ID to payload guarantees
codePayloadSeals = 53 // index mapping block ID to payload seals
codeCollectionBlock = 54 // index mapping collection ID to block ID
Expand Down
Loading