Add transaction TTL to payments API #2167

rvl · 2020-09-23T06:34:48Z

Issue Number

ADP-93 / #1840

Overview

Add transaction TTL to swagger spec. User provides the value in seconds.
Add transaction TTL to API types
Add TTL slot calculation to API handler function
Adjust mkStdTx to make ttl easier - it now takes the expiry slot directly.
Integration tests

Comments

Next PRs

Allow deleting expired transactions ⇒ Allow deleting expired transactions from the API #2262
Add CLI option cardano-wallet transaction create [--ttl=SECONDS] ⇒ Add CLI option for transaction TTL #2267
Perhaps clean up the large number of function arguments and return values in transaction layer functions.

rvl · 2020-10-13T06:09:02Z

bors try

iohk-bors · 2020-10-13T06:20:13Z

try

Build failed:

buildkite/cardano-wallet

rvl · 2020-10-13T06:23:20Z

Fixed code warning, doh.

bors try

iohk-bors · 2020-10-13T06:57:59Z

try

Build failed:

ci/hydra-build:required

Need to fix something.

lib/core-integration/src/Test/Integration/Framework/DSL.hs

lib/core-integration/src/Test/Integration/Scenario/API/Shelley/Transactions.hs

lib/core/src/Cardano/Wallet.hs

lib/core/src/Cardano/Wallet/Api/Types.hs

lib/core/src/Cardano/Wallet/DB/Sqlite.hs

specifications/api/swagger.yaml

KtorZ · 2020-10-22T15:59:28Z

lib/core-integration/src/Test/Integration/Scenario/API/Shelley/Transactions.hs

+
+        -- Expected and actual are fairly close. Any difference should only be
+        -- due to slot rounding.
+        slotDiff txExpectedExp txActualExp `shouldSatisfy` (< 50)


What about also waiting for the transaction to expire and verify that its state eventually becomes reported as expired?

This test case is testing that a custom transaction expiry works, and then I test expiry and reporting of the tx status in the next test case TRANS_TTL_03.

KtorZ · 2020-10-22T15:59:58Z

lib/core/src/Cardano/Wallet.hs

@@ -43,7 +43,7 @@

 module Cardano.Wallet
    (
-    -- * Developement
+    -- * Development


2-year old typo, never too late to fix 😅

KtorZ · 2020-10-22T16:01:14Z

lib/core/src/Cardano/Wallet.hs

+    expTime <- addUTCTime ttl <$> getCurrentTime
+    try $ ti $ ceilingSlotAt expTime
+  where
+    ttl = fromMaybe defaultTTL maybeTTL


KtorZ · 2020-10-22T16:03:06Z

lib/core/src/Cardano/Wallet/Api/Types.hs

    } deriving (Eq, Generic, Show)

 data PostTransactionFeeData (n :: NetworkDiscriminant) = PostTransactionFeeData
    { payments :: (NonEmpty (AddressAmount (ApiT Address, Proxy n)))
    , withdrawal :: !(Maybe ApiWithdrawalPostData)
    , metadata :: !(Maybe (ApiT TxMetadata))
+    , timeToLive :: !(Maybe (Quantity "second" NominalDiffTime))


rvl · 2020-10-26T06:04:56Z

bors r+

2167: Add transaction TTL to payments API r=rvl a=rvl ### Issue Number ADP-93 / #1840 ### Overview - [x] Add transaction TTL to swagger spec. User provides the value in seconds. - [x] Add transaction TTL to API types - [x] Add TTL slot calculation to API handler function - [x] Adjust mkStdTx to make ttl easier - it now takes the expiry slot directly. - [x] Integration tests ### Comments Next PR - [ ] Add CLI option `cardano-wallet transaction create [--ttl=SECONDS]` - [ ] Perhaps clean up the large number of function arguments and return values in transaction layer functions. Co-authored-by: Rodney Lorrimar <[email protected]>

iohk-bors · 2020-10-26T08:31:20Z

Timed out.

rvl · 2020-10-27T06:04:02Z

bors r+

2167: Add transaction TTL to payments API r=rvl a=rvl ### Issue Number ADP-93 / #1840 ### Overview - [x] Add transaction TTL to swagger spec. User provides the value in seconds. - [x] Add transaction TTL to API types - [x] Add TTL slot calculation to API handler function - [x] Adjust mkStdTx to make ttl easier - it now takes the expiry slot directly. - [x] Integration tests ### Comments Next PRs - [x] Allow deleting expired transactions ⇒ #2262 - [x] Add CLI option `cardano-wallet transaction create [--ttl=SECONDS]` ⇒ #2267 - [ ] Perhaps clean up the large number of function arguments and return values in transaction layer functions. Co-authored-by: Rodney Lorrimar <[email protected]>

iohk-bors · 2020-10-27T08:14:34Z

Build failed:

ci/hydra-build:required

  src/Test/Integration/Framework/DSL.hs:1486:7:
  1) API Specifications, SHELLEY_TRANSACTIONS, TRANS_TTL_03 - Expired transactions
       expected a successful response but got an error: DecodeFailure "{\"code\":\"created_invalid_transaction\",\"message\":\"That's embarrassing. It looks like I've created an invalid transaction that could not be parsed by the node. Here's an error message that may help with debugging: HardForkApplyTxErrFromEra S (Z (WrapApplyTxErr {unwrapApplyTxErr = ApplyTxError [LedgerFailure (UtxowFailure (UtxoFailure (ExpiredUTxO (SlotNo 2907) (SlotNo 2908))))]}))\"}"
       While verifying (Status {statusCode = 500, statusMessage = "Internal Server Error"},Left (DecodeFailure "{\"code\":\"created_invalid_transaction\",\"message\":\"That's embarrassing. It looks like I've created an invalid transaction that could not be parsed by the node. Here's an error message that may help with debugging: HardForkApplyTxErrFromEra S (Z (WrapApplyTxErr {unwrapApplyTxErr = ApplyTxError [LedgerFailure (UtxowFailure (UtxoFailure (ExpiredUTxO (SlotNo 2907) (SlotNo 2908))))]}))\"}"))

KtorZ · 2020-10-27T08:31:53Z

lib/core-integration/src/Test/Integration/Scenario/API/Shelley/Transactions.hs

+
+        -- this transaction is going to expire really soon.
+        basePayload <- mkTxPayload ctx wb amt fixturePassphrase
+        let payload = addTxTTL 0.1 basePayload


Sounds like this is a bit flaky :s, we should choose a time slightly larger in front of the time slot.

Hmm yes, not good.

The TTL must be small enough that the tx expires before it can get into a block, but large enough that the node allows it into its mempool.

I need another approach to testing this which isn't flaky.

@rvl a possible approach could be to double-spend? If we make two transactions that are using a common UTxO, both should be "accepted" in the mempool and only one would go through ultimately. Then, we can set a large-enough but still short TTL and simply wait for either one to expire.

So, the test could be:

a) Setup a wallet with a single UTxO
b) Send funds to A with TTL t
c) Forget transaction
d) Send funds to B with TTL t

Yet, this maybe does not test what you intend to test? If we want to check that the transaction is not inserted because it is expired (and not because it is double-spending), then it doesn't work :/

rvl · 2020-10-28T06:20:32Z

bors r+

2167: Add transaction TTL to payments API r=rvl a=rvl ### Issue Number ADP-93 / #1840 ### Overview - [x] Add transaction TTL to swagger spec. User provides the value in seconds. - [x] Add transaction TTL to API types - [x] Add TTL slot calculation to API handler function - [x] Adjust mkStdTx to make ttl easier - it now takes the expiry slot directly. - [x] Integration tests ### Comments Next PRs - [x] Allow deleting expired transactions ⇒ #2262 - [x] Add CLI option `cardano-wallet transaction create [--ttl=SECONDS]` ⇒ #2267 - [ ] Perhaps clean up the large number of function arguments and return values in transaction layer functions. Co-authored-by: Rodney Lorrimar <[email protected]>

iohk-bors · 2020-10-28T07:13:46Z

Build failed:

buildkite/cardano-wallet

Failures:

  src/Test/Integration/Scenario/API/Shelley/Transactions.hs:1132:18:
  1) API Specifications, SHELLEY_TRANSACTIONS, TRANS_EXTERNAL_02 - Multiple Outputs Transaction - Shelley witnesses
       Waited longer than 90s (more than 2 epochs) to resolve action: "wFaucet and wSrc balances are as expected".
       expected: Quantity {getQuantity = 999989843200}
        but got: Quantity {getQuantity = 999989843000}

  To rerun use: --match "/API Specifications/SHELLEY_TRANSACTIONS/TRANS_EXTERNAL_02 - Multiple Outputs Transaction - Shelley witnesses/"

Randomized with seed 734248806

Finished in 2189.6516 seconds
687 examples, 1 failure, 4 pending

KtorZ · 2020-10-29T15:37:57Z

bors retry

iohk-bors · 2020-10-29T17:55:04Z

Build succeeded:

2262: Allow deleting expired transactions from the API r=rvl a=rvl ### Issue Number ADP-93 / #1840 ### Overview - [x] Using the existing transaction delete endpoint, also permit removing expired transactions. - [x] Simplify DB model and state machine tests for this function - [x] Integration test ### Comments - Based on PR #2167 branch - merge that first. 2282: Increase bors timeout from 2h to 3h r=Anviking a=Anviking # Issue Number #2279 # Overview  - [x] Increase bors timeout from 2h to 3h # Comments It seems hydra is often slow to schedule/run the jobs, causing ≈18% of bors r+ to fail. It should be better to increase the timeout to 3h, than to have it fail. This doesn't affect the timeout of buildkite or hydra themselves.   Co-authored-by: Rodney Lorrimar <[email protected]> Co-authored-by: Johannes Lund <[email protected]>

2262: Allow deleting expired transactions from the API r=rvl a=rvl ### Issue Number ADP-93 / #1840 ### Overview - [x] Using the existing transaction delete endpoint, also permit removing expired transactions. - [x] Simplify DB model and state machine tests for this function - [x] Integration test ### Comments - Based on PR #2167 branch - merge that first. Co-authored-by: Rodney Lorrimar <[email protected]>

rvl added the ADDING FEATURE Mark a PR as adding a new feature, for auto-generated CHANGELOG label Sep 23, 2020

rvl added this to the (ADP-93) Transaction scheduler milestone Sep 23, 2020

rvl self-assigned this Sep 23, 2020

rvl force-pushed the rvl/1840/tx-ttl-api branch from 1d6b0e2 to 2e2c62c Compare October 1, 2020 08:40

rvl changed the title ~~Add transaction TTL to swagger spec~~ Add transaction TTL to payments API Oct 1, 2020

rvl force-pushed the rvl/1840/tx-ttl-api branch 2 times, most recently from cebf2d3 to f3b251d Compare October 2, 2020 10:57

rvl changed the base branch from master to rvl/1838/ttl-tx October 2, 2020 10:57

rvl force-pushed the rvl/1838/ttl-tx branch 4 times, most recently from 0c634ac to c938a0c Compare October 9, 2020 09:22

KtorZ force-pushed the rvl/1838/ttl-tx branch from c938a0c to 327e021 Compare October 9, 2020 16:25

Base automatically changed from rvl/1838/ttl-tx to master October 9, 2020 19:30

rvl force-pushed the rvl/1840/tx-ttl-api branch 4 times, most recently from f6fc3bd to 6bb0383 Compare October 13, 2020 06:08

iohk-bors bot added a commit that referenced this pull request Oct 13, 2020

Try #2167:

5a7686e

