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.

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

refactor deferred balance to use memkv #287

Merged
merged 5 commits into from
Jun 14, 2023
Merged

refactor deferred balance to use memkv #287

merged 5 commits into from
Jun 14, 2023

Conversation

udpatil
Copy link
Contributor

@udpatil udpatil commented Jun 14, 2023

Describe your changes and provide context

This PR does the following:

  • refactors the deferred logic to use a memKV store instead of an in memory map (allows us to avoid committing the changes when we discard other changes upon antehandler failure)
  • refactors the feecollectors / deferred balance usage to key on module-txIndex-denom -> sdk.Coin
    • when writing the deferred balances, it first adds up all of the tx specific balances, then writes them in sorted order of the module addresses
  • refactors the anteDependencyGenerator to take in a txIndex to allow for generating the dependencies appropriately for feecollector resources
  • adds in a txIndex to sdk.Context to contain the currently executing TX index throughout delivertx context in order to allow for writing to the partitioned KV store to avoid resource overlap
  • Adds a new instantiation function for BankKeeper to instantiate with the deferredCache initialized (deferred usages panic if not set up with this new function) (have to keep old one because wasmd breaks otherwise)
  • Introduces the resource types specific to the deferredCache Store Key (because this is a separate KV store from bank store it has its own resource type and isn't under the Bank resource type

Still in progress:

  • removing the context cache usages + deferred bank ops in memory structure
  • adding unit tests for the memKV deferredCache that is part of bank keeper

Testing performed to validate your change

local sei testing + adding more unit tests

@codecov
Copy link

codecov bot commented Jun 14, 2023

Codecov Report

Merging #287 (e5067f1) into main (db47ec3) will decrease coverage by 0.25%.
The diff coverage is 67.60%.

❗ Current head e5067f1 differs from pull request most recent head 21c819d. Consider uploading reports for the commit 21c819d to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #287      +/-   ##
==========================================
- Coverage   55.17%   54.93%   -0.25%     
==========================================
  Files         619      618       -1     
  Lines       51569    51531      -38     
==========================================
- Hits        28455    28310     -145     
- Misses      21063    21169     +106     
- Partials     2051     2052       +1     
Impacted Files Coverage Δ
baseapp/baseapp.go 72.32% <0.00%> (-1.43%) ⬇️
types/accesscontrol/comparator.go 75.67% <ø> (ø)
types/accesscontrol/resource.go 0.00% <ø> (ø)
x/accesscontrol/types/graph.go 77.82% <ø> (ø)
x/auth/ante/basic.go 69.91% <0.00%> (ø)
x/auth/ante/sigverify.go 63.35% <0.00%> (ø)
x/bank/types/key.go 35.13% <0.00%> (-23.96%) ⬇️
types/handler.go 58.92% <8.33%> (ø)
types/context.go 73.88% <52.94%> (-0.15%) ⬇️
x/bank/keeper/deferred_cache.go 77.33% <77.33%> (ø)
... and 4 more

... and 4 files with indirect coverage changes

@@ -970,7 +970,7 @@ func (app *BaseApp) runTx(ctx sdk.Context, mode runTxMode, txBytes []byte) (gInf
// Dont need to validate in checkTx mode
if ctx.MsgValidator() != nil && mode == runTxModeDeliver {
storeAccessOpEvents := msCache.GetEvents()
accessOps, _ := app.anteDepGenerator([]acltypes.AccessOperation{}, tx)
accessOps := ctx.TxMsgAccessOps()[acltypes.ANTE_MSG_INDEX]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

cc @BrandonWeng is there a specific reason you have previously called the dependency generator here? I assume it was because you may not have been aware that the -1 key was reserved for the access ops generated for the ante handler ahead of time? but just want to make sure there wasnt some other reason

Copy link
Contributor

Choose a reason for hiding this comment

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

Teah that was most likely the case

@@ -21,6 +21,9 @@ var (
}
)

// use -1 to indicate that it is prior to msgs in the tx
Copy link
Contributor

@alexanderbez alexanderbez Jun 14, 2023

Choose a reason for hiding this comment

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

nit: What is prior to messages in the transaction?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the ante handler, since that also needs to declare resource usages. When constructing the DAG, we generate the dependencies on a per message basis, so for simplicity we treat the antehandler as the -1st message in the list of messages

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I meant the godoc should be updated to reflect that :p

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah yeah, will do

@udpatil udpatil force-pushed the deferred-use-kvstore branch from e5067f1 to 85b0d5f Compare June 14, 2023 15:48
@udpatil udpatil requested a review from alexanderbez June 14, 2023 16:27
Copy link
Contributor

@BrandonWeng BrandonWeng left a comment

Choose a reason for hiding this comment

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

not a blocker for this PR but could we add some unit tests for the error cases? the happy paths are tested quite well already


deferredStore := d.getModuleTxIndexedStore(ctx, moduleAddr, txIndex)
// Bank invariants require to not store zero balances, so we follow the same pattern in deferred cache.
if balance.IsZero() {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit but could we add unit tests for failure scenarios? The happy paths seem to be covered quite well already. Not a blocker for this PR

@udpatil udpatil enabled auto-merge (squash) June 14, 2023 18:39
@udpatil udpatil merged commit 9b0673b into main Jun 14, 2023
@udpatil udpatil deleted the deferred-use-kvstore branch June 14, 2023 18:45
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