DA: Consensus sampling #708

danielSanchezQ · 2024-08-28T15:57:49Z

Add sampling to consensus.
Added a verification of block so it is not included if as a non-proposer you do not see the included blobs in your sample view.
As a proposer include only the blobs that you see available in your sample view.

danielSanchezQ · 2024-08-28T15:59:24Z

nomos-services/cryptarchia-consensus/src/lib.rs

+    fn validate_block(
+        block: &Block<ClPool::Item, DaPool::Item>,
+        sampled_blobs_ids: &BTreeSet<DaPool::Key>,
+    ) -> bool {
+        let validated_blobs = block
+            .blobs()
+            .all(|blob| sampled_blobs_ids.contains(&blob.blob_id()));
+        validated_blobs
+    }
 }


This is very temporary and will have to include many other things later one. But I think for now will do.

holisticode

Basically looks good to me. Left a couple of questions, mainly.

holisticode · 2024-08-28T23:49:44Z

nomos-services/cryptarchia-consensus/src/lib.rs

-                    .with_blobs_certificates(da_certs)
+                    .with_blobs_info(
+                        da_blobs_info.filter(move |info| blobs_ids.contains(&info.blob_id())),
+                    )
                    .build()
                else {
                    panic!("Proposal block should always succeed to be built")


Is this temporary too?

No, if it panics is because we forget to add something. Exactly why we want it to panic. A panic is an error in coding that is not catched in compilation time.

holisticode · 2024-08-28T23:53:20Z

nomos-services/cryptarchia-consensus/src/lib.rs

-
-        match futures::join!(cl_txs, da_certs) {
-            (Ok(cl_txs), Ok(da_certs)) => {
+        let blobs_ids = get_sampled_blobs(sampling_relay);


So this is the proposer who is querying for sampled blobs, not the mempool, right? Does it actually make any difference here?

The mempool will trigger the sampling, but it has no usage for the already sampled blobs.
Btw, mempools will change once we actually have TXs, as blobs are part of the TXs themselves. Probably DA mempool gonna be remove entirely.

holisticode · 2024-08-28T23:54:39Z

nomos-services/cryptarchia-consensus/src/lib.rs

+        .send(DaSamplingServiceMsg::MarkInBlock { blobs_id })
+        .await
+    {
+        error!("Error marking in block for blobs ids: {blobs_id:?}");


I am actually seeing a couple of places where errors are just logged.
What would actually the consequence be of an error here, for example? Is logging enough?

This should never happen as it is internally to the node (msg from service to service). We could actually panic here. Or try to recover. There is no clear policy on this cases. Ideas and suggestions are welcome (open an issue for them).

I have learnt in other contexts that when a node encounters an issue it can't actually recover from, it should shutdown, but not panic.

The question here is, what does such an error as this one mean here. Is it fatal, an unrecoverable situation? Actually not really, but something is very fishy still.

Still, a node shutdown should be preferred to a panic IMO.

At the moment there is no really a difference between shutdown and panic.

A panic should be used when a path is a bug, meaning that the programmed logic is wrong, it should be fixed.

An error that does not matter (the node may continue) should be logged and execution should continue.

When something neither the node nor we expect then as you said it should be shutdown/restart.

In this specific case it was logged so we can see something went wrong then we should try to recover the relay later on (as it was broken), and if its not possible then the node could shut down. But still do not have any recoverable measures in place (anywhere). Not something that we are prioritising now.
But if you are interested it would be really beneficial to prepare a design for such policies!

bacv · 2024-08-29T09:20:42Z

nomos-da/network/subnetworks-assignations/src/versions/v2.rs

@@ -63,7 +65,7 @@ impl FillWithOriginalReplication {
 }

 impl MembershipHandler for FillWithOriginalReplication {
-    type NetworkId = u16;
+    type NetworkId = u32;


There's SubnetworkId type in nomos-da-network-core, maybe we could use it in all network related places?

Yes. I changed it as it was a miss aligned type. Also using the SubnetworkId makes it to introduce a full crate just to use the type here, and as it is a type alias atm I do not think is completely worth it.

bacv · 2024-08-29T09:33:21Z

nomos-services/data-availability/sampling/src/backend/kzgrs.rs

+        todo!()
+    }
+
+    async fn mark_in_block(&mut self, _blobs_id: &[Self::BlobId]) {


In the context sampling, it seems that this could better be a mark_complete or similar.
mark_in_block is actually happening in the consensus, this will just receive a notification about it and make actions according to it.

Sure, this makes sense.

danielSanchezQ · 2024-08-30T17:02:38Z

@bacv, is it possible that this now fails because of the tests we just added? As I'm changing consensus 🤔
How are we checking the dissemination is ok? Consensus wise should be fine as we do not have any blob in the blocks.

bacv · 2024-08-30T17:09:10Z

@bacv, is it possible that this now fails because of the tests we just added? As I'm changing consensus 🤔 How are we checking the dissemination is ok? Consensus wise should be fine as we do not have any blob in the blocks.

Da integration tests are using consensus, this pr changed the generic params that needs to be passed to it, and the tests still uses old ones. I'll make a pr that fixes them (to run locally use cargo test -p nomos-da-tests -F libp2p)

danielSanchezQ · 2024-08-30T17:14:02Z

@bacv, is it possible that this now fails because of the tests we just added? As I'm changing consensus 🤔 How are we checking the dissemination is ok? Consensus wise should be fine as we do not have any blob in the blocks.

Da integration tests are using consensus, this pr changed the generic params that needs to be passed to it, and the tests still uses old ones. I'll make a pr that fixes them (to run locally use cargo test -p nomos-da-tests -F libp2p)

Ok, I see them now. I couldn't reproduce them before. I'll fix it in this PR don't worry!

bacv · 2024-08-30T17:14:44Z

@bacv, is it possible that this now fails because of the tests we just added? As I'm changing consensus 🤔 How are we checking the dissemination is ok? Consensus wise should be fine as we do not have any blob in the blocks.

I also see another issue in the nodes test, the DA-Sampling service is required by consensus, but it's not started in the node.

danielSanchezQ · 2024-08-30T17:15:55Z

@bacv, is it possible that this now fails because of the tests we just added? As I'm changing consensus 🤔 How are we checking the dissemination is ok? Consensus wise should be fine as we do not have any blob in the blocks.

I also see another issue in the nodes test, the DA-Sampling service is required by consensus, but it's not started in the node.

Ah damn, its true. You added recently in a different PR right? i'll wait for that then.

bacv · 2024-08-30T17:17:18Z

@bacv, is it possible that this now fails because of the tests we just added? As I'm changing consensus 🤔 How are we checking the dissemination is ok? Consensus wise should be fine as we do not have any blob in the blocks.

I also see another issue in the nodes test, the DA-Sampling service is required by consensus, but it's not started in the node.

Ah damn, its true. You added recently in a different PR right? i'll wait for that then.

In that one the verifier and indexer were added, so sampling should be added in a similar fashion.

danielSanchezQ · 2024-08-30T17:19:29Z

@bacv, is it possible that this now fails because of the tests we just added? As I'm changing consensus 🤔 How are we checking the dissemination is ok? Consensus wise should be fine as we do not have any blob in the blocks.

I also see another issue in the nodes test, the DA-Sampling service is required by consensus, but it's not started in the node.

Ah damn, its true. You added recently in a different PR right? i'll wait for that then.

In that one the verifier and indexer were added, so sampling should be added in a similar fashion.

Cool, so I'll just add it here as well then. Thanks!

danielSanchezQ · 2024-09-03T14:24:50Z

Thanks for the effort @bacv ! 🎸

danielSanchezQ added consensus da labels Aug 28, 2024

danielSanchezQ requested review from holisticode, bacv and zeegomo August 28, 2024 15:57

danielSanchezQ self-assigned this Aug 28, 2024

danielSanchezQ commented Aug 28, 2024

View reviewed changes

holisticode approved these changes Aug 28, 2024

View reviewed changes

bacv reviewed Aug 29, 2024

View reviewed changes

danielSanchezQ force-pushed the consensus-sampling branch from 07a8d90 to 441ddb2 Compare August 30, 2024 15:50

danielSanchezQ force-pushed the consensus-sampling branch from 24c30da to 204b6ce Compare September 2, 2024 15:16

danielSanchezQ added 8 commits September 3, 2024 10:59

Add sampling relay to consensus and massage all generics

b87528e

Pipe in sampling filtering of blob info

3ab49f3

Add mark in block

47ebf53

Pipe validate block

d8565bf

Refactor mark_in_block -> mark_complete

44e09c4

Fix generics on tests

7f278fe

Fix generics on tests

0367bba

Fix rebase

4887347

danielSanchezQ force-pushed the consensus-sampling branch from 204b6ce to 4887347 Compare September 3, 2024 09:09

danielSanchezQ and others added 3 commits September 3, 2024 11:12

Cargo fmt after rebase

6f3710f

Sampling service configuration

b79946b

Sampling service config in indexer integration tests

cd17400

bacv approved these changes Sep 3, 2024

View reviewed changes

danielSanchezQ merged commit a13f861 into master Sep 3, 2024
12 checks passed

danielSanchezQ deleted the consensus-sampling branch September 3, 2024 15:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DA: Consensus sampling #708

DA: Consensus sampling #708

danielSanchezQ commented Aug 28, 2024

danielSanchezQ Aug 28, 2024

holisticode left a comment

holisticode Aug 28, 2024

danielSanchezQ Aug 29, 2024

holisticode Aug 28, 2024

danielSanchezQ Aug 29, 2024

holisticode Aug 28, 2024

danielSanchezQ Aug 29, 2024

holisticode Aug 29, 2024

danielSanchezQ Aug 29, 2024

bacv Aug 29, 2024

danielSanchezQ Aug 29, 2024 •

edited

Loading

bacv Aug 29, 2024

danielSanchezQ Aug 29, 2024

danielSanchezQ commented Aug 30, 2024

bacv commented Aug 30, 2024

danielSanchezQ commented Aug 30, 2024

bacv commented Aug 30, 2024

danielSanchezQ commented Aug 30, 2024

bacv commented Aug 30, 2024

danielSanchezQ commented Aug 30, 2024

danielSanchezQ commented Sep 3, 2024

DA: Consensus sampling #708

DA: Consensus sampling #708

Conversation

danielSanchezQ commented Aug 28, 2024

Choose a reason for hiding this comment

holisticode left a 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

danielSanchezQ Aug 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielSanchezQ commented Aug 30, 2024

bacv commented Aug 30, 2024

danielSanchezQ commented Aug 30, 2024

bacv commented Aug 30, 2024

danielSanchezQ commented Aug 30, 2024

bacv commented Aug 30, 2024

danielSanchezQ commented Aug 30, 2024

danielSanchezQ commented Sep 3, 2024

danielSanchezQ Aug 29, 2024 •

edited

Loading