Lazy Deposit All Module Accounts During EndBlock#58
Conversation
BrandonWeng
force-pushed
the
bweng-gas-used-fix
branch
from
e0f2f01 to
55250a6
Compare
| return nil | ||
| } | ||
|
|
||
| // Processing in a given TX is sequential so no need for dependency |
There was a problem hiding this comment.
why are we adding this check here? AddEdge is meant to be a helper that solely adds an edge, whereas helpers such as AddNodeBuildDependency will do the business logic of ensuring there arent edges within the same tx? I guess its fine to have this as an invariant for edge creation
x/bank/keeper/keeper.go
Outdated
| // It deducts the balance from an accAddress and stores the balance in a mapping for ModuleAccounts. | ||
| // In the EndBlocker, it will then perform one deposit for each module account. | ||
| // It will panic if the module account does not exist. | ||
| func (k BaseKeeper) LazySendCoinsFromAccountToModule( |
There was a problem hiding this comment.
Instead of calling this LazySend, maybe we can call it something like DeferredSend to make more contextual sense that it performs the send at the end block?
x/bank/keeper/keeper.go
Outdated
| } | ||
|
|
||
| // Iterates on all the lazy deposits and deposit them into the store | ||
| func (k BaseKeeper) WriteLazyDepositsToModuleAccounts(ctx sdk.Context) []abci.Event { |
There was a problem hiding this comment.
don't we also need to call this in the bank endblock logic?
There was a problem hiding this comment.
I was thinking of calling it in the sei-chain app - the baseapp in sei-cosmos doesn't have access to the bank keeper
BrandonWeng
force-pushed
the
bweng-gas-used-fix
branch
from
a4df7cf to
ffe4d63
Compare
BrandonWeng
force-pushed
the
bweng-gas-used-fix
branch
from
ffe4d63 to
9249e70
Compare
| baseGas := uint64(59142) // baseGas is the gas consumed before tx msg | ||
| // check block gas is always consumed - this value may change if we update the logic for | ||
| // how gas is consumed | ||
| baseGas := uint64(52585) // baseGas is the gas consumed before tx msg |
There was a problem hiding this comment.
We no longer charge everytime so the base gas fee goes down too
| type basicGasMeter struct { | ||
| limit Gas | ||
| consumed Gas | ||
| lock *sync.Mutex |
There was a problem hiding this comment.
BlockGas can/will be modified concurrently
There was a problem hiding this comment.
would it make sense to make Gas an atomic by using
atomic.AddUint64 and atomic. AddUint64? This way if we add / remove any functions that touch gas, we know that the locking is already handled at a lower layer. O/w we need to be careful of ensuring every function that accesses any Gas variable handles the mutex \cc @codchen . Not a blocker, though
| type basicGasMeter struct { | ||
| limit Gas | ||
| consumed Gas | ||
| lock *sync.Mutex |
There was a problem hiding this comment.
would it make sense to make Gas an atomic by using
atomic.AddUint64 and atomic. AddUint64? This way if we add / remove any functions that touch gas, we know that the locking is already handled at a lower layer. O/w we need to be careful of ensuring every function that accesses any Gas variable handles the mutex \cc @codchen . Not a blocker, though
| // DeferredSendCoinsFromAccountToModule transfers coins from an AccAddress to a ModuleAccount. | ||
| // It deducts the balance from an accAddress and stores the balance in a mapping for ModuleAccounts. | ||
| // In the EndBlocker, it will then perform one deposit for each module account. | ||
| // It will panic if the module account does not exist. |
There was a problem hiding this comment.
does the entire block get rolled back? IIRC it does, but wanted to double check
There was a problem hiding this comment.
It would get stuck, but the block will not be committed - in this case we would be able upload a new binary to fix it.
4 participants
Describe your changes and provide context
Relevant PR:
sei-protocol/sei-chain#313
We are moving the module deposits to the end of the block and transferring them all at once. This is to prevent the gas fee deposit from being a single source of a bottleneck when it comes to concurrency.
Testing performed to validate your change
Ran 3 iterations of load tests with 5 rounds, and 400 orders per block
