Page MenuHomePhabricator

[Chronik] Fix `catch_merge_errors` by keeping intermediate results
ClosedPublic

Authored by tobias_ruck on Sep 19 2023, 16:53.

Details

Summary

The current catch_merge_errors function is insufficient, for the following reason:

  1. When using merge_cf, it seems like RocksDB applies them on the fly for potentially many values, even if they've been read (and thus applied) before already.
  2. This means that a previous successful call to merge_cf would be reverted by the old catch_merge_errors, as any failed merge op would cause all previous successful applications to be discarded, causing a sort-of spooky removal action at a distance.

The solution is to keep previous successful applications of merge operands and to return the value from just before the error occured as the result of the merge function.

Test Plan

ninja check-crates && ninja check-functional

Diff Detail

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

Event Timeline

Fabien added inline comments.
chronik/chronik-db/src/io/merge.rs
208 ↗(On Diff #42277)

Nit: for the sake of avoiding override issues, use 1-2-4-8 so the sum is always different from the value.
Here your test can pass in either of these situations:

  • The error happened, the merge occurred up to 2 and 1 + 2 = 3
  • The error happened, the merge got turned into a no-op somehow and the 3 operand applied
This revision is now accepted and ready to land.Sep 20 2023, 09:03

Use 1-2-4-8 chain as suggested