Soft Bundle authority API implementation by shio-coder · Pull Request #17868 · MystenLabs/sui

shio-coder · 2024-05-22T07:36:57Z

Description

This is a follow up change after PR #17508 , for SIP-19.

The changeset focuses on sui-core, particularly authority server, to add a new API SoftBundleCertifiedTransactionsV3 that allows submission of a Soft Bundle through gRPC.

The API accepts a list of CertifiedTransaction for input, then returns either nothing (should wait_for_effects be false) or a list of HandleCertificateResponseV3 containing the execution result.

The API is named _v3 because it reuses HandleCertificateResponseV3.

Besides introducing a new feature flag and a protocol config field, a local node config field enable_soft_bundle is also added, to allow a validator node to turn off soft bundle support without having to upgrade the entire network. This can be useful should the feature causes undesired effect.

Test plan

The feature is not exactly user-facing and do not break anything.

Release notes

Protocol: New feature flag and protocol config fields will be introduced.
Nodes (Validators and Full nodes): A new API SoftBundleCertifiedTransactionsV3 will be added.
Indexer:
JSON-RPC:
GraphQL:
CLI:
Rust SDK:

vercel · 2024-05-22T07:37:03Z

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	Jun 15, 2024 6:26am

vercel · 2024-05-22T07:37:04Z

@shio-coder is attempting to deploy a commit to the Mysten Labs Team on Vercel.

A member of the Team first needs to authorize it.

crates/sui-types/src/error.rs

crates/sui-core/src/authority_server.rs

mystenmark · 2024-06-06T17:15:16Z

crates/sui-core/src/authority_server.rs

            // For owned objects this will return without waiting for certificate to be sequenced
            // First do quick dirty non-async check
-            if !epoch_store.is_tx_cert_consensus_message_processed(&certificate)? {
+            if !epoch_store.is_all_tx_certs_consensus_message_processed(&verified_certificates)? {


iiuc, this means that if a single transaction in the bundle is unprocessed, we will re-submit the entire bundle. That's okay from the protocol perspective, but is that what is desired for the submitter of the soft bundle? Or would they rather fail in this case, since it is known their bundle will not be sequenced in the order they expect?

The intention here is to wait for all txs to be sequenced before proceeding. In a Soft Bundle context, the check was done previously (by using self.state.is_tx_already_executed()).

However this check cannot be enforced as another thread or another validator may be submitting a subset of the bundle and/or the entire bundle.

This falls into the case where Soft Bundle cannot guarantee specified ordering.

mystenmark · 2024-06-06T17:17:08Z

crates/sui-core/src/authority_server.rs

+                    })
+                    .collect::<Vec<_>>();
+                self.consensus_adapter.submit_batch(
+                    &transactions,


This seems like a potential bug, since we may be submitting already sequenced transactions, which would then be inserted into process_consensus_transactions. But it may be benign since insertions into that table are already racing with consensus handler. @mwtian any thoughts?

My reading of the code is that this change does not seem to introduce new behavior. Transactions inflight or already sequenced in consensus can be submitted to consensus adapter, as mentioned by the "quick dirty non-async check" above. This behavior should be tolerated right now. I think only unique transactions by digests are or can be used for epoch change and revert.

I agree - any extra checkpointed transactions in that table are ignored and not reverted

lxfind · 2024-06-10T16:30:44Z

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

            .contains_key(key)?)
    }

-    pub fn check_consensus_messages_processed<'a>(


Do we have to remove the lifetime and pass-by-reference? I think we should be able to call this from is_any_tx_certs_consensus_message_processed and also pass by reference?

I have updated this a bit so that there is no copy involved. Now is_any_tx_certs_consensus_message_processed would have lifetime and pass-by-reference, then it owns keys until it is passed to and consumed by this function.

lxfind · 2024-06-10T16:35:49Z

crates/sui-types/src/messages_grpc.rs

