Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sending a Request::Peers request to a zcashd node results in a spurious Response::Peers #5421

Closed
softminus opened this issue Oct 19, 2022 · 7 comments
Labels
C-bug Category: This is a bug

Comments

@softminus
Copy link

softminus commented Oct 19, 2022

The spurious Response::Peers value is unit length and only contains the IP address of the node we connected to. Here's what it looks like:

Starting new connection: peer addr is 157.245.172.190:8233
peers response: Peers { peers: 1 }
peers is: [MetaAddr { addr: 157.245.172.190:8233, services: Some(NODE_NETWORK), untrusted_last_seen: Some(DateTime32 { timestamp: 1666049426, calendar: 2022-10-17T23:30:26Z }), last_response: None, last_attempt: None, last_failure: None, last_connection_state: NeverAttemptedGossiped }]

Code to reproduce this is at https://github.com/superbaud/zcash-cotyledon/blob/main/src/main.rs

Looking at the debug.log from the zcashd node, we see:

2022-10-19T00:30:18.501716Z  INFO main: receive version message: /Seeder-and-feeder:0.0.0-alpha0/: version 170100, blocks=0, us=0.0.0.0:8233, peer=169036, peeraddr=97.XX.XX.XX:49475
2022-10-19T00:30:18.501785Z  INFO main: AdvertizeLocal: advertizing address 157.245.172.190:8233

and it indeed looks like the spurious Response::Peers is from the function AdvertizeLocal in zcashd, which sends its own address -- unsolicited -- to the connected peer (with PushAddress).

My guess is that the zebra-networking code interprets that unsolicited address as a reply to the Request::Peers request (rather than dropping it on the floor or handing it to the inbound service, if one is present).

@softminus softminus added C-bug Category: This is a bug S-needs-triage Status: A bug report needs triage labels Oct 19, 2022
@teor2345
Copy link
Contributor

Zebra doesn't handle unsolicited address advertisements using the inbound service, for security reasons. If it did, a node could eventually replace all the addresses in its address book, by using large unsolicited requests.

Instead, if there is no active peers request, Zebra caches the last unsolicited peers response from each peer, then uses it to provide a synthetic response to the next peers request to that peer. But it prefers multi-peer responses to single-peer responses. (If an unsolicited response only contains one peer, and there is already a cached multi-peer response, it keeps the multi-peer response. Otherwise, it replaces the older response with a newer one.)

So I'm not sure if there is actually a bug here, the actual peers response will get cached, and returned in response to the next peers request. This works well for zebrad's use case, where we periodically ask randomly selected peers for some addresses.

If it doesn't work for the DNS seeder, we could add a config option to turn off the caching, or to ignore single-address responses?

@softminus
Copy link
Author

If it doesn't work for the DNS seeder, we could add a config option to turn off the caching, or to ignore single-address responses?

Yeah, an option to ignore caching altogether would be optimal!

Thanks for explaining this clearly (I found the PR where this was introduced #3294) and it does make sense for the zebrad use case, but it's kind of confusing for the seeder use case -- we'd rather get uncached multi-peer responses even if we have to wait for them.

@teor2345
Copy link
Contributor

If it doesn't work for the DNS seeder, we could add a config option to turn off the caching, or to ignore single-address responses?

Yeah, an option to ignore caching altogether would be optimal!

Thanks for explaining this clearly (I found the PR where this was introduced #3294) and it does make sense for the zebrad use case, but it's kind of confusing for the seeder use case -- we'd rather get uncached multi-peer responses even if we have to wait for them.

After thinking about this a bit more, I'm not sure if disabling caching will always work for you. I think there's a race condition.

The problem is that:

  • zcashd sends unsolicited single-address peers responses, sometimes at the same time as a multi-peer response
  • zcashd rate-limits multi-peer responses
  • Zebra drops unsolicited responses unless there is a pending request for them

If you want to get the unfiltered responses, then disabling caching might not help, because:

  • Zebra will drop the response if it comes after an unsolicited single-address response, and before the next peers request
  • You can't re-request the peers, because zcashd rate-limits those requests

So with caching, you can re-request the peers and get the cached multi-peer response. Without it, the multi-peer response could get dropped.

Instead, we could always send every peers response to the inbound service, regardless of caching or any requests. That way, you could get a reliable stream of peers by making requests, and processing the responses via a separate inbound service for each isolated peer?

@softminus
Copy link
Author

That rationale makes sense, and thank you for explaining the moving parts here. Regardless of what action we'll take, these details are very helpful.

Instead, we could always send every peers response to the inbound service, regardless of caching or any requests. That way, you could get a reliable stream of peers by making requests, and processing the responses via a separate inbound service for each isolated peer?

I think this would be a pretty invasive change for zebra-network which would also force changes for Zebra as well, and I can't really justify that ask, since as I understand it, the benefits would be minimal.

Such a change would still require me to write code that distinguishes between the (relatively useless, for my purpose) single-address peers responses and the multi-peer responses (since I would be fed every peers response) -- and once I have that code, I can feed it the cached responses and get pretty much the same results, since the zebra-network logic prefers to cache a multi-peer response over a single-peer response.

The thing I want to make sure is that the peers response cache is per-connection and gets initialized anew each time there's a new connection (that there isn't a global cache or something like that).

@teor2345
Copy link
Contributor

The thing I want to make sure is that the peers response cache is per-connection and gets initialized anew each time there's a new connection (that there isn't a global cache or something like that).

Yes, that's correct, the cache is per-connection. A new Connection struct is created for each connection, and dropped when the peer disconnects.

Here's the cache implementation:

Struct:

/// A cached copy of the last unsolicited `addr` or `addrv2` message from this peer.
///
/// When Zebra requests peers, the cache is consumed and returned as a synthetic response.
/// This works around `zcashd`'s address response rate-limit.
///
/// Multi-peer `addr` or `addrv2` messages replace single-peer messages in the cache.
/// (`zcashd` also gossips its own address at regular intervals.)
pub(super) cached_addrs: Vec<MetaAddr>,

Store:

Message::Addr(ref addrs) => {
// Workaround `zcashd`'s `getaddr` response rate-limit
if addrs.len() > 1 {
// Always refresh the cache with multi-addr messages.
debug!(%msg, "caching unsolicited multi-addr message");
self.cached_addrs = addrs.clone();
Consumed
} else if addrs.len() == 1 && self.cached_addrs.len() <= 1 {
// Only refresh a cached single addr message with another single addr.
// (`zcashd` regularly advertises its own address.)
debug!(%msg, "caching unsolicited single addr message");
self.cached_addrs = addrs.clone();
Consumed
} else {
debug!(
%msg,
"ignoring unsolicited single addr message: already cached a multi-addr message"
);
Consumed
}
}

Retrieve:

// Consume the cached addresses from the peer,
// to work-around a `zcashd` response rate-limit.
(AwaitingRequest, Peers) if !self.cached_addrs.is_empty() => {
let cached_addrs = std::mem::take(&mut self.cached_addrs);
debug!(
addrs = cached_addrs.len(),
"responding to Peers request using cached addresses",
);
Ok(Handler::Finished(Ok(Response::Peers(cached_addrs))))
}

@softminus
Copy link
Author

Perfect, thank you so much for your support and explanations!

@softminus
Copy link
Author

once I have that code, I can feed it the cached responses and get pretty much the same results, since the zebra-network logic prefers to cache a multi-peer response over a single-peer response.

I have this working by the way!

@mpguerra mpguerra removed the S-needs-triage Status: A bug report needs triage label Dec 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug
Projects
None yet
Development

No branches or pull requests

3 participants