wip: lru changes

[0xJepsen]

Closes #7471 , closes https://github.com/paradigmxyz/reth/issues/6836

I made the cache module in the reth-network crate public so that i could use the LruMap in the reth-blockchain-tree crate.

[emhane]

nice @0xJepsen ! I was thinking that we would not remove the LruCache, but instead change it internally to an LruMap<T, ()>. wdyt?

edit: it was using lru::LruCache I see on second look, what do you think about moving your changes into https://github.com/paradigmxyz/reth/blob/2e87b2a8d57813ce61f8898cf89d7b0dda2ab27d/crates/net/network/src/cache.rs#L8-L16, and importing the type reth_network::cache::LruCache? possibly out of scope

[0xJepsen]

what do you think about moving your changes into

To clarify, are you suggesting rather than using the LruMap i use the local LruCache rather than the external one? I like that approach i am down to give it a try. One thing that will be nice is that it should have the insert_and_get_evicted method that makes the changes to insert_block less of a barrow checker battle as far as i can tell.

[emhane]

I was thinking if we could swat two flies at once by moving the LruMap<T, ()> into LruCache https://github.com/paradigmxyz/reth/issues/6836, wdyt?

[0xJepsen]

LruMap<T, ()>

Okay i can gave this a go. This made the changes in the block_buffer.rs pretty painless but there are lots of trait bounds im working through now. One that i wanted a second opinion to is that the current hasher RandomState doesn't implement Clone.

My question here is: Should I attempt to implement Clone for the RandomState? or should we specify a different hasher? Or is possible we do not need the LruCache to be Clone?

pub struct LruMap<K, V, L = ByLength, S = RandomState>(schnellru::LruMap<K, V, L, S>)
where
    K: Hash + PartialEq + Clone,
    L: Limiter<K, V>,
    S: BuildHasher;

impl<K, V, L, S> fmt::Debug for LruMap<K, V, L, S>
where
    K: Hash + PartialEq + fmt::Display + Clone,
    V: fmt::Debug,
    L: Limiter<K, V> + fmt::Debug,
    S: BuildHasher + Clone,
{
    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
        let mut debug_struct = f.debug_struct("LruMap");

        debug_struct.field("limiter", self.limiter());

        debug_struct.field(
            "res_fn_iter",
            &format_args!(
                "Iter: {{{} }}",
                self.iter().map(|(k, v)| format!(" {k}: {v:?}")).format(",")
            ),
        );

        debug_struct.finish()
    }
}

[emhane]

My question here is: Should I attempt to implement Clone for the RandomState? or should we specify a different hasher? Or is possible we do not need the LruCache to be Clone?

nice! we can skip the LruCache being clone. question is if that breaks a lot of the code throughout the workspace that may be cloning LruCache, and if that's then enough to be out of scope.

[0xJepsen]

My question here is: Should I attempt to implement Clone for the RandomState? or should we specify a different hasher? Or is possible we do not need the LruCache to be Clone?

nice! we can skip the LruCache being clone. question is if that breaks a lot of the code throughout the workspace that may be cloning LruCache, and if that's then enough to be out of scope.

So where it breaks is in the crates/net/network/src/transactions/fetcher.rs file looks like in two places:

https://github.com/paradigmxyz/reth/blob/00420477c59214e0d078592c19d77f5b7fa98d7e/crates/net/network/src/transactions/fetcher.rs#L667-L678

and

https://github.com/paradigmxyz/reth/blob/00420477c59214e0d078592c19d77f5b7fa98d7e/crates/net/network/src/transactions/fetcher.rs#L908-L915

Both are debug statements, so maybe i can get around this.

[0xJepsen]

My question here is: Should I attempt to implement Clone for the RandomState? or should we specify a different hasher? Or is possible we do not need the LruCache to be Clone?

nice! we can skip the LruCache being clone. question is if that breaks a lot of the code throughout the workspace that may be cloning LruCache, and if that's then enough to be out of scope.

