[consensus handler] reorder txns by gas price #12266

emmazzz · 2023-05-31T00:01:43Z

Description

This PR adds a step in consensus handler to reorder transactions in a Narwhal commit by their gas prices. Right now the reordering is purely based on gas price, which will be later changed to be based on object hotness, etc, as well.

Test Plan

Added a test.

If your changes are not user-facing and not a breaking change, you can skip the following section. Otherwise, please indicate what changed, and then add to the Release Notes section as highlighted during the release process.

Type of Change (Check all that apply)

protocol change
user-visible impact
breaking change for a client SDKs
breaking change for FNs (FN binary must upgrade)
breaking change for validators or node operators (must upgrade binaries)
breaking change for on-chain data layout
necessitate either a data wipe or data migration

Release notes

Previously transactions in a Narwhal commit were ordered as the sub-dag is flattened in a depth-first traversal. Now we have added a round of ordering that orders the user transactions in the same commit by gas price, where a transaction with higher gas price will be added for execution in the transaction manager first.

vercel · 2023-05-31T00:01:51Z

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

Name	Status	Preview	Comments	Updated (UTC)
offline-signer-helper	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jun 12, 2023 5:03am
sui-kiosk	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jun 12, 2023 5:03am

4 Ignored Deployments

Name	Status	Updated (UTC)
explorer	⬜️ Ignored (Inspect)	Jun 12, 2023 5:03am
explorer-storybook	⬜️ Ignored (Inspect)	Jun 12, 2023 5:03am
sui-wallet-kit	⬜️ Ignored (Inspect)	Jun 12, 2023 5:03am
wallet-adapter	⬜️ Ignored (Inspect)	Jun 12, 2023 5:03am

emmazzz · 2023-05-31T00:04:01Z

crates/sui-protocol-config/src/lib.rs

@@ -465,6 +466,10 @@ pub struct ProtocolConfig {
    /// 3f+1 must vote), while 0bps would indicate that 2f+1 is sufficient.
    buffer_stake_for_protocol_upgrade_bps: Option<u64>,

+    /// What version of reordering algorithm we are using for user transactions included in a Narwhal
+    /// commit in consensus handler.
+    consensus_user_transaction_reordering_version: Option<u64>,


I thought this is similar to gas model in the sense that we will probably iterate a few times so I added it as a version number instead of feature flag.

we could still use a feature flag, but make it an enum instead of a bool - i think that will help readability, since the enum variants can be descriptive.

mwtian

PR looks good. Is there a discussion thread on ordering transactions by gas price?

mwtian · 2023-05-31T04:29:37Z

crates/sui-core/src/consensus_handler.rs

@@ -357,6 +370,22 @@ impl<T: ParentSync + Send + Sync> ExecutionState for ConsensusHandler<T> {
    }
 }

