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

Jump to bottom

RPC error should result in a short ban #5874

Closed
SWvheerden opened this issue Oct 26, 2023 · 0 comments · Fixed by #5884
Closed

RPC error should result in a short ban #5874

SWvheerden opened this issue Oct 26, 2023 · 0 comments · Fixed by #5884
Assignees
@hansieodendaal
Labels
release-blocker Something that needs to be fixed before a release can be made

Comments

@SWvheerden
Copy link
Collaborator

RPC timeouts and the like should result in a short ban,
If these are not banned a malicious peer can use RPC errors to keep a node busy while blocking other calls.

For example, sync, incoming block etc are all time-sensitive and should be completed as soon as possible. A peer can delay these by using RPC error to fake intent without doing anything.
@SWvheerden SWvheerden added the release-blocker Something that needs to be fixed before a release can be made label Oct 26, 2023
@hansieodendaal hansieodendaal mentioned this issue Oct 30, 2023
Merged
4 tasks
SWvheerden pushed a commit that referenced this issue Oct 31, 2023
@hansieodendaal
feat: make prc errors ban-able for sync (#5884)
4ca664e 
Description
---
Made RPC errors ban-able during header-sync and horizon-sync ban-able
with a short ban duration.

Closes #5874

Motivation and Context
---
During sync, we establish a client-server connection and then execute
RPC methods from the client to the server, which we know should succeed,
so that any RPC errors afterwards are a banable offence. Something else
to consider is that sync only acts if better peer metadata is received
from a peer, translating to malicious behaviour should a peer not want
to play the protocol afterwards.

How Has This Been Tested?
---
Integration-level unit tests in `base_layer\core\tests\tests`:
- `block_sync.rs`
- `header_sync.rs`
- `horizon_sync.rs` (still in progress)

What process can a PR reviewer use to test or verify this change?
---
- Code walk-through
- Review unit tests ^^

<!-- Checklist -->
<!-- 1. Is the title of your PR in the form that would make nice release
notes? The title, excluding the conventional commit
tag, will be included exactly as is in the CHANGELOG, so please think
about it carefully. -->


Breaking Changes
---

- [x] None
- [ ] Requires data directory on base node to be deleted
- [ ] Requires hard fork
- [ ] Other - Please specify

<!-- Does this include a breaking change? If so, include this line as a
footer -->
<!-- BREAKING CHANGE: Description what the user should do, e.g. delete a
database, resync the chain -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@hansieodendaal hansieodendaal

Labels
release-blocker Something that needs to be fixed before a release can be made
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

feat: make prc errors ban-able for sync hansieodendaal/tari
2 participants
@SWvheerden @hansieodendaal