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.

Sign up for GitHub

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

Jump to bottom

Fees are excessively high sometimes #2062

Closed
Fell-x27 opened this issue Aug 24, 2020 · 4 comments
Closed

Fees are excessively high sometimes #2062

Fell-x27 opened this issue Aug 24, 2020 · 4 comments
Assignees
@KtorZ
Labels
Bug PRIORITY:MEDIUM A bug that needs to be addressed after ongoing stories and tasks. SEVERITY:MEDIUM Visible impact on a core functionality or some important performance degradation.

Comments

@Fell-x27
Copy link

Fell-x27 commented Aug 24, 2020 •
edited
Loading

Context

Sometimes when you want to estimate fees, you can get weird result;

Information -
Version 2020.8.3 (git revision: b5b292b)
Platform Linux
Installation From Hydra

Steps to Reproduce

  1. Try to estimate fees when payments + expected_fees_for_this_payments >= payments amount
  2. ...
  3. PROFIT!1

Expected behavior

Fees for this transaction even if there is not enough money for it. I just want to know estimated fees for transaction like this. It works well for migrations, but there is a problem #2061, so I can't use migrations instead.

Actual behavior

Your fees are about 1.8446744062438E+19

Comment

Yep, I can handle it by myself with some nasty solutions, and I will, but would be nice if I have not to do this.
@KtorZ
Copy link
Member

KtorZ commented Aug 24, 2020

Looks like a proper under/over flow in the fee estimation.

@KtorZ KtorZ added Bug SEVERITY:HIGH A core functionality of the system isn't unresponsive or returning invalid data. labels Aug 24, 2020
@KtorZ KtorZ changed the title Fees are crazy sometimes Fees are excessively high sometimes Aug 26, 2020
@KtorZ KtorZ self-assigned this Aug 27, 2020
@KtorZ KtorZ added PRIORITY:HIGH Require immediate attention. PRIORITY:MEDIUM A bug that needs to be addressed after ongoing stories and tasks. SEVERITY:MEDIUM Visible impact on a core functionality or some important performance degradation. and removed PRIORITY:HIGH Require immediate attention. SEVERITY:HIGH A core functionality of the system isn't unresponsive or returning invalid data. labels Aug 27, 2020
@KtorZ
Copy link
Member

KtorZ commented Aug 27, 2020

I've been able to reproduce with an integration test scenario by trying to estimate fee for a wallet using the total balance (available UTxO + rewards). This indeed reaches an edge case of the fee estimation where fees are too big and some extra computation is done to calculate the estimated amount.

@KtorZ KtorZ mentioned this issue Aug 27, 2020
Merged
iohk-bors bot added a commit that referenced this issue Sep 2, 2020
@iohk-bors @KtorZ
Merge #2084 #2086
db981dd 
2084: More measures post restoration benchmarks + SeqAnyState r=KtorZ a=KtorZ

# Issue Number

<!-- Put here a reference to the issue this PR relates to and which requirements it tackles -->

#2032 

# Overview

<!-- Detail in a few bullet points the work accomplished in this PR -->

- d820f36 make proportion more fine-grained to allow fractions of percent
- ce3f2a4 define a 'SeqAnyState' analogous to the 'RndAnyState' for benchmarks
- 02aba16 perform additional checks after restoration benchmarks
- 793de1c extend restoration benchmarks with a special 1% random wallet

# Comments

<!-- Additional comments or screenshots to attach if any -->

On the testnet (although I messed up and the rnd % benchmark are actually doing 1%, 2% and 4% restorations!)

