Add transaction expiry slot for pending transactions #1879

rvl · 2020-07-08T06:51:04Z

Issue Number

Relates to ADP-93 / #1838.

Overview

Adds expiry slot to pending transactions in order to implement transaction TTL.
Expiry slot and time is reported in the transaction history.
At the expiry slot, pending transactions are marked Expired.
Default TTL of 7200 slots is still hard coded.

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

rvl · 2020-10-02T03:24:38Z

bors try

iohk-bors · 2020-10-02T04:26:47Z

try

Build succeeded:

KtorZ · 2020-10-06T07:43:20Z

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

+            , expectField #expiresAt (`shouldSatisfy` isJust)
+            ]
+
+        -- This stuff would be easier with Control.Lens...


Are we tracking that in a technical debt ticket 🤔 ?

No, because I sent a message about this to slack, and got exactly 0 replies.

KtorZ · 2020-10-06T07:47:45Z

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

+        let sl = sinceBlock ^. #absoluteSlotNumber . #getApiT
+
+        -- The expected expiry slot (adds the hardcoded default ttl)
+        let ttl = sl + SlotNo 7200


Should we perhaps expose that default "magic number" ? Or at least, have it behind a named constant in the integration DSL so that we do not duplicate it all over the place.

KtorZ · 2020-10-06T07:54:21Z

lib/core/src/Cardano/Wallet.hs

@@ -840,6 +840,7 @@ restoreBlocks ctx wid blocks nodeTip = db & \DBLayer{..} -> mapExceptT atomicall
    let k = gp ^. #getEpochStability
    let localTip = currentTip $ NE.last cps