So where it breaks is in the crates/net/network/src/transactions/fetcher.rs file looks like in two places:

https://github.com/paradigmxyz/reth/blob/00420477c59214e0d078592c19d77f5b7fa98d7e/crates/net/network/src/transactions/fetcher.rs#L667-L678

and

https://github.com/paradigmxyz/reth/blob/00420477c59214e0d078592c19d77f5b7fa98d7e/crates/net/network/src/transactions/fetcher.rs#L908-L915

Both are debug statements, so maybe i can get around this.

Was able to get around these with as_deref but i am hacking on redoing the remove_lru()as the LruMap does not have the pop_front that was used by the hashset. I think there is this change, the contains and also the iter that are broken with the LruMap but i'll see what i can do to fix them. https://github.com/paradigmxyz/reth/blob/dd83c9c4f8194c6ced33b1a453d6458b18f72316/crates/net/network/src/cache.rs#L54-L57

[0xJepsen]

My question here is: Should I attempt to implement Clone for the RandomState? or should we specify a different hasher? Or is possible we do not need the LruCache to be Clone?

nice! we can skip the LruCache being clone. question is if that breaks a lot of the code throughout the workspace that may be cloning LruCache, and if that's then enough to be out of scope.

So where it breaks is in the crates/net/network/src/transactions/fetcher.rs file looks like in two places: https://github.com/paradigmxyz/reth/blob/00420477c59214e0d078592c19d77f5b7fa98d7e/crates/net/network/src/transactions/fetcher.rs#L667-L678

and https://github.com/paradigmxyz/reth/blob/00420477c59214e0d078592c19d77f5b7fa98d7e/crates/net/network/src/transactions/fetcher.rs#L908-L915

Both are debug statements, so maybe i can get around this.

Was able to get around these with as_deref but i am hacking on redoing the remove_lru()as the LruMap does not have the pop_front that was used by the hashset. I think there is this change, the contains and also the iter that are broken with the LruMap but i'll see what i can do to fix them.

https://github.com/paradigmxyz/reth/blob/dd83c9c4f8194c6ced33b1a453d6458b18f72316/crates/net/network/src/cache.rs#L54-L57

I got these figured out but now I am unsure how to handle these not using clone on the backups in the fetcher https://github.com/paradigmxyz/reth/blob/05d9cf950b85f2af84a04012b9d60a1de1f36978/crates/net/network/src/transactions/fetcher.rs#L1468-L1484

[emhane]

I got these figured out but now I am unsure how to handle these not using clone on the backups in the fetcher

https://github.com/paradigmxyz/reth/blob/05d9cf950b85f2af84a04012b9d60a1de1f36978/crates/net/network/src/transactions/fetcher.rs#L1468-L1484

amazing! thanks. as for those, that's in a test, so anything ugly will do. like a new fn in test module

fn clone_backup_peers(&backups: &LruCache<PeerId>) -> LruCache<PeerId> {
    let mut backups_clone = LruCache::new();
    for peer_id in backups.iter() {
       ...
    }
}

maybe iter().rev() if test fails since iter returns peers in lru order

[emhane]

@0xJepsen anything I can help with here?

[0xJepsen]

@0xJepsen anything I can help with here?

Been a little bogged down with new work. I remember wrestling with these tests for a bit. I think there is some inconsistency in the way the remove method is implemented on the LRU cache vs the LRU map. If you have any additional insight I could set asside some time this weekend to take another stab at it.

[emhane]

@0xJepsen anything I can help with here?

Been a little bogged down with new work. I remember wrestling with these tests for a bit. I think there is some inconsistency in the way the remove method is implemented on the LRU cache vs the LRU map. If you have any additional insight I could set asside some time this weekend to take another stab at it.

I'd be happy to merge with the broken tests, and fix them myself. the tests in crates/net/network/src/cache.rs should cover the code that you implemented on LruCache I should think, these other tests in transaction fetcher are unrelated. @mattsse wdyt, cool with this?