Skip to content

Defer store read/write from iterator loop#571

Merged
codchen merged 1 commit intomasterfrom
tony-chen-fix-iterators
Feb 20, 2023
Merged

Defer store read/write from iterator loop#571
codchen merged 1 commit intomasterfrom
tony-chen-fix-iterators

Conversation

@codchen
Copy link
Collaborator

@codchen codchen commented Feb 17, 2023

Describe your changes and provide context

Performing store access before an iterator is closed may cause deadlocks (see #567 for context).
This PR refactors existing logic to move store access that were previously within iterator's scope to be after iterator closure.

Note that there are still cases for this in oracle keeper, but all the methods there are only called in oracle's Mid/EndBlock, which are not parallelized in any way and thus not subject to the deadlock issue

Testing performed to validate your change

unit tests + existing tests

@github-actions
Copy link

Code Coverage

Package Line Rate Complexity Health
github.com/sei-protocol/sei-chain/aclmapping/bank 83% 0
github.com/sei-protocol/sei-chain/aclmapping/dex 100% 0
github.com/sei-protocol/sei-chain/aclmapping/oracle 100% 0
github.com/sei-protocol/sei-chain/aclmapping/staking 82% 0
github.com/sei-protocol/sei-chain/aclmapping/tokenfactory 96% 0
github.com/sei-protocol/sei-chain/aclmapping/utils 0% 0
github.com/sei-protocol/sei-chain/aclmapping/wasm 83% 0
github.com/sei-protocol/sei-chain/app 60% 0
github.com/sei-protocol/sei-chain/app/antedecorators 76% 0
github.com/sei-protocol/sei-chain/oracle/price-feeder/config 85% 0
github.com/sei-protocol/sei-chain/oracle/price-feeder/oracle 67% 0
github.com/sei-protocol/sei-chain/oracle/price-feeder/oracle/provider 49% 0
github.com/sei-protocol/sei-chain/oracle/price-feeder/router/v1 45% 0
github.com/sei-protocol/sei-chain/store/whitelist/cachemulti 100% 0
github.com/sei-protocol/sei-chain/store/whitelist/kv 100% 0
github.com/sei-protocol/sei-chain/store/whitelist/multi 100% 0
github.com/sei-protocol/sei-chain/utils 46% 0
github.com/sei-protocol/sei-chain/utils/datastructures 90% 0
github.com/sei-protocol/sei-chain/utils/logging 100% 0
github.com/sei-protocol/sei-chain/x/dex 58% 0
github.com/sei-protocol/sei-chain/x/dex/cache 87% 0
github.com/sei-protocol/sei-chain/x/dex/client/cli/query 23% 0
github.com/sei-protocol/sei-chain/x/dex/contract 51% 0
github.com/sei-protocol/sei-chain/x/dex/exchange 89% 0
github.com/sei-protocol/sei-chain/x/dex/keeper 63% 0
github.com/sei-protocol/sei-chain/x/dex/keeper/abci 29% 0
github.com/sei-protocol/sei-chain/x/dex/keeper/msgserver 78% 0
github.com/sei-protocol/sei-chain/x/dex/keeper/query 82% 0
github.com/sei-protocol/sei-chain/x/dex/migrations 88% 0
github.com/sei-protocol/sei-chain/x/dex/types 1% 0
github.com/sei-protocol/sei-chain/x/dex/types/utils 100% 0
github.com/sei-protocol/sei-chain/x/dex/types/wasm 83% 0
github.com/sei-protocol/sei-chain/x/epoch 11% 0
github.com/sei-protocol/sei-chain/x/epoch/keeper 66% 0
github.com/sei-protocol/sei-chain/x/epoch/types 2% 0
github.com/sei-protocol/sei-chain/x/mint 0% 0
github.com/sei-protocol/sei-chain/x/mint/keeper 61% 0
github.com/sei-protocol/sei-chain/x/mint/simulation 95% 0
github.com/sei-protocol/sei-chain/x/mint/types 3% 0
github.com/sei-protocol/sei-chain/x/nitro/client/cli 45% 0
github.com/sei-protocol/sei-chain/x/nitro/keeper 80% 0
github.com/sei-protocol/sei-chain/x/nitro/replay 63% 0
github.com/sei-protocol/sei-chain/x/oracle 65% 0
github.com/sei-protocol/sei-chain/x/oracle/keeper 83% 0
github.com/sei-protocol/sei-chain/x/oracle/simulation 18% 0
github.com/sei-protocol/sei-chain/x/oracle/types 3% 0
github.com/sei-protocol/sei-chain/x/tokenfactory/keeper 85% 0
github.com/sei-protocol/sei-chain/x/tokenfactory/types 2% 0
Summary 15% (5128 / 34208) 0

Copy link
Contributor

@philipsu522 philipsu522 left a comment

Choose a reason for hiding this comment

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

I wonder if it's possible to add some sort of unit test that ensures that panics to ensure that there is no store read/write within iterator loop? We can also have a whitelist of "stores" that are allowed to do this (i.e. oracle). Wdyt?
This can be a separate PR though

@codchen codchen merged commit 35ccc42 into master Feb 20, 2023
@masih masih deleted the tony-chen-fix-iterators branch October 31, 2025 15:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants