Skip to content
This repository has been archived by the owner on Aug 18, 2020. It is now read-only.

[RCD-44] & [RCD-45] & [RCD-46] - Review fee calculation and Fix prefiltering not ignoring inputs' ids [develop] #3875

Merged
merged 3 commits into from
Nov 21, 2018

Conversation

KtorZ
Copy link
Contributor

@KtorZ KtorZ commented Nov 20, 2018

Description

See #3861 and #3855

This is port to develop which also includes extra integration tests to emphasize on the fixed issues.

Linked issue

RCD-46
RCD-45
RCD-44

Type of change

  • 🐞 Bug fix (non-breaking change which fixes an issue)
  • 🛠 New feature (non-breaking change which adds functionality)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)
  • 🏭 Refactoring that does not change existing functionality but does improve things like code readability, structure etc
  • 🔨 New or improved tests for existing code
  • ⛑ git-flow chore (backport, hotfix, etc)

Developer checklist

  • I have read the style guide document, and my code follows the code style of this project.
  • If my code deals with exceptions, it follows the guidelines.
  • I have updated any documentation accordingly, if needed. Documentation changes can be reflected in opening a PR on cardanodocs.com, amending the inline Haddock comments, any relevant README file or one of the document listed in the docs directory.
  • CHANGELOG entry has been added and is linked to the correct PR on GitHub.

Testing checklist

  • I have added tests to cover my changes.
  • All new and existing tests passed.

QA Steps

Added the following tests:

        -- Initial Selection:      Final Selection:
        --   inputs : [200000]       inputs : [200000]
        --   outputs: [1]            outputs: [1]
        --   changes: [199999]       changes: [28094]
        --   fee+   : 171905         fee+   : 171817
        --
        --           Actual fee: 171905 (+88)
        randomTest "fee calculation: no extra inputs, no extra change" 1 $ run $ do
            source <- fixtureWallet (Core.mkCoin <$> [200000])
            resp <- makePayment (Core.mkCoin 1) source =<< getRandomAddress
            expectConfirmation source resp

        -- Initial Selection:      Final Selection:
        --   inputs : [171906]       inputs : [171906]
        --   outputs: [1]            outputs: [1]
        --   changes: [171905]       changes: []
        --   fee+   : 171905         fee+   : 167862
        --
        --           Actual fee: 167862 (+4043)
        randomTest "fee calculation: empty a wallet" 1 $ run $ do
            source <- fixtureWallet (Core.mkCoin <$> [171906])
            resp <- makePayment (Core.mkCoin 1) source =<< getRandomAddress
            expectConfirmation source resp

        -- Initial Selection:      Final Selection:
        --   inputs : [100000]       inputs : [100000, 100000]
        --   outputs: [1]            outputs: [1]
        --   changes: [99999]        changes: [19964]
        --   fee+   : 171905         fee+   : 179947
        --
        --           Actual fee: 180035 (+88)
        randomTest "fee calculation: needs extra input, no extra change" 1 $ run $ do
            source <- fixtureWallet (Core.mkCoin <$> [100000, 100000])
            resp <- makePayment (Core.mkCoin 1) source =<< getRandomAddress
            expectConfirmation source resp

        -- Initial Selection:      Final Selection:
        --   inputs : [30000]        inputs : [30000, 30000, 30000, 30000,
        --                                     30000, 30000, 30000, 30000]
        --   outputs: [42]           outputs: [42]
        --   changes: [29958]        changes: [11055]
        --   fee+   : 171905         fee+   : 228815
        --
        --           Actual fee: 228903 (+88)
        randomTest "fee calculation: needs many extra inputs" 1 $ run $ do
            source <- fixtureWallet (replicate 8 (Core.mkCoin 30000))
            resp <- makePayment (Core.mkCoin 42) source =<< getRandomAddress
            expectConfirmation source resp

Without patch:

Transactions
  fee calculation: no extra inputs, no extra change
    +++ OK, passed 1 tests.
  fee calculation: empty a wallet FAILED [1]
  fee calculation: needs extra input, no extra change FAILED [2]
  fee calculation: needs many extra inputs FAILED [3]

Failures:

  integration/Util.hs:187:5: 
  1) Transactions fee calculation: empty a wallet
       uncaught exception: IOException of type UserError
       user error (transactions didn't complete on time: this is unexpected)
       (after 1 test)

  integration/Util.hs:187:5: 
  2) Transactions fee calculation: needs extra input, no extra change
       uncaught exception: IOException of type UserError
       user error (transactions didn't complete on time: this is unexpected)
       (after 1 test)

  integration/Util.hs:187:5: 
  3) Transactions fee calculation: needs many extra inputs
       uncaught exception: IOException of type UserError
       user error (transactions didn't complete on time: this is unexpected)
       (after 1 test)

Finished in 550.9976 seconds
4 examples, 3 failures

With patch:

Transactions
  fee calculation: no extra inputs, no extra change
    +++ OK, passed 1 tests.
  fee calculation: empty a wallet
    +++ OK, passed 1 tests.
  fee calculation: needs extra input, no extra change
    +++ OK, passed 1 tests.
  fee calculation: needs many extra inputs
    +++ OK, passed 1 tests.

Finished in 225.7984 seconds
4 examples, 0 failures

Screenshots (if available)

How to merge

Send the message bors r+ to merge this PR. For more information, see
docs/how-to/bors.md.

So, basically, by conflating a bit the selected entries and changes, a
lot of things become easier.  At the price of one thing: fairness.

The previous code was splitting fee across change proportionally to
inputs.  So here, I just split the fee across all changes, regardless of
the input. So everyone's got to pay the same part of for the
transaction.

One could see it as another type of fairness 🙃 ...  But
that's also a lot simpler to handle, because we can just manipulate all
inputs and all changes directly and compute fee for those directly.
Prefiltering seems to totally ignore txIds from Inputs while creating
blockMeta. On the other hand it uses TxIns to see which utxos are used.
This means that if an account has only inputs in a tx, the tx will be
marked as failed (`WontApply`).

This can happens when it creating a transction that consumes its utxo
entirely (no change outputs).
@KtorZ KtorZ self-assigned this Nov 20, 2018
@KtorZ KtorZ requested a review from kderme November 20, 2018 12:31
Copy link
Member

@kderme kderme left a comment

Choose a reason for hiding this comment

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

LGTM!

@KtorZ KtorZ merged commit 0bd5d15 into develop Nov 21, 2018
@iohk-bors iohk-bors bot deleted the KtorZ/RCD-45-RCD-44/review-fee-calculation branch November 21, 2018 08:37
@KtorZ KtorZ mentioned this pull request Jan 4, 2019
12 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants