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

feat(wallet): Detect Reorgs And RBFs #1081

Merged
merged 10 commits into from May 30, 2023
Merged

feat(wallet): Detect Reorgs And RBFs #1081

merged 10 commits into from May 30, 2023

Conversation

coreyphillips
Copy link
Collaborator

@coreyphillips coreyphillips commented May 26, 2023

Description

  • Bitkit now tracks transactions with less than 6-confirmations and notifies users of a relevant reorg or RBF.
  • Added unconfirmedTransactions to wallet reducer.
  • Created several new methods for tracking unconfirmed txs.
  • Added a button to inject a fake transaction in the Dev menu.
  • Added exists to IActivityItem types.
  • Updated mergeActivityItems method.
  • Added methods to clear store objects when rescanning.

With this update we're concerned with two main things that can happen to transactions with less than 6 confirmations.

  1. A transaction can get pushed back to the mempool due to a reorg.
  2. A zero-conf transaction can get removed from the mempool if it has insufficient fees for the given fee environment, a reorg occurs that knocks it from the mempool or it's RBF'd.

In the event number one occurs, Bitkit will detect that a previously confirmed transaction is now no longer confirmed and notifies the user accordingly. This is a best-case scenario where a user may only need to wait for the tx to confirm again. The only action that needs to be taken is ensuring the activity item is no longer marked as confirmed and the stored transaction height is adjusted.

In the event number two occurs, Bitkit will detect that a tracked transaction is no longer in the mempool and alert the user accordingly of a potential reorg or RBF. This situation is more severe since the transaction in question is now gone, reverting ownership of any received or sent funds. In this situation, Bitkit will set the exists state to false in the activity list item and highlight the tx entry in red in the activity list. Bitkit will also perform a rescan of the wallet to ensure funds and stored transactions are accurate.

Type of change

  • New feature (non-breaking change which adds functionality)

Tests

  • No test

Screenshot / Video

ghost-tx
reorg
removed-from-mempool

QA Notes

To test a reorg:

  1. Send funds to the wallet: /regtest-deposit <address> 0.00005000
  2. Mine a block: /regtest-mine 1
  3. Invalidate the block you just mined: /regtest-invalidate-block <blockhash>
    Expected: A blue notification should appear that matches the attached screenshot after detecting the reorg. The transaction should no longer appear as confirmed in the activity list.

To test an RBF or getting bumped from the mempool:

  1. Navigate to "Settings->Dev Settings->Inject Fake Transaction"
    Expected: A red notification should appear that matches the attached screenshot after detecting the ghost/invalid tx. The tx in question should be red in the activity list. When tapping the activity list item the status should display "Removed from Mempool".

Notes

  • I added translations for the new notifications using Google translate. Please let me know if they need further adjustments.

Bitkit now tracks < 6-conf txs and notifies users of a relevant reorg or RBF.
Added "unconfirmedTransactions" to wallet reducer.
Created several new methods for tracking unconfirmed txs.
Added a button to inject a fake transaction in the Dev menu.
Added "exists" to IActivityItem types.
Updated mergeActivityItems method.
Added methods to clear store objects when rescanning.
Copy link
Collaborator

@limpbrains limpbrains left a comment

Choose a reason for hiding this comment

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

LN transactions are now shown in red too

image

src/utils/i18n/locales/ru/wallet.json Show resolved Hide resolved
@limpbrains
Copy link
Collaborator

Also I've pushed fix for e2e tests

src/store/migrations/index.ts Outdated Show resolved Hide resolved
src/store/types/activity.ts Outdated Show resolved Hide resolved
@coreyphillips
Copy link
Collaborator Author

Tested the unconfirmedTransactions migration from old state to new without issue on my end. Please let me know if you encounter any issues during your testing.

@@ -325,6 +326,15 @@ const OnchainActivityDetail = ({
);
}

if (activityType === EActivityType.onchain && !exists) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

this will always be true since we're in OnchainActivityDetail

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is to appease typescript. Otherwise, exists would have to be added to each ActivityItem type.

Copy link
Collaborator

Choose a reason for hiding this comment

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

No this is not true. TS will know that activityType will always be EActivityType.onchain here and that exists will be there.

src/store/actions/wallet.ts Outdated Show resolved Hide resolved
src/store/actions/wallet.ts Outdated Show resolved Hide resolved
@coreyphillips coreyphillips requested a review from pwltr May 30, 2023 13:11
@coreyphillips coreyphillips merged commit 3798240 into master May 30, 2023
4 checks passed
@coreyphillips coreyphillips deleted the detect-reorgs branch May 30, 2023 13:40
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

3 participants