Test gossip propagation #1342
Test gossip propagation #1342
Conversation
njgheorghita
force-pushed
the
test-gossip-propagation
branch
2 times, most recently
from
0064571
to
823e23d
Compare
| @@ -170,6 +170,9 @@ impl HistoryNetworkApiServer for HistoryNetworkApi { | |||
| } | |||
|
|
|||
| /// Send an OFFER request with given ContentKey, to the designated peer and wait for a response. | |||
| /// If the content value is provided, a "populated" offer is used, which will not store the | |||
There was a problem hiding this comment.
Should we add this to other *_rpc.rs files?
Related to this, maybe state shouldn't supported "unpopulated" version.
There was a problem hiding this comment.
Maybe this is true for beacon network as well? I'll wait till @ogenev gets back and check with him, and if so remove the feature for those subnetworks
| None, | ||
| ) | ||
| .await | ||
| .is_err()); |
There was a problem hiding this comment.
Would be worth checking the error itself, because it could fail for some other reason, e.g. wrong test configuration, not being connected/able to talk to the target node, etc.
| trace: None, | ||
| } | ||
| })?; | ||
| match self.store.read().get(&content_key) { |
There was a problem hiding this comment.
Question more for my understanding: considering that now we are getting the value from the DB before the OFFER request anyway, why not use Request::PopulatedOffer instead?
And if we switch to Request::PopulatedOffer, do we ever use Request::Offer? And if not, can we remove it?
There was a problem hiding this comment.
I think the whole offer/populated offer is a result of what was easiest for testing, especially back in the early days of the network when these endpoints were a bit more useful. Aka, if I want to test basic gossip across the network, and I know my node has these content keys stored locally, the unpopulated offer endpoint is easier to use than the populated endpoint.
Why not use
Request::PopulatedOfferinstead?
Well, in the use case of... I know I have this content key stored locally, and I want to gossip it to peer A, this endpoint is still easier, because I don't have to manually go into the store and retrieve the content value, like I would if my only option was a populated offer. Now, is this a use case we even care about / worth supporting, is a valid question. Now that our network has been pretty thoroughly tested, at least in terms of the basic offer/accept flow, how useful is this endpoint?
Basically, it seems possible that we could remove it. But that would require a quick audit of all our tests / hive to make sure it's not being used anywhere. Essentially, this seems like a fairly low-priority issue, and not worth the time right now. But maybe once we start reaching some stability with the state network, it'll be worth re-auditing the jsonrpc api for outdated endpoints and we can revisit this then?
There was a problem hiding this comment.
I wasn't talking about JSON rpc endpoint, as much as I was talking about Request object and how we handle it internally.
For example, the json rpc handler can retrieve the content value from the local storage, and call network.overlay.send_populated_offer(..) instead. In that case, I believe we don't need Request::Offer type at all (but still support json rpc offer endpoint).
If you are still not sure what I mean, we can have a look at the code together. It's very possible that I'm not understanding how some things work.
There was a problem hiding this comment.
Ahh, ok yeah that's interesting. Off the top of my head that seems feasible, but will probably need to do a little tinkering to confirm and make sure there aren't any edge cases that wouldn't be supported. Not super high-priority imo but I put it on my flamingo list of todos
njgheorghita
force-pushed
the
test-gossip-propagation
branch
from
823e23d
to
736e076
Compare
What was wrong?
Did some digging while debugging #1341 - but looks like our code is working as expected 🥳 Though I feel like it's useful to keep this test in for sanity.
Also added a check to our
OFFERfunctionality to make sure that we can only offer content that's present in the local datastore for unpopulated offers.How was it fixed?
To-Do