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.

Sign up for GitHub

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

gossip(gossip,net,sync,tvu): Channel(Managed -> Unmanaged) overhaul #165

Closed
wants to merge 1 commit into from
+424 −240
Closed

gossip(gossip,net,sync,tvu): Channel(Managed -> Unmanaged) overhaul #165

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions src/gossip/fuzz.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -380,7 +380,9 @@ pub fn run() !void {
};

// batch it
var packet_batch = std.ArrayList(Packet).init(allocator);
var packet_batch = std.ArrayList(Packet).init(gossip_service_fuzzer.allocator);
defer packet_batch.deinit();

try packet_batch.append(send_packet);
msg_count +|= 1;

Expand All @@ -391,7 +393,7 @@ pub fn run() !void {
}

// send it
try gossip_service_fuzzer.packet_outgoing_channel.send(packet_batch);
try gossip_service_fuzzer.packet_outgoing_channel.send(packet_batch.moveToUnmanaged());

std.time.sleep(SLEEP_TIME);

Expand Down
42 changes: 28 additions & 14 deletions src/gossip/ping_pong.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,31 +209,38 @@ pub const PingCache = struct {
}

/// Filters valid peers according to `PingCache` state and returns them along with any possible pings that need to be sent out.
///
/// *Note*: caller is responsible for deinit `ArrayList`(s) returned!
pub fn filterValidPeers(
self: *Self,
allocator: std.mem.Allocator,
our_keypair: KeyPair,
peers: []ContactInfo,
) error{OutOfMemory}!struct { valid_peers: std.ArrayList(usize), pings: std.ArrayList(PingAndSocketAddr) } {
peers: []const ContactInfo,
out: struct {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why did you change this to pass in the arraylist's as a parameter vs return them? seems like everywhere we use it we init&deinit so we dont really take advantage of this approach

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Passing it in like this avoids the anonymous struct return, which has to always be deconstructed; another route would be to return a tuple which we could easily deconstruct at the call-site, but it'd be two arraylists. Obviously they're different types, but I dislike the visual ambiguity, so I opted for turning these into out parameters; another consideration is that it would allow us to more easily re-use a buffer in the future if the opportunity ever came about.

valid_peers: *std.ArrayList(usize),
pings: *std.ArrayList(PingAndSocketAddr),
},
) error{OutOfMemory}!void {
const now = std.time.Instant.now() catch @panic("time not supported by OS!");
var valid_peers = std.ArrayList(usize).init(allocator);
var pings = std.ArrayList(PingAndSocketAddr).init(allocator);
// var valid_peers = std.ArrayList(usize).init(allocator);
// var pings = std.ArrayList(PingAndSocketAddr).init(allocator);
const valid_peers = out.valid_peers;
const pings = out.pings;

valid_peers.clearRetainingCapacity();
pings.clearRetainingCapacity();

try valid_peers.ensureTotalCapacityPrecise(peers.len);
try pings.ensureTotalCapacityPrecise(peers.len);

for (peers, 0..) |*peer, i| {
if (peer.getSocket(socket_tag.GOSSIP)) |gossip_addr| {
const result = self.check(now, PubkeyAndSocketAddr{ .pubkey = peer.pubkey, .socket_addr = gossip_addr }, &our_keypair);
if (result.passes_ping_check) {
try valid_peers.append(i);
valid_peers.appendAssumeCapacity(i);
}
if (result.maybe_ping) |ping| {
try pings.append(.{ .ping = ping, .socket = gossip_addr });
pings.appendAssumeCapacity(.{ .ping = ping, .socket = gossip_addr });
}
}
}

return .{ .valid_peers = valid_peers, .pings = pings };
}

// only used in tests/benchmarks
Expand Down Expand Up @@ -269,9 +276,16 @@ test "gossip.ping_pong: PingCache works" {
try testing.expect(!resp.passes_ping_check);
try testing.expect(resp.maybe_ping != null);

var result = try ping_cache.filterValidPeers(testing.allocator, our_kp, &[_]ContactInfo{});
defer result.valid_peers.deinit();
defer result.pings.deinit();
var valid_peers = std.ArrayList(usize).init(std.testing.allocator);
defer valid_peers.deinit();

var pings = std.ArrayList(PingAndSocketAddr).init(std.testing.allocator);
defer pings.deinit();

try ping_cache.filterValidPeers(our_kp, &[_]ContactInfo{}, .{
.valid_peers = &valid_peers,
.pings = &pings,
});

try testing.expect(ping != null);
}
Expand Down
Loading
Loading