Suppress scope value owner migration events. #2195
Conversation
…ted when the migration ran on testnet.
WalkthroughThis pull request introduces changes to suppress event emissions during the metadata migration process and implements a no-operation event manager. The Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
internal/sdk/events.go (2)
1-8: Consider renaming the package to avoid confusion with imports.The package name
sdkmight lead to confusion as it's the same as one of the imported packages. Consider renaming it to something more specific, likenoopeventsoreventssuppression, to clearly indicate its purpose and avoid potential naming conflicts.
1-46: Overall implementation aligns well with PR objectives.The
NoOpEventManagerimplementation successfully addresses the PR objective of suppressing event emissions during the metadata migration process. Key points:
- All methods of the
sdk.EventManagerIinterface are correctly implemented to do nothing or return empty results.- The code is well-structured, with clear comments explaining the purpose and behavior of each component.
- The implementation will effectively prevent the generation of a large number of events during migration, addressing the performance issues and potential memory leaks mentioned in the PR objectives.
This no-op event manager can be selectively used for non-testnet nodes, allowing for the suppression of events on mainnet while still emitting events on testnet, as specified in the PR objectives.
Consider adding a configuration option or environment variable to enable/disable this no-op event manager, allowing for more flexible control over event suppression in different environments or scenarios.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- .changelog/unreleased/improvements/2195-hide-md-mig-events.md (1 hunks)
- internal/sdk/events.go (1 hunks)
- x/metadata/keeper/migrations_v4.go (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .changelog/unreleased/improvements/2195-hide-md-mig-events.md
🧰 Additional context used
📓 Learnings (1)
x/metadata/keeper/migrations_v4.go (2)
Learnt from: SpicyLemon PR: provenance-io/provenance#2140 File: x/metadata/keeper/migrations_v4.go:57-80 Timestamp: 2024-10-02T02:06:21.664Z Learning: The `V3WriteNewScope` method in `x/metadata/keeper/migrations_v4.go` must exist until after the migration has been executed and will be deleted as part of the `viridian` cleanup.Learnt from: SpicyLemon PR: provenance-io/provenance#2140 File: x/metadata/keeper/migrations_v4.go:57-80 Timestamp: 2024-10-08T18:12:51.935Z Learning: The `V3WriteNewScope` method in `x/metadata/keeper/migrations_v4.go` must exist until after the migration has been executed and will be deleted as part of the `viridian` cleanup.
🔇 Additional comments (10)
internal/sdk/events.go (6)
10-13: LGTM: NoOpEventManager struct and interface assertion.The
NoOpEventManagerstruct is correctly declared as an empty struct, and the interface assertion ensures it implementssdk.EventManagerI. The comment clearly explains the purpose of the struct.
15-18: LGTM: NewNoOpEventManager function.The
NewNoOpEventManagerfunction is correctly implemented, creating and returning a newNoOpEventManagerinstance. The comment clearly explains the purpose of the function.
20-30: LGTM: Events and ABCIEvents methods.Both
EventsandABCIEventsmethods are correctly implemented, returning empty events instead of nil to matchsdk.EventManagerbehavior. This approach ensures consistency and prevents potential nil pointer issues. The comments clearly explain the reasoning behind the implementation.
32-40: LGTM: EmitTypedEvent and EmitTypedEvents methods.Both
EmitTypedEventandEmitTypedEventsmethods are correctly implemented for a no-op event manager. They ignore their arguments and always return nil, which is the expected behavior. The comments clearly explain this behavior.
42-43: LGTM: EmitEvent method.The
EmitEventmethod is correctly implemented for a no-op event manager. It ignores its argument and does nothing, which is the expected behavior. The comment clearly explains this behavior.
45-46: LGTM: EmitEvents method.The
EmitEventsmethod is correctly implemented for a no-op event manager. It ignores its argument and does nothing, which is the expected behavior. The comment clearly explains this behavior.x/metadata/keeper/migrations_v4.go (4)
13-13: Confirm usage of internal package importThe code imports
internalsdkfrom an internal package:internalsdk "github.com/provenance-io/provenance/internal/sdk"Importing from an internal package is generally intended for use only within that package and may lead to visibility or compatibility issues. Please verify that importing
internalsdkhere is acceptable and will not cause future maintainability problems.
Line range hint
57-79: Ensure retention ofV3WriteNewScopefor migration purposesThe
V3WriteNewScopefunction is marked as deprecated and intended for removal post-migration:// Deprecated: Only exists to facilitate testing of the migration of the metadata module from v3 to v4. func (k Keeper) V3WriteNewScope(ctx sdk.Context, scope types.Scope) error { // function body... }According to the retrieved learnings, this method must remain until after the migration is executed and will be deleted during the
viridiancleanup. Please ensure that this function is retained until it is safe to remove.
28-28:⚠️ Potential issueAssess security implications of bypassing marker restrictions
The migration bypasses marker send restrictions with:
ctx = markertypes.WithBypass(ctx)While this is necessary to facilitate the migration, please verify that:
- Scope Limitation: The bypass is strictly limited to the migration process and does not persist beyond it.
- Security Measures: Adequate safeguards are in place to prevent unauthorized access or actions during the bypass.
- Restoration of Context: Normal marker restrictions are reinstated after the migration to maintain system integrity.
20-26:⚠️ Potential issueReview event suppression logic during migration
The code conditionally suppresses event emissions during migration on non-testnet environments:
// Lines 24-26 if sdk.GetConfig().GetBech32AccountAddrPrefix() != "tp" { ctx = ctx.WithEventManager(internalsdk.NewNoOpEventManager()) }While this approach addresses performance issues caused by excessive events, please ensure that:
- Event Dependencies: Suppressing events does not negatively impact any modules or processes that rely on these events during migration.
- Network Identification: The check for the Bech32 account address prefix accurately identifies the testnet environment. If other environments share the "tp" prefix or if there are additional prefixes in use, consider refining the condition.
- Future Compatibility: This logic remains valid for future testnets or network changes.
You can use the following script to verify that no critical dependencies on these events exist:
✅ Verification successful
Event Suppression Logic Verified Successfully
The event suppression during migration does not impact other modules or processes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify if any modules depend on migration events. # Search for any handlers or listeners that might be affected. rg 'AddEventListener|HandleEvent|OnEvent' -A 5 # Check for any usage of migration events in the codebase. rg 'Migrate3To4' -A 20 | rg 'EmitEvent|EventManager'Length of output: 200
* Create a no-op event manager and use that during the metadata module migration. * Do not suppress the events for a testnet upgrade since they were emitted when the migration ran on testnet. * Add changelog entry.
…tion events), #2196 (timeout_commit), #2197 (recordspec cmd), #2198 (ParameterChangeProposal) #2199 (wasm build-address cmd). (#2200) * Suppress scope value owner migration events. (#2195) * Create a no-op event manager and use that during the metadata module migration. * Do not suppress the events for a testnet upgrade since they were emitted when the migration ran on testnet. * Add changelog entry. * Update all the spec proto links to reference v1.20.0 (#2192) * Update all the spec proto links to reference v1.20.0 (instead of 1.19.0). * Add changelog entry. * When prepping a release, combine the dependency bump changelog entries. (#2181) * Add a note to get-dep-changes to alert folks that changing those formats might break other things. * Create an awk script that will combine dependency changelog entries. Update prep-release to use it. Also apply a couple fixes that are alread in the release branch (and will be in main shortly). Also tweak the step 4 and 5 names to provide more context, and fix the verbose output header when recombining the sections. * Add changelog entry. * Clarify the new comment in get-dep-changes.sh. * Update stuff that uses or talks about RELEASE_NOTES.md because it should actually be RELEASE_CHANGELOG.md. The SDK uses _NOTES but only puts a blurb in there, so it's not a changelog. But we include a changelog, so it makes sense to keep it named that way. * Fix the `query metadata recordspec` command when given a rec-spec-id. (#2197) * [2148]: Fix the query metadata recordspec command to correctly use the RecordSpecification query (instead of RecordSpecificationsForContractSpecification) when provided a record specification id. * [2148]: Add changelog entry. * Fix decoding of gov props with a ParameterChangeProposal in them. (#2198) * Write a unit test that fails to parse a gov proposal with a ParameterChangeProposal in it because that type isn't being registered anymore. * Register the params module stuff with the codecs since there's some gov props with a ParameterChangeProposal in them. * Add changelog entry. * Hard-code consensus.timeout_commit to 3.5s for mainnet. (#2196) * [2121]: Change the default consensus timeout value to 3.5 seconds. * [2121]: Hard-code the consensus.timeout_commit value. * [2121]: Fix TestIsTestnetFlagSet to not be affected by existing env vars. * [2121]: Fix a couple unit tests that broke when I changed the default commit timout. * [2121]: Only hard-code the timeout commit on non-testnets. * [2121]: Change the default back to 1.5s for faster default testnets. * [2121]: Fix the TestPreUpgradeCmd that broke because of the hard-coded timeout commit. * [2121]: Add some unit tests that make sure the consensus timeout commit value is behaving as expected. * [2121]: Add changelog entry. * [2121]: When forcing the timeout_commit to be 3.5 seconds, also force the skip flag to be false. * [2121]: Update warnAboutSettings: Evaluate the timeout commit and skip-timeout-commit fields separately. Issue a warning if skip-timeout-commit is true. Issue a warning if the timeout commit is not exactly what we want it to be. * Fix: Add node flag to WASM queries (build-address) (#2199)
Description
This PR updates the metadata module migration from v3 to v4 (to move value-owner records into the bank module). It will no longer emit any events when run on a mainnet node. Any events for other parts of the upgrade will still be included, though, just not the events from the metadata module migration.
For each scope value owner, there is at least 6 events (more if they've opted into quarantine). Both mainnet and testnet have around 300,000 scopes and most (if not all) have a value owner. That's around 1.8M events that would be emitted.
On testnet, when we ran the
viridian-rc1upgrade, it included all of those events in the block-results for the upgrade height. I had to write a custom executable to extract it, and as JSON, it was almost 400MB. We could not find a way to allow a node to actually return that info via the block-results query.In some unit tests I wrote and played with, reading the block result from state and unmarshalling it always took less than half a second. But that's a small fraction of the amount of time between request and timeout. My theory is that some limit is being hit, causing a panic in a sub-process that isn't being properly handled. When the panic happens, the request processing is halted and the node never sends any sort of response, leading ultimately to a timeout from something. Further, I believe that when that panic happens, there's memory that has been allocated, but not released, leading to a memory leak. This might explain why some modes had memory problems (some causing system crashes) after the upgrade.
So, to be on the safe side, we'll omit the events from the metadata module migration.
Also, so that
v1.20.0-rc2andv1.20.0can be state compatible, this change only affects non-testnet nodes. That way, if you're recreating a node, playing through all the blocks, then, on theviridian-rc1upgrade height, you can switch tov1.20.0and get the correct result. I.e. for a testnet node, all the events will still be emitted during the metadata v3 to v4 module migration.Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/) or specification (x/<module>/spec/).godoccomments..changelog/unreleased(see Adding Changes).Files changedin the Github PR explorer.Codecov Reportin the comment section below once CI passes.Summary by CodeRabbit
New Features
Bug Fixes
Documentation