```
All results:
BenchResults:
  qualifier: Seq Empty Wallet
  restorationTime: 521.8 s
  listingAddressesTime: 15.90 ms
  estimatingFeesTime: 2.015 s
  utxoStatistics:
    = Total value of -0 lovelace across 0 UTxOs
     ... 10                0
     ... 100               0
     ... 1000              0
     ... 10000             0
     ... 100000            0
     ... 1000000           0
     ... 10000000          0
     ... 100000000         0
     ... 1000000000        0
     ... 10000000000       0
     ... 100000000000      0
     ... 1000000000000     0
     ... 10000000000000    0
     ... 100000000000000   0
     ... 1000000000000000  0
     ... 10000000000000000 0
     ... 45000000000000000 0

BenchResults:
  qualifier: Rnd Empty Wallet
  restorationTime: 377.9 s
  listingAddressesTime: 989.2 μs
  estimatingFeesTime: 32.40 s
  utxoStatistics:
    = Total value of -0 lovelace across 0 UTxOs
     ... 10                0
     ... 100               0
     ... 1000              0
     ... 10000             0
     ... 100000            0
     ... 1000000           0
     ... 10000000          0
     ... 100000000         0
     ... 1000000000        0
     ... 10000000000       0
     ... 100000000000      0
     ... 1000000000000     0
     ... 10000000000000    0
     ... 100000000000000   0
     ... 1000000000000000  0
     ... 10000000000000000 0
     ... 45000000000000000 0

BenchResults:
  qualifier: Rnd 0.1% Wallet
  restorationTime: 496.8 s
  listingAddressesTime: 72.31 ms
  estimatingFeesTime: 218.5 ms
  utxoStatistics:
    = Total value of 34915991397627 lovelace across 500 UTxOs
     ... 10                1
     ... 100               14
     ... 1000              1
     ... 10000             5
     ... 100000            36
     ... 1000000           16
     ... 10000000          12
     ... 100000000         28
     ... 1000000000        21
     ... 10000000000       138
     ... 100000000000      216
     ... 1000000000000     12
     ... 10000000000000    0
     ... 100000000000000   0
     ... 1000000000000000  0
     ... 10000000000000000 0
     ... 45000000000000000 0

BenchResults:
  qualifier: Rnd 0.2% Wallet
  restorationTime: 698.7 s
  listingAddressesTime: 643.8 ms
  estimatingFeesTime: 317.9 ms
  utxoStatistics:
    = Total value of 80967721697473 lovelace across 921 UTxOs
     ... 10                1
     ... 100               20
     ... 1000              1
     ... 10000             9
     ... 100000            64
     ... 1000000           28
     ... 10000000          19
     ... 100000000         42
     ... 1000000000        38
     ... 10000000000       246
     ... 100000000000      426
     ... 1000000000000     26
     ... 10000000000000    1
     ... 100000000000000   0
     ... 1000000000000000  0
     ... 10000000000000000 0
     ... 45000000000000000 0

BenchResults:
  qualifier: Rnd 0.4% Wallet
  restorationTime: 1016 s
  listingAddressesTime: 1.500 s
  estimatingFeesTime: 525.2 ms
  utxoStatistics:
    = Total value of 190005683160290 lovelace across 1894 UTxOs
     ... 10                2
     ... 100               45
     ... 1000              2
     ... 10000             12
     ... 100000            147
     ... 1000000           65
     ... 10000000          87
     ... 100000000         106
     ... 1000000000        93
     ... 10000000000       500
     ... 100000000000      772
     ... 1000000000000     59
     ... 10000000000000    3
     ... 100000000000000   1
     ... 1000000000000000  0
     ... 10000000000000000 0
     ... 45000000000000000 0

BenchResults:
  qualifier: Seq 0.1% Wallet
  restorationTime: 458.2 s
  listingAddressesTime: 25.73 ms
  estimatingFeesTime: 27.27 ms
  utxoStatistics:
    = Total value of 2912204907552 lovelace across 38 UTxOs
     ... 10                0
     ... 100               0
     ... 1000              0
     ... 10000             1
     ... 100000            0
     ... 1000000           2
     ... 10000000          1
     ... 100000000         2
     ... 1000000000        2
     ... 10000000000       11
     ... 100000000000      18
     ... 1000000000000     1
     ... 10000000000000    0
     ... 100000000000000   0
     ... 1000000000000000  0
     ... 10000000000000000 0
     ... 45000000000000000 0

BenchResults:
  qualifier: Seq 0.2% Wallet
  restorationTime: 510.7 s
  listingAddressesTime: 35.70 ms
  estimatingFeesTime: 39.94 ms
  utxoStatistics:
    = Total value of 6462355327839 lovelace across 86 UTxOs
     ... 10                1
     ... 100               1
     ... 1000              1
     ... 10000             1
     ... 100000            2
     ... 1000000           3
     ... 10000000          1
     ... 100000000         4
     ... 1000000000        2
     ... 10000000000       26
     ... 100000000000      42
     ... 1000000000000     2
     ... 10000000000000    0
     ... 100000000000000   0
     ... 1000000000000000  0
     ... 10000000000000000 0
     ... 45000000000000000 0

BenchResults:
  qualifier: Seq 0.4% Wallet
  restorationTime: 551.5 s
  listingAddressesTime: 61.45 ms
  estimatingFeesTime: 66.49 ms
  utxoStatistics:
    = Total value of 15179718492947 lovelace across 219 UTxOs
     ... 10                1
     ... 100               3
     ... 1000              1
     ... 10000             3
     ... 100000            8
     ... 1000000           4
     ... 10000000          6
     ... 100000000         19
     ... 1000000000        14
     ... 10000000000       59
     ... 100000000000      96
     ... 1000000000000     5
     ... 10000000000000    0
     ... 100000000000000   0
     ... 1000000000000000  0
     ... 10000000000000000 0
     ... 45000000000000000 0
```

