🚧[under construction]🚧 Pebble Database Backend

[AlexHentschel]

This is a collection of TODOs that I thought about when reviewing the PRs for migrating the protocol storage within nodes from badger to pebble (PRs #6197 and #6207)

High Priority

• the Finalizer for Collection Nodes needs clear documentation whether it tolerates repeated inputs (finalizing already finalized blocks) and whether it is concurrency safe.

  • the previous badger-backed implementation used a transaction. Here is was reasonably apparent that the implementation tolerated repeated inputs (👉 code): it was one entirely atomic operation that worked on a database state that either had seen all finalization updates or none. Furthermore, the finalizer was intrinsically concurrency safe, utilizing concurrency safety from badger.
  • In the pebble-backed implementation this has changed: we can read state that does not yet have the latest finalization applied, we collect all necessary updates - including the write for updating the latest finalized height. However in the case of concurrent calls to MakeFinalize, by the time the writes are committed, the latest finalized block might have already have changed - possibly having a newer value than the value we triaged to write. If we now write the pending updates, we essentially decrease the value of "latest finalized block" - which would definitely violate the protocol's safety guarantees.
  • My gut feeling is that the actual implementation is still fine, because we only call the finalizer from one specific component. However, this is nowhere documented and safety precautions to prevent accidentally breaking this undocumented but correctness-relevant assumption.

  Goal: improve code maintainability by documenting and enforcing correct usage!

  low-hanging fruit for optimization:

  • previously we only used direct badger reads and writes so we would atomically operate on one specific database state. Now that the reads anyway need to be front-loaded, we should use the storage abstractions including a cache. This increases performance without notably driving up implementation complexity.

• The pebble-backed Finalizer for the consensus nodes has the same problem: safety under concurrent calls is insufficiently documented (problem already existed before the pebble migration).

Medium Priority

Low Priority

• in file engine/common/follower/integration_test.go we should replace EECC with EFM in all the comments, because the term EECC is outdated.

[github-actions]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.