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

Out of gas stuck escrow fee #148

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

freeelancer
Copy link

If transactions are included in the block but goes out of gas, escrowed fees become stuck in the feemarket-fee-collector. This PR attempts to fix this by distributing the fees (based on the param.DistributeFees) through subsequent transactions in the posthandler.

@aljo242
Copy link
Collaborator

aljo242 commented Oct 19, 2024

Hey @freeelancer thanks for opening this PR!

I see a few issues with the current implementation of this:

  • The query is calling GetAllBalances() for the fee market module account. This works in the isolated scenario shown, but I think this will have issues as is. For example, if params.DistributeFees == false for 10 blocks and some transactions run, the module account will accumulate fees. All of these fees will be distributed upon our failed transaction, when it should only be the amount that was escrowed.
  • I'm not sure if the provided test is reproducing an actual issue here. This test is causing out of gas to be called in the ante handler, in which case this transaction should never be included into a block and the state commitment is to the CheckTx commitment tree.
  • Transactions that are included in the block, but run of out gas will be guaranteed to at least pass the anteHandler. If they run out of gas during message processing, the post handler will always be run for them.

Is this an issue you ran into during some testing or with integration into a chain?

}
if feesToDistribute.AmountOf(fee.Denom).GT(fee.Amount) {
events = append(events, sdk.NewEvent(
feemarkettypes.EventTypeDistributeStuckEscrowFees,
Copy link
Collaborator

Choose a reason for hiding this comment

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

we're not necessarily dealing with stuck fees every time this code is run

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.

2 participants