-
Notifications
You must be signed in to change notification settings - Fork 53
fix(consensus): get votes to actually send and get retained #1196
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
base: main
Are you sure you want to change the base?
Changes from all commits
5a2c265
4677256
a8adcaf
6d54d28
7602b01
feac128
43c2a6a
14ea135
a19dcd6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -66,7 +66,7 @@ const GossipMessageWithEndpoint = struct { | |
|
|
||
| pub const PULL_REQUEST_RATE: Duration = .fromSecs(1); | ||
| pub const PULL_RESPONSE_TIMEOUT: Duration = .fromSecs(5); | ||
| pub const ACTIVE_SET_REFRESH_RATE: Duration = .fromSecs(15); | ||
| pub const ACTIVE_SET_REFRESH_RATE: Duration = .fromMillis(500); | ||
| pub const DATA_TIMEOUT: Duration = .fromSecs(15); | ||
| pub const TABLE_TRIM_RATE: Duration = .fromSecs(10); | ||
| pub const BUILD_MESSAGE_LOOP_MIN: Duration = .fromSecs(1); | ||
|
|
@@ -384,6 +384,8 @@ pub const GossipService = struct { | |
| for (push_msg_queue.queue.items) |*v| v.deinit(push_msg_queue.data_allocator); | ||
| push_msg_queue.queue.deinit(); | ||
| } | ||
|
|
||
| // self.broker.deinit(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is this intentional?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, nice catch. At some point early on I was debugging gossip, and wanted to fix some leaks for some local testing code, but the specific way in which this works caused issues in the actual validator on exits I believe, so I commented it out. Probably will just delete it, unless you think it's worth handling deallocation here for whatever reason. |
||
| } | ||
|
|
||
| pub const RunThreadsParams = struct { | ||
|
|
@@ -926,15 +928,16 @@ pub const GossipService = struct { | |
| defer pull_req_timer.reset(); | ||
| // this also includes sending ping messages to other peers | ||
| const now = getWallclockMs(); | ||
| const pull_req_packets = self.buildPullRequests( | ||
| var pull_req_packets = self.buildPullRequests( | ||
| self.allocator, | ||
| random, | ||
| pull_request.MAX_BLOOM_SIZE, | ||
| now, | ||
| ) catch |e| { | ||
| self.logger.err().logf("failed to generate pull requests: {any}", .{e}); | ||
| break :pull_blk; | ||
| }; | ||
| defer pull_req_packets.deinit(); | ||
| defer pull_req_packets.deinit(self.allocator); | ||
| for (pull_req_packets.items) |packet| { | ||
| try self.packet_outgoing_channel.send(packet); | ||
| } | ||
|
|
@@ -1211,11 +1214,12 @@ pub const GossipService = struct { | |
| /// to be sent to a random set of gossip nodes. | ||
| fn buildPullRequests( | ||
| self: *GossipService, | ||
| gpa: std.mem.Allocator, | ||
| random: std.Random, | ||
| /// the bloomsize of the pull request's filters | ||
| bloom_size: usize, | ||
| now: u64, | ||
| ) !ArrayList(Packet) { | ||
| ) !std.ArrayListUnmanaged(Packet) { | ||
| const zone = tracy.Zone.init(@src(), .{ .name = "gossip buildPullRequests" }); | ||
| defer zone.deinit(); | ||
|
|
||
|
|
@@ -1278,13 +1282,13 @@ pub const GossipService = struct { | |
| if (num_peers != 0) n_packets += filters.items.len; | ||
| if (entrypoint_index != null) n_packets += filters.items.len; | ||
|
|
||
| var packet_batch = try ArrayList(Packet).initCapacity(self.allocator, n_packets); | ||
| packet_batch.appendNTimesAssumeCapacity(Packet.ANY_EMPTY, n_packets); | ||
| var packet_index: usize = 0; | ||
| var packet_batch: std.ArrayListUnmanaged(Packet) = .empty; | ||
| errdefer packet_batch.deinit(gpa); | ||
| try packet_batch.ensureTotalCapacityPrecise(gpa, n_packets); | ||
|
|
||
| // update wallclock and sign | ||
| self.my_contact_info.wallclock = now; | ||
| const my_contact_info_value = SignedGossipData.initSigned( | ||
| const my_contact_info_value: SignedGossipData = .initSigned( | ||
| &self.my_keypair, | ||
| // safe to copy contact info since it is immediately serialized | ||
| .{ .ContactInfo = self.my_contact_info }, | ||
|
|
@@ -1302,12 +1306,10 @@ pub const GossipService = struct { | |
| } | ||
| if (peer_contact_info.gossip_addr) |gossip_addr| { | ||
| const message: GossipMessage = .{ .PullRequest = .{ filter_i, my_contact_info_value } }; | ||
| var packet = &packet_batch.items[packet_index]; | ||
|
|
||
| const bytes = try bincode.writeToSlice(&packet.buffer, message, bincode.Params{}); | ||
| const packet = try packet_batch.addOne(gpa); | ||
| const bytes = try bincode.writeToSlice(&packet.buffer, message, .standard); | ||
| packet.size = bytes.len; | ||
| packet.addr = gossip_addr; | ||
| packet_index += 1; | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -1316,10 +1318,9 @@ pub const GossipService = struct { | |
| if (entrypoint_index) |entrypoint_idx| { | ||
| const entrypoint = self.entrypoints[entrypoint_idx]; | ||
| for (filters.items) |filter| { | ||
| const packet = &packet_batch.items[packet_index]; | ||
| const packet = try packet_batch.addOne(gpa); | ||
| const message: GossipMessage = .{ .PullRequest = .{ filter, my_contact_info_value } }; | ||
| try packet.populateFromBincode(entrypoint.addr, message); | ||
| packet_index += 1; | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -2049,6 +2050,12 @@ pub const LocalMessageBroker = struct { | |
| else => {}, | ||
| } | ||
| } | ||
|
|
||
| fn deinit(self: *const LocalMessageBroker) void { | ||
| if (self.vote_collector) |vcs| { | ||
| while (vcs.tryReceive()) |vote| vote.deinit(vcs.allocator); | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| /// stats that we publish to prometheus | ||
|
|
@@ -2955,11 +2962,11 @@ fn testBuildPullRequests( | |
| } | ||
| } | ||
|
|
||
| var packets = gossip_service.buildPullRequests(random, 2, now) catch |err| { | ||
| var packets = gossip_service.buildPullRequests(allocator, random, 2, now) catch |err| { | ||
| std.log.err("\nThe failing now time is: '{d}'\n", .{now}); | ||
| return err; | ||
| }; | ||
| defer packets.deinit(); | ||
| defer packets.deinit(allocator); | ||
|
|
||
| try std.testing.expect(packets.items.len > 1); | ||
| try std.testing.expect(!std.mem.eql(u8, packets.items[0].data(), packets.items[1].data())); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's rare to have more than 2-3 signers in a transaction so I doubt a hashmap would be favorable over a linear search.
Is "This method can result in us signing the same message for the same keypair more than once." supposed to be part of the todo, or is it a separate note?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair point - I left this in as part of @Rexicon226's original suggestion (pinged in case he has any specific views or intel on this).
The redundant signage should probably be its own comment.