-
Notifications
You must be signed in to change notification settings - Fork 180
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
[Awaiting Mainnet25] Clean up database prefixes #5134
Conversation
- change the prefix of ReceiptMetas (previously it had been the same as Results) - move deprecation notes inline with code definitions
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5134 +/- ##
==========================================
- Coverage 56.36% 56.34% -0.03%
==========================================
Files 977 977
Lines 91915 91915
==========================================
- Hits 51805 51786 -19
- Misses 36268 36286 +18
- Partials 3842 3843 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
_ = 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) |
There was a problem hiding this comment.
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 prefix31
_ = 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
🚢 This PR is not downward-compatible. It should be merged after the last HCU of Mainnet24 and before the Mainnet25 network upgrade.