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

Recover possible transaction in conflicted cache when RBF #4561

Merged

Conversation

chenyukang
Copy link
Collaborator

@chenyukang chenyukang commented Aug 1, 2024

What problem does this PR solve?

When we replace some transactions in RBF, some old transactions may be okay to be recovered, since there are no conflicts after removing transactions.

If we don't recover these transactions, it may introduce a scenario of a possible cycling attack in lightning network.

In the Lightning Network, the flow of funds across multiple hops is achieved through HTLCs. If an intermediate node discovers that the subsequent node has not acted as agreed, it can issue a timeout transaction, and after a certain waiting period, reclaim its own funds.

Assume A -> B -> C, where the attacker controls both A and C, and attempts to prevent B's timeout transaction from being confirmed on-chain while stealing B's money through a preimage transaction.

The attack method can be described as follows:

B1 is a transaction that is prepared to spend B0. B0 has already been included in a block, and B1 is in the transaction pool. Here, B1 is a timeout transaction.

B0 ---> B1

A1 and A2 are two chained transactions that are prepared to spend A0. A0 has already been included in a block, and A1 and A2 are in the transaction pool.

A0 ---> A1 ---> A2

Construct a B2 transaction, which spends B0 and A1.

A0 ---> A1 ---> A2
         |
    ----------> B2
   |
B0 ---> B1

As long as B2’s feerate is higher than B1's and A2's, it will evict B1 and A2 from the transaction:

A0 ---> A1
         |
B0 -----------> B2

Construct an A3 transaction, which spends A0 and has a feerate higher than A1's and B2's. It will evict A1 and B2 from the transaction pool:

A0 ---> A3
B0

In this way, both B1 and B2 will disappear from the transaction pool. By carefully timing the submission of the transactions, B2 will also not be broadcast to the network.

More reference:
https://blog.satsbridge.com/lightning-replacement-cycling-attack-explained-45636e41bc6f
https://twitter.com/mononautical/status/1715736832950825224

What is changed and how it works?

What's Changed:

Add conflicts_outputs_cache to track the relationship of input -> tx in RBF, then add possible transactions into the verify pool, so that they may be recovered.

This PR is based on the new-verify branch, one reason is we can not recursively call submit_entry in async. Another reason is new-verify's worker-consumer model is more suitable for retrying submit transactions.

Related changes

  • PR to update owner/repo:
  • Need to cherry-pick to the release branch

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No code ci-runs-only: [ quick_checks,linters ]

Side effects

  • Performance regression
  • Breaking backward compatibility

Release note

Title Only: Include only the PR title in the release note.

@chenyukang chenyukang requested a review from a team as a code owner August 1, 2024 00:53
@chenyukang chenyukang requested review from zhangsoledad and removed request for a team August 1, 2024 00:53
@chenyukang chenyukang force-pushed the yukang-fix-cycling-attack-rbf branch from ed82636 to 75d237d Compare August 1, 2024 02:51
@chenyukang chenyukang changed the title [WIP] Recover possible transaction in conflicted cache when RBF Recover possible transaction in conflicted cache when RBF Aug 1, 2024
@chenyukang chenyukang force-pushed the yukang-fix-cycling-attack-rbf branch from 75d237d to db66275 Compare August 12, 2024 13:08
@chenyukang chenyukang changed the base branch from yukang-new-verify to develop August 12, 2024 13:08
@chenyukang
Copy link
Collaborator Author

it is ready to be merged, since #4291 is merged.

Comment on lines 64 to +65
conflicts_cache: LruCache::new(CONFLICTES_CACHE_SIZE),
conflicts_outputs_cache: lru::LruCache::new(CONFLICTES_INPUTS_CACHE_SIZE),
Copy link
Member

Choose a reason for hiding this comment

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

we may add two config options for CONFLICTES_CACHE_SIZE and CONFLICTES_INPUTS_CACHE_SIZE

@quake quake added this pull request to the merge queue Sep 2, 2024
Merged via the queue into nervosnetwork:develop with commit a73b7f4 Sep 2, 2024
33 checks passed
@chenyukang
Copy link
Collaborator Author

Some tradeoff for current solution:

  1. The current solution is when performing RBF (Replace-by-Fee) and replacing other transactions, we collect the inputs from the evicted transactions by the way(exclude the inputs used by the newly submitted transactions), and then check whether the existing conflicted pool contains any transactions spending those remaining inputs without other conflicts, and then add them back. Implementation is trivial here; the key is to avoid introducing performance regression.

  2. Bitcoin's RBF has two modes discussed: one is Full-Replace-By-Fee, and the other is First-Seen-Safe, the latter mode can prevent this attack.

    1. First-Seen-Safe requires that the outputs of the replaced transaction must be a subset of the new transaction. This means that for a replacement to succeed, the new transaction cannot conflict with multiple transactions. Therefore, the step as long as B2’s feerate is higher than B1's and A2's, it will evict B1 and A2 from the transaction pool cannot be achieved, so the attack will not succeed.
    2. First-Seen-Safe is a more restrictive form of RBF.
    3. First-Seen-Safe RBF was previously discussed in Bitcoin, but it doesn’t seem to have been merged in the end, and the main reason was that no consensus was reached. Some maintainers preferred Full-RBF for simplicity:
      1. PR #6176 on GitHub
      2. Discussion on Bitcoin Development
      3. bitcoin wiki

@doitian doitian mentioned this pull request Oct 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants