Remove deadline clock in POA and replace with tokio time functions. #2109

AurelienFT · 2024-08-20T08:40:07Z

Linked Issues/PRs

Closes #1917

Description

This PR removes the DeadlineClock structure and his usage in POA. It's replaced by a mechanism using Tokio features.
This changes avoid the service to hangs up on an error, it will now wait and retry a block production

Checklist

Breaking changes are clearly marked as such in the PR description and changelog
New behavior is reflected in tests
The specification matches the implemented behavior (link update PR if changes are needed)

Before requesting review

I have reviewed the code myself
I have created follow-up issues caused by this PR and linked them here

After merging, notify other teams

Dentosal

Good cleanup. Removes code and adds tests. LGTM.

crates/services/consensus_module/poa/src/service.rs

crates/services/consensus_module/poa/src/service_test/trigger_tests.rs

Rewrite test to avoid being time based. Remove reset of `last_block_created` in `into_task` of POA

crates/services/consensus_module/poa/src/service_test/trigger_tests.rs

MitchTurner · 2024-08-20T15:28:19Z

crates/services/consensus_module/poa/src/service_test/trigger_tests.rs

@@ -194,6 +200,60 @@ async fn interval_trigger_produces_blocks_periodically() -> anyhow::Result<()> {
    Ok(())
 }

+// In case of error during block creation, the block should be retried after a delay of 1 second.
+#[tokio::test(start_paused = true)]
+async fn retry_block_creation_in_case_error() -> anyhow::Result<()> {


I know the other tests in this file don't do this yet, but can we add a given/when/then to clearly mark the SUT, the conditions, and the expected result?

It's not super clear here anyway, since we don't get to handle the service directly (which I would assume is the SUT here). But at least say:
given: a service that just failed to commit a block
when: we wait for 2 seconds
then: the service succeeds at committing a block

Reorganized it.

MitchTurner · 2024-08-20T15:35:48Z

crates/services/consensus_module/poa/src/service_test/trigger_tests.rs

@@ -194,6 +200,60 @@ async fn interval_trigger_produces_blocks_periodically() -> anyhow::Result<()> {
    Ok(())
 }

+// In case of error during block creation, the block should be retried after a delay of 1 second.
+#[tokio::test(start_paused = true)]
+async fn retry_block_creation_in_case_error() -> anyhow::Result<()> {


[nit]: I don't think all the comments are necessary.

For example, the comment above the test name could just be the test name. I don't mind verbose test names:

async fn service__if_commit_result_fails_then_retry_commit_result_after_trigger_duration

I think the comments above the expect_commit_results are good. But a lot of the other comments are redundant.

Ok, I removed the others comments !

@mmyyrroonn

## Version v0.34.0 ### Added - [2051](#2051): Add support for AWS KMS signing for the PoA consensus module. The new key can be specified with `--consensus-aws-kms AWS_KEY_ARN`. - [2092](#2092): Allow iterating by keys in rocksdb, and other storages. - [2096](#2096): GraphQL endpoint to fetch blob byte code by its blob ID. ### Changed - [2106](#2106): Remove deadline clock in POA and replace with tokio time functions. #### Breaking - [2051](#2051): Misdocumented `CONSENSUS_KEY` environ variable has been removed, use `CONSENSUS_KEY_SECRET` instead. Also raises MSRV to `1.79.0`. ### Fixed - [2106](#2106): Handle the case when nodes with overriding start on the fresh network. - [2105](#2105): Fixed the rollback functionality to work with empty gas price database. ## What's Changed * doc: refine the transaction example in the README by @mmyyrroonn in #2072 * AWS KMS block signing support and Rust 1.79 by @Dentosal in #2051 * feat(iterators): allow key-only iteration by @rymnc in #2092 * feat: graphql endpoint to fetch the blob byte code by its blob ID by @netrome in #2096 * Small improvements for tests to make them more stable by @xgreenx in #2103 * Fixed the rollback functionality to work with empty gas price database by @xgreenx in #2105 * Bump wasmtime version by @Dentosal in #2089 * Handle the case when nodes with overriding start on the fresh network by @xgreenx in #2106 * Remove deadline clock in POA and replace with tokio time functions. by @AurelienFT in #2109 ## New Contributors * @mmyyrroonn made their first contribution in #2072 **Full Changelog**: v0.33.0...v0.34.0

@mmyyrroonn

## Version v0.34.0 ### Added - [2051](FuelLabs/fuel-core#2051): Add support for AWS KMS signing for the PoA consensus module. The new key can be specified with `--consensus-aws-kms AWS_KEY_ARN`. - [2092](FuelLabs/fuel-core#2092): Allow iterating by keys in rocksdb, and other storages. - [2096](FuelLabs/fuel-core#2096): GraphQL endpoint to fetch blob byte code by its blob ID. ### Changed - [2106](FuelLabs/fuel-core#2106): Remove deadline clock in POA and replace with tokio time functions. #### Breaking - [2051](FuelLabs/fuel-core#2051): Misdocumented `CONSENSUS_KEY` environ variable has been removed, use `CONSENSUS_KEY_SECRET` instead. Also raises MSRV to `1.79.0`. ### Fixed - [2106](FuelLabs/fuel-core#2106): Handle the case when nodes with overriding start on the fresh network. - [2105](FuelLabs/fuel-core#2105): Fixed the rollback functionality to work with empty gas price database. ## What's Changed * doc: refine the transaction example in the README by @mmyyrroonn in FuelLabs/fuel-core#2072 * AWS KMS block signing support and Rust 1.79 by @Dentosal in FuelLabs/fuel-core#2051 * feat(iterators): allow key-only iteration by @rymnc in FuelLabs/fuel-core#2092 * feat: graphql endpoint to fetch the blob byte code by its blob ID by @netrome in FuelLabs/fuel-core#2096 * Small improvements for tests to make them more stable by @xgreenx in FuelLabs/fuel-core#2103 * Fixed the rollback functionality to work with empty gas price database by @xgreenx in FuelLabs/fuel-core#2105 * Bump wasmtime version by @Dentosal in FuelLabs/fuel-core#2089 * Handle the case when nodes with overriding start on the fresh network by @xgreenx in FuelLabs/fuel-core#2106 * Remove deadline clock in POA and replace with tokio time functions. by @AurelienFT in FuelLabs/fuel-core#2109 ## New Contributors * @mmyyrroonn made their first contribution in FuelLabs/fuel-core#2072 **Full Changelog**: FuelLabs/fuel-core@v0.33.0...v0.34.0

Remove deadline clock in POA and replace with tokio time functions.

3845ce1

AurelienFT requested review from xgreenx and Dentosal August 20, 2024 08:40

Dentosal previously approved these changes Aug 20, 2024

View reviewed changes

Merge branch 'master' into remove_deadline_clock_poa

9b735ef

xgreenx reviewed Aug 20, 2024

View reviewed changes

crates/services/consensus_module/poa/src/service.rs Show resolved Hide resolved

xgreenx reviewed Aug 20, 2024

View reviewed changes

crates/services/consensus_module/poa/src/service_test/trigger_tests.rs Outdated Show resolved Hide resolved

AurelienFT and others added 2 commits August 20, 2024 11:24

Merge branch 'master' into remove_deadline_clock_poa

dc2d48d

Remove panic in case signature fails.

15d2779

Rewrite test to avoid being time based. Remove reset of `last_block_created` in `into_task` of POA

AurelienFT dismissed Dentosal’s stale review via 15d2779 August 20, 2024 10:46

Merge branch 'master' into remove_deadline_clock_poa

ca8d358

AurelienFT requested review from Dentosal and xgreenx August 20, 2024 10:47

AurelienFT self-assigned this Aug 20, 2024

xgreenx previously approved these changes Aug 20, 2024

View reviewed changes

xgreenx and others added 3 commits August 20, 2024 14:39

Merge branch 'master' into remove_deadline_clock_poa

9c9a60a

Merge branch 'master' into remove_deadline_clock_poa

f4caa24

Fixed the off chain worker to subscribe for event on creation

220dc30

xgreenx dismissed their stale review via 220dc30 August 20, 2024 15:15

Add update of last_block_created in POA into_task

89c0dcd

AurelienFT requested a review from xgreenx August 20, 2024 15:19

MitchTurner reviewed Aug 20, 2024

View reviewed changes

xgreenx and others added 3 commits August 20, 2024 17:48

Fix compilation

afcf1b4

Merge branch 'master' into remove_deadline_clock_poa

77eb09c

Rename test of block production retry and reorganize it.

69db97d

xgreenx previously approved these changes Aug 20, 2024

View reviewed changes

Format the test

bccf923

AurelienFT dismissed xgreenx’s stale review via bccf923 August 20, 2024 16:07

xgreenx approved these changes Aug 20, 2024

View reviewed changes

MitchTurner approved these changes Aug 20, 2024

View reviewed changes

xgreenx enabled auto-merge (squash) August 20, 2024 16:09

xgreenx merged commit 2caca61 into master Aug 20, 2024
32 checks passed

xgreenx deleted the remove_deadline_clock_poa branch August 20, 2024 16:24

xgreenx mentioned this pull request Aug 20, 2024

Release v0.34.0 #2117

Merged

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove deadline clock in POA and replace with tokio time functions. #2109

Remove deadline clock in POA and replace with tokio time functions. #2109

AurelienFT commented Aug 20, 2024 •

edited by xgreenx

Loading

Dentosal left a comment

MitchTurner Aug 20, 2024

AurelienFT Aug 20, 2024

MitchTurner Aug 20, 2024

AurelienFT Aug 20, 2024

Remove deadline clock in POA and replace with tokio time functions. #2109

Remove deadline clock in POA and replace with tokio time functions. #2109

Conversation

AurelienFT commented Aug 20, 2024 • edited by xgreenx Loading

Linked Issues/PRs

Description

Checklist

Before requesting review

After merging, notify other teams

Dentosal left a comment

Choose a reason for hiding this comment

MitchTurner Aug 20, 2024

Choose a reason for hiding this comment

AurelienFT Aug 20, 2024

Choose a reason for hiding this comment

MitchTurner Aug 20, 2024

Choose a reason for hiding this comment

AurelienFT Aug 20, 2024

Choose a reason for hiding this comment

AurelienFT commented Aug 20, 2024 •

edited by xgreenx

Loading