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

Tx metadata included in fee calculations #2075

Closed
rvl opened this issue Aug 26, 2020 · 4 comments
Closed

Tx metadata included in fee calculations #2075

rvl opened this issue Aug 26, 2020 · 4 comments
Assignees
Labels
ADDING FEATURE Mark a PR as adding a new feature, for auto-generated CHANGELOG

Comments

@rvl
Copy link
Contributor

rvl commented Aug 26, 2020

Context

ADP-307

Adding metadata to transactions increases their size on chain. Therefore metadata incurs fees.

Decision

  1. Include metadata in any call to computeTxSize.

Acceptance Criteria

  1. Adding metadata to a transaction must increase its size, and therefore its fee.

Development

As above.

QA

  1. Added unit test "metadata incurs fees" to lib/shelley/test/unit/Cardano/Wallet/Shelley/TransactionSpec.hs ⇒ PR Add unit test for transaction metadata fee calculation #2108.
  2. Added integration test "TRANS_ESTIMATE_xxx - fee estimation includes metadata" to lib/core-integration/src/Test/Integration/Scenario/API/Shelley/Transactions.hs ⇒ PR Tx metadata API integration tests #2096.
@piotr-iohk
Copy link
Contributor

I have managed to brake it:

It seems that estimating fee with too large metadata (and actually sendindg such tx too) results in 500:

Result:

Code = 500,
Message = Something went wrong

In the log there is:

[2020-09-03 11:01:12.86 UTC] [RequestId 0] [POST] /v2/wallets/1ceb45b37a94c7022837b5ca14045f11a5927c65/payment-fees
generated a coin selection with an excessively large fee.
actual fee: 3.194 Ada
coin selection:
  inputs: - 1st d1727eae (~ 22490393 @ 0150e277...8094ee36)
  withdrawal: -0
  reclaim: -0
  outputs: - 1000000 @ 013e8da9...8094ee36
  change: [18296768]
  deposit: -0

CallStack (from HasCallStack):
  error, called at src/Cardano/Wallet/Primitive/Fee.hs:182:9 in cardano-wallet-core-2020.8.3-EUxOQdVGrF0B4QcgxDCOoV:Cardano.Wallet.Primitive.Fee
  adjustForFee, called at src/Cardano/Wallet.hs:1288:24 in cardano-wallet-core-2020.8.3-EUxOQdVGrF0B4QcgxDCOoV:Cardano.Wallet

I have created PR extending tx meta tests -> #2110.
TRANSMETA_CREATE_03, TRANSMETA_ESTIMATE_03 - seem to cover that case there and are currently failing.

@KtorZ
Copy link
Member

KtorZ commented Sep 3, 2020

Awe crap. I think that's because the guard which checks whether metadata are too large occurs only when one tries to submit the transaction, which happens after this invariant is triggered. We should move the transaction size guard earlier in the pipeline.

@piotr-iohk
Copy link
Contributor

BTW, Seems that latency benchamark failed for the same reason -> https://buildkite.com/input-output-hk/cardano-wallet-nightly/builds/630#f25ae394-5bd6-4422-ae66-12bf6700a0b3

iohk-bors bot added a commit that referenced this issue Sep 3, 2020
2108: Add unit test for transaction metadata fee calculation r=Anviking a=rvl

### Issue Number

ADP-307 / #2075 

### Overview

- Property tests that the fee calculation produces a higher result when there is metadata present.
- The test is along the lines of the withdrawal fee calculation test, which I reworded slightly.


Co-authored-by: Rodney Lorrimar <rodney.lorrimar@iohk.io>
Co-authored-by: KtorZ <matthias.benkort@gmail.com>
iohk-bors bot added a commit that referenced this issue Sep 3, 2020
2108: Add unit test for transaction metadata fee calculation r=Anviking a=rvl

### Issue Number

ADP-307 / #2075 

### Overview

- Property tests that the fee calculation produces a higher result when there is metadata present.
- The test is along the lines of the withdrawal fee calculation test, which I reworded slightly.


Co-authored-by: Rodney Lorrimar <rodney.lorrimar@iohk.io>
Co-authored-by: KtorZ <matthias.benkort@gmail.com>
iohk-bors bot added a commit that referenced this issue Sep 4, 2020
2110: Tx metadata tests r=rvl a=piotr-iohk

# Issue Number

#2075
#2074

# Overview

- 8a1b1eb
  Extend tx with meta test to check if metadata available on tx list and tx get
  
- a90cefa
  Additional tests for fee estimation with metadata

# Comments

TRANSMETA_CREATE_03, TRANSMETA_ESTIMATE_03 - are currently failing -> #2075 (comment).


Co-authored-by: Piotr Stachyra <piotr.stachyra@iohk.io>
Co-authored-by: KtorZ <matthias.benkort@gmail.com>
Co-authored-by: Rodney Lorrimar <rodney.lorrimar@iohk.io>
iohk-bors bot added a commit that referenced this issue Sep 4, 2020
2108: Add unit test for transaction metadata fee calculation r=Anviking a=rvl

### Issue Number

ADP-307 / #2075 

### Overview

- Property tests that the fee calculation produces a higher result when there is metadata present.
- The test is along the lines of the withdrawal fee calculation test, which I reworded slightly.


Co-authored-by: Rodney Lorrimar <rodney.lorrimar@iohk.io>
Co-authored-by: KtorZ <matthias.benkort@gmail.com>
iohk-bors bot added a commit that referenced this issue Sep 4, 2020
2108: Add unit test for transaction metadata fee calculation r=Anviking a=rvl

### Issue Number

ADP-307 / #2075 

### Overview

- Property tests that the fee calculation produces a higher result when there is metadata present.
- The test is along the lines of the withdrawal fee calculation test, which I reworded slightly.


Co-authored-by: Rodney Lorrimar <rodney.lorrimar@iohk.io>
Co-authored-by: KtorZ <matthias.benkort@gmail.com>
iohk-bors bot added a commit that referenced this issue Sep 5, 2020
2108: Add unit test for transaction metadata fee calculation r=Anviking a=rvl

### Issue Number

ADP-307 / #2075 

### Overview

- Property tests that the fee calculation produces a higher result when there is metadata present.
- The test is along the lines of the withdrawal fee calculation test, which I reworded slightly.


Co-authored-by: Rodney Lorrimar <rodney.lorrimar@iohk.io>
Co-authored-by: KtorZ <matthias.benkort@gmail.com>
@piotr-iohk
Copy link
Contributor

lgtm.
The issue no longer occurs. When metadata is too big error message is presented for both: sending tx and fee estimation:

Code = 400,
Message = {"code":"transaction_is_too_big","message":"I am afraid that the transaction you're trying to submit is too large! The network allows transactions only as large as 16384 bytes! As it stands, the current transaction only allows me to select up to 0 inputs. Note that I am selecting inputs randomly, so retrying *may work* provided I end up choosing bigger inputs sufficient to cover the transaction cost. Alternatively, try sending to less recipients or with smaller metadata."}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ADDING FEATURE Mark a PR as adding a new feature, for auto-generated CHANGELOG
Projects
None yet
Development

No branches or pull requests

3 participants