+fn reorder_by_gas_price(sequenced_transactions: &mut [VerifiedSequencedConsensusTransaction]) {


optional: maybe just order_by_gas_price?

emmazzz · 2023-05-31T07:11:31Z

Is there a discussion thread on ordering transactions by gas price?

@mwtian There's a long thread here. This PR is meant to be a starting point for us to experiment with and come up with later steps.

andll · 2023-05-31T16:32:23Z

crates/sui-core/src/consensus_handler.rs

+            .protocol_config()
+            .consensus_user_transaction_reordering_version_as_option()
+        {
+            reorder_by_gas_price(&mut sequenced_transactions[1..]);


nit: Can we add a monitored_scope here?

mystenmark · 2023-05-31T17:22:54Z

crates/sui-protocol-config/src/lib.rs

@@ -465,6 +466,10 @@ pub struct ProtocolConfig {
    /// 3f+1 must vote), while 0bps would indicate that 2f+1 is sufficient.
    buffer_stake_for_protocol_upgrade_bps: Option<u64>,

+    /// What version of reordering algorithm we are using for user transactions included in a Narwhal
+    /// commit in consensus handler.
+    consensus_user_transaction_reordering_version: Option<u64>,


we could still use a feature flag, but make it an enum instead of a bool - i think that will help readability, since the enum variants can be descriptive.

mystenmark · 2023-05-31T17:29:23Z

crates/sui-core/src/consensus_handler.rs

+                    tracking_id: _,
+                    kind: ConsensusTransactionKind::UserTransaction(cert),
+                }) => cert.gas_price(),
+                // Non-user transactions are considered to have gas price of zero and are put to the end.


i believe this sorts ConsensusCommitPrologue txns to the end, which is definitely not what we want.

On line 306, where this function is called, we are sorting only the second transactions onwards so the first txn in the vector which is consensus commit prologue txn should not be affected.

mwtian · 2023-05-31T19:10:18Z

Is there a discussion thread on ordering transactions by gas price?

@mwtian There's a long thread here. This PR is meant to be a starting point for us to experiment with and come up with later steps.

Great, thanks for the pointer. Looking forward to integrating load shedding logic later.

lxfind · 2023-06-02T04:54:58Z

crates/sui-core/src/consensus_handler.rs

+            ConsensusTransactionOrdering::ByGasPrice
+        ) {
+            let _scope = monitored_scope("HandleConsensusOutput::order_by_gas_price");
+            order_by_gas_price(&mut sequenced_transactions);


What is the typical and max size of sequenced_transactions?

The size of sequenced_transactions is the number of Sui transactions in a Narwhal commit + 1 (prologue). In 100 node deployments, Narwhal commits happen every 1~2s. Actual size of sequenced_transactions is correlated with TPS. The size limit is much larger based on Narwhal limits.

If we are concerned about added latency due to this additional step, I can run an experiment in private testnet with high TPS and observe what the monitored scope metrics show us.

I don't think we're going to have any problems caused by sorting - consensus handler is dominated by db reads and writes. Anything that is purely in memory like this should not cause problems

## Description This PR adds a step in consensus handler to reorder transactions in a Narwhal commit by their gas prices. Right now the reordering is purely based on gas price, which will be later changed to be based on object hotness, etc, as well. ## Test Plan Added a test. --- If your changes are not user-facing and not a breaking change, you can skip the following section. Otherwise, please indicate what changed, and then add to the Release Notes section as highlighted during the release process. ### Type of Change (Check all that apply) - [ ] protocol change - [x] user-visible impact - [ ] breaking change for a client SDKs - [ ] breaking change for FNs (FN binary must upgrade) - [x] breaking change for validators or node operators (must upgrade binaries) - [ ] breaking change for on-chain data layout - [ ] necessitate either a data wipe or data migration ### Release notes Previously transactions in a Narwhal commit were ordered as the sub-dag is flattened in a depth-first traversal. Now we have added a round of ordering that orders the user transactions in the same commit by gas price, where a transaction with higher gas price will be added for execution in the transaction manager first.

emmazzz requested review from andll, mwtian and mystenmark May 31, 2023 00:02

emmazzz commented May 31, 2023

View reviewed changes

emmazzz force-pushed the emma/order-by-gp branch from 12c7051 to c2eb9d9 Compare May 31, 2023 01:03

vercel bot deployed to Preview – offline-signer-helper May 31, 2023 01:04 View deployment

mwtian approved these changes May 31, 2023

View reviewed changes

andll reviewed May 31, 2023

View reviewed changes

mystenmark reviewed May 31, 2023

View reviewed changes

vercel bot deployed to Preview – offline-signer-helper June 1, 2023 21:48 View deployment

emmazzz requested a review from mystenmark June 1, 2023 21:52

emmazzz force-pushed the emma/order-by-gp branch from 19224af to 88594fc Compare June 1, 2023 22:37

vercel bot deployed to Preview – offline-signer-helper June 1, 2023 22:38 View deployment

lxfind reviewed Jun 2, 2023

View reviewed changes

emmazzz force-pushed the emma/order-by-gp branch 3 times, most recently from b9a0201 to 39c5526 Compare June 7, 2023 18:11

vercel bot temporarily deployed to Preview – offline-signer-helper June 7, 2023 18:21 Inactive

emmazzz added 2 commits June 11, 2023 21:39

[consensus handler] reorder txns by gas price

7f73a12

address comments

aaee5b9

emmazzz force-pushed the emma/order-by-gp branch from d408bb6 to aaee5b9 Compare June 12, 2023 05:02

vercel bot deployed to Preview – offline-signer-helper June 12, 2023 05:03 View deployment

vercel bot deployed to Preview – sui-kiosk June 12, 2023 05:03 View deployment

emmazzz merged commit 1009fed into main Jun 12, 2023

emmazzz deleted the emma/order-by-gp branch June 12, 2023 05:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[consensus handler] reorder txns by gas price #12266

[consensus handler] reorder txns by gas price #12266

emmazzz commented May 31, 2023 •

edited

Loading

vercel bot commented May 31, 2023 •

edited

Loading

emmazzz May 31, 2023

mystenmark May 31, 2023

emmazzz May 31, 2023

mwtian left a comment

mwtian May 31, 2023

emmazzz commented May 31, 2023

andll May 31, 2023

mystenmark May 31, 2023

mystenmark May 31, 2023

emmazzz May 31, 2023 •

edited

Loading

mwtian commented May 31, 2023

lxfind Jun 2, 2023

mwtian Jun 2, 2023

emmazzz Jun 2, 2023

mystenmark Jun 5, 2023

[consensus handler] reorder txns by gas price #12266

[consensus handler] reorder txns by gas price #12266

Conversation

emmazzz commented May 31, 2023 • edited Loading

Description

Test Plan

Type of Change (Check all that apply)

Release notes

vercel bot commented May 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mwtian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emmazzz commented May 31, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emmazzz May 31, 2023 • edited Loading

Choose a reason for hiding this comment

mwtian commented May 31, 2023

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

emmazzz commented May 31, 2023 •

edited

Loading

vercel bot commented May 31, 2023 •

edited

Loading

emmazzz May 31, 2023 •

edited

Loading