feat(chain): block reconstruction #1650

andrussal · 2025-09-05T14:42:12Z

1. What does this PR implement?

Reconstructs block from Proposal. When receiving Proposal from network(which contains only transaction references), we need to retrieve transactions from mempool and construct full block. Spec

DON'T MEGRE TO MASTER UNTIL FULL FLOW WITH PROPOSALS IS WORKING

2. Does the code have enough context to be clearly understood?

Yes

3. Who are the specification authors and who is accountable for this PR?

@andrussal

4. Is the specification accurate and complete?

Yes

5. Does the implementation introduce changes in the specification?

Yes

Checklist

1. The PR title follows the Conventional Commits specification.
2. Description added.
3. Context and links to Specification document(s) added.
4. Main contact(s) (developers and specification authors) added
5. Implementation and Specification are 100% in sync including changes. This is critical.
6. Link PR to a specific milestone.

zeegomo

I think we should keep this in a separate branch until we can re-enable tests since those are critical and we need to be sure everything is working

zeegomo · 2025-09-09T10:02:03Z

nomos-core/chain-defs/src/block/mod.rs

 /// A block
+#[expect(
+    clippy::unsafe_derive_deserialize,
+    reason = "Temporary dummy signature"


is the signature supposed to be added to the block?

Now I added signature to block so we can construct Proposal and also store it. Please let me know if this is correct.

nomos-core/chain-defs/src/block/mod.rs

nomos-core/chain-defs/src/block/wire.rs

nomos-services/chain-service/src/lib.rs

tests/src/tests/cryptarchia/bootstrap.rs

nomos-services/chain-service/src/lib.rs

andrussal · 2025-09-10T08:06:36Z

I think we should keep this in a separate branch until we can re-enable tests since those are critical and we need to be sure everything is working

I created new branch where we maybe can merge meanwhile? feat/consensus-refactor-base

danielSanchezQ · 2025-09-17T11:32:32Z

@andrussal what is the status of this? If we need other stuff before, maybe lets move it to draft for the time being?

zeegomo

Good improvement! I left a few additional comments, we're close 🚀

zeegomo · 2025-09-23T13:45:32Z

nomos-core/chain-defs/src/block/mod.rs

 pub struct Block<Tx> {
    header: Header,
+    signature: ed25519_dalek::Signature,
+    service_reward: Option<Fr>,


we should probably resolve this to a Tx anyway

zeegomo · 2025-09-23T13:46:44Z

nomos-core/chain-defs/src/block/mod.rs

+    pub fn validate(&self) -> Result<(), Error>
+    where
+        Tx: Transaction,
+        Tx::Hash: Into<Fr>,
+    {
+        if !self.header.is_valid_bedrock_version() {
+            return Err(Error::Signing("Invalid header version".to_owned()));
+        }
+
+        if self.transactions.len() > MAX_TRANSACTIONS {
+            return Err(Error::Signing(format!(
+                "Too many transactions (max {MAX_TRANSACTIONS})"
+            )));
+        }
+
+        let leader_public_key = self.header.leader_proof().leader_key();
+        let header_bytes = <Header as SerdeOp>::serialize(&self.header)?;
+
+        leader_public_key
+            .verify(&header_bytes, &self.signature)
+            .map_err(|e| Error::Signing(format!("Invalid signature: {e}")))?;
+
+        Ok(())
+    }


I think we can make it so it's impossible to build an invalid block instead of requiring validation

zeegomo · 2025-09-23T13:47:23Z

nomos-core/chain-defs/src/block/mod.rs

+        let mempool_transactions: Vec<Fr> = self
+            .transactions
+            .iter()
+            .map(|tx| tx.hash().into())


if reference is not a list of TxHash we should probably update it

zeegomo · 2025-09-23T13:50:23Z

nomos-core/chain-defs/src/header/mod.rs

+    pub proposal_version: u8,
+    pub cryptarchia_version: u8,


where are these coming from?

zeegomo · 2025-09-23T13:51:33Z

nomos-services/chain/chain-service/src/sync/block_provider.rs

+            let Ok(signature) = header.sign(&dummy_signing_key) else {
+                error!("Failed to sign header in block provider");
+                return None;
+            };


any reason this should fail in tests?

zeegomo · 2025-09-23T13:55:01Z

nomos-services/chain/chain-service/src/lib.rs

-                        match Self::process_block_and_update_state(
-                            cryptarchia.clone(),
-                            &leader,
-                            block.clone(),
-                            blob_validation.as_ref(),
-                            &storage_blocks_to_remove,
-                            &relays,
-                            &self.new_block_subscription_sender,
-                            &self.lib_subscription_sender,
-                            &self.service_resources_handle.state_updater
-                        ).await {
-                            Ok((new_cryptarchia, new_storage_blocks_to_remove)) => {
-                                cryptarchia = new_cryptarchia;
-                                storage_blocks_to_remove = new_storage_blocks_to_remove;


I know it's pre-existing, but can't we merge these two branches?

zeegomo · 2025-09-23T13:56:52Z

nomos-services/chain/chain-service/src/lib.rs

+            .into_iter()
+            .filter(|hash| {
+                !reconstructed_transactions
+                    .iter()
+                    .any(|tx| tx.hash() == *hash)
+            })
+            .collect();
+


maybe we should return the unmatched ones in the error variant?

zeegomo · 2025-09-23T13:57:31Z

nomos-services/chain/chain-service/src/lib.rs

+
+    let header = proposal.header().clone();
+    let service_reward = proposal.references().service_reward;
+    let signature = *proposal.signature();


this is where we should probably recover the reward tx imo, but ofc not something to add now because we need some additional work on sdp

andrussal requested review from zeegomo, youngjoon-lee and danielSanchezQ as code owners September 5, 2025 14:42

andrussal marked this pull request as draft September 5, 2025 14:42

github-actions bot assigned andrussal Sep 5, 2025

andrussal changed the title ~~WIP: feat(chain): block reconstruction~~ [DRAFT] feat(chain): block reconstruction Sep 5, 2025

andrussal added the chain label Sep 5, 2025

andrussal added this to the Iteration 15 milestone Sep 5, 2025

andrussal force-pushed the feat/chain-block-reconstruction branch 2 times, most recently from b51d554 to 3746c59 Compare September 8, 2025 11:27

Base automatically changed from zeegomo/block to master September 8, 2025 11:29

andrussal force-pushed the feat/chain-block-reconstruction branch 3 times, most recently from a0194b9 to 3cbdd17 Compare September 8, 2025 14:52

andrussal marked this pull request as ready for review September 8, 2025 15:20

andrussal changed the title ~~[DRAFT] feat(chain): block reconstruction~~ feat(chain): block reconstruction Sep 8, 2025

github-actions bot added the feature label Sep 8, 2025

andrussal force-pushed the feat/chain-block-reconstruction branch from 282ac09 to 0a7aa82 Compare September 9, 2025 06:51

zeegomo requested changes Sep 9, 2025

View reviewed changes

andrussal force-pushed the feat/chain-block-reconstruction branch from 45587dc to 1dbe1af Compare September 10, 2025 07:42

andrussal changed the base branch from master to feat/consensus-refactor-base September 10, 2025 08:05

andrussal requested a review from zeegomo September 10, 2025 08:13

andrussal force-pushed the feat/chain-block-reconstruction branch from f3a4e23 to 4230f7e Compare September 16, 2025 10:20

andrussal requested review from bacv, pradovic, ntn-x2 and davidrusu as code owners September 16, 2025 10:20

andrussal force-pushed the feat/chain-block-reconstruction branch from 4230f7e to 063440f Compare September 17, 2025 08:55

andrussal changed the base branch from feat/consensus-refactor-base to master September 17, 2025 09:19

andrussal force-pushed the feat/chain-block-reconstruction branch from 063440f to c58fc59 Compare September 17, 2025 09:33

andrussal removed request for danielSanchezQ, bacv, ntn-x2 and pradovic September 18, 2025 08:22

andrussal added 7 commits September 23, 2025 14:53

feat(chain): block reconstruction

73fca5d

feat(chain): verify sig

1454e6c

feat(chain): const

a9e1250

feat(chain): test

c27986e

feat(chain): comment

637def6

feat(chain): fmt

cbafd6b

feat(chain): update signature

0665313

andrussal force-pushed the feat/chain-block-reconstruction branch from c58fc59 to 0665313 Compare September 23, 2025 13:42

andrussal changed the base branch from master to feat/consensus-refactor-base September 23, 2025 13:42

andrussal added 2 commits September 23, 2025 15:47

feat(chain): rm comment

a57d97e

feat(chain): rename fn

5ec79ed

zeegomo requested changes Sep 23, 2025

View reviewed changes

feat(chain): block reconstruction #1650

Are you sure you want to change the base?

feat(chain): block reconstruction #1650

Uh oh!

Conversation

andrussal commented Sep 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

1. What does this PR implement?

2. Does the code have enough context to be clearly understood?

3. Who are the specification authors and who is accountable for this PR?

4. Is the specification accurate and complete?

5. Does the implementation introduce changes in the specification?

Checklist

Uh oh!

zeegomo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

andrussal commented Sep 10, 2025

Uh oh!

danielSanchezQ commented Sep 17, 2025

Uh oh!

zeegomo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

andrussal commented Sep 5, 2025 •

edited

Loading