rvl force-pushed the rvl/1840/tx-ttl-api branch from 6bb0383 to e4b840e Compare October 13, 2020 06:21

iohk-bors bot added a commit that referenced this pull request Oct 13, 2020

Try #2167:

647b56b

rvl force-pushed the rvl/1840/tx-ttl-api branch from e4b840e to e8e1e41 Compare October 15, 2020 05:41

KtorZ reviewed Oct 19, 2020

View reviewed changes

rvl force-pushed the rvl/1840/tx-ttl-api branch from e8e1e41 to 7781555 Compare October 19, 2020 10:42

Fix haddock typo

f73dd8a

rvl force-pushed the rvl/1840/tx-ttl-api branch from 31b9a57 to a736044 Compare October 22, 2020 06:09

rvl added 4 commits October 22, 2020 22:33

Invert pending tx expiry logic

5d358a6

Add transaction TTL to payments in swagger spec

3df0b84

Add transaction TTL to payments API

7159d78

Regenerate API tests golden json

c824d00

rvl force-pushed the rvl/1840/tx-ttl-api branch from 5de8604 to c824d00 Compare October 22, 2020 12:37

rvl mentioned this pull request Oct 22, 2020

Allow deleting expired transactions from the API #2262

Merged

3 tasks

KtorZ approved these changes Oct 22, 2020

View reviewed changes

Fix DB model updatePendingTxForExpiry

3a1bd17

KtorZ reviewed Oct 27, 2020

View reviewed changes

Mark flaky test as pending, will fix it later

53e4416

iohk-bors bot merged commit c06c779 into master Oct 29, 2020

iohk-bors bot deleted the rvl/1840/tx-ttl-api branch October 29, 2020 17:55

Anviking mentioned this pull request Nov 9, 2020

Flaky test - various DB properties causing timeout #2292

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add transaction TTL to payments API #2167

Add transaction TTL to payments API #2167

rvl commented Sep 23, 2020 •

edited

Loading

rvl commented Oct 13, 2020

iohk-bors bot commented Oct 13, 2020

rvl commented Oct 13, 2020

iohk-bors bot commented Oct 13, 2020 •

edited by rvl

Loading

KtorZ Oct 22, 2020

rvl Oct 26, 2020

KtorZ Oct 22, 2020

KtorZ Oct 22, 2020

KtorZ Oct 22, 2020

rvl commented Oct 26, 2020

iohk-bors bot commented Oct 26, 2020

rvl commented Oct 27, 2020

iohk-bors bot commented Oct 27, 2020 •

edited by KtorZ

Loading

KtorZ Oct 27, 2020

rvl Oct 27, 2020

KtorZ Oct 27, 2020

rvl commented Oct 28, 2020

iohk-bors bot commented Oct 28, 2020 •

edited by rvl

Loading

KtorZ commented Oct 29, 2020

iohk-bors bot commented Oct 29, 2020

Add transaction TTL to payments API #2167

Add transaction TTL to payments API #2167

Conversation

rvl commented Sep 23, 2020 • edited Loading

Issue Number

Overview

Comments

rvl commented Oct 13, 2020

iohk-bors bot commented Oct 13, 2020

try

rvl commented Oct 13, 2020

iohk-bors bot commented Oct 13, 2020 • edited by rvl Loading

try

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rvl commented Oct 26, 2020

iohk-bors bot commented Oct 26, 2020

rvl commented Oct 27, 2020

iohk-bors bot commented Oct 27, 2020 • edited by KtorZ Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rvl commented Oct 28, 2020

iohk-bors bot commented Oct 28, 2020 • edited by rvl Loading

KtorZ commented Oct 29, 2020

iohk-bors bot commented Oct 29, 2020

rvl commented Sep 23, 2020 •

edited

Loading

iohk-bors bot commented Oct 13, 2020 •

edited by rvl

Loading

iohk-bors bot commented Oct 27, 2020 •

edited by KtorZ

Loading

iohk-bors bot commented Oct 28, 2020 •

edited by rvl

Loading