Page MenuHomePhabricator

[Chronik] Fix: Race condition when re-orging a block with txs
ClosedPublic

Authored by tobias_ruck on Aug 8 2023, 22:01.

Details

Summary

The current built-in node crashes with a Couldn't find coin for input error when a block containing txs gets reorged in some situations.

Here's what happened (with the bug-relevant bits bold):

  1. Connected block A, which contained tx T
  2. Received block B, which also contained T
  3. Avalanche selected block B to be finalized
  4. Node disconnected block A (Chronik didn't receive this yet)
  5. Node added T back to the mempool (due to the re-org)
  6. Node connected block B, removing T and marking its coins as spent
  7. Only now Chronik disconnects block A
  8. Chronik adds T back to the mempool (due to the re-org), however, FindCoins can't find the coins anymore, as they've been spent in step 6.

To fix this, we thread the spent_coins from the node to the validation interface queue, by adding spent_coins to TransactionAddedToMempool. This way Chronik is guaranteed to have the spent coins ready without having to query them from the node.

We use a std::shared_ptr<const std::vector<Coin>>, to ensure coins aren't freed and reused before they're used by the indexer. The interfaces take an owned shared_ptr, to make sure the memory isn't freed too early. In the (relatively far) future, we might move spent_coins into CTransaction, which would manage the memory for us.

List of changes:

  1. Add (Rust) CCoin to the FFI pointing to C++'s Coin.
  2. Change bridge_tx to take a vector of spent coins instead of ChronikBridge (and fix the bug).
  3. Thread the spent coins to handle_tx_added_to_mempool and feed them to bridge_tx.

Depends on D14386.

Test Plan

ninja check-functional

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Fabien requested changes to this revision.Aug 9 2023, 11:49

Can you add a functional test attempting to reproduce the bug ?

src/validation.cpp
821 ↗(On Diff #41752)
822 ↗(On Diff #41752)
826 ↗(On Diff #41752)

you should pass it as a shared pointer

899 ↗(On Diff #41752)

dito

This revision now requires changes to proceed.Aug 9 2023, 11:49
Fabien requested changes to this revision.Aug 17 2023, 07:03

you're missing the other points, clearing my queue

This revision now requires changes to proceed.Aug 17 2023, 07:03

use std::move when making the shared_ptr

Fabien requested changes to this revision.Aug 18 2023, 10:44
Fabien added inline comments.
src/validation.cpp
904 ↗(On Diff #41848)

This can be factored like so std::vector<Coin> spent_coins = getSpentCoins(ptx);, with getSpentCoins() being some static function in the file. It will avoid duplicates, and RVO (Return Value Optimization) will take care of avoiding any copy.

This revision now requires changes to proceed.Aug 18 2023, 10:44

factor coin finding code into getSpentCoins

This revision is now accepted and ready to land.Aug 18 2023, 15:30