[fastpath] support locking transactions and returning effects without signatures #20128

mwtian · 2024-10-31T23:41:53Z

Description

Allow locking transactions without getting back a signed transaction.
Allow getting back unsigned effects for previously executed transactions.
Add basic types to support using finalized effects without aggregated effects sigs.
Handle ConsensusTransactionKind::UserTransaction in callsites that already handle ConsensusTransactionKind::CertifiedTransaction.
Support reconfiguring components other than QuorumDriver, in reconfig observers.
Remove some unused code.

Test plan

CI

Release notes

Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required.

For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates.

vercel · 2024-10-31T23:41:57Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
sui-docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Nov 7, 2024 6:49pm

3 Skipped Deployments

Name	Status	Preview	Updated (UTC)
multisig-toolkit	⬜️ Ignored (Inspect)	Visit Preview	Nov 7, 2024 6:49pm
sui-kiosk	⬜️ Ignored (Inspect)	Visit Preview	Nov 7, 2024 6:49pm
sui-typescript-docs	⬜️ Ignored (Inspect)	Visit Preview	Nov 7, 2024 6:49pm

arun-koshy · 2024-11-01T02:23:23Z

crates/sui-core/src/consensus_validator.rs

                        return Err(SuiError::UnexpectedMessage(
                            "ConsensusTransactionKind::UserTransaction is unsupported".to_string(),
                        )
                        .into());
                    }
-                    // TODO(fastpath): implement verification for uncertified user transactions.


why is this todo no longer required? Is it because we verify when we vote?

Yeah I was thinking the verifications in vote_transactions() make this TODO obsolete. But it may be better to still keep track of the TODO to move those checks here.

… signatures

akichidis

I am good to approved but let's catch up regarding the QuorumExecuted changes , would like to make sure I understand this correctly

akichidis · 2024-11-07T00:01:33Z

crates/sui-core/src/authority.rs

