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

Network filter for publish and confirm_ack messages #2539

Closed

Conversation

guilhermelawless
Copy link
Contributor

@guilhermelawless guilhermelawless commented Feb 5, 2020

To-do test cases:

  • Block processor full
  • Vote processor full
  • Unchecked cleanup
  • Dropped inactive vote cache
  • Dropped active election
  • Dropped active election with inactive_vote_cache insertion
  • Bootstrap frontier confirmation

Adds duplicate detection of publish and confirm_ack messages before they are deserialized. When they are unique, the digest is saved and passed around to network which may drop the messages if the block or vote processor is full.

After processing, there are instances where digests have to be cleared from the filter to ensure the original message can be received again:

  • Cleaning up a long unchecked block erases that element from the publish filter
  • Reaching max capacity of the inactive votes cache clears the oldest item from the cache and the confirm_ack filter. For this reason, inactive_votes_cache now stores the digests, and votes pulled from this cache for new elections are now erased.
  • Interactions between bootstrap frontier confirmation and inactive_votes_cache
  • An active election is dropped. The vote digest is now saved here as well. Special case if a vote came from inactive_votes_cache, must ensure the digest is passed along.

The blocks_filter has been removed due to redundancy. 16 extra bytes are used for each hash in an (inactive) vote stored in memory.

@guilhermelawless guilhermelawless added documentation This item indicates the need for or supplies updated or expanded documentation networking beta testing wanted labels Feb 5, 2020
@guilhermelawless guilhermelawless added this to the V21.0 milestone Feb 5, 2020
@guilhermelawless guilhermelawless self-assigned this Feb 5, 2020
cryptocode
cryptocode previously approved these changes Feb 6, 2020
Copy link
Contributor

@cryptocode cryptocode left a comment

Choose a reason for hiding this comment

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

Looks good, and sanitizers happy

@guilhermelawless guilhermelawless changed the title Network filter for publish and confirm_ack messages. Network filter for publish and confirm_ack messages Feb 6, 2020
clemahieu
clemahieu previously approved these changes Feb 6, 2020
Copy link
Contributor

@clemahieu clemahieu left a comment

Choose a reason for hiding this comment

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

Looks good after the fix for the stack referenc.

nano/node/active_transactions.cpp Outdated Show resolved Hide resolved
cryptocode
cryptocode previously approved these changes Feb 6, 2020
@Srayman
Copy link
Contributor

Srayman commented Feb 8, 2020

If a publish message is dropped when the block processor is full, should it be removed from the filter so that it can try to process it the next time it's seen? How would this impact bootstrapping the block if the publish message is dropped but the filter still contains the block?

See this line.
https://github.com/guilhermelawless/nano-node/blob/b5f82d75ed5664ecad4016782ebde72780fe542f/nano/node/network.cpp#L446

@guilhermelawless
Copy link
Contributor Author

That's absolutely missing, thank you @Srayman .

@guilhermelawless guilhermelawless force-pushed the stream-filter branch 3 times, most recently from 4c0e7bb to 84af7de Compare February 10, 2020 08:08
@guilhermelawless guilhermelawless changed the title Network filter for publish and confirm_ack messages WIP: Network filter for publish and confirm_ack messages Feb 11, 2020
@guilhermelawless guilhermelawless force-pushed the stream-filter branch 2 times, most recently from d163eaf to 8ba2d4e Compare February 13, 2020 18:15
@guilhermelawless guilhermelawless changed the title Network filter for publish and confirm_ack messages WIP: Network filter for publish and confirm_ack messages Feb 20, 2020
@guilhermelawless guilhermelawless removed the documentation This item indicates the need for or supplies updated or expanded documentation label Mar 3, 2020
…hey are deserialized.

When they are unique, the digest is saved and passed around to network processing which may drop the messages if the block or vote processor is full.

After processing, there are instances where digests have to be cleared from the filter to ensure the original message can be received again:
- Cleaning up a long unchecked block erases that element from the publish filter.
- Reaching max capacity of the inactive votes cache clears the oldest item from the cache and the confirm_ack filter. For this reason, inactive_votes_cache now stores the digests, and votes pulled from this cache for new elections are now erased.
- Interactions between bootstrap frontier confirmation and inactive_votes_cache
- An active election is dropped. The vote digest is now saved here as well. Special case if a vote came from inactive_votes_cache, must ensure the digest is passed along.

Many tests were added to ensure proper clearing of the filter when blocks and votes are dropped.

The blocks_filter has been removed due to redundancy. 16 extra bytes are used for each vote in an (inactive) vote stored in memory.
@guilhermelawless guilhermelawless changed the title WIP: Network filter for publish and confirm_ack messages Network filter for publish and confirm_ack messages Mar 3, 2020
cryptocode
cryptocode previously approved these changes Mar 3, 2020
SergiySW
SergiySW previously approved these changes Mar 4, 2020
@guilhermelawless guilhermelawless dismissed stale reviews from SergiySW and cryptocode via 2ee598b March 4, 2020 21:06
cryptocode
cryptocode previously approved these changes Mar 4, 2020
cryptocode
cryptocode previously approved these changes Mar 5, 2020
cryptocode
cryptocode previously approved these changes Mar 5, 2020
This reverts commit c56e0ce. Not necessary as the vote goes into inactive votes cache and can later be removed if necessary.
@guilhermelawless
Copy link
Contributor Author

Moving with #2643 in V21, filtering votes is left for a future version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
networking performance Performance/resource utilization improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants