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

Detect reverted transactions #355

Merged
merged 14 commits into from
Mar 10, 2020
Merged

Conversation

jaspervdm
Copy link
Contributor

@jaspervdm jaspervdm commented Mar 5, 2020

Whenever the wallet updates the status of unspent outputs (e.g. during a scan) and an output has disappeared, there are 2 common scenarios:

  • The funds have been spent in another wallet from the same seed
  • The transaction is reverted by a reorg

This PR introduces functionality to detect the second scenario for received transactions. This is done by checking the on-chain presence of the corresponding kernel for each disappeared, previously-confirmed transaction. If it does not exist, it was removed due to a reorg. In that case the output(s) are changed to OutputStatus::Reverted and the transaction log entry type to TxLogEntryType::TxReverted, with confirmed set to false.
When the user runs the info command (through cli or api), any reverted funds will show up in a separate field.

Any reverted output is treated in the same way as an unconfirmed output, meaning its on-chain status is checked regularly. If it reappears, the transaction and its outputs are marked as confirmed again.

Before we merge this PR, there are a few things left to consider:

  • Short reorgs are common and transactions could get temporarily reverted through them. Should we add some kind of "revert_length" field on reverted transactions (in blocks or seconds)?
  • Should reverted transaction be cancellable, so they don't show up as reverted anymore?
  • Should we add an API call to query the list of reverted transactions?--> Will be done in a separate PR

@yeastplume
Copy link
Member

All looks good to me so far. With respect to open questions::

Short reorgs are common and transactions could get temporarily reverted through them. Should we add some kind of "revert_length" field on reverted transactions (in blocks or seconds)?

That sounds like a good idea, would give you information to decide that a transaction definitely isn't going to reappear in its current form.

Should reverted transaction be cancellable, so they don't show up as reverted anymore?

Yes, I think so. They're in the same state as if the transaction had never been finalized, so users should be able to clear it up with the other party and remove the tx from their logs.

Should we add an API call to query the list of reverted transactions?

Rather than a separate function, this sounds like it would be better as an additional filter to the existing retrieve_txs function.

@jaspervdm
Copy link
Contributor Author

First 2 items are handled. The third one would be best done by adding a field to filter specific tx types in retrieve_txs, which I will add in a separate PR.

@yeastplume could you have another look, now that the PR is finished?

Copy link
Member

@yeastplume yeastplume left a comment

Choose a reason for hiding this comment

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

👍 Going to restart the windows tests and see what happens this time

@yeastplume
Copy link
Member

Okay, going to go as far as to boot into windows to see what's going on with these test failures.

@jaspervdm jaspervdm merged commit 7a95f42 into mimblewimble:master Mar 10, 2020
antiochp pushed a commit to antiochp/grin-wallet that referenced this pull request Aug 7, 2020
* Detect reverted transactions

* Add reverted amount to funds display

* Support cancelling reverted txs

* Add reverted_after field for reverted transactions

* Update grin dependency to master branch

* Panic on failed test cleanup

* Only delete test dir if it exists

* Stop wallet proxy in accounts test

* Stop proxy thread in all controller tests

* Typo

* Add sleep after revert test

* Longer sleep in revert tests
bayk added a commit to mwcproject/mwc-wallet that referenced this pull request Jul 21, 2024
…ble#355)

* Detect reverted transactions
* Add reverted amount to funds display
* Support cancelling reverted txs
* Add reverted_after field for reverted transactions
* Update grin dependency to master branch
* Panic on failed test cleanup
* Only delete test dir if it exists
* Stop wallet proxy in accounts test
* Stop proxy thread in all controller tests
* Typo
* Add sleep after revert test
* Longer sleep in revert tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants