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

Some bandwidth savings #6243

Open
wants to merge 8 commits into
base: master
from

Conversation

@moneromooo-monero
Copy link
Contributor

moneromooo-monero commented Dec 16, 2019

No description provided.

@@ -257,7 +257,10 @@ namespace cryptonote
BEGIN_KV_SERIALIZE_MAP()
KV_SERIALIZE(current_height)
KV_SERIALIZE(cumulative_difficulty)
KV_SERIALIZE(cumulative_difficulty_top64)
if (is_store)
KV_SERIALIZE(cumulative_difficulty_top64)

This comment has been minimized.

Copy link
@vtnerd

vtnerd Jan 2, 2020

Contributor

The goal is to potentially send this optionally in a release or two?

This comment has been minimized.

Copy link
@moneromooo-monero

moneromooo-monero Jan 3, 2020

Author Contributor

If I remember, yes :)

@@ -263,28 +263,30 @@ namespace nodetool
}
//--------------------------------------------------------------------------------------------------
inline
bool peerlist_manager::get_peerlist_head(std::vector<peerlist_entry>& bs_head, bool anonymize, uint32_t depth)
bool peerlist_manager::get_peerlist_head(std::vector<peerlist_entry>& bs_head, bool anonymize, uint32_t depth, const std::function<bool(const peerlist_entry&)> &addfunc)

This comment has been minimized.

Copy link
@vtnerd

vtnerd Jan 2, 2020

Contributor

I think this should be a std::set as an argument instead of a callback function. The default argument would be a a default constructed set - which should never allocate any memory. This should be a bit faster than an indirect function.

This comment has been minimized.

Copy link
@moneromooo-monero

moneromooo-monero Jan 3, 2020

Author Contributor

It'll also be much less able. I guess we don't care, do we.

@@ -1915,9 +1915,6 @@ namespace nodetool
template<class t_payload_net_handler>
bool node_server<t_payload_net_handler>::get_local_node_data(basic_node_data& node_data, const network_zone& zone)
{
time_t local_time;
time(&local_time);
node_data.local_time = local_time; // \TODO This can be an identifying value across zones (public internet to tor/i2p) ...

This comment has been minimized.

Copy link
@vtnerd

vtnerd Jan 2, 2020

Contributor

This value (peer local time) was never being used anymore? I wish I tracked this down previously, or at least I thought I did :/

This comment has been minimized.

Copy link
@moneromooo-monero

moneromooo-monero Jan 3, 2020

Author Contributor

Correct.

m_payload_handler.get_payload_sync_data(rsp.payload_data);
for (const auto &e: rsp.local_peerlist_new)
context.sent_addresses.insert(e.adr);

This comment has been minimized.

Copy link
@vtnerd

vtnerd Jan 2, 2020

Contributor

The idea is that eventually we stop sending peerlists ... ? Interesting because this could reveal when someone just moved into your whitelist, and thus potentially leaking current connections. However, the privacy leak isn't guaranteed because the p2p code will probe the greylist over time and not make a permanent connection. Constantly sending random peers mitigates any leak.

I'm not immediately aware of a major privacy issue - Dandelion++ fluff stage should "break-up" any analysis of knowing the network graph. I (may have) incorrectly stated somewhere that we may need to "fluff" blocks in Monero - if it were true, than this change could be bad. I need to read the dandelion/dandelion++ to confirm this though - I forgot how they analyze their protocol which determines the importance of keeping the network graph concealed.

I'm primarily listing this to make everyone aware of any subtle changes here, and make sure they aren't a surprise to all.

This comment has been minimized.

Copy link
@moneromooo-monero

moneromooo-monero Jan 3, 2020

Author Contributor

Yes. We'd be sending updates only to long connected peers. I guess we could make it so get_peerlist_head does its usual thing, then the caller calculates the delta and sends it. This would fix that possible vector.

This comment has been minimized.

Copy link
@moneromooo-monero

moneromooo-monero Jan 3, 2020

Author Contributor

Which I did.

This comment has been minimized.

Copy link
@moneromooo-monero

moneromooo-monero Jan 3, 2020

Author Contributor

Another consequence of this sytem:
Currently, when we bump an address off our grey list as it fails to connect, we're pretty likely to get it re-relayed to us by a peer, so it gets back into the grey list. With this change, it becomes much less likely.

Nodes remember which connections have been sent which peer addresses
and won't send it again. This causes more addresses to be sent as
the connection lifetime grows, since there is no duplication anymore,
which increases the diffusion speed of peer addresses. The whole
white list is now considered for sending, not just the most recent
seen peers. This further hardens against topology discovery, though
it will more readily send peers that have been last seen earlier
than it otherwise would. While this does save a fair amount of net
bandwidth, it makes heavy use of std::set lookups, which does bring
network_address::less up the profile, though not too aggressively.
Also removes a potential fingerprinting vector
@moneromooo-monero moneromooo-monero force-pushed the moneromooo-monero:bw branch from 79e5f2d to 0b46bf1 Jan 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.