<!-- 
Don't forget to:

 ✓ Self-review your changes to make sure nothing unexpected slipped through
 ✓ Assign yourself to the PR
 ✓ Assign one or several reviewer(s)
 ✓ Once created, link this PR to its corresponding ticket
 ✓ Assign the PR to a corresponding milestone
 ✓ Acknowledge any changes required to the Wiki
-->


2086: Fix underflow in fee estimation in the presence of withdrawal and max amount r=KtorZ a=KtorZ

# Issue Number

<!-- Put here a reference to the issue this PR relates to and which requirements it tackles -->

#2062 

# Overview

<!-- Detail in a few bullet points the work accomplished in this PR -->

10ab550 out of paranoia, add an invariant to make transactions never leave fees beyond a certain amount.
3d4469c pass down withdrawal value to 'handleCannotCoverFee' to prevent underflow in fee calculation.
f6e74dc add extra scenario covering fee estimation edge-case in the presence of withdrawal

TODO: I'd like to add one property or scenario to exercise the invariant and shows that it operates as expected.

# Comments

<!-- Additional comments or screenshots to attach if any -->

<!-- 
Don't forget to:

 ✓ Self-review your changes to make sure nothing unexpected slipped through
 ✓ Assign yourself to the PR
 ✓ Assign one or several reviewer(s)
 ✓ Once created, link this PR to its corresponding ticket
 ✓ Assign the PR to a corresponding milestone
 ✓ Acknowledge any changes required to the Wiki
-->


Co-authored-by: KtorZ <[email protected]>
Co-authored-by: IOHK <[email protected]>
@KtorZ KtorZ mentioned this issue Sep 4, 2020
Merged
1 task
iohk-bors bot added a commit that referenced this issue Sep 4, 2020
@iohk-bors @KtorZ
Merge #2113
be96282 
2113: Relax fee upper bound invariant r=KtorZ a=KtorZ

# Issue Number

<!-- Put here a reference to the issue this PR relates to and which requirements it tackles -->

#2062 

# Overview

<!-- Detail in a few bullet points the work accomplished in this PR -->

- [ ] I have relaxed a bit the upper bound. This invariant is really to prevent catastrophic situations which would induce the lost of thousands / millions of Ada so it needs not to be very precise. We do make quite a lot of approximations and roundings when estimating fees, and the minimal UTxO value may also induce some extra amount to be counted towards fees. 

# Comments

<!-- Additional comments or screenshots to attach if any -->

<!-- 
Don't forget to:

 ✓ Self-review your changes to make sure nothing unexpected slipped through
 ✓ Assign yourself to the PR
 ✓ Assign one or several reviewer(s)
 ✓ Once created, link this PR to its corresponding ticket
 ✓ Assign the PR to a corresponding milestone
 ✓ Acknowledge any changes required to the Wiki
-->


Co-authored-by: KtorZ <[email protected]>
@KtorZ
Copy link
Member

KtorZ commented Sep 7, 2020

QA

  • Added an extra integration test scenario illustrating the problem (fee calculation fallback with withdrawal) (f6e74dc)

  • Also added an invariant in-code to make sure that this does not happen by accident. The invariant checks that fee are within an acceptable bound and crashes the server otherwise.

@piotr-iohk
Copy link
Contributor

lgtm.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@KtorZ KtorZ

Labels
Bug PRIORITY:MEDIUM A bug that needs to be addressed after ongoing stories and tasks. SEVERITY:MEDIUM Visible impact on a core functionality or some important performance degradation.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@KtorZ @Fell-x27 @piotr-iohk