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

IF: fork database transition not thread safe #2083

Closed
Tracked by #2110
heifner opened this issue Jan 12, 2024 · 5 comments · Fixed by #2113
Closed
Tracked by #2110

IF: fork database transition not thread safe #2083

heifner opened this issue Jan 12, 2024 · 5 comments · Fixed by #2113
Assignees
Labels
👍 lgtm OCI Work exclusive to OCI team

Comments

@heifner
Copy link
Member

heifner commented Jan 12, 2024

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 #1941 & #2045 in leap-util blocklog. The fork_database wrapper can examine file to determine which internal implementation is in use.

@greg7mdp
Copy link
Contributor

greg7mdp commented Jan 12, 2024

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
Copy link
Member Author

heifner commented Jan 13, 2024

  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
Copy link
Contributor

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
Copy link
Member Author

heifner commented Jan 13, 2024

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).

@bhazzard bhazzard added 👍 lgtm and removed triage labels Jan 16, 2024
@heifner heifner self-assigned this Jan 19, 2024
@heifner heifner added the OCI Work exclusive to OCI team label Jan 19, 2024
@heifner heifner added this to the Leap v6.0.0-rc1 milestone Jan 19, 2024
heifner added a commit that referenced this issue Jan 19, 2024
…ant-finality.

GH-2083 Moved mutex out of fork database impl so it can protect transition from legacy to instant-finality.
@heifner
Copy link
Member Author

heifner commented Jan 22, 2024

Resolved by #2113

@heifner heifner closed this as completed Jan 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
👍 lgtm OCI Work exclusive to OCI team
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants