Allow producer to produce predefined blocks #2081

MitchTurner · 2024-08-14T16:07:56Z

Linked Issues/PRs

Part of #1902

Description

Provide mechanism for taking predefined blocks and including them in block production.

Does not provide the mechanism for introducing those predefined blocks into the running system, e.g. parsing from file or something. That will come after.

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

[Add or remove entries as needed]

Rust SDK
Sway compiler
Platform documentation (for out-of-organization contributors, the person merging the PR will do this)
Someone else?

crates/services/producer/src/block_producer.rs

crates/services/producer/src/block_producer/tests.rs

Dentosal · 2024-08-16T11:33:05Z

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

                        }
+                        should_continue = true;
+                    } else {
+                        tracing::error!("The PoA task should be the holder of the `Sender`");


I don't understand this message here. If I read this correctly, this branch is taken iff the PoA task drops the channel, which happens only if it panics. Maybe "The PoA task panicked!" or something like that would be appropriate? Maybe this function should panic as well?

This comment was here before. I agree it's not clear.

crates/services/producer/src/block_producer.rs

Dentosal · 2024-08-16T11:40:32Z

The whole mechanism should be documented somehow, somewhere. Probably in doc comments in some relevant trait or function.

xgreenx · 2024-08-16T11:32:14Z

tests/tests/snapshot.rs

@@ -28,10 +28,10 @@ async fn loads_snapshot() {
    // setup config
    let starting_state = StateConfig {
        last_block: Some(LastBlockConfig {
-            block_height: (u32::MAX - 1).into(),
+            block_height: (u32::MAX - 2).into(),


Why do we need to change this file?

Because the code internally calls next_block now. If it's u32::Max (as it becomes) then next_block will panic.

Then test shouldn't produce any blocks, maybe we need to disable block producer

Adding Trigger::Never doesn't seem to fix the issue; the producer is still being run.

xgreenx · 2024-08-16T11:38:14Z

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

+        // Update last block time
+        self.last_height = *sealed_block.entity.header().height();
+        self.last_timestamp = sealed_block.entity.header().time();
+        self.last_block_created = last_block_created;


Because you don't set the timer here, I think after applying predefined blocks the server will halt in the case of Interval trigger more

Maybe we could somehow reuse main part of the produce_block function

It will be removed during #1917

xgreenx · 2024-08-16T11:41:04Z

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

+
+            for (tx_id, err) in skipped_transactions {
+                let maybe_tx = txs.iter().find(|tx| tx.id(chain_id) == tx_id);
+                if let Some(tx) = maybe_tx {


We should always be able to find the transaction. Maybe it is better to return an error

xgreenx · 2024-08-16T11:43:40Z

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

+            let txs = predefined_block.transactions();
+
+            for (tx_id, err) in skipped_transactions {
+                let maybe_tx = txs.iter().find(|tx| tx.id(chain_id) == tx_id);


It is very performance-heavy. It is better to iterate over all transactions in the block and check that it is in the hash map of failed transactions.

xgreenx · 2024-08-16T11:44:59Z

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

+                let maybe_tx = txs.iter().find(|tx| tx.id(chain_id) == tx_id);
+                if let Some(tx) = maybe_tx {
+                    tracing::error!(
+                "During block production got invalid transaction {:?} with error {:?}",


We agreed that in the case of discarded predefined transactions we want to print them in JSON format with some BEGIN {JSON_TX} END marker.

xgreenx · 2024-08-16T11:48:40Z

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

+        let maybe_block = self.predefined_blocks.get_block(&next_height);
+        if let Some(block) = maybe_block {
+            self.produce_predefined_block(&block, &chain_id).await?;
+            should_continue = true;


Could we please make an early return here? To avoid shifting a big part of the code into the if statement?

We can move the code below into the else { in a separate PR

xgreenx · 2024-08-16T11:50:45Z

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

+        (2u32.into(), block_for_height(2)),
+        (3u32.into(), block_for_height(3)),
+        (4u32.into(), block_for_height(4)),


Could we also test the case when we produce some blocks first form the interval, then we have predefined blocks, please?

xgreenx · 2024-08-16T11:51:42Z

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

+        let actual = block_receiver.recv().await.unwrap();
+        assert_eq!(expected, actual);
+    }
+    let maybe_produced_block = block_receiver.recv().await.unwrap();


Could this test also verify the production of the block after pre-defined blocks, please? Like 3 predefined blocks, and 1-3 non predefined(from interval)

…rror instead of ignore

Follow-up PR for #2081. Closes #1902 Add a CLI argument to process predefined blocks. ## Checklist - [x] New behavior is reflected in tests ### Before requesting review - [x] I have reviewed the code myself --------- Co-authored-by: Mitch Turner <james.mitchell.turner@gmail.com>

MitchTurner added 6 commits August 14, 2024 17:57

Create new port method for producing predefined blocks

84ccb2e

Fix test

c242410

Add other part of test

a79c3d2

Remove tracing stuff

062fdc9

Remove unnecessary drop

2d3e3da

Add test for producing predefined block at producer level

42ca09a

MitchTurner commented Aug 15, 2024

View reviewed changes

crates/services/producer/src/block_producer.rs Outdated Show resolved Hide resolved

MitchTurner added 9 commits August 15, 2024 16:57

WIP make more resilient

2216015

Fix dumb test

1d74d13

Report all skipped txs in predefined blocks

e88f96d

Move predefined blocks stuff to a port

1521563

Change Fake to simple impl

4c75879

Update CHANGELOG

7c5ffa5

Merge branch 'master' into feature/produce-predefined-blocks

b0b17ec

Fix compilation errors upstream

7133392

Fix broken test

3745ef9

MitchTurner commented Aug 15, 2024

View reviewed changes

crates/services/producer/src/block_producer.rs Outdated Show resolved Hide resolved

MitchTurner commented Aug 15, 2024

View reviewed changes

crates/services/producer/src/block_producer.rs Outdated Show resolved Hide resolved

MitchTurner commented Aug 15, 2024

View reviewed changes

crates/services/producer/src/block_producer/tests.rs Outdated Show resolved Hide resolved

MitchTurner added 4 commits August 16, 2024 12:39

Add tests for all the important fields for predefined blocks

c653643

Include randomized recipient contract ids

a1711ad

Merge branch 'master' into feature/produce-predefined-blocks

2b50d37

Remove regression

ea34fb1

MitchTurner marked this pull request as ready for review August 16, 2024 10:54

MitchTurner requested a review from a team August 16, 2024 10:54

MitchTurner self-assigned this Aug 16, 2024

MitchTurner added 2 commits August 16, 2024 13:00

Do not include mint tx in txs source

0254734

Appease Clippy-sama

48773ce

xgreenx mentioned this pull request Aug 16, 2024

Pass predefined blocks via the CLI #2094

Merged

2 tasks

Dentosal reviewed Aug 16, 2024

View reviewed changes

crates/services/producer/src/block_producer.rs Outdated Show resolved Hide resolved

xgreenx reviewed Aug 16, 2024

View reviewed changes

MitchTurner added 5 commits August 16, 2024 13:53

Include test for alternating predefined blocks

859bf50

Improve test, early return :(, improve skipped tx reporting, return e…

b1f3843

…rror instead of ignore

Fix import order, early return :(

ea26f63

Fix formatting

058fdc4

Add minor optimization

6a2deed

MitchTurner requested review from xgreenx and Dentosal August 16, 2024 13:07

Disable block production to avoid overflow

0d310e5

xgreenx approved these changes Aug 16, 2024

View reviewed changes

MitchTurner enabled auto-merge (squash) August 16, 2024 16:38

MitchTurner merged commit 9de2c08 into master Aug 16, 2024
27 of 28 checks passed

MitchTurner deleted the feature/produce-predefined-blocks branch August 16, 2024 17:19

xgreenx mentioned this pull request Aug 18, 2024

Release v0.33.0 #2099

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow producer to produce predefined blocks #2081

Allow producer to produce predefined blocks #2081

MitchTurner commented Aug 14, 2024 •

edited

Loading

Dentosal Aug 16, 2024

MitchTurner Aug 16, 2024

Dentosal commented Aug 16, 2024

xgreenx Aug 16, 2024

MitchTurner Aug 16, 2024

xgreenx Aug 16, 2024

MitchTurner Aug 16, 2024

xgreenx Aug 16, 2024

xgreenx Aug 16, 2024

xgreenx Aug 16, 2024

xgreenx Aug 16, 2024

xgreenx Aug 16, 2024

xgreenx Aug 16, 2024

xgreenx Aug 16, 2024

xgreenx Aug 16, 2024

xgreenx Aug 16, 2024

Allow producer to produce predefined blocks #2081

Allow producer to produce predefined blocks #2081

Conversation

MitchTurner commented Aug 14, 2024 • edited Loading

Linked Issues/PRs

Description

Checklist

Before requesting review

After merging, notify other teams

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dentosal commented Aug 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MitchTurner commented Aug 14, 2024 •

edited

Loading