@@ -1062,7 +1079,7 @@ impl AuthorityState {

    pub(crate) fn check_system_overload(
        &self,
-        consensus_adapter: &Arc<ConsensusAdapter>,
+        consensus_overload_checker: &(impl ConsensusOverloadChecker + ?Sized),


I know that is legit, but why not an Arc<dyn ConsensusOverloadChecker> ?

That will require a clone of the Arc<> at the callsite, which seems unnecessary. Also &Arc<dyn ConsensusOverloadChecker> does not work which is my initial attempt.

akichidis · 2024-11-07T00:07:02Z

crates/sui-core/src/authority.rs

+        transaction_digest: &TransactionDigest,
+    ) -> SuiResult<Option<(TransactionEffects, TransactionEvents)>> {
+        let effects = self
+            .get_transaction_cache_reader()


Haven't followed the cache work semantics, but my understanding is that it will guarantee to check in storage if data are not in cache, right?

As clarified by @mystenmark, yes.

Yep, apologies, that was a comment that I put yesterday before our discussion. All good.

akichidis · 2024-11-07T00:17:16Z

crates/sui-core/src/authority_server.rs

@@ -781,7 +773,7 @@ impl ValidatorService {
                    ConsensusTransactionKind::CertifiedTransaction(tx) => {
                        tx.contains_shared_object()
                    }
-                    ConsensusTransactionKind::UserTransaction(_) => true,
+                    ConsensusTransactionKind::UserTransaction(tx) => tx.contains_shared_object(),


We might want to have for the UserTransaction without a shared obj a separate metric I guess?

Yeah this metric is not too useful so far. For what it is measuring, maybe removing all the conditional is better.

crates/sui-core/src/consensus_adapter.rs

crates/sui-core/src/consensus_handler.rs

consensus/core/src/transaction.rs

akichidis · 2024-11-07T00:53:00Z

crates/sui-benchmark/src/fullnode_reconfig_observer.rs

-    async fn run(&mut self, quorum_driver: Arc<QuorumDriver<NetworkAuthorityClient>>) {
+    async fn run(
+        &mut self,
+        updatable: Arc<dyn AuthorityAggregatorUpdatable<NetworkAuthorityClient>>,


I think the updatable is a bit generic. Why not just authority_aggregator?

This value is not authority_aggregator itself, but a value that supports updating its underneath authority_aggregator, including QuorumDriver and TransactionDriver later. Renamed this to driver.

akichidis · 2024-11-07T16:28:43Z

crates/sui-core/src/authority/authority_per_epoch_store.rs

+        if let Some(signed_transaction) = signed_transaction {
+            batch.insert_batch(
+                &self.signed_transactions,
+                std::iter::once((
+                    *signed_transaction.digest(),
+                    signed_transaction.serializable_ref(),
+                )),
+            )?;
+        }


Is my understanding correct that we'll no longer need for fastpath the signed_transactions anyways since we do not individually sign the transactions anymore? Looking on the read path this is mostly being used to tell via our APIs whether the transaction has been signed or not. Not sure if those APIs will be used in the future.

Transactions will not be signed with BLS any more. But if there are use cases for handle_transaction_info_request() other than getting effects, we will have to figure out ways to migrate the existing users. I will leave a TODO over that API.

akichidis · 2024-11-07T16:40:29Z

crates/sui-types/src/quorum_driver_types.rs

-pub struct VerifiedExecuteTransactionResponseV3 {
-    pub effects: VerifiedCertifiedTransactionEffects,
-    pub events: Option<TransactionEvents>,
-    // Input objects will only be populated in the happy path
-    pub input_objects: Option<Vec<Object>>,
-    // Output objects will only be populated in the happy path
-    pub output_objects: Option<Vec<Object>>,
-    pub auxiliary_data: Option<Vec<u8>>,
-}


I know this is dead code but is it ok to remove?

Since there is no reference to this type else where, it should be safe to remove.

mwtian requested review from a team and mystenmark as code owners October 31, 2024 23:41

vercel bot deployed to Preview – sui-docs October 31, 2024 23:43 View deployment

vercel bot deployed to Preview – sui-typescript-docs October 31, 2024 23:43 View deployment

mwtian force-pushed the tmw/fastpath-lock branch from 4b31444 to 0938b37 Compare November 1, 2024 00:07

mwtian temporarily deployed to sui-typescript-aws-kms-test-env November 1, 2024 00:07 — with GitHub Actions Inactive

mwtian removed the request for review from a team November 1, 2024 00:07

vercel bot deployed to Preview – sui-docs November 1, 2024 00:08 View deployment

mwtian force-pushed the tmw/fastpath-lock branch from 0938b37 to 9a73738 Compare November 1, 2024 00:10

mwtian temporarily deployed to sui-typescript-aws-kms-test-env November 1, 2024 00:10 — with GitHub Actions Inactive

mwtian requested review from lxfind, aschran, akichidis and arun-koshy November 1, 2024 00:11

vercel bot deployed to Preview – sui-docs November 1, 2024 00:12 View deployment

mwtian force-pushed the tmw/fastpath-lock branch from 9a73738 to 7efa105 Compare November 1, 2024 01:13

mwtian temporarily deployed to sui-typescript-aws-kms-test-env November 1, 2024 01:13 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs November 1, 2024 01:14 View deployment

mwtian force-pushed the tmw/fastpath-lock branch from 7efa105 to 460cc2c Compare November 1, 2024 01:44

mwtian temporarily deployed to sui-typescript-aws-kms-test-env November 1, 2024 01:44 — with GitHub Actions Inactive

mwtian changed the title ~~[fastpath] supporting locking transactions and returning effects without signatures~~ [fastpath] support locking transactions and returning effects without signatures Nov 1, 2024

vercel bot deployed to Preview – sui-docs November 1, 2024 01:45 View deployment

arun-koshy approved these changes Nov 1, 2024

View reviewed changes

mwtian force-pushed the tmw/fastpath-lock branch from 460cc2c to a98452b Compare November 1, 2024 04:27

mwtian temporarily deployed to sui-typescript-aws-kms-test-env November 1, 2024 04:27 — with GitHub Actions Inactive

[fastpath] support locking transactions and returning effects without…

96a00d4

… signatures

mwtian force-pushed the tmw/fastpath-lock branch from a98452b to 96a00d4 Compare November 1, 2024 04:28

mwtian temporarily deployed to sui-typescript-aws-kms-test-env November 1, 2024 04:28 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs November 1, 2024 04:30 View deployment

mwtian requested a review from bmwill November 1, 2024 05:03

mwtian temporarily deployed to sui-typescript-aws-kms-test-env November 5, 2024 00:37 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs November 5, 2024 00:38 View deployment

.

a860847

mwtian force-pushed the tmw/fastpath-lock branch from dfa9202 to a860847 Compare November 5, 2024 05:47

mwtian temporarily deployed to sui-typescript-aws-kms-test-env November 5, 2024 05:47 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs November 5, 2024 05:48 View deployment

akichidis reviewed Nov 7, 2024

View reviewed changes

mwtian temporarily deployed to sui-typescript-aws-kms-test-env November 7, 2024 18:16 — with GitHub Actions Inactive

mwtian requested a review from akichidis November 7, 2024 18:16

vercel bot deployed to Preview – sui-docs November 7, 2024 18:17 View deployment

mwtian force-pushed the tmw/fastpath-lock branch from 601d1aa to 754ebfc Compare November 7, 2024 18:17

mwtian temporarily deployed to sui-typescript-aws-kms-test-env November 7, 2024 18:17 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs November 7, 2024 18:19 View deployment

akichidis approved these changes Nov 7, 2024

View reviewed changes

.

11003ee

mwtian force-pushed the tmw/fastpath-lock branch from 754ebfc to 11003ee Compare November 7, 2024 18:47

mwtian temporarily deployed to sui-typescript-aws-kms-test-env November 7, 2024 18:47 — with GitHub Actions Inactive

mwtian enabled auto-merge (squash) November 7, 2024 18:48

vercel bot deployed to Preview – sui-docs November 7, 2024 18:49 View deployment

mwtian merged commit d7b6639 into main Nov 7, 2024
52 checks passed

mwtian deleted the tmw/fastpath-lock branch November 7, 2024 19:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[fastpath] support locking transactions and returning effects without signatures #20128

[fastpath] support locking transactions and returning effects without signatures #20128

mwtian commented Oct 31, 2024 •

edited

Loading

vercel bot commented Oct 31, 2024 •

edited

Loading

arun-koshy Nov 1, 2024

mwtian Nov 1, 2024

akichidis left a comment

akichidis Nov 7, 2024

mwtian Nov 7, 2024

akichidis Nov 7, 2024

mwtian Nov 7, 2024

akichidis Nov 7, 2024

akichidis Nov 7, 2024

mwtian Nov 7, 2024

akichidis Nov 7, 2024

mwtian Nov 7, 2024

akichidis Nov 7, 2024

mwtian Nov 7, 2024

akichidis Nov 7, 2024

mwtian Nov 7, 2024

[fastpath] support locking transactions and returning effects without signatures #20128

[fastpath] support locking transactions and returning effects without signatures #20128

Conversation

mwtian commented Oct 31, 2024 • edited Loading

Description

Test plan

Release notes

vercel bot commented Oct 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akichidis 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

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

mwtian commented Oct 31, 2024 •

edited

Loading

vercel bot commented Oct 31, 2024 •

edited

Loading