-
Notifications
You must be signed in to change notification settings - Fork 217
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
Database changes for resubmission of pending transactions #2570
Conversation
0ac5c53
to
7f53256
Compare
lib/core/src/Cardano/Wallet.hs
Outdated
pending <- atomically $ getLocalTxSubmissionPending (PrimaryKey wid) | ||
-- re-submit those transactions, ignore errors | ||
forM_ pending $ \st -> do | ||
-- TODO: compare slots to schedule retries less often |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah! That's exactly the bit I was looking for 😶; no schedule yet that is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably won't need anything more sophisticated that resend every n slots.
(Maybe resending every slot is overkill, but perhaps not).
-> SqlPersistT IO [(W.SlotNo, LocalTxSubmission)] | ||
listPendingLocalTxSubmissionQuery wid = fmap unRaw <$> rawSql query params | ||
where | ||
query = "SELECT meta.slot,?? FROM (select tx_id, status, slot, MAX(slot) AS latest_slot FROM tx_meta WHERE wallet_id=? GROUP by tx_id) AS meta INNER JOIN local_tx_submission ON meta.tx_id=local_tx_submission.tx_id WHERE status=? AND slot=latest_slot" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This MAX(slot)
suggests that we are now storing multiple metadata per transaction? This raises an alarm in my head for some reasons I am not yet able to explain. I'd like to review the implication of this a bit more carefully (will do in a next review).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, this check isn't needed. There is only ever one TxMeta per txid per wallet. In the case of rollback, the status field is mutated.
d2fecfa
to
8bee89a
Compare
8bee89a
to
0c5a214
Compare
c80226d
to
e267153
Compare
0c5a214
to
39f3c0c
Compare
e267153
to
74e1b08
Compare
39f3c0c
to
afd0f98
Compare
I came across this code, and wondered: how should tx retrying work across era-boundaries? IIRC, the node has some way of converting past-era txs to new-era txs? If so I guess we should simply try to decode the |
945ec7e
to
a174b7a
Compare
2585: Switch to use separate Win32-network repository r=rvl a=newhoggy # Issue Number N/A # Overview `Win32-network` has been move to a separate repository. Updating dependencies accordingly. # Comments N/A 2586: Add logging of tx submission r=rvl a=rvl ### Issue Number ADP-630 / ADP-764 ### Overview These are some logging/worker improvements I made in #2570. I'll put them in a separate PR because they overlap with #2583. - Adds logging of transaction submission results in the wallet layer. - Fixes the logging types in the tracer bundle. - Removes duplication between `initWorker` and `registerWorker`. Co-authored-by: John Ky <[email protected]> Co-authored-by: Rodney Lorrimar <[email protected]> Co-authored-by: IOHK <[email protected]>
afd0f98
to
4c5c65b
Compare
072f69f
to
7347449
Compare
newtype PrimaryKey key = PrimaryKey key | ||
deriving (Show, Eq, Ord) | ||
newtype PrimaryKey key = PrimaryKey { unPrimaryKey :: key } | ||
deriving (Show, Eq, Ord, Generic, NFData) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
side-note: I am not sure this PrimaryKey
type is any useful nowadays. Not the point of this PR, but we could perhaps leave a TODO / FIXME note about removing this altogether, it just creates noise in many places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes - that's what I thought too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll sort this in the next PR.
@@ -1517,6 +1534,7 @@ newDBLayerWith cacheBehavior tr ti SqliteContext{runQuery} = do | |||
Nothing -> pure errNoSuchTransaction | |||
Just _ -> do | |||
count <- deletePendingOrExpiredTx wid tid | |||
deleteLocalTxSubmission wid [TxId tid] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't be possible maybe to handle that using database constraints and have a cascade delete referencing the TxMeta table? There can't be a local submission without metadata, so if we delete the metadata, it makes sense to delete the tx submission in cascade and we don't have to worry about doing it manually 🤔 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes - good idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done - it makes things simpler.
lib/core/src/Cardano/Wallet.hs
Outdated
-- | ||
-- The current implementation is really basic. Retry every 10 slots. | ||
scheduleLocalTxSubmission :: LocalTxSubmissionStatus tx -> SlotNo | ||
scheduleLocalTxSubmission st = (st ^. #latestSubmission) + 10 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So that's about half the block frequency on mainnet, which would means effectively retrying on every block. Maybe a little excessive for exchanges & al, but I see no harm doing this otherwise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in #2611
lib/core/src/Cardano/Wallet.hs
Outdated
(st ^. #txId) | ||
(st ^. #submittedTx) | ||
sl | ||
watchNodeTip submitPending |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
768b17d
to
f0bdffc
Compare
f0bdffc
to
ad4fed0
Compare
92557f4
to
923d3df
Compare
32f7078
to
0e9623f
Compare
I believe all review comments so far have been addressed. |
bors try |
tryBuild failed: |
bors r+ |
2570: Database changes for resubmission of pending transactions r=rvl a=rvl ### Issue Number ADP-764 ### Overview Database layer support for transaction resubmission. - [x] Adds a new table with serialized transactions, and the latest slot that they have been submitted at. - [x] Add DBLayer method to list pending transactions for resubmissions - [x] Add DBLayer method to add a submitted transaction to the pool of transactions. - [x] Update db model and state machine tests with generators for new methods - [x] Swagger update The wallet layer and server changes are in PR #2611. Co-authored-by: Rodney Lorrimar <[email protected]>
Build failed:
|
bors r+ |
2570: Database changes for resubmission of pending transactions r=Anviking a=rvl ### Issue Number ADP-764 ### Overview Database layer support for transaction resubmission. - [x] Adds a new table with serialized transactions, and the latest slot that they have been submitted at. - [x] Add DBLayer method to list pending transactions for resubmissions - [x] Add DBLayer method to add a submitted transaction to the pool of transactions. - [x] Update db model and state machine tests with generators for new methods - [x] Swagger update The wallet layer and server changes are in PR #2611. Co-authored-by: Rodney Lorrimar <[email protected]>
Build failed:
|
bors r+ |
2570: Database changes for resubmission of pending transactions r=Anviking a=rvl ### Issue Number ADP-764 ### Overview Database layer support for transaction resubmission. - [x] Adds a new table with serialized transactions, and the latest slot that they have been submitted at. - [x] Add DBLayer method to list pending transactions for resubmissions - [x] Add DBLayer method to add a submitted transaction to the pool of transactions. - [x] Update db model and state machine tests with generators for new methods - [x] Swagger update The wallet layer and server changes are in PR #2611. Co-authored-by: Rodney Lorrimar <[email protected]>
Build failed:
|
bors r+ |
2570: Database changes for resubmission of pending transactions r=Anviking a=rvl ### Issue Number ADP-764 ### Overview Database layer support for transaction resubmission. - [x] Adds a new table with serialized transactions, and the latest slot that they have been submitted at. - [x] Add DBLayer method to list pending transactions for resubmissions - [x] Add DBLayer method to add a submitted transaction to the pool of transactions. - [x] Update db model and state machine tests with generators for new methods - [x] Swagger update The wallet layer and server changes are in PR #2611. Co-authored-by: Rodney Lorrimar <[email protected]>
Build failed:
|
bors r+ |
2570: Database changes for resubmission of pending transactions r=Anviking a=rvl ### Issue Number ADP-764 ### Overview Database layer support for transaction resubmission. - [x] Adds a new table with serialized transactions, and the latest slot that they have been submitted at. - [x] Add DBLayer method to list pending transactions for resubmissions - [x] Add DBLayer method to add a submitted transaction to the pool of transactions. - [x] Update db model and state machine tests with generators for new methods - [x] Swagger update The wallet layer and server changes are in PR #2611. Co-authored-by: Rodney Lorrimar <[email protected]>
Build failed:
|
bors r+ |
Build succeeded: |
2611: Retry submission of pending transactions r=rvl a=rvl ### Issue Number ADP-764 ### Overview Adds tx resubmission to the wallet server. - [x] Add a wallet thread for resubmitting transactions - [x] Initial version resubmits every pending tx about every 10 blocks after initial submission. - [x] Unit test retry thread - [x] Design doc ### Comments - Based on PR #2570 branch - merge that first. Co-authored-by: Rodney Lorrimar <[email protected]>
2611: Retry submission of pending transactions r=rvl a=rvl ### Issue Number ADP-764 ### Overview Adds tx resubmission to the wallet server. - [x] Add a wallet thread for resubmitting transactions - [x] Initial version resubmits every pending tx about every 10 blocks after initial submission. - [x] Unit test retry thread - [x] Design doc ### Comments - Based on PR #2570 branch - merge that first. Co-authored-by: Rodney Lorrimar <[email protected]>
Issue Number
ADP-764
Overview
Database layer support for transaction resubmission.
The wallet layer and server changes are in PR #2611.