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

fix(dht): check for empty body contents in initial msg validation #5123

Merged

Conversation

sdbondi
Copy link
Member

@sdbondi sdbondi commented Jan 17, 2023

Description

Discards messages with empty body contents in initial message validation

Motivation and Context

RFC-172 Stabalisation PR adds a rule that routing nodes should discard a message with empty body contents.

How Has This Been Tested?

New unit test

@CjS77 CjS77 added the P-acks_required Process - Requires more ACKs or utACKs label Jan 18, 2023
@stringhandler stringhandler merged commit 48bf2d9 into tari-project:development Jan 18, 2023
@sdbondi sdbondi deleted the dht-empty-body-validation branch January 24, 2023 04:57
stringhandler pushed a commit that referenced this pull request Jan 31, 2023
Description
---
Bans peers who send empty encrypted messages. Significantly updates tests to check for more failure modes and assert ban status for each.

Closes [issue 5132](#5132).

Motivation and Context
---
An [earlier PR](#5123) introduces an error when a peer sends an empty encrypted message, which is not allowed. However, the peer was not banned.

Further, [another PR](#5129) updates the handling of unsigned encrypted messages to ensure that bans are done correctly, but does not update tests to check for the bug that led to it.

This PR updates the banning logic to ban a peer who forwards an empty encrypted message, which is always detectable.

It also significantly refactors and updates tests. For each relevant high-level message failure mode, we test for proper error detection. We also check for the proper ban status of the forwarding peer.

How Has This Been Tested?
---
[Who tests the testers?](https://en.wikipedia.org/wiki/Quis_custodiet_ipsos_custodes%3F)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P-acks_required Process - Requires more ACKs or utACKs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants