fix(static-file): pass producer as `Arc<Mutex<_>>` to ensure only one is active #7143

shekhirin · 2024-03-14T17:21:07Z

This PR fixes a critical bug with multiple StaticFileProducer instances writing to static files at the same time, potentially causing duplicate and inconsistent data.

We need to have at most one StaticFileProducer active at a time. Otherwise, such a situation is possible:

StaticFileHook is started in the engine loop via polling the next hook

reth/crates/consensus/beacon/src/engine/mod.rs

Lines 1860 to 1870 in 3d86c4a

    
           if let Poll::Ready(result) = this.hooks.poll_next_hook( 
        
               cx, 
        
               EngineContext { 
        
                   tip_block_number: this.blockchain.canonical_tip().number, 
        
                   finalized_block_number: this 
        
                       .blockchain 
        
                       .finalized_block_number() 
        
                       .map_err(RethError::Provider)?, 
        
               }, 
        
               this.sync.is_pipeline_active(), 
        
           )? {

On the next engine loop iteration, sync controlled is polled

reth/crates/consensus/beacon/src/engine/mod.rs

Lines 1839 to 1840 in 3d86c4a

// process sync events if any

match this.sync.poll(cx) {
Sync controller spawns the pipeline, and we call the StaticFileProducer before actually starting the execution of all stages

reth/crates/stages/src/pipeline/mod.rs

Lines 185 to 186 in 3d86c4a

pub async fn run_loop(&mut self) -> Result<ControlFlow, PipelineError> {

self.produce_static_files()?;
Both StaticFileHook and Pipeline::produce_static_files run the StaticFileProducer, and it writes to static files from both places.

Having only one StaticFileProducer can be ensured via sharing one copy between everyone using Arc<Mutex<StaticFileProducer>>, so that the user will need to lock the static file producer first for the duration of its usage. This PR introduces a newtype wrapper StaticFileProducer(Arc<Mutex<StaticFileProducerInner>>) that makes it easier to initialize and pass it around.

An additional measure that can be done to prevent the static files corruption is implemented #7137.

mattsse

it's not super pretty, but this should do it

mattsse · 2024-03-14T17:29:53Z

crates/static-file/src/static_file_producer.rs

@@ -21,7 +21,7 @@ pub type StaticFileProducerResult = RethResult<StaticFileTargets>;
 pub type StaticFileProducerWithResult<DB> = (StaticFileProducer<DB>, StaticFileProducerResult);

 /// Static File producer routine. See [StaticFileProducer::run] for more detailed description.
-#[derive(Debug, Clone)]
+#[derive(Debug)]


shekhirin · 2024-03-14T17:33:23Z

it's not super pretty, but this should do it

can do it prettier via a type alias that's public and a private internal type

mattsse · 2024-03-14T17:43:45Z

can do it prettier via a type alias

no aliases please :)

shekhirin · 2024-03-14T17:46:14Z

can do it prettier via a type alias

no aliases please :)

newtype

crates/consensus/beacon/src/engine/sync.rs

crates/static-file/Cargo.toml

rkrasiuk · 2024-03-15T12:38:05Z

crates/revm/src/stack.rs

@@ -131,7 +131,7 @@ where
    ) -> Option<CallOutcome> {
        call_inspectors!([&mut self.custom_print_tracer], |inspector| {
            if let Some(outcome) = inspector.call(context, inputs) {
-                return Some(outcome);


how do these keep making their way back into the codebase

… is active (paradigmxyz#7143) Co-authored-by: joshieDo <ranriver@protonmail.com>

shekhirin added 3 commits March 14, 2024 17:20

feat(static-file): always pass as Arc<Mutex<_>>

c16604e

fix fmt

154eb25

fix lint

9b1f91a

mattsse reviewed Mar 14, 2024

View reviewed changes

fix lint

bd8758c

shekhirin added 3 commits March 14, 2024 18:16

test fixes

e9da721

fix doc test

d8841a8

newtype wrapper

e7f0930

shekhirin force-pushed the alexey/static-files-arc-mutex branch from 6e8c0e7 to e7f0930 Compare March 14, 2024 19:09

shekhirin added 2 commits March 14, 2024 19:14

fix doc comments

aa3d1a5

remove parking_lot where not needed, remove manual drop

df69df9

shekhirin marked this pull request as ready for review March 14, 2024 19:29

shekhirin requested review from joshieDo, rkrasiuk, Rjected, onbjerg and gakonst as code owners March 14, 2024 19:29

reorder locking to prevent ro tx leak

f7f3f30

shekhirin force-pushed the alexey/static-files-arc-mutex branch from c79d9ec to f7f3f30 Compare March 14, 2024 19:37

shekhirin requested a review from mattsse March 14, 2024 20:11

joshieDo reviewed Mar 14, 2024

View reviewed changes

crates/consensus/beacon/src/engine/sync.rs Outdated Show resolved Hide resolved

no snapshots

031e65b

shekhirin added C-enhancement New feature or request A-consensus Related to the consensus engine A-static-files Related to static files labels Mar 14, 2024

shekhirin changed the title ~~feat(static-file): always pass producer as Arc<Mutex<_>>~~ fix(static-file): always pass producer as Arc<Mutex<_>> to ensure only one active at a time Mar 14, 2024

shekhirin changed the title ~~fix(static-file): always pass producer as Arc<Mutex<_>> to ensure only one active at a time~~ fix(static-file): pass producer as Arc<Mutex<_>> to ensure only one active Mar 14, 2024

shekhirin changed the title ~~fix(static-file): pass producer as Arc<Mutex<_>> to ensure only one active~~ fix(static-file): pass producer as Arc<Mutex<_>> to ensure only one is active Mar 14, 2024

joshieDo reviewed Mar 15, 2024

View reviewed changes

crates/static-file/Cargo.toml Show resolved Hide resolved

joshieDo approved these changes Mar 15, 2024

View reviewed changes

add test for race condition

a6bc0e2

rkrasiuk reviewed Mar 15, 2024

View reviewed changes

mattsse approved these changes Mar 15, 2024

View reviewed changes

shekhirin added this pull request to the merge queue Mar 15, 2024

Merged via the queue into main with commit 52d4983 Mar 15, 2024
28 checks passed

shekhirin deleted the alexey/static-files-arc-mutex branch March 15, 2024 13:36

Ruteri pushed a commit to Ruteri/reth that referenced this pull request Apr 17, 2024

fix(static-file): pass producer as Arc<Mutex<_>> to ensure only one…

03f00df

… is active (paradigmxyz#7143) Co-authored-by: joshieDo <ranriver@protonmail.com>

Ruteri pushed a commit to Ruteri/reth that referenced this pull request Apr 17, 2024

fix(static-file): pass producer as Arc<Mutex<_>> to ensure only one…

15c9a1a

… is active (paradigmxyz#7143) Co-authored-by: joshieDo <ranriver@protonmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(static-file): pass producer as `Arc<Mutex<_>>` to ensure only one is active #7143

fix(static-file): pass producer as `Arc<Mutex<_>>` to ensure only one is active #7143

shekhirin commented Mar 14, 2024 •

edited

mattsse left a comment

mattsse Mar 14, 2024

shekhirin commented Mar 14, 2024

mattsse commented Mar 14, 2024

shekhirin commented Mar 14, 2024

rkrasiuk Mar 15, 2024 •

edited

	if let Poll::Ready(result) = this.hooks.poll_next_hook(
	cx,
	EngineContext {
	tip_block_number: this.blockchain.canonical_tip().number,
	finalized_block_number: this
	.blockchain
	.finalized_block_number()
	.map_err(RethError::Provider)?,
	},
	this.sync.is_pipeline_active(),
	)? {

	pub async fn run_loop(&mut self) -> Result<ControlFlow, PipelineError> {
	self.produce_static_files()?;

	// process sync events if any
	match this.sync.poll(cx) {

fix(static-file): pass producer as Arc<Mutex<_>> to ensure only one is active #7143

fix(static-file): pass producer as Arc<Mutex<_>> to ensure only one is active #7143

Conversation

shekhirin commented Mar 14, 2024 • edited

mattsse left a comment

Choose a reason for hiding this comment

mattsse Mar 14, 2024

Choose a reason for hiding this comment

shekhirin commented Mar 14, 2024

mattsse commented Mar 14, 2024

shekhirin commented Mar 14, 2024

rkrasiuk Mar 15, 2024 • edited

Choose a reason for hiding this comment

fix(static-file): pass producer as `Arc<Mutex<_>>` to ensure only one is active #7143

fix(static-file): pass producer as `Arc<Mutex<_>>` to ensure only one is active #7143

shekhirin commented Mar 14, 2024 •

edited

rkrasiuk Mar 15, 2024 •

edited