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

Set lower feecap on PoSt messages with low balance #4217

Merged
merged 5 commits into from
Nov 20, 2020

Conversation

magik6k
Copy link
Contributor

@magik6k magik6k commented Oct 7, 2020

More testing definitely todo

mm.GasPremium = msg.GasPremium
}

mm.GasFeeCap, err = s.api.GasEstimateFeeCap(ctx, &mm, 2, types.EmptyTSK)
Copy link
Contributor

@Kubuxu Kubuxu Oct 7, 2020

Choose a reason for hiding this comment

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

This should for sure be more than 2, if you are setting Premium for 5 block inclusion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Set it a bit higher, bust still pretty low - this is estimating the lowest fee we'll set on the message if we don't have enough funds in any account to set more. If we have enough funds, we'll use the default estimation from GasEstimateMessageGas above

@magik6k magik6k force-pushed the fix/post-smaller-feecap branch 4 times, most recently from d43e256 to bcab3e5 Compare October 23, 2020 20:23
}

leastBad := address.Undef
bestAvail := minFunds
Copy link
Member

Choose a reason for hiding this comment

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

I feel like this method could be written more cleanly as something like:

for _,a := range append(controlAddrs, owner, worker) {
  ok, err := maybeUseAddress(a, &leastBad, &bestAvail)
  if ok {
   return leastBad, bestAvail
  }
}

@magik6k magik6k force-pushed the fix/post-smaller-feecap branch from c56b4ec to bd34424 Compare October 27, 2020 03:10
Copy link
Contributor

@arajasek arajasek left a comment

Choose a reason for hiding this comment

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

This should work, but I'm confused about the logic. We seem to be doing:

- if we find any address with more than `goodFunds`, use that address
- if not, of all the addresses that have more than `minFunds`, use the one with most balance
- if no such addresses exist, eff it, use worker (even if there's a control address with greater balance than worker)

Why not do:

- if we find any address with more than `goodFunds`, use that address
- if not, use the address that has the most balance (regardless of whether it is better than `minFunds` or not)

The change would probably be to initialize bestAvail to worker's balance at line 39, and remove the worker from the list of addresses we call maybeUseAddress with (and remove the special-case in 53-61).

@magik6k magik6k force-pushed the fix/post-smaller-feecap branch from bd34424 to 2571277 Compare October 28, 2020 21:14
@magik6k magik6k force-pushed the fix/post-smaller-feecap branch from 2571277 to 9f807f4 Compare November 19, 2020 17:02
@magik6k magik6k force-pushed the fix/post-smaller-feecap branch from 81ae8ca to 1999156 Compare November 19, 2020 20:00
@magik6k magik6k merged commit 96f0040 into master Nov 20, 2020
@magik6k magik6k deleted the fix/post-smaller-feecap branch November 20, 2020 12:15
bibibong pushed a commit to EpiK-Protocol/go-epik that referenced this pull request Jan 7, 2021
…st-smaller-feecap

Set lower feecap on PoSt messages with low balance
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.

4 participants