Store fallback peers in TransactionFetcher as a PeerId, usize tuple #6559

AbhinavMir · 2024-02-12T11:06:48Z

potential fix #6528

AbhinavMir · 2024-02-12T19:44:17Z

@emhane for context: I started by changing fallback_peers: LruCache<PeerId> to fallback_peers: LruCache<(PeerId, usize)> - and used the errors from there to guide me to wherever this was used. I'm sure I missed a few things, so happy to address any feedback!

(Will fix any formatting issues as I see them!)

emhane

coming along well

emhane · 2024-02-12T20:44:56Z

crates/net/network/src/transactions/fetcher.rs

@@ -870,7 +882,7 @@ pub(super) struct TxFetchMetadata {
    /// The number of times a request attempt has been made for the hash.
    retries: u8,
    /// Peers that have announced the hash, but to which a request attempt has not yet been made.
-    fallback_peers: LruCache<PeerId>,
+    fallback_peers: LruCache<(PeerId, usize)>,


could you please make a new type

struct TxSizeMetadata { peer_id: PeerId tx_encoded_len: usize, }

and then implement Hash and Eq for this, so that only the PeerId is considered. that way if a peer announces the same hash twice with two different sizes, it's only inserted once into the cache and the second time, the first entry is replaced.

Makes sense! Would fallback_peer be essentially an array of TxSizeMetadata eg. Vec<TxSizeMetadata>?

we want to keep the cache with its nice lru properties that suit our use case here, so a LruCache<TxSizeMetada>.

although we could use an LruMap, and add the PeerId as key and PackedOption<usize> as the value. otherwise, we would need a method TxSizeMetadata::get_size(&self) -> Option<usize> which returns None if the size is zero. as mentioned in the issue, zero is a reserved value here.

Hi @emhane , I saw this issue was closed. Should I reopen this? I wasn't working yesterday so didn't have a chance to get back. I think the PR closed automatically b/c of deletion of branch. Just making sure!

Thanks!

There's actually some non-trivial changes I had to work with, so I will implement the changes on main directly!

@emhane Is PackedOption from the Cranelift crate?

yepp that's right! since you implemented get_size now manually, we don't need to use that type. we can just use the TxSizeMetadata type directly in the fallback peers cache, so we revert to a LruCache from LruMap, since we don't need a kv map anymore, just a set. We would have used PackedOption<usize> for the tx_encoded_len otherwise. PackedOption essentially does the same thing: has a reserved value (in our case 0), and stores that in place of the Option variant None. this saves memory, check out https://stackoverflow.com/a/16515488. try running that program in Rust Playground with usize (tx_encoded_len type).

Makes perfect sense, did some playing around with it. I've made a decent amount of progress with using LruCache. Just wanted to ask: Is peer_id strictly unique? For the fallback_peers.removal() type functions, the remove() method expects value (as defined here), so I can "get" the txSizeMetadata object, and pass it to the remove method, but for that I just wanted to be sure that peer_id is unique, and then find a way to implement get_txSizeMetadata_from_peer_id(). Thx!

peer_id is unique!

crates/net/network/src/transactions/fetcher.rs

emhane

nice progress!

crates/net/network/src/transactions/fetcher.rs

emhane · 2024-04-02T11:41:25Z

close to finish line @AbhinavMir !

AbhinavMir · 2024-04-02T12:53:27Z

I'm sorry I had stopped working on this, had a bunch of things happen at the same time! I'll try to finish this up EoD. I'll make those changes! @emhane

onbjerg · 2024-04-18T11:21:56Z

Hey @AbhinavMir! Anything we can help with to get this over the line? 😊 Let me know!

AbhinavMir · 2024-04-20T06:56:58Z

Hi @onbjerg - I had lost context due to being busy, but wondering if I am back in the right direction? I removed packedOption and updated methods all around. There should be a warning, but if everything else looks good pls lmk!

emhane

maybe @onbjerg can better help you out from here, thanks for the effort @AbhinavMir !

emhane · 2024-04-20T17:59:32Z

crates/net/network/src/transactions/fetcher.rs

+    pub(super) fn is_idle(&self, metadata: &TxSizeMetadata) -> bool {
+        let Some(inflight_count) = self.active_peers.peek(&metadata.peer_id) else { return true };


would prefer if you revert this and extract the peer id reference before call to this function

Done! Would this also change how the getter function for this (get_idle_peer_for) would work? I'll correspond with Oliver on this!

emhane · 2024-04-20T18:02:37Z

crates/net/network/src/transactions/fetcher.rs

-    fn get_size(&self) -> Option<usize> {
-        if self.tx_encoded_len == 0 {
-            None
-        } else {
-            Some(self.tx_encoded_len)
-        }
-    }


this is the code that is equivalent to PackedOption. so removing this OR PackedOption, that was the goal. not both. sorry if this was too advanced, hope you learned smthg new :)

My bad! I missed this! I've reverted it. I am a little new to Rust was wasn't the easiest, but lots of fun learning.

ok looks really good now!

Thank you, let me know if I need anything else to have it merged. Unsure what should I do about the warnings generated during build, though.

before a PR is merged, CI should be all green, have you tried fixing merge conflicts?

Just fixed the merge conflicts, but it seems like some methods and variables, since newly introduced, are unused. What is best steps for dealing with this?

… reference before call

onbjerg · 2024-04-22T08:52:49Z

crates/net/network/src/transactions/fetcher.rs

+                entry.fallback_peers_mut().remove(&TxSizeMetadata {
+                    peer_id: *peer_failed_to_serve,
+                    tx_encoded_len: 0,
+                });


this doesn't seem correct (i see this over the rest of the pr too), will this not only remove it if the entry matches the peer id and tx_encoded_len is 0?

nope, because of PartialEq impl, it only considers the PeerId. at best this would be abstracted away by adding a method to TxFetchMetadata

pub fn remove_fallback_peer(&mut self, peer_id: PeerId) { ... }

i'd prefer we add remove_fallback_peer since we always remove using tx_encoded_len: 0, it reduces confusion

onbjerg · 2024-04-22T08:54:24Z

crates/net/network/src/transactions/fetcher.rs

+
+impl Hash for TxSizeMetadata {
+    fn hash<H: Hasher>(&self, state: &mut H) {
+        self.peer_id.hash(state);


i think this is unexpected behavior, so not sure i am a fan. it should at least be documented

yes docs on the TxSizeMetadata type would be good, something like 'TxSizeMetadata is unique for PeerId'

onbjerg · 2024-04-22T08:54:39Z

crates/net/network/src/transactions/fetcher.rs

+    fn get_size(&self) -> Option<usize> {
+        if self.tx_encoded_len == 0 {
+            None
+        } else {
+            Some(self.tx_encoded_len)
+        }
+    }


any reason this doesn't just return usize?

have you read the docs for PackedOption type mentioned in the issue? it's similar to that type

yes, i understand that it is the same as PackedOption, but i do not see why we need an option in the first place. is it because you want 0 to represent that the peer has not told us how big the tx is?

emhane · 2024-04-22T13:10:46Z

crates/net/network/src/transactions/fetcher.rs

@@ -1072,13 +1081,45 @@ impl Default for TransactionFetcher {
    }
 }

+#[derive(Debug, Clone)]
+pub(super) struct TxSizeMetadata {


Suggested change

pub(super) struct TxSizeMetadata {

pub struct TxSizeMetadata {

this fixes one warning, please address the other warnings about missing documentation. as for the warning about unused function name, please add an underscore at the start of the method name. I will use the methods when this pr is merged.

emhane · 2024-04-22T13:13:31Z

crates/net/network/src/transactions/fetcher.rs

@@ -1072,13 +1081,45 @@ impl Default for TransactionFetcher {
    }
 }

+#[derive(Debug, Clone)]


Suggested change

#[derive(Debug, Clone)]

#[derive(Debug, Clone, Constructor)]

one last thing to address, asides that which @onbjerg says, is to import derive_more::Constructor, then replace all the places in the code where you make an instance of the type. so this

TxSizeMetadat::new(peer_id, 0)

instead of making it like this

TxSizeMetadata { peer_id: <value>, len: <value>, }

emhane · 2024-04-22T13:18:57Z

btw @AbhinavMir, try the command make lint from the Makefile

mattsse · 2024-05-21T11:52:53Z

what to do with this @emhane ?

please take over or close

emhane · 2024-05-21T14:10:56Z

what to do with this @emhane ?

please take over or close

ok, I can take over :)

AbhinavMir marked this pull request as ready for review February 12, 2024 19:22

AbhinavMir requested review from emhane, mattsse and Rjected as code owners February 12, 2024 19:22

mattsse added the A-networking Related to networking in general label Feb 12, 2024

mattsse assigned emhane Feb 12, 2024

emhane requested changes Feb 12, 2024

View reviewed changes

emhane deleted the branch paradigmxyz:main February 13, 2024 19:08

emhane closed this Feb 13, 2024

emhane reopened this Feb 15, 2024

emhane changed the base branch from emhane/speed-up-request-buffered-hashes to main February 15, 2024 11:37

AbhinavMir closed this Feb 18, 2024

Add TxSizeMetadata

f77ae9a

AbhinavMir reopened this Feb 18, 2024

AbhinavMir added 2 commits February 17, 2024 23:55

Implement TxSizeMetadata::get_size

2767ebc

Move fallback_peers to LruMap from LruCache

f1a102e

emhane reviewed Feb 27, 2024

View reviewed changes

crates/net/network/src/transactions/fetcher.rs Show resolved Hide resolved

crates/net/network/src/transactions/fetcher.rs Outdated Show resolved Hide resolved

emhane reviewed Apr 2, 2024

View reviewed changes

crates/net/network/src/transactions/fetcher.rs Outdated Show resolved Hide resolved

Remove packed option

3f7e0f8

emhane reviewed Apr 20, 2024

View reviewed changes

AbhinavMir added 4 commits April 20, 2024 14:14

Reimplement get_size and Refactor is_idle function to extract peer_id…

2a1f652

… reference before call

Fix module declaration for is_idle()

2add9c8

Use correct type for backups cache in transaction fetcher tests

f7ce99e

Merge branch 'main' into fallback_peer_tuple

841c863

onbjerg reviewed Apr 22, 2024

View reviewed changes

emhane requested changes Apr 22, 2024

View reviewed changes

emhane mentioned this pull request May 30, 2024

chore(net): get and peek for LruCache #8508

Merged

AbhinavMir closed this Jun 3, 2024

		pub(super) fn is_idle(&self, metadata: &TxSizeMetadata) -> bool {
		let Some(inflight_count) = self.active_peers.peek(&metadata.peer_id) else { return true };

	pub(super) struct TxSizeMetadata {
	pub struct TxSizeMetadata {

Store fallback peers in TransactionFetcher as a PeerId, usize tuple #6559

Store fallback peers in TransactionFetcher as a PeerId, usize tuple #6559

Conversation

AbhinavMir commented Feb 12, 2024

AbhinavMir commented Feb 12, 2024 • edited Loading

emhane left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AbhinavMir Feb 12, 2024 • edited Loading

Choose a reason for hiding this comment

emhane Feb 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AbhinavMir Feb 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emhane left a comment

Choose a reason for hiding this comment

emhane commented Apr 2, 2024

AbhinavMir commented Apr 2, 2024

onbjerg commented Apr 18, 2024

AbhinavMir commented Apr 20, 2024

emhane left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AbhinavMir Apr 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emhane commented Apr 22, 2024

mattsse commented May 21, 2024

emhane commented May 21, 2024

AbhinavMir commented Feb 12, 2024 •

edited

Loading

AbhinavMir Feb 12, 2024 •

edited

Loading

emhane Feb 13, 2024 •

edited

Loading

AbhinavMir Feb 15, 2024 •

edited

Loading

AbhinavMir Apr 21, 2024 •

edited

Loading