+    updatePendingTx (PrimaryKey wid) (view #slotNo localTip)


update feels a bit general here. When reading this, my first thought is "update what?". What about using a more specific qualifier like: "discardNowExpiredTx" or, whatever you'd come up with ?

updatePendingTxForExpiry

KtorZ · 2020-10-06T07:56:34Z

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

@@ -1332,9 +1333,11 @@ mkApiTransactionFromInfo ti (TransactionInfo txid ins outs ws meta depth txtime
        case meta ^. #status of
            Pending  -> #pendingSince
            InLedger -> #insertedAt
+            Expired  -> #pendingSince


Shall we perhaps use yet-another-field for tracking the time since which the transaction has expired?

I think "pending_since" is the correct field. The time at which the transaction expired is in "expires_at".
So both times are captured for expired transactions.

Fair enough!

KtorZ · 2020-10-06T08:02:53Z

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

+data ApiSlotReference = ApiSlotReference
+    { time :: !UTCTime
+    , absoluteSlotNumber :: !(ApiT SlotNo)
+    } deriving (Eq, Generic, Show)


What about including also the epoch and slot within epoch to be consistent with other time references? We use:

data ApiTimeReference = ApiTimeReference { time :: !UTCTime , block :: !ApiBlockReference } deriving (Eq, Generic, Show) data ApiBlockReference = ApiBlockReference { epochNumber :: !(ApiT EpochNo) , slotNumber :: !(ApiT SlotInEpoch) , height :: !(Quantity "block" Natural) , absoluteSlotNumber :: !(ApiT SlotNo) } deriving (Eq, Generic, Show)

Although indeed, the TTL is about slots and not blocks so the height don't make much sense. The other however do. So I'd suggest to have something along the lines of:

data ApiTimeReferenceBySlot = ApiTimeReferenceBySlot { time :: !UTCTime , slot :: !ApiSlotReference } deriving (Eq, Generic, Show) data ApiSlotReference = ApiBlockReference { epochNumber :: !(ApiT EpochNo) , slotNumber :: !(ApiT SlotInEpoch) , absoluteSlotNumber :: !(ApiT SlotNo) } deriving (Eq, Generic, Show)

Not only is TTL about slots, but it's about slots in the future -- which cannot have a block height.

OK I have added epoch and slot numbers.

For consistency, It'd be better to have them nested under a slot field, don't you think 🤔 ?

Yeah maybe - I am going to straighten out the types a bit (without changing existing API fields).

KtorZ · 2020-10-07T06:26:20Z

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

 minUTxOValue :: Natural
 minUTxOValue = 1_000_000

+-- | Wallet server's chosen transaction TTL value (in slots) when none is given.
+defaultTxTTL :: SlotNo
+defaultTxTTL = 7200


KtorZ · 2020-10-07T06:28:04Z

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

+data ApiSlotReference = ApiSlotReference
+    { time :: !UTCTime
+    , absoluteSlotNumber :: !(ApiT SlotNo)
+    } deriving (Eq, Generic, Show)


For consistency, It'd be better to have them nested under a slot field, don't you think 🤔 ?

KtorZ · 2020-10-07T06:28:19Z

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

@@ -174,6 +175,9 @@ newDBLayer timeInterpreter = do
                                       Pending Tx
        -----------------------------------------------------------------------}

+        , updatePendingTxForExpiry = \pk tip -> ExceptT $ do
+            alterDB errNoSuchWallet db (mUpdatePendingTx pk tip)


rvl · 2020-10-09T09:11:40Z

@KtorZ I made the API types more consistent (last 2 commits).

ApiBlockReference is just ApiSlotReference with block information added.

ApiNetworkTip is replaced with ApiSlotReference.

The end result is that all references to a slot/time look the same in the API.

The Haskell API types have SlotId and block info nested rather than inline. I hope in future that we can change the swagger spec to match. But for now there are some non-generic Aeson instances to maintain backwards compatibility.

ApiBlockReference is just ApiSlotReference with block information added -- block references gain slot info. ApiNetworkTip is replaced with ApiSlotReference -- so it gains a "time" field. The Haskell API types have SlotId and block info nested rather than inline. I hope in future that we can change the swagger spec to match. But for now there are some non-generic Aeson instances to maintain backwards compatibility.

KtorZ

Nice work!

KtorZ · 2020-10-09T15:34:37Z

The Haskell API types have SlotId and block info nested rather than inline. I hope in future that we can change the swagger spec to match. But for now there are some non-generic Aeson instances to maintain backwards compatibility.

I pointed that out in my review from yesterday which I apparently didn't submit :| ... I agree that having one flat and the other nested is sort of inconsistent and I'd very much prefer getting rid of this inconsistency but its also neat-picking at this stage. So happy to move forward with that one.

KtorZ · 2020-10-09T16:25:14Z

bors merge

iohk-bors · 2020-10-09T19:30:05Z

Build succeeded:

@KtorZ

2235: Fix extra-source-files not containing files used by TH r=hasufell a=hasufell 2239: WIP: Bump version from 2020.9.30 to 2020.10.13 r=KtorZ a=jonathanknowles  Compatible with [`jormungandr@v0.9.0`](https://github.com/input-output-hk/jormungandr/releases/tag/v0.9.0) and [`cardano-node@1.21.1`](https://github.com/input-output-hk/cardano-node/releases/tag/1.21.1). ## New Features - Adds the ability for users to select their own SMASH servers for stakepool listings. (#2214) - Adds transaction expiry slots for pending transactions. (#1879) ## Improvements - Adds a 100-wallet scenario to the latency benchmark. (#2223) - Adds an executable `shelley-test-cluster` which starts an integration test cluster that includes faucets. (#2178) - Extends `isOurs` to return the derivation path of an address. (#2219) ## Resolved Issues - Make pool garbage collection handle an unknown current epoch. (#2203) - Fixes incorrect mainnet network parameters returned from API. (#2226) ## Known Issues **_This section is a work in progress._** - Wallet restoration status reported incorrectly on mainnet. ([ADP-483](https://jira.iohk.io/browse/ADP-483)) ## Documentation  Cardano (cardano-node) | ITN (Jörmungandr) --- | --- [API Documentation](https://input-output-hk.github.io/cardano-wallet/api/v2020-10-13) | [API Documentation](https://input-output-hk.github.io/cardano-wallet/api/v2020-10-13) [CLI Manual](https://github.com/input-output-hk/cardano-wallet/wiki/Wallet-command-line-interface/f71ecb7ece3deaec5bf60f4a4beea1d3950e0d46) | [CLI Manual](https://github.com/input-output-hk/cardano-wallet/wiki/Wallet-command-line-interface-jormungandr/f71ecb7ece3deaec5bf60f4a4beea1d3950e0d46) [Docker Manual](https://github.com/input-output-hk/cardano-wallet/wiki/Docker/f71ecb7ece3deaec5bf60f4a4beea1d3950e0d46) | [Docker Manual](https://github.com/input-output-hk/cardano-wallet/wiki/Docker-jormungandr/f71ecb7ece3deaec5bf60f4a4beea1d3950e0d46) ## Installation Instructions ### Cardano (cardano-node) 1. Install [`cardano-node@1.21.1`](https://github.com/input-output-hk/cardano-node/releases/tag/1.21.1). 2. Download the provided `cardano-wallet` for your platform, and uncompress it in a directory that is on your `$PATH`, e.g. `/usr/local/bin`. Or `%PATH%` on Windows. 4. Start `cardano-wallet --help` and see available parameters. #### Docker Pull from DockerHub and verify the version matches 2020.10.13. ``` $ docker pull inputoutput/cardano-wallet:2020.10.13 $ docker run --rm inputoutput/cardano-wallet:2020.10.13 version ``` ### ITN (jormungandr) 1. Install [`jormungandr@v0.9.0`](https://github.com/input-output-hk/jormungandr/releases/tag/v0.9.0). 2. Download the provided `cardano-wallet-jormungandr` for your platform, and uncompress it in a directory that is on your `$PATH`, e.g. `/usr/local/bin`. Or `%PATH%` on Windows. 3. (optional) Install the bash/zsh auto-completion script according to the [jormungandr cli manual](https://github.com/input-output-hk/cardano-wallet/wiki/Wallet-Command-Line-Interface/{{JORM_CLI_WIKI_COMMIT}}) 4. Start `cardano-wallet --help` and see available parameters. #### Docker Pull from DockerHub and verify the version matches 2020.10.13 ``` $ docker pull inputoutput/cardano-wallet:2020.10.13-jormungandr $ docker run --rm inputoutput/cardano-wallet:2020.10.13-jormungandr version ``` ## Signatures  Name | Role | Approval --- | --- | ---: Matthias Benkort @KtorZ | Technical Team Lead | ⌛ Piotr Stachyra @piotr-iohk | QA Engineer | ⌛ Tatyana Valkevych @tatyanavych | Release Manager | ⌛ Co-authored-by: Julian Ospald <julian.ospald@iohk.io> Co-authored-by: Rodney Lorrimar <rodney.lorrimar@iohk.io> Co-authored-by: Jonathan Knowles <jonathan.knowles@iohk.io> Co-authored-by: IOHK <devops+stack-project@iohk.io>

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

rvl added this to the (ADP-93) Transaction scheduler milestone Jul 8, 2020

rvl self-assigned this Jul 8, 2020

jonathanknowles reviewed Jul 9, 2020

View reviewed changes

lib/core/src/Cardano/Wallet/Primitive/Types.hs Outdated Show resolved Hide resolved

jonathanknowles reviewed Jul 9, 2020

View reviewed changes

lib/core/src/Cardano/Wallet/Primitive/Types.hs Outdated Show resolved Hide resolved

rvl force-pushed the rvl/1838/ttl-tx branch 2 times, most recently from 19d373c to b2830bc Compare July 14, 2020 12:02

rvl force-pushed the rvl/1838/ttl-tx branch from b2830bc to 91fa154 Compare July 29, 2020 05:56

rvl force-pushed the rvl/1838/ttl-tx branch 2 times, most recently from 8775410 to 3c6f96d Compare August 5, 2020 06:56

rvl force-pushed the rvl/1838/ttl-tx branch 2 times, most recently from b43d675 to 86273d4 Compare August 13, 2020 06:07

rvl force-pushed the rvl/1838/ttl-tx branch 3 times, most recently from b21f9d8 to 25374a2 Compare August 26, 2020 01:34

rvl force-pushed the rvl/1838/ttl-tx branch 2 times, most recently from 8c998f0 to a76ae7e Compare September 23, 2020 05:59

rvl changed the title ~~wip: Add transaction expiry slot for pending transactions~~ Add transaction expiry slot for pending transactions Sep 23, 2020

rvl mentioned this pull request Sep 23, 2020

Add transaction TTL to payments API #2167

Merged

8 tasks

rvl force-pushed the rvl/1838/ttl-tx branch 4 times, most recently from 2792944 to aef0bdc Compare October 1, 2020 05:45

rvl marked this pull request as ready for review October 1, 2020 07:50

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

Try #1879:

ff5b6ac

rvl force-pushed the rvl/1838/ttl-tx branch from 3deebc5 to 776d478 Compare October 2, 2020 10:04

KtorZ reviewed Oct 6, 2020

View reviewed changes

rvl force-pushed the rvl/1838/ttl-tx branch from 776d478 to 8170767 Compare October 7, 2020 06:13

KtorZ reviewed Oct 7, 2020

View reviewed changes

rvl added 7 commits October 9, 2020 18:56

swagger.yaml: Add transaction expires_at and expired status

52008cc

Add transaction expiry slot for pending transactions

1b83368

tests: add TRANS_CREATE_10 - Pending transaction expiry

e0f788b

Rename ApiTimeReference -> ApiTimeReferenceWithBlock

932409b

Add epoch and local slot index to tx expiry slot reference

f54c3ed

Rename updatePendingTx to updatePendingTxForExpiry

4119672

Add default TTL constant to tests DSL

7db4d33

rvl force-pushed the rvl/1838/ttl-tx branch from d6cf7bb to 7c691f3 Compare October 9, 2020 09:02

rvl force-pushed the rvl/1838/ttl-tx branch from 7c691f3 to 0c634ac Compare October 9, 2020 09:20

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

KtorZ approved these changes Oct 9, 2020

View reviewed changes

Update golden API files because the time field is added

327e021

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

iohk-bors bot merged commit ce650ab into master Oct 9, 2020

iohk-bors bot deleted the rvl/1838/ttl-tx branch October 9, 2020 19:30

jonathanknowles mentioned this pull request Oct 13, 2020

Bump version from 2020.9.30 to 2020.10.13 #2239

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add transaction expiry slot for pending transactions #1879

Add transaction expiry slot for pending transactions #1879

rvl commented Jul 8, 2020 •

edited

Loading

rvl commented Oct 2, 2020

iohk-bors bot commented Oct 2, 2020

KtorZ Oct 6, 2020

rvl Oct 7, 2020

KtorZ Oct 6, 2020

rvl Oct 7, 2020

KtorZ Oct 6, 2020

rvl Oct 7, 2020

KtorZ Oct 6, 2020

rvl Oct 7, 2020

KtorZ Oct 7, 2020

KtorZ Oct 6, 2020

rvl Oct 7, 2020

KtorZ Oct 7, 2020

rvl Oct 8, 2020

KtorZ Oct 7, 2020

KtorZ Oct 7, 2020

KtorZ Oct 7, 2020

rvl commented Oct 9, 2020

KtorZ left a comment

KtorZ commented Oct 9, 2020

KtorZ commented Oct 9, 2020

iohk-bors bot commented Oct 9, 2020

Add transaction expiry slot for pending transactions #1879

Add transaction expiry slot for pending transactions #1879

Conversation

rvl commented Jul 8, 2020 • edited Loading

Issue Number

Overview

rvl commented Oct 2, 2020

iohk-bors bot commented Oct 2, 2020

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

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

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

Choose a reason for hiding this comment

rvl commented Oct 9, 2020

KtorZ left a comment

Choose a reason for hiding this comment

KtorZ commented Oct 9, 2020

KtorZ commented Oct 9, 2020

iohk-bors bot commented Oct 9, 2020

rvl commented Jul 8, 2020 •

edited

Loading