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

Failure limit on tx proposals #3296

Open
wants to merge 50 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 49 commits
Commits
Show all changes
50 commits
Select commit Hold shift + click to select a range
1e52e1b
Track failed tx proposals in consensus
NotGyro Mar 8, 2023
9e7465b
Push an Instant::now() for every time validation fails on a tx proposal
NotGyro Mar 9, 2023
94f99d9
Fix unnecessary .clone()
NotGyro Mar 14, 2023
8b2b437
Cargo fmt
NotGyro Mar 14, 2023
d818079
Return to tracking clients as soon as they submit a proposeTX, per Ja…
NotGyro Mar 27, 2023
cf0b095
Cargo fmt
NotGyro Mar 27, 2023
9016e8e
Failure limit on tx proposals
NotGyro Mar 28, 2023
c4c9fb2
Cargo fmt.
NotGyro Mar 30, 2023
91bf8cd
Actually close sessions with problem clients
NotGyro Mar 30, 2023
ed3ecfe
Cargo fmt
NotGyro Mar 30, 2023
df61256
Adding a comment
NotGyro Mar 30, 2023
ee2c475
Cargo fmt
NotGyro Mar 30, 2023
72cd0d5
Test ensuring client_close() is called when limit is hit
NotGyro Apr 1, 2023
232f149
Cargo fmt
NotGyro Apr 1, 2023
a32f637
Fixes and improvements to tx proposal ratelimiting system.
NotGyro Apr 4, 2023
3ee8518
cargo clippy --fix
NotGyro Apr 4, 2023
8dcdf8d
Track failed tx proposals in consensus
NotGyro Mar 8, 2023
f4e80eb
Push an Instant::now() for every time validation fails on a tx proposal
NotGyro Mar 9, 2023
0d2ab75
Fix unnecessary .clone()
NotGyro Mar 14, 2023
9e557dd
Cargo fmt
NotGyro Mar 14, 2023
c342f6c
Return to tracking clients as soon as they submit a proposeTX, per Ja…
NotGyro Mar 27, 2023
ecd5836
Cargo fmt
NotGyro Mar 27, 2023
93d6030
Apply suggestions from code review
NotGyro Apr 7, 2023
6b3861f
Respond to suggestions on the pull request, no longer using reference…
NotGyro Apr 7, 2023
ee9ec68
Merge in suggested changes
NotGyro Apr 7, 2023
bf6b2d3
chore(deps): bump syn from 1.0.109 to 2.0.11 (#3295)
dependabot[bot] Mar 30, 2023
429d3ea
Update log messages (#3301)
varsha888 Apr 3, 2023
b130c4a
move transaction summary computation to separate crate (#3132)
ryankurte Apr 4, 2023
31455af
include necessary optional dep when the serde feature is enabled (#3303)
eranrund Apr 5, 2023
3492fff
Merge in suggested changes
NotGyro Apr 7, 2023
be4fd13
cargo fmt
NotGyro Apr 7, 2023
c9ecddf
Merge branch 'master' into milliec/03-07-Track_failed_tx_proposals_in…
NotGyro Apr 7, 2023
7651276
Refactor out a shared compute authenticated sender and computer desti…
wjuan-mob Apr 6, 2023
c1014d0
chore(deps): bump serde from 1.0.154 to 1.0.159 (#3294)
dependabot[bot] Apr 6, 2023
f618719
Failure limit on tx proposals
NotGyro Mar 28, 2023
9749e59
Actually close sessions with problem clients
NotGyro Mar 30, 2023
c1c00c6
Fixes within the process of rebasing
NotGyro Apr 7, 2023
2ed4c51
Merge branch 'milliec/03-07-Track_failed_tx_proposals_in_consensus' i…
NotGyro Apr 7, 2023
6c5f429
Merge branch 'master' into milliec/03-28-Failure_limit_on_tx_proposals
NotGyro Apr 8, 2023
81c621d
cargo fmt
NotGyro Apr 10, 2023
cf078af
chore(deps): bump bitflags from 1.3.2 to 2.0.1 (#3250)
dependabot[bot] Apr 10, 2023
766fa33
refactor mobilecoind api to not depend on consensus-api (#3307)
cbeck88 Apr 10, 2023
94a7b15
unforked diesel and updated necessary code (#3304)
briancorbin Apr 10, 2023
9562735
Failure limit on tx proposals
NotGyro Mar 28, 2023
5bd1b98
Cargo fmt.
NotGyro Mar 30, 2023
2b0c4ef
Actually close sessions with problem clients
NotGyro Mar 30, 2023
67c2284
Push an Instant::now() for every time validation fails on a tx proposal
NotGyro Mar 9, 2023
65bd6ad
Cargo fmt
NotGyro Mar 14, 2023
7be7728
Respond to suggestions on the pull request, no longer using reference…
NotGyro Apr 7, 2023
23a168e
Merge branch 'master' into milliec/03-28-Failure_limit_on_tx_proposals
NotGyro Apr 11, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,12 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).

The crates in this repository do not adhere to [Semantic Versioning](https://semver.org/spec/v2.0.0.html) at this time.

## [5.0.0]

### Changed

- mobilecoind now has its own version of the `LastBlockInfo` proto message. ([#3307])

## [4.1.0]

### Added
Expand Down
38 changes: 22 additions & 16 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 0 additions & 3 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -242,8 +242,5 @@ schnorrkel-og = { git = "https://github.com/mobilecoinfoundation/schnorrkel.git"
# See: https://github.com/pyfisch/cbor/pull/198
serde_cbor = { git = "https://github.com/mobilecoinofficial/cbor", rev = "4c886a7c1d523aae1ec4aa7386f402cb2f4341b5" }

# Override diesel dependency with our fork, to statically link SQLite.
diesel = { git = "https://github.com/mobilecoinofficial/diesel", rev = "026f6379715d27c8be48396e5ca9059f4a263198" }

# Fix issues with recent nightlies, bump curve25519-dalek version
x25519-dalek = { git = "https://github.com/mobilecoinfoundation/x25519-dalek.git", rev = "4fbaa3343301c62cfdbc3023c9f485257e6b718a" }
2 changes: 1 addition & 1 deletion attest/core/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ mc-util-encodings = { path = "../../util/encodings" }
mc-util-repr-bytes = { path = "../../util/repr-bytes", features = ["hex_fmt"] }

base64 = { version = "0.21", default-features = false, features = ["alloc"] }
bitflags = "1.3"
bitflags = { version = "2.0", default-features = false, features = ["serde"] }
chrono = { version = "0.4.24", default-features = false, features = ["alloc"] }
digest = "0.10"
displaydoc = { version = "0.2", default-features = false }
Expand Down
2 changes: 1 addition & 1 deletion attest/core/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -390,7 +390,7 @@ pub enum ReportDetailsError {

bitflags! {
/// Revocation cause flags
#[derive(Deserialize, Serialize)]
#[derive(Clone, Debug, Deserialize, Eq, Hash, Ord, PartialEq, PartialOrd, Serialize)]
pub struct RevocationCause: u64 {
/// Cause reason was not given (but still revoked)
const UNSPECIFIED = 0;
Expand Down
1 change: 1 addition & 0 deletions attest/core/src/ias/verify.rs
Original file line number Diff line number Diff line change
Expand Up @@ -336,6 +336,7 @@ impl<'src> TryFrom<&'src VerificationReport> for VerificationReportData {
"SIGNATURE_INVALID" => Err(IasQuoteError::SignatureInvalid),
"GROUP_REVOKED" => Err(IasQuoteError::GroupRevoked(
revocation_reason
.clone()
.ok_or_else(|| JsonError::FieldMissing("revocationReason".to_string()))?,
platform_info_blob
.ok_or_else(|| JsonError::FieldMissing("platformInfoBlob".to_string()))?,
Expand Down
20 changes: 20 additions & 0 deletions consensus/api/proto/consensus_config.proto
Original file line number Diff line number Diff line change
Expand Up @@ -69,4 +69,24 @@ message ConsensusNodeConfig {

// SCP message signing key.
external.Ed25519Public scp_message_signing_key = 8;

// Maximum number of client session tracking structures to retain in
// a least-recently-used cache.
Copy link
Collaborator

Choose a reason for hiding this comment

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

🙃 Don't think "least recently used" is a hyphenated word

Suggest looking at my comment on "denial-of-service" before deciding

Suggested change
// a least-recently-used cache.
// a least recently used cache.

//
// This corresponds to Config::client_tracking_capacity
uint64 client_tracking_capacity = 9;
Copy link
Collaborator

Choose a reason for hiding this comment

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

❓ I don't think it matters as they're all dynamically sized, but why uint64 here? Considering tx_failure_limit is uint32


// How many seconds to retain instances of proposed-transaction
// failures, per-client. This is used to implement DOS-protection,
// protecting against clients who make too many failed
// transaction proposals within this span.
uint64 tx_failure_window_seconds = 10;
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍 Like the call out of the units in the name since there is no type safety.


// How many tx proposal failures within the rolling window are required
// before it's treated as concerning, thereby tripping denial-of-service
Copy link
Collaborator

Choose a reason for hiding this comment

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

💭 interestingly "denial-of-service" is often hyphenated on the interwebs. It looks like the rules are pretty loose when to hyphenate, some say since "denial-of-service" is a compound adjective it can be hyphenated.

// protection?
// In other words, how many failed transaction proposals are permitted
// within the last tx_failure_window_seconds before a user is
// disconnected or temporarily blocked?
uint32 tx_failure_limit = 11;
}
15 changes: 12 additions & 3 deletions consensus/enclave/trusted/Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

22 changes: 22 additions & 0 deletions consensus/service/config/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,24 @@ pub struct Config {
/// config setting to match.
#[clap(long, default_value = "10000", env = "MC_CLIENT_TRACKING_CAPACITY")]
pub client_tracking_capacity: usize,

/// How many seconds to retain instances of proposed-transaction
/// failures, per-client. This is used to implement DOS-protection,
/// along the lines of kicking clients who make too many failed transaction
/// proposals within the span of tx_failure_window
// TODO: slam-testing to derive reasonable default
#[clap(long, default_value = "30", value_parser = parse_duration_in_seconds, env = "MC_CLIENT_TX_FAILURE_WINDOW")]
pub tx_failure_window: Duration,

/// How many tx proposal failures within the rolling window are required
/// before it's treated as concerning, thereby tripping denial-of-service
/// protection?
/// In other words, how many failed transaction proposals are permitted
/// within the last tx_failure_window seconds before a user is
/// disconnected or temporarily blocked?
// TODO: slam-testing to derive reasonable default
#[clap(long, default_value = "16384", env = "MC_CLIENT_TX_FAILURE_LIMIT")]
pub tx_failure_limit: u32,
}

impl Config {
Expand Down Expand Up @@ -224,6 +242,8 @@ mod tests {
tokens_path: None,
block_version: BlockVersion::ZERO,
client_tracking_capacity: 4096,
tx_failure_window: Duration::from_secs(30),
tx_failure_limit: 16384,
};

assert_eq!(
Expand Down Expand Up @@ -293,6 +313,8 @@ mod tests {
tokens_path: None,
block_version: BlockVersion::ZERO,
client_tracking_capacity: 4096,
tx_failure_window: Duration::from_secs(30),
tx_failure_limit: 16384,
};

assert_eq!(
Expand Down
Loading