+/// Soft Bundle request.  See [SIP-19](https://github.com/sui-foundation/sips/blob/main/sips/sip-19.md).
+#[derive(Clone, Debug, Serialize, Deserialize)]
+pub struct HandleSoftBundleCertificatesRequestV3 {
+    pub certificates: Vec<CertifiedTransaction>,


I think we should use the NonEmpty type here so that we don't have to worry about whether it is empty.

This is part of gRPC protocol definition and it seems Serialize is not implemented for NonEmpty

crates/sui-core/src/authority_server.rs

crates/sui-types/src/error.rs

crates/sui-core/src/authority_server.rs

lxfind · 2024-06-10T17:10:33Z

crates/sui-core/src/authority_server.rs

+            let should_submit = if is_soft_bundle {
+                fp_ensure!(
+                    !epoch_store
+                        .is_any_tx_certs_consensus_message_processed(&verified_certificates)?,


If you move this check to be together with all the other soft bundle specific checks, here you can simply submit if any cert is not already submitted. Once you do so, we could then even remove the is_soft_bundle argument from this function.

I have moved the check to soft_bundle_validity_check, which is closer to its RPC handler.
Now the only place that relies on is_soft_bundle is where we determine whether it is necessary to check if the first certificate has been executed.

lxfind

Overall LGTM. Thanks!

crates/sui-core/src/authority_server.rs

## Description This is a follow up change after PR #17508 and #17868 , for SIP-19. The change adds a unit test for soft bundle failure modes, as well as enable soft bundle feature flag in devnet and testnet. ## Test plan The feature is not exactly user-facing and do not break anything. --- ## Release notes - [x] Protocol: Enable soft bundle on Protocol Version 50 in devnet and testnet. - [ ] Nodes (Validators and Full nodes): - [ ] Indexer: - [ ] JSON-RPC: - [ ] GraphQL: - [ ] CLI: - [ ] Rust SDK: --------- Co-authored-by: Mark Logan <103447440+mystenmark@users.noreply.github.com>

## Description This is a follow up change after PR MystenLabs#17508 , for [SIP-19](https://github.com/sui-foundation/sips/blob/main/sips/sip-19.md). The changeset focuses on sui-core, particularly authority server, to add a new API `SoftBundleCertifiedTransactionsV3` that allows submission of a Soft Bundle through gRPC. The API accepts a list of `CertifiedTransaction` for input, then returns either nothing (should `wait_for_effects` be `false`) or a list of `HandleCertificateResponseV3` containing the execution result. The API is named `_v3` because it reuses `HandleCertificateResponseV3`. Besides introducing a new feature flag and a protocol config field, a local node config field `enable_soft_bundle` is also added, to allow a validator node to turn off soft bundle support without having to upgrade the entire network. This can be useful should the feature causes undesired effect. ## Test plan The feature is not exactly user-facing and do not break anything. --- ## Release notes - [x] Protocol: New feature flag and protocol config fields will be introduced. - [x] Nodes (Validators and Full nodes): A new API `SoftBundleCertifiedTransactionsV3` will be added. - [ ] Indexer: - [ ] JSON-RPC: - [ ] GraphQL: - [ ] CLI: - [ ] Rust SDK: --------- Co-authored-by: Mark Logan <mark@mystenlabs.com>

## Description This is a follow up change after PR MystenLabs#17508 and MystenLabs#17868 , for SIP-19. The change adds a unit test for soft bundle failure modes, as well as enable soft bundle feature flag in devnet and testnet. ## Test plan The feature is not exactly user-facing and do not break anything. --- ## Release notes - [x] Protocol: Enable soft bundle on Protocol Version 50 in devnet and testnet. - [ ] Nodes (Validators and Full nodes): - [ ] Indexer: - [ ] JSON-RPC: - [ ] GraphQL: - [ ] CLI: - [ ] Rust SDK: --------- Co-authored-by: Mark Logan <103447440+mystenmark@users.noreply.github.com>

shio-coder requested a review from mystenmark as a code owner May 22, 2024 07:36

vercel bot deployed to Preview – sui-docs May 22, 2024 07:38 View deployment

lxfind requested review from bmwill and lxfind May 22, 2024 16:04

mystenmark reviewed May 31, 2024

View reviewed changes

crates/sui-types/src/error.rs Outdated Show resolved Hide resolved

crates/sui-core/src/authority_server.rs Outdated Show resolved Hide resolved

crates/sui-core/src/authority_server.rs Outdated Show resolved Hide resolved

shio-coder force-pushed the authority_draft branch from 29e3e14 to f1887d5 Compare June 2, 2024 09:16

shio-coder requested a review from mystenmark June 2, 2024 09:16

vercel bot deployed to Preview – sui-docs June 2, 2024 09:17 View deployment

mystenmark reviewed Jun 6, 2024

View reviewed changes

shio-coder force-pushed the authority_draft branch from f1887d5 to 2f698d0 Compare June 7, 2024 05:45

vercel bot deployed to Preview – sui-docs June 7, 2024 05:50 View deployment

mystenmark approved these changes Jun 10, 2024

View reviewed changes

lxfind reviewed Jun 10, 2024

View reviewed changes

shio-coder force-pushed the authority_draft branch from 2f698d0 to 133df46 Compare June 13, 2024 02:58

vercel bot deployed to Preview – sui-docs June 13, 2024 02:59 View deployment

shio-coder requested a review from lxfind June 13, 2024 03:11

lxfind reviewed Jun 13, 2024

View reviewed changes

crates/sui-core/src/authority_server.rs Outdated Show resolved Hide resolved

shio-coder force-pushed the authority_draft branch from 133df46 to 98bf253 Compare June 13, 2024 23:50

shio-coder requested a review from lxfind June 13, 2024 23:51

vercel bot deployed to Preview – sui-docs June 13, 2024 23:52 View deployment

vercel bot deployed to Preview – sui-docs June 14, 2024 05:21 View deployment

shio-coder force-pushed the authority_draft branch from 127e2ff to e8882b2 Compare June 15, 2024 04:38

vercel bot deployed to Preview – sui-docs June 15, 2024 04:39 View deployment

vercel bot deployed to Preview – sui-docs June 15, 2024 04:50 View deployment

shio-coder and others added 3 commits June 14, 2024 23:25

Soft Bundle authority API implementation

31baeb6

Suggested fixes

c5d60e5

Fix according to comments

6459780

shio-coder added 4 commits June 14, 2024 23:25

Reject bundle if any certificate has been processed

bc60eb5

Move soft bundle validation to separated function and use NonEmpty

9c2ea97

Remove is_soft_bundle arg

e6382c2

Fix broken tests

8fbc7d2

longbowlu force-pushed the authority_draft branch from 185f18c to 8fbc7d2 Compare June 15, 2024 06:25

vercel bot deployed to Preview – sui-docs June 15, 2024 06:26 View deployment

mystenmark merged commit ff05210 into MystenLabs:main Jun 17, 2024

shio-coder mentioned this pull request Jun 18, 2024

Enable soft bundle in devnet and testnet #18302

Merged

7 tasks

Conversation

shio-coder commented May 22, 2024

Description

Test plan

Release notes

Uh oh!

vercel bot commented May 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vercel bot commented May 22, 2024

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!

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!

shio-coder Jun 13, 2024 • 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shio-coder Jun 13, 2024 • 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!

lxfind left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

vercel bot commented May 22, 2024 •

edited

Loading

shio-coder Jun 13, 2024 •

edited

Loading

shio-coder Jun 13, 2024 •

edited

Loading