refactor(gossip) Add methods, data structures, and edits to improve memory safety in gossip #189

yewman · 2024-07-05T17:14:00Z

Memory Leaks

Found a few more memory leaks while thinking about clone / deinit implementation.
Implemented basic fix - call bincode.free
Ideally should add test cases for unhappy paths

ThreadSafeContactInfo

In a number of places, ContactInfo was being passed around outside of lock boundaries for purposes not requiring access to the heap memory contained within. Whilst these usages were not causing any errors, creating and using an explicitly thread safe type seems like a good idea as makes the code more robust, simpler to understand, and easier to debug.

Clone

Implement clone methods for instances where we need access to the heap allocated memory of gossip values outside of a table lock scope.

Other

Add saturating subs for timestamp operations
Remove parameter to processMessages - no need to parse a parameter which is contained in self

InKryption

Just a few notes, otherwise LGTM.

src/accountsdb/download.zig

src/gossip/ping_pong.zig

InKryption · 2024-07-05T18:21:01Z

Th failing CI is due to #157.
Edit: I re-ran the job, so it shouldn't fail unless we're particularly unlucky with entropy.

src/accountsdb/download.zig

src/gossip/table.zig

src/gossip/ping_pong.zig

src/gossip/service.zig

…ctures outside of table lock boundaries

…unnecessary at it is currently only used safely within lock boundaries

…quest

…r dicussion

… minimise time with write lock

…o convert to millis everywhere, would rather extend Timer to use comptime types or provide additional access methods like read_ms() etc

…sages without table lock

0xNineteen

lgtm - should be good to merge after you add the docs

0xNineteen

lgtm - should get @dnut approval too since he has a lot of context around this problem

dnut

Looks good, I just have a few minor concerns.

I like the approach of just returning a lean contact info with the bare minimum data, instead of requiring more allocations. It actually moves us back away from the new ContactInfo, closer towards something resembling LegacyContactInfo. If we go all-in on this approach, at some point we could probably get rid of the contact info mapping that I added to facilitate the migration to ContactInfo.

sig/src/gossip/table.zig

Line 90 in b311478

converted_contact_infos: AutoArrayHashMap(Pubkey, ContactInfo),

I can imagine some approaches where a caller could specify exactly the minimum amount of data they need about a contact. Something that feels like graphql.

src/gossip/table.zig

src/gossip/service.zig

InKryption

Need to go over some details concerning bincode.

src/bincode/bincode.zig

yewman changed the title ~~refactor(gossip): Add methods, data structures, and edits to improve memory safety in gossip~~ refactor(gossip) WIP: Add methods, data structures, and edits to improve memory safety in gossip Jul 5, 2024

InKryption assigned yewman Jul 5, 2024

InKryption reviewed Jul 5, 2024

View reviewed changes

src/accountsdb/download.zig Outdated Show resolved Hide resolved

src/gossip/ping_pong.zig Show resolved Hide resolved

src/gossip/ping_pong.zig Show resolved Hide resolved

0xNineteen requested changes Jul 8, 2024

View reviewed changes

yewman force-pushed the harnew/gossip-safety branch 2 times, most recently from b7d92fa to 24b8ac4 Compare July 9, 2024 19:52

yewman changed the title ~~refactor(gossip) WIP: Add methods, data structures, and edits to improve memory safety in gossip~~ refactor(gossip) Add methods, data structures, and edits to improve memory safety in gossip Jul 10, 2024

yewman requested a review from 0xNineteen July 10, 2024 20:43

yewman added 11 commits July 11, 2024 08:01

Remove redundant parameter from processMessages

9dc11fe

Implement ThreadSafeContactInfo to avoid moving ContactInfo data stru…

68a8e4c

…ctures outside of table lock boundaries

Modify getContactInfo to use ThreadSafeContactInfo, arguably this is …

0436566

…unnecessary at it is currently only used safely within lock boundaries

Fix unsafe access of gossip values in GossipService.handleBatchPullRe…

