Use events for adding and removing committees. by afck · Pull Request #3432 · linera-io/linera-protocol

afck · 2025-02-27T16:45:59Z

Motivation

To make epoch changes scalable, we want to use events instead of messages (#365).

Proposal

Remove the committee messages and add add ProcessNewEpoch and ProcessRemovedEpoch system operations instead. The committee change operations on the admin change create events now, and the two new operations consume those events, i.e. verify that they exist, change the epoch and committees on the local chain, and create an OracleResponse::Event entry in the execution outcome.

Clients get notified about new epochs because they always follow/synchronize the admin chain and thus store the epoch change events locally. They will then run process_inbox which, in addition to incoming messages, looks for epoch change events, and includes the new operations in the proposal accordingly.

Test Plan

Several existing tests verify that reconfigurations are processed correctly. These check the new mechanism now instead of the message-based one.

Release Plan

Nothing to do / These changes follow the usual release cycle.

Links

deuszx

First pass through the code. It looks good overall but I had some more general questions. I think the general approach is slightly hacky (i.e. looping through the storage in search for interesting events). It could be improved if we had an option to subscribe for specific event streams in the client.

I am not giving an approval yet - I want to see the answers and then will re-review it.

linera-client/src/chain_listener.rs

linera-core/src/unit_tests/worker_tests.rs

linera-core/src/client/mod.rs

linera-execution/src/system.rs

afck · 2025-03-03T14:20:18Z

Closing this in favor of #3453.
(The implementation will probably use some of this PR's code, but also change a lot.)

afck · 2025-03-06T10:34:41Z

Reopening as a draft. I will mark it ready for review when #3453 is fully implemented.

…nsistencies. (#3489) ## Motivation Working on #3432 I found a few issues that can be fixed separately. ## Proposal * Move the operation inspection code from the two `published_blob_ids` implementations to `Operation` and deduplicate it. * Use the `select!` macro in the client listener. This will make it easier to add another stream, to listen for admin chain events. * Fix a typo where a blob ID was called `chain_id`. * Add a check that events don't get overwritten. Testing this will be complicated, and possibly require more SDK features: #3488 * Treat `stage_new_committee` like other client operations. The issue with conflicting blocks needs to be addressed for all operations anyway, and admin operations are always on a single-user chain in our case. #3172 * Use `stage_new_committee` in the node service` instead of manually creating the operation. There is no reason to have different semantics here in CLI vs. the node service API, and to have the user input the current epoch manually. ## Test Plan CI should catch regressions. I created #3488 for the events check. ## Release Plan - Nothing to do / These changes follow the usual release cycle. ## Links - Found while working on #3432. - [reviewer checklist](https://github.com/linera-io/linera-protocol/blob/main/CONTRIBUTING.md#reviewer-checklist)

ma2bd · 2025-03-01T21:35:44Z

linera-base/src/data_types.rs

    /// The block's validation round.
    Round(Option<u32>),
+    /// An event was read.
+    Event(EventId, Vec<u8>),


Ok so if we don't include the hash of the certificate that created the event, this means that a user event (stream) can become deprecated -- which may be a good thing after all (except for fast blocks).

Not sure: This oracle response does re-certify the event itself (including its content), just not the certificate that created it.

ma2bd · 2025-03-01T21:50:29Z

linera-client/src/chain_listener.rs

+        let chain_ids = {
+            let guard = context.lock().await;
+            let mut chain_ids = BTreeSet::from_iter(guard.wallet().chain_ids());
+            chain_ids.insert(guard.wallet().genesis_admin_chain());


Does this code belong here?

I could add a getter to the wallet that returns the wallet's chains plus the admin chain.

ma2bd · 2025-03-01T21:50:59Z

linera-client/src/chain_listener.rs

    {
-        let mut guard = listening.lock().await;
-        if guard.contains(&chain_id) {
+        if !listening.lock().await.insert(chain_id) {


ma2bd · 2025-03-01T21:55:02Z

linera-client/src/chain_listener.rs

+        let admin_listener: NotificationStream = if client.admin_id() == chain_id {
+            Box::pin(stream::pending())
+        } else {
+            Box::pin(client.subscribe_to(client.admin_id()).await?)


Is this creating a distinct admin chain listener for all mon-admin chains?

No, just a local entry in the notifier.

ma2bd · 2025-03-01T21:56:14Z

linera-core/src/client/mod.rs

        #[cfg(with_metrics)]
        let _latency = metrics::PROCESS_INBOX_WITHOUT_PREPARE_LATENCY.measure_latency();

+        self.synchronize_chain_state(self.admin_id).await?;


This could be a bit expensive

Yes, I need to revisit whether it's actually necessary, once we've got all the tests working.

It's the only way to learn about admin chain events now. It doesn't send us a message anymore.

We could make it a separate command, but I'd rather not: It means that more users won't end up migrating to new epochs.

(Edit: Actually, maybe it is redundant, since we're already doing this in synchronize_from_validators. Will remove if the tests pass without it.)

Done in 34d744e.

MathieuDutSik · 2025-03-09T13:31:53Z

linera-execution/src/system.rs

+    #[error(transparent)]
+    BcsError(#[from] bcs::Error),


The BcsError could be put in the ViewError. Non blocker of course.

Not sure: I've also heard the argument that we should create errors only in their own module.

MathieuDutSik · 2025-03-09T13:51:09Z

linera-execution/src/system.rs

+                            bcs::to_bytes(&blob_hash)?,
+                        );
                    }
                    AdminOperation::RemoveCommittee { epoch } => {


It is a little strange to have such a formulation.
I would expect RemoveCommittee { committee }.
Then maybe rename RemoveFromCommittee.
Not blocker of course.

I agree, it should probably be called RevokeEpoch? But then I'd rename the other one also from CreateCommittee to CreateEpoch.

Not sure what the best names are.

I created #3516 for this.

deuszx · 2025-03-10T09:48:05Z

linera-base/src/data_types.rs

    }

+    /// Creates a new committee [`BlobContent`] from the provided serialized committee.
+    pub fn new_committee(committee: impl Into<Box<[u8]>>) -> Self {


This API should be more type-safe. Right now it accepts any bytes blindly a s Committee which can led to problems. Why not pass the actual committee instead and serialise it here?

I created #3520 for this.

deuszx · 2025-03-10T09:49:14Z

linera-base/src/identifiers.rs

    /// A blob containing an application description.
    ApplicationDescription,
+    /// A blob containing a committee of validators.
+    Committee,


Since we're adding more and more here I think we should have a discussion and some design doc (agreement) what can/should become a BlobType.

deuszx · 2025-03-10T09:55:24Z

linera-core/src/client/mod.rs

+        let (mut min_epoch, mut next_epoch) = {
+            let query = ChainInfoQuery::new(self.chain_id).with_committees();
+            let info = *self
+                .client
+                .local_node
+                .handle_chain_info_query(query)
+                .await?
+                .info;
+            let committees = info
+                .requested_committees
+                .ok_or(LocalNodeError::InvalidChainInfoResponse)?;
+            let min_epoch = *committees.keys().next().unwrap_or(&Epoch::ZERO);
+            let epoch = info.epoch.ok_or(LocalNodeError::InvalidChainInfoResponse)?;
+            (min_epoch, epoch.try_add_one()?)
+        };
+        let mut epoch_change_ops = Vec::new();
+        while self.has_admin_event(EPOCH_STREAM_NAME, next_epoch).await? {
+            epoch_change_ops.push(Operation::System(SystemOperation::ProcessNewEpoch(
+                next_epoch,
+            )));
+            next_epoch.try_add_assign_one()?;
+        }
+        while self
+            .has_admin_event(REMOVED_EPOCH_STREAM_NAME, min_epoch)
+            .await?
+        {
+            epoch_change_ops.push(Operation::System(SystemOperation::ProcessRemovedEpoch(
+                min_epoch,
+            )));
+            min_epoch.try_add_assign_one()?;
+        }
+        let mut epoch_change_ops = epoch_change_ops.into_iter();


Can we extract this to a helper method? Sth like collect_committee_changes ?

deuszx · 2025-03-10T09:59:59Z

linera-sdk/src/test/validator.rs

+        ));
+        let blob_hash = committee_blob.id().hash;
+        self.storage
+            .write_blob(&committee_blob)


I think changing the resource policy should not require re-publishing a new committee. There should be a separate mechanism for that. If we change the cost of a Wasm multiplication will we also publish a new committee? It should be stored somewhere in the "system storage" and its updates should require quorum of validators.

But it also needs to be applied separately to each chain, just like committee changes. It makes sense to me to treat all changes with that property as one thing. Anyway, this wasn't changed in this PR, so it's probably a separate discussion.

linera-execution/src/system.rs

bart-linera · 2025-03-10T10:54:54Z

linera-execution/src/system.rs

+                    stream_id: StreamId::system(EPOCH_STREAM_NAME),
+                    key: bcs::to_bytes(&epoch)?,
+                };
+                let bytes = match txn_tracker.next_replayed_oracle_response()? {


This pattern of checking for oracle response mismatches is now repeated in a few different places, maybe it could somehow be extracted into a method on TransactionTracker? (Although it might not be so simple, because one of the parameters would have to be a pattern... it would need some thought.)

Anyway, not a blocker.

Yes, that's a good point: We could pass in the closure that extracts the data by matching the pattern.

What's probably less easy to do is to also pass in the closure that uses the actual oracle, since that's going to be async and often borrow a lot of references.

But yes, I'd prefer to do that in a separate PR.

I created #3519.

deuszx

LGTM

…errors.

…3540) ## Motivation Epoch changes were recently changed to use events (#3432), but the `TestValidator::change_resource_control_policy` was still trying to receive epoch change system messages instead of creating an operation to process the new epoch. ## Proposal Use the new `SystemOperation::ProcessNewEpoch` to update the chains to use the new epoch, and skip updating the admin chain because it fails to try to process the same epoch twice. ## Test Plan Tested with a rebased version of the tests in #3509. The tests stop working after #3432 but pass again after this PR. ## Release Plan - These changes follow the usual release cycle, because it fixes an issue with SDK that is not present in any released version. ## Links - [reviewer checklist](https://github.com/linera-io/linera-protocol/blob/main/CONTRIBUTING.md#reviewer-checklist)

## Motivation As of #3432 we are not using system channels anymore. ## Proposal Remove the `SystemChannel` type and any system channel-specific logic, in particular the "off-chain subscription handling". ## Test Plan The `social` example tests make sure that _user_ pub-sub channels still work. ## Release Plan - Nothing to do / These changes follow the usual release cycle. ## Links - [reviewer checklist](https://github.com/linera-io/linera-protocol/blob/main/CONTRIBUTING.md#reviewer-checklist)

afck force-pushed the epoch-events branch from a6c50a4 to ea00574 Compare February 27, 2025 17:35

afck requested review from bart-linera, deuszx and ma2bd February 27, 2025 19:40

afck marked this pull request as ready for review February 27, 2025 19:40

deuszx reviewed Feb 28, 2025

View reviewed changes

deuszx mentioned this pull request Feb 28, 2025

Clients follow only block headers #3437

Open

ma2bd mentioned this pull request Mar 1, 2025

Add option to not close chains on benchmark #3447

Merged

afck closed this Mar 3, 2025

afck reopened this Mar 6, 2025

afck marked this pull request as draft March 6, 2025 10:35

afck force-pushed the epoch-events branch 2 times, most recently from aa3299f to 016c387 Compare March 6, 2025 10:41

afck mentioned this pull request Mar 6, 2025

Check for overwriting events, fix typos, duplications, and minor inconsistencies. #3489

Merged

afck force-pushed the epoch-events branch from 016c387 to 60bb9d9 Compare March 6, 2025 18:13

ma2bd reviewed Mar 6, 2025

View reviewed changes

afck force-pushed the epoch-events branch 3 times, most recently from dcf3eee to fbc6f93 Compare March 9, 2025 12:36

afck marked this pull request as ready for review March 9, 2025 13:11

MathieuDutSik reviewed Mar 9, 2025

View reviewed changes

deuszx reviewed Mar 10, 2025

View reviewed changes

linera-execution/src/system.rs Show resolved Hide resolved

deuszx reviewed Mar 10, 2025

View reviewed changes

linera-execution/src/system.rs Outdated Show resolved Hide resolved

deuszx reviewed Mar 10, 2025

View reviewed changes

linera-execution/src/system.rs Show resolved Hide resolved

deuszx reviewed Mar 10, 2025

View reviewed changes

linera-execution/src/system.rs Show resolved Hide resolved

afck force-pushed the epoch-events branch from 34d744e to 94cccbc Compare March 10, 2025 10:52

bart-linera approved these changes Mar 10, 2025

View reviewed changes

deuszx approved these changes Mar 10, 2025

View reviewed changes

afck added 9 commits March 10, 2025 12:34

Use events for adding and removing committees.

2f60cee

Rename EPOCH_STREAM_NAME to NEW_EPOCH_STREAM_NAME.

8bc8d05

Use blobs for committees.

235e245

Remove redundant synchronize_chain_state call.

b01a1ba

Extract collect_epoch_changes.

2162eb6

Update comments on committee operations.

36a1a97

Apply the same committee error handling logic on all chains; improve …

3282e72

…errors.

Add a comment to next_replayed_oracle_response.

56be2de

Fix type in documentation.

014c9b5

afck force-pushed the epoch-events branch from 3c42e8b to 014c9b5 Compare March 10, 2025 11:36

afck merged commit edc6de0 into linera-io:main Mar 10, 2025
25 checks passed

afck deleted the epoch-events branch March 10, 2025 12:18

jvff mentioned this pull request Mar 11, 2025

Fix epoch update in TestValidator::change_resource_control_policy #3540

Merged

This was referenced Mar 18, 2025

Write committees as blobs to storage. #3453

Closed

Remove system channels. #3634

Merged

Conversation

afck commented Feb 27, 2025

Motivation

Proposal

Test Plan

Release Plan

Links

Uh oh!

deuszx left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

afck commented Mar 3, 2025

Uh oh!

afck commented Mar 6, 2025

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!

Choose a reason for hiding this comment

Uh oh!

afck Mar 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

afck Mar 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deuszx left a comment

Choose a reason for hiding this comment

afck Mar 9, 2025 •

edited

Loading

afck Mar 10, 2025 •

edited

Loading