fix(engine): cleanup leaked context

[fgimenez]

After the introduction on std::thread::scope for creating StateRootTask, we started using Box::leak to keep alive values required to return and move around the state hook closure. This caused the variables in the kept context to cause errors like:

WARN Long-lived read transaction has been timed out

This PR includes changes to the parameters needed to create the blinded provider factory required by StateRootTask using Arc instead of references, which don't have a big performance impact and are automatically cleaned up without lifetime issues.

A provider transaction reference was one of the parameters required to create the blinded provider factory. Instead of it, we are now passing an Arc<Provider>, which can obtain the transaction reference as needed and doesn't impose additional lifetime constraints. This change requires further changes in all the code that use the cursors, limited to the trie crates.

[shekhirin]

This works

[codspeed-hq]

CodSpeed Performance Report

Merging #13749 will degrade performances by 14.87%

_{Comparing fgimenez/cleanup-leaked-context (cd63408) with main (20a3a6a)}

Summary

❌ 3 regressions
✅ 74 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

	Benchmark	main	fgimenez/cleanup-leaked-context	Change
❌	hash builder[init size 10000 | update size 100 | num updates 1]	8.9 ms	10.2 ms	-12.23%
❌	hash builder[init size 10000 | update size 100 | num updates 3]	26.2 ms	30.1 ms	-12.84%
❌	hash builder[init size 10000 | update size 100 | num updates 5]	42.8 ms	50.3 ms	-14.87%

[mattsse]

these changes makes sense overall, but I assume the lifetime issue is more tricky because we use a borrowed tx for the HashedPostStateCursor etc.

which wouldn't be 'static that the roottask requires.

although we have ownership of the consistentview, so this problem seems to arise from us using tx_ref, if I understood this correctly we could perhaps consume the provider_ro and its tx (into_tx instead of tx_ref)?

[mattsse]

I think this is the problematic section because this borrows and is not 'static

2276 | let provider_ro = consistent_view.provider_ro()?;
| ----------- binding provider_ro declared here
...
2286 | DatabaseHashedCursorFactory::new(provider_ro.tx_ref()),
| ^^^^^^^^^^^---------
| |
| borrowed value does not live long enough
| argument requires that provider_ro is borrowed for

[fgimenez]

yes exactly, we previously had the same issue with nodes_sorted and state_sorted, was fixed by turning reference parameters into Arcs here a5103d5, currently in progress passing ownership of the provider factory as you mentioned

[mattsse]

which would clash with these here I assume

[mattsse]

hmm, what's the main reason why we need Arc now in a bunch of places. this appears a bit invasive now

is this because of

reth/crates/trie/db/src/trie_cursor.rs

Line 19 in 8efe441

pub struct DatabaseTrieCursorFactory<'a, TX>(&'a TX);

reth/crates/trie/db/src/hashed_cursor.rs

Line 12 in 8efe441

pub struct DatabaseHashedCursorFactory<'a, TX>(&'a TX);

?
but we need an owned instance?

because we previously could use both factories with a single reference and we're forced to own so we can clone+spawn, this then does not work for this pr.

what if we instead of having two factory types use only one and implement both traits for that,
another idea would be to not require &Tx and Tx: DbTx and instead use a new helper trait that can handle both &Tx and owned provider?

[mattsse]

hmm, why does this Ref type now requre an owned instance?

[fgimenez]

good point, LatestStateProviderRef and LatestStateProvider can be merged with the current approach, will address this depending on how we decide to go forward

[fgimenez]

because we previously could use both factories with a single reference and we're forced to own so we can clone+spawn, this then does not work for this pr.

yes, that's exactly the case. moreover, several places use the from_tx method for creating both factories and other types that depend on them, for instance reth_trie_db::Proof

reth/crates/trie/db/src/proof.rs

Lines 37 to 40 in 83b5619

	/// Create a new [Proof] instance from database transaction.
	fn from_tx(tx: &'a TX) -> Self {
	Self::new(DatabaseTrieCursorFactory::new(tx), DatabaseHashedCursorFactory::new(tx))
	}

given that we are forced to own, having the ability to create the factories and related types from an owned instance is pervasive and affects a lot of places.

what if we instead of having two factory types use only one and implement both traits for that,
another idea would be to not require &Tx and Tx: DbTx and instead use a new helper trait that can handle both &Tx and owned provider?

thx a lot for the suggestions! will give them a try, i think the problem with the first one is that in any case we need an owned instance, and the changes would cascade as we have now, will let you know how it goes

[fgimenez]

another idea would be to not require &Tx and Tx: DbTx and instead use a new helper trait that can handle both &Tx and owned provider?

Finally went for this second option, i already had in place a DatabaseRef trait that was being implemented for the provider, i've added a wrapper type for the transaction that also implements it. Now the cursor factories keep the old new constructor that accepts a &Tx (and keeps being used the same as it was) and have a new from_provider used in the engine. PTAL, still a bunch of changes but limited to the trie crates

[fgimenez]

i've added a wrapper type for the transaction that also implements it.

a bit simpler now without the wrapper, implementing the helper trait directly on &Tx