Bugfix: Race conditions between read and write into database #819
Conversation
Reused patter of adapter in the `fuel-core`.
# Conflicts: # fuel-block-producer/src/block_producer.rs # fuel-block-producer/src/block_producer/tests.rs # fuel-block-producer/src/mocks.rs # fuel-core/src/executor.rs # fuel-core/src/schema/block.rs # fuel-core/src/schema/tx.rs # fuel-core/src/service/modules.rs # fuel-poa-coordinator/src/service.rs # fuel-poa-coordinator/tests/test_trigger.rs
Implemented with dynamic resolution approach for `DatabaseTransaction`.
Added more comments.
fuel-block-producer/src/ports.rs
Outdated
|
|
||
| pub trait Executor<Database: ?Sized>: Sync + Send { | ||
| /// Executes the block and commits the result of the execution into the inner `Database`. | ||
| fn execute_and_commit( |
There was a problem hiding this comment.
Ideally we wouldn't have definitions like this in our ports that aren't used at all by the current crate. Although we can deal with this during the refactor.
There was a problem hiding this comment.
You are right. Let's keep only the required methods(I removed it)
fuel-core/src/database.rs
Outdated
| @@ -54,6 +52,8 @@ use std::{ | |||
|
|
|||
There was a problem hiding this comment.
these new imports should probably be sorted in with other fuel_* imports
There was a problem hiding this comment.
This is a good incremental step towards a fuel-core-storage crate, while also addressing our immediate issues with the block fetching race condition 👍🏻
Even though the architecture doc laid out a case against having globally shared traits, I think it's justifiable in certain circumstances with traits like Transactional. This is because it's very narrow in scope and doesn't bleed any irrelevant concerns across domains (e.g. changes to the TxPool db trait are still isolated from other domain crates).
|
Would be nice if we had a way to test that this solves the race condition, but a clean solution for doing so eludes me 🤔 |
Close #791.
This change would be nice to do after #812 with
Transaction<Database>structure fromfuel-core-storage. But while it is not merged, it is a variant of implementation without a known type. A workaround is to define aDatabaseTransaction<Database>trait and use theBox<dyn DatabaseTransaction<Database>>type instead.The main idea of the change is to return a
UncommittedResultfromExecutorand commit it after all updates of theDatabaseat the end of the block production loop ofPoACoordinator.