search

AntelopeIO / leap

C++ implementation of the Antelope protocol
Other
116 stars 70 forks source link

IF: fork database transition not thread safe #2083

Closed heifner closed 8 months ago

heifner commented 8 months ago

Currently fork_database is thread safe via internal mutex. During the transition of dpos fork database to instant finality fork database threads from API calls and net_plugin can be running. Need to provide a wrapper around the two fork databases that is thread safe and can provide the interfaces needed by API calls, net_plugin, and controller.

Also handle [greg todo] leftover from https://github.com/AntelopeIO/leap/issues/1941 & #2045 in leap-util blocklog. The fork_database wrapper can examine file to determine which internal implementation is in use.

greg7mdp commented 8 months ago

Need to provide a wrapper around the two fork databases that is thread safe and can provide the interfaces needed by API calls, net_plugin, and controller.

Is it really necessary? I would not have thought so because:

  1. only the main thread modifies fork_database, right?
  2. only one of the two fork_database is valid at any one time
  3. the main thread can convert fork_db<legacy> -> fork_db<new> while net_plugin and APIs still access fork_db<legacy>, and then we have to make an atomic switch where maybe we freeze net_plugin (maybe the same way as when we take a snapshot)?
heifner commented 8 months ago
  1. Correct.
  2. Correct.
  3. Correct. The way we freeze net_plugin currently during snapshot would not prevent a call into fork database. It might be possible via freezing net_plugin and API calls, but would have to verify all currently running calls are complete. Not really sure how you do that without a mutex or atomic counter somewhere. There is already a mutex in fork database that provides the currency projection. I think we can just move that mutex up to a wrapper that also does the switch from one to the other.
greg7mdp commented 8 months ago

Oh wait, because there is a mutex in fork_db I think we might be fine without the wrapper. What about:

  1. store a shared_ptr to fork_db in controller (instead of the struct itself).
  2. when time comes for the conversion, lock the mutex on fork_db<legacy>
  3. once we have the lock, do the conversion fork_db<legacy> -> fork_db<new>, but keep a shared_ptr to fork_db<legacy> so it stays alive. update controller to point to fork_db<new>.
  4. release the mutex of fork_db<legacy>. net_plugin and APIs would finish their work using the old fork_db, but that should be fine.
heifner commented 8 months ago

That doesn't work without an atomic shared ptr. Boost has an atomic shared ptr we could use for this.

Without atomic shared ptr: Net thread begins access of control->shared_ptr->fork_db Main thread locks fork_db<legacy> , does not stop net thread from accessing shared_ptr. Main thread modifies shared_ptr while net thread is accessing it (not safe).

heifner commented 8 months ago

Resolved by #2113