3070872

…quest

Add comments on memory unsafety in GossipService.buildPushMessages fo…

1ae6985

…r dicussion

Remove redundant, potentially dangerous argument for PullRequestTask,…

a1adf4f

… minimise time with write lock

Add saturating subs to timestamp operations

56296f5

Address minor PR comments

37d60a8

Change to use timer instead of wall clock, not that nice as we need t…

713538b

…o convert to millis everywhere, would rather extend Timer to use comptime types or provide additional access methods like read_ms() etc

Implement clone for gossip data to allow for safe construction of mes…

4e55d79

…sages without table lock

Implement fix for memory leaks in gossip: needs testing

65b5c8f

0xNineteen reviewed Jul 11, 2024

View reviewed changes

yewman force-pushed the harnew/gossip-safety branch from 01211f8 to 77c641d Compare July 11, 2024 15:41

Add gossip diagrams to readme.md

945ab44

yewman force-pushed the harnew/gossip-safety branch from 77c641d to 945ab44 Compare July 11, 2024 17:06

Implement Timer wrapper for save timing units

75c2fe6

yewman requested a review from 0xNineteen July 11, 2024 17:27

a few small fixes

b311478

0xNineteen previously approved these changes Jul 11, 2024

View reviewed changes

0xNineteen requested a review from dnut July 11, 2024 18:30

yewman requested a review from 0xNineteen July 11, 2024 22:20

dnut reviewed Jul 15, 2024

View reviewed changes

src/gossip/table.zig Show resolved Hide resolved

src/gossip/service.zig Outdated Show resolved Hide resolved

src/gossip/service.zig Show resolved Hide resolved

src/gossip/service.zig Show resolved Hide resolved

Implement bincode init method and update gossip serialise test

88c4fa2

yewman dismissed 0xNineteen’s stale review via 88c4fa2 July 15, 2024 17:11

Address PR comments

d169cef

yewman requested review from dnut and InKryption July 15, 2024 18:03

InKryption reviewed Jul 15, 2024

View reviewed changes

src/bincode/bincode.zig Outdated Show resolved Hide resolved

yewman and others added 3 commits July 16, 2024 05:28

Add comments on post deserialise method

cbde33e

Rename post deserialise callback

eeed38e

add additional comment

ea4b3a8

0xNineteen previously approved these changes Jul 15, 2024

View reviewed changes

fix lint

060dc49

0xNineteen dismissed their stale review via 060dc49 July 15, 2024 21:11

0xNineteen approved these changes Jul 15, 2024

View reviewed changes

0xNineteen merged commit 7bb6345 into main Jul 15, 2024
5 checks passed

0xNineteen deleted the harnew/gossip-safety branch July 15, 2024 21:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(gossip) Add methods, data structures, and edits to improve memory safety in gossip #189

refactor(gossip) Add methods, data structures, and edits to improve memory safety in gossip #189

yewman commented Jul 5, 2024 •

edited

Loading

InKryption left a comment

InKryption commented Jul 5, 2024 •

edited

Loading

0xNineteen left a comment

0xNineteen left a comment

dnut left a comment •

edited

Loading

InKryption left a comment

refactor(gossip) Add methods, data structures, and edits to improve memory safety in gossip #189

refactor(gossip) Add methods, data structures, and edits to improve memory safety in gossip #189

Conversation

yewman commented Jul 5, 2024 • edited Loading

InKryption left a comment

Choose a reason for hiding this comment

InKryption commented Jul 5, 2024 • edited Loading

0xNineteen left a comment

Choose a reason for hiding this comment

0xNineteen left a comment

Choose a reason for hiding this comment

dnut left a comment • edited Loading

Choose a reason for hiding this comment

InKryption left a comment

Choose a reason for hiding this comment

yewman commented Jul 5, 2024 •

edited

Loading

InKryption commented Jul 5, 2024 •

edited

Loading

dnut left a comment •

edited

Loading