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

Remove hybrid PoA block trigger mode #1232

Merged
merged 2 commits into from
Jun 29, 2023
Merged

Remove hybrid PoA block trigger mode #1232

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
187f17e
Remove hybrid PoA block trigger mode
Dentosal Jun 28, 2023
d2be7d2
Merge branch 'master' into dento/remove-hybrid-mode
Dentosal Jun 29, 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
63 changes: 2 additions & 61 deletions bin/fuel-core/src/cli/run/consensus.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,28 +12,13 @@ pub struct PoATriggerArgs {
#[clap(flatten)]
instant: Instant,
#[clap(flatten)]
hybrid: Hybrid,
#[clap(flatten)]
interval: Interval,
}

// Convert from arg struct to PoATrigger enum
impl From<PoATriggerArgs> for PoATrigger {
fn from(value: PoATriggerArgs) -> Self {
match value {
PoATriggerArgs {
hybrid:
Hybrid {
idle_time: Some(idle_time),
min_time: Some(min_time),
max_time: Some(max_time),
},
..
} => PoATrigger::Hybrid {
min_block_time: min_time.into(),
max_tx_idle_time: idle_time.into(),
max_block_time: max_time.into(),
},
PoATriggerArgs {
interval: Interval { period: Some(p) },
..
Expand All @@ -50,7 +35,7 @@ impl From<PoATriggerArgs> for PoATrigger {

#[derive(Debug, Clone, clap::Args)]
#[clap(
group = ArgGroup::new("instant-mode").args(&["instant"]).conflicts_with_all(&["interval-mode", "hybrid-mode"]),
group = ArgGroup::new("instant-mode").args(&["instant"]).conflicts_with_all(&["interval-mode"]),
)]
struct Instant {
/// Use instant block production mode.
Expand All @@ -68,39 +53,7 @@ enum Boolean {

#[derive(Debug, Clone, clap::Args)]
#[clap(
group = ArgGroup::new("hybrid-mode")
.args(&["min_time", "idle_time", "max_time"])
.multiple(true)
.conflicts_with_all(&["interval-mode", "instant-mode"]),
)]
struct Hybrid {
/// Hybrid trigger option.
/// Sets a minimum lower bound between blocks. This should be set high enough to ensure
/// peers can sync the blockchain.
/// Cannot be combined with other poa mode options (instant or interval).
#[arg(long = "poa-hybrid-min-time", requires_all = ["idle_time", "max_time"], env)]
min_time: Option<Duration>,
/// Hybrid trigger option.
/// Sets the max time block production will wait after a period of inactivity before producing
/// a new block. If there are txs available but not enough for a full block,
/// this is how long the trigger will wait for more txs.
/// This ensures that if a burst of transactions are submitted,
/// they will all be included into the next block instead of making a new block immediately and
/// then waiting for the minimum block time to process the rest.
/// Cannot be combined with other poa mode options (instant or interval).
#[arg(long = "poa-hybrid-idle-time", requires_all = ["min_time", "max_time"], env)]
idle_time: Option<Duration>,
/// Hybrid trigger option.
/// Sets the maximum time block production will wait to produce a block (even if empty). This
/// ensures that there is a regular cadence even under sustained load.
/// Cannot be combined with other poa mode options (instant or interval).
#[arg(long = "poa-hybrid-max-time", requires_all = ["min_time", "idle_time"], env)]
max_time: Option<Duration>,
}

#[derive(Debug, Clone, clap::Args)]
#[clap(
group = ArgGroup::new("interval-mode").args(&["period"]).conflicts_with_all(&["instant-mode", "hybrid-mode"]),
group = ArgGroup::new("interval-mode").args(&["period"]).conflicts_with_all(&["instant-mode"]),
)]
struct Interval {
/// Interval trigger option.
Expand All @@ -124,22 +77,10 @@ mod tests {
trigger: PoATriggerArgs,
}

pub fn hybrid(min_t: u64, mi_t: u64, max_t: u64) -> Trigger {
Trigger::Hybrid {
min_block_time: StdDuration::from_secs(min_t),
max_tx_idle_time: StdDuration::from_secs(mi_t),
max_block_time: StdDuration::from_secs(max_t),
}
}

#[test_case(&[] => Ok(Trigger::Instant); "defaults to instant trigger")]
#[test_case(&["", "--poa-instant=false"] => Ok(Trigger::Never); "never trigger if instant is explicitly disabled")]
#[test_case(&["", "--poa-interval-period=1s"] => Ok(Trigger::Interval { block_time: StdDuration::from_secs(1)}); "uses interval mode if set")]
#[test_case(&["", "--poa-hybrid-min-time=1s", "--poa-hybrid-idle-time=2s", "--poa-hybrid-max-time=3s"] => Ok(hybrid(1,2,3)); "uses hybrid mode if set")]
#[test_case(&["", "--poa-interval-period=1s", "--poa-hybrid-min-time=1s", "--poa-hybrid-idle-time=2s", "--poa-hybrid-max-time=3s"] => Err(()); "can't set hybrid and interval at the same time")]
#[test_case(&["", "--poa-instant=true", "--poa-hybrid-min-time=1s", "--poa-hybrid-idle-time=2s", "--poa-hybrid-max-time=3s"] => Err(()); "can't set hybrid and instant at the same time")]
#[test_case(&["", "--poa-instant=true", "--poa-interval-period=1s"] => Err(()); "can't set interval and instant at the same time")]
#[test_case(&["", "--poa-hybrid-min-time=1s"] => Err(()); "can't set hybrid min time without idle and max")]
fn parse(args: &[&str]) -> Result<Trigger, ()> {
Command::try_parse_from(args)
.map_err(|_| ())
Expand Down
16 changes: 0 additions & 16 deletions crates/services/consensus_module/poa/src/config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,20 +42,4 @@ pub enum Trigger {
Never,
/// A new block is produced periodically. Used to simulate consensus block delay.
Interval { block_time: Duration },
/// A new block will be produced when the timer runs out.
/// Set to `max_block_time` when the txpool is empty, otherwise
/// `min(max_block_time, max_tx_idle_time)`. If it expires,
/// but minimum block time hasn't expired yet, then the deadline
/// is set to `last_block_created + min_block_time`.
/// See https://github.com/FuelLabs/fuel-core/issues/50#issuecomment-1241895887
/// Requires `min_block_time` <= `max_tx_idle_time` <= `max_block_time`.
Hybrid {
/// Minimum time between two blocks, even if there are more txs available
min_block_time: Duration,
/// If there are txs available, but not enough for a full block,
/// this is how long the block is waiting for more txs
max_tx_idle_time: Duration,
/// Time after which a new block is produced, even if it's empty
max_block_time: Duration,
},
}
68 changes: 1 addition & 67 deletions crates/services/consensus_module/poa/src/service.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,9 +228,6 @@ where
Trigger::Interval { block_time } => {
increase_time(self.last_timestamp, block_time)
}
Trigger::Hybrid { min_block_time, .. } => {
increase_time(self.last_timestamp, min_block_time)
}
},
RequestType::Trigger => {
let now = Tai64::now();
Expand Down Expand Up @@ -357,36 +354,6 @@ where
.set_deadline(last_block_created + block_time, OnConflict::Overwrite)
.await;
}
(
Trigger::Hybrid {
max_block_time,
min_block_time,
max_tx_idle_time,
},
RequestType::Trigger,
) => {
let consumable_gas = self.txpool.total_consumable_gas();

// If txpool still has more than a full block of transactions available,
// produce new block in min_block_time.
if consumable_gas > self.block_gas_limit {
self.timer
.set_timeout(min_block_time, OnConflict::Max)
.await;
} else if self.txpool.pending_number() > 0 {
// If we still have available txs, reduce the timeout to max idle time
self.timer
.set_timeout(max_tx_idle_time, OnConflict::Max)
.await;
} else {
self.timer
.set_timeout(max_block_time, OnConflict::Max)
.await;
}
}
(Trigger::Hybrid { .. }, RequestType::Manual) => {
unreachable!("Trigger types hybrid cannot be used with manual. This is enforced during config validation")
}
}

Ok(())
Expand All @@ -403,28 +370,6 @@ where
Ok(())
}
Trigger::Never | Trigger::Interval { .. } => Ok(()),
Trigger::Hybrid {
max_tx_idle_time,
min_block_time,
..
} => {
let consumable_gas = self.txpool.total_consumable_gas();

// If we have over one full block of transactions and min_block_time
// has expired, start block production immediately
if consumable_gas > self.block_gas_limit
&& self.last_block_created + min_block_time < Instant::now()
{
self.produce_next_block().await?;
} else if self.txpool.pending_number() > 0 {
// We have at least one transaction, so tx_max_idle_time is the limit
self.timer
.set_timeout(max_tx_idle_time, OnConflict::Min)
.await;
}

Ok(())
}
}
}

Expand All @@ -434,13 +379,7 @@ where
unreachable!("Timer is never set in this mode");
}
// In the Interval mode the timer expires only when a new block should be created.
// In the Hybrid mode the timer can be either:
// 1. min_block_time expired after it was set when a block
// would have been produced too soon
// 2. max_tx_idle_time expired after a tx has arrived
// 3. max_block_time expired
// => we produce a new block in any case
Trigger::Interval { .. } | Trigger::Hybrid { .. } => {
Trigger::Interval { .. } => {
self.produce_next_block().await?;
Ok(())
}
Expand Down Expand Up @@ -477,11 +416,6 @@ where
.set_timeout(block_time, OnConflict::Overwrite)
.await;
}
Trigger::Hybrid { max_block_time, .. } => {
self.timer
.set_timeout(max_block_time, OnConflict::Overwrite)
.await;
}
};

Ok(self)
Expand Down
64 changes: 0 additions & 64 deletions crates/services/consensus_module/poa/src/service_test.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ use fuel_core_types::{
primitives::SecretKeyWrapper,
SealedBlock,
},
fuel_asm::*,
fuel_crypto::SecretKey,
fuel_tx::{
field::GasLimit,
Expand Down Expand Up @@ -387,69 +386,6 @@ async fn does_not_produce_when_txpool_empty_in_instant_mode() {
task.on_txpool_event().await.unwrap();
}

#[tokio::test(start_paused = true)]
async fn hybrid_production_doesnt_produce_empty_blocks_when_txpool_is_empty() {
// verify the PoA service doesn't alter the hybrid block timing when
// receiving txpool events if txpool is actually empty
let mut rng = StdRng::seed_from_u64(2322);
let secret_key = SecretKey::random(&mut rng);

const TX_IDLE_TIME_MS: u64 = 50u64;

let mut block_producer = MockBlockProducer::default();

block_producer
.expect_produce_and_execute_block()
.returning(|_, _, _| panic!("Block production should not be called"));

let mut block_importer = MockBlockImporter::default();

block_importer
.expect_commit_result()
.returning(|_| panic!("Block importer should not be called"));

block_importer
.expect_block_stream()
.returning(|| Box::pin(tokio_stream::pending()));

let mut txpool = MockTransactionPool::no_tx_updates();
txpool.expect_total_consumable_gas().returning(|| 0);
txpool.expect_pending_number().returning(|| 0);

let config = Config {
trigger: Trigger::Hybrid {
min_block_time: Duration::from_millis(100),
max_tx_idle_time: Duration::from_millis(TX_IDLE_TIME_MS),
max_block_time: Duration::from_millis(1000),
},
block_gas_limit: 1000000,
signing_key: Some(Secret::new(secret_key.into())),
metrics: false,
..Default::default()
};

let p2p_port = generate_p2p_port();

let task = MainTask::new(
&BlockHeader::new_block(BlockHeight::from(1u32), Tai64::now()),
config,
txpool,
block_producer,
block_importer,
p2p_port,
);

let service = Service::new(task);
service.start_and_await().await.unwrap();

// wait max_tx_idle_time - causes block production to occur if
// pending txs > 0 is not checked.
time::sleep(Duration::from_millis(TX_IDLE_TIME_MS)).await;

service.stop_and_await().await.unwrap();
assert!(service.state().stopped());
}

fn test_signing_key() -> Secret<SecretKeyWrapper> {
let mut rng = StdRng::seed_from_u64(0);
let secret_key = SecretKey::random(&mut rng);
Expand Down
Loading
Loading