Skip to content

Commit

Permalink
Remove handle_certificate() shared transaction limit (MystenLabs#6587)
Browse files Browse the repository at this point in the history
Since all owned transactions go through Narwhal now, only rate limiting shared object transactions is insufficient to protect against Narwhal overload. Back pressure needs to be implemented inside Narwhal, and may need to propagate to signing / charging for owned object transactions. For now, just remove the shared transactions rate limit.

Also remove a few monitored scopes that were added incorrectly.
  • Loading branch information
mwtian authored Dec 5, 2022
1 parent 77ab658 commit 67625be
Show file tree
Hide file tree
Showing 5 changed files with 4 additions and 22 deletions.
4 changes: 1 addition & 3 deletions crates/sui-core/src/authority_active/execution_driver/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@

use std::{sync::Arc, time::Duration};

use mysten_metrics::{monitored_scope, spawn_monitored_task};
use mysten_metrics::spawn_monitored_task;
use prometheus::{
register_int_counter_with_registry, register_int_gauge_with_registry, IntCounter, IntGauge,
Registry,
Expand Down Expand Up @@ -80,8 +80,6 @@ where

// Loop whenever there is a signal that a new transactions is ready to process.
loop {
let _scope = monitored_scope("ExecutionDriver");

let certificate = if let Some(cert) = ready_certificates_stream.recv().await {
cert
} else {
Expand Down
10 changes: 0 additions & 10 deletions crates/sui-core/src/authority_server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,10 +48,6 @@ mod server_tests;
const MIN_BATCH_SIZE: u64 = 1000;
const MAX_DELAY_MILLIS: u64 = 5_000; // 5 sec

// Assuming 200 consensus tps * 5 sec consensus latency = 1000 inflight consensus txns.
// Leaving a bit more headroom to cap the max inflight consensus txns to 1000*2 = 2000.
const MAX_PENDING_CONSENSUS_TRANSACTIONS: u64 = 2000;

pub struct AuthorityServerHandle {
tx_cancellation: tokio::sync::oneshot::Sender<()>,
local_addr: Multiaddr,
Expand Down Expand Up @@ -405,12 +401,6 @@ impl ValidatorService {
// For owned objects this will return without waiting for certificate to be sequenced
// First do quick dirty non-async check
if !state.consensus_message_processed(&certificate)? {
// Note that num_inflight_transactions() only include user submitted transactions, and only user txns can be dropped here.
// This backpressure should not affect system transactions, e.g. for checkpointing.
if consensus_adapter.num_inflight_transactions() > MAX_PENDING_CONSENSUS_TRANSACTIONS {
return Err(tonic::Status::resource_exhausted("Reached {MAX_PENDING_CONSENSUS_TRANSACTIONS} concurrent consensus transactions",
));
}
let _metrics_guard = if shared_object_tx {
Some(metrics.consensus_latency.start_timer())
} else {
Expand Down
4 changes: 1 addition & 3 deletions narwhal/consensus/src/consensus.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use crate::{metrics::ConsensusMetrics, SequenceNumber};
use config::Committee;
use crypto::PublicKey;
use fastcrypto::hash::Hash;
use mysten_metrics::{monitored_scope, spawn_monitored_task};
use mysten_metrics::spawn_monitored_task;
use std::{
cmp::{max, Ordering},
collections::HashMap,
Expand Down Expand Up @@ -311,8 +311,6 @@ where
async fn run_inner(mut self) -> StoreResult<()> {
// Listen to incoming certificates.
loop {
let _scope = monitored_scope("NarwhalConsensus");

tokio::select! {
Some(certificate) = self.rx_new_certificates.recv() => {
// If the core already moved to the next epoch we should pull the next
Expand Down
4 changes: 1 addition & 3 deletions narwhal/primary/src/core.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ use crypto::{NetworkPublicKey, PublicKey, Signature};
use fastcrypto::{hash::Hash as _, SignatureService};
use futures::StreamExt;
use futures::{future::OptionFuture, stream::FuturesUnordered};
use mysten_metrics::{monitored_scope, spawn_monitored_task};
use mysten_metrics::spawn_monitored_task;
use network::{anemo_ext::NetworkExt, CancelOnDropHandler, P2pNetwork, ReliableNetwork};
use std::time::Duration;
use std::{collections::HashMap, sync::Arc, time::Instant};
Expand Down Expand Up @@ -671,8 +671,6 @@ impl Core {
pub async fn run(mut self) {
info!("Core on node {} has started successfully.", self.name);
loop {
let _scope = monitored_scope("NarwhalCore");

let result = tokio::select! {
Some((certificate, notify)) = self.rx_certificates.recv() => {
match self.sanitize_certificate(&certificate).await {
Expand Down
4 changes: 1 addition & 3 deletions narwhal/worker/src/batch_maker.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ use std::convert::TryInto;

use futures::{Future, StreamExt};

use mysten_metrics::{monitored_scope, spawn_monitored_task};
use mysten_metrics::spawn_monitored_task;
use std::sync::Arc;
use tokio::{
sync::watch,
Expand Down Expand Up @@ -110,8 +110,6 @@ impl BatchMaker {
let mut batch_pipeline = FuturesOrdered::new();

loop {
let _scope = monitored_scope("NarwhalBatchMaker");

tokio::select! {
// Assemble client transactions into batches of preset size.
// Note that transactions are only consumed when the number of batches
Expand Down

0 comments on commit 67625be

Please sign in to comment.