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

Track failed tx proposals in consensus #3208

Merged
merged 9 commits into from
Apr 8, 2023
Merged

Track failed tx proposals in consensus #3208

merged 9 commits into from
Apr 8, 2023

Conversation

NotGyro
Copy link
Contributor

@NotGyro NotGyro commented Mar 8, 2023

  • Adds a field, tx_proposal_failures to ClientSessionTracking
  • Implements the method fail_tx_proposal() on ClientSessionTracking
  • Invokes session.fail_tx_proposal(now) when a tx proposal is found to be invalid.

Motivation

This will be used to track how many failed tx proposals have been sent recently, so that bad connections can be dropped, to prevent attacks and harmful behavior from buggy clients. See: #2977

Future Work

  • Actually drop clients
  • Prometheus metrics related to this system.
  • Code to determine if a particular client / IP address needs to be blocked temporarily or indefinitely.

@NotGyro
Copy link
Contributor Author

NotGyro commented Mar 8, 2023 •
edited
Loading

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

@jcape jcape mentioned this pull request Mar 10, 2023
Open
3 tasks
@NotGyro NotGyro requested review from samdealy, jcape and cbeck88 and removed request for samdealy March 27, 2023 23:57
@NotGyro NotGyro force-pushed the milliec/03-07-Track_failed_tx_proposals_in_consensus branch from 08fe3f6 to cf0b095 Compare March 27, 2023 23:58
@NotGyro NotGyro marked this pull request as ready for review March 28, 2023 00:20
@NotGyro NotGyro added the consensus Related only to the consensus protocol or service label Mar 28, 2023
@NotGyro NotGyro self-assigned this Mar 28, 2023
@varsha888 varsha888 requested a review from awygle March 28, 2023 17:26
@NotGyro NotGyro mentioned this pull request Mar 28, 2023
Open
awygle
awygle approved these changes Mar 29, 2023
View reviewed changes
Copy link
Contributor

@awygle awygle left a comment

Choose a reason for hiding this comment

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

This seems fine as far as it goes. I assume this tracking will actually be used in a follow-on PR?

samdealy
samdealy approved these changes Mar 29, 2023
View reviewed changes
consensus/service/src/api/client_api_service.rs Outdated Show resolved Hide resolved
consensus/service/src/api/client_api_service.rs Outdated Show resolved Hide resolved
consensus/service/src/api/client_api_service.rs Outdated Show resolved Hide resolved
consensus/service/src/api/client_api_service.rs Show resolved Hide resolved
@NotGyro NotGyro force-pushed the milliec/03-07-Track_failed_tx_proposals_in_consensus branch from c9ecddf to 13376da Compare April 8, 2023 02:01
@NotGyro NotGyro merged commit 11a6767 into master Apr 8, 2023
@NotGyro NotGyro deleted the milliec/03-07-Track_failed_tx_proposals_in_consensus branch April 8, 2023 02:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@samdealy samdealy samdealy approved these changes

@awygle awygle awygle approved these changes

@jcape jcape Awaiting requested review from jcape

@cbeck88 cbeck88 Awaiting requested review from cbeck88

Assignees

@NotGyro NotGyro

Labels
consensus Related only to the consensus protocol or service
Projects
Core PRs
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@NotGyro @awygle @samdealy