Skip to content

Commit

Permalink
[Soft bundle] enable soft bundle byte size checks on API (#20421)
Browse files Browse the repository at this point in the history
## Description 

Following the work on #20325 ,
this PR is essential to ensure that submitted soft bundle transactions
will make it through consensus and won't be rejected due to excessive
size (for the edge cases). Now the API will return a
`SoftBundleTooLarge` error if the soft bundle in size bytes is ` >
consensus_block_size/2` .

## 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.

- [ ] Protocol: 
- [ ] Nodes (Validators and Full nodes): 
- [ ] Indexer: 
- [ ] JSON-RPC: 
- [ ] GraphQL: 
- [ ] CLI: 
- [ ] Rust SDK:
- [ ] REST API:
  • Loading branch information
akichidis authored Nov 27, 2024
1 parent c275945 commit f0ade5e
Show file tree
Hide file tree
Showing 3 changed files with 107 additions and 6 deletions.
31 changes: 26 additions & 5 deletions crates/sui-core/src/authority_server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1002,6 +1002,7 @@ impl ValidatorService {
&self,
certificates: &NonEmpty<CertifiedTransaction>,
epoch_store: &Arc<AuthorityPerEpochStore>,
total_size_bytes: u64,
) -> Result<(), tonic::Status> {
let protocol_config = epoch_store.protocol_config();
let node_config = &self.state.config;
Expand All @@ -1024,6 +1025,7 @@ impl ValidatorService {
// - All certs must not be already executed.
// - All certs must have the same gas price.
// - Number of certs must not exceed the max allowed.
// - Total size of all certs must not exceed the max allowed.
fp_ensure!(
certificates.len() as u64 <= protocol_config.max_soft_bundle_size(),
SuiError::UserInputError {
Expand All @@ -1033,6 +1035,23 @@ impl ValidatorService {
}
.into()
);

// We set the soft bundle max size to be half of the consensus max transactions in block size. We do this to account for
// serialization overheads and to ensure that the soft bundle is not too large when is attempted to be posted via consensus.
// Although half the block size is on the extreme side, it's should be good enough for now.
let soft_bundle_max_size_bytes =
protocol_config.consensus_max_transactions_in_block_bytes() / 2;
fp_ensure!(
total_size_bytes <= soft_bundle_max_size_bytes,
SuiError::UserInputError {
error: UserInputError::SoftBundleTooLarge {
size: total_size_bytes,
limit: soft_bundle_max_size_bytes,
},
}
.into()
);

let mut gas_price = None;
for certificate in certificates {
let tx_digest = *certificate.digest();
Expand Down Expand Up @@ -1099,8 +1118,9 @@ impl ValidatorService {
let mut total_size_bytes = 0;
for certificate in &certificates {
// We need to check this first because we haven't verified the cert signature.
total_size_bytes +=
certificate.validity_check(epoch_store.protocol_config(), epoch_store.epoch())?;
total_size_bytes += certificate
.validity_check(epoch_store.protocol_config(), epoch_store.epoch())?
as u64;
}

self.metrics
Expand All @@ -1112,11 +1132,11 @@ impl ValidatorService {
.observe(total_size_bytes as f64);

// Now that individual certificates are valid, we check if the bundle is valid.
self.soft_bundle_validity_check(&certificates, &epoch_store)
self.soft_bundle_validity_check(&certificates, &epoch_store, total_size_bytes)
.await?;

info!(
"Received Soft Bundle with {} certificates, from {}, tx digests are [{}]",
"Received Soft Bundle with {} certificates, from {}, tx digests are [{}], total size [{}]bytes",
certificates.len(),
client_addr
.map(|x| x.to_string())
Expand All @@ -1125,7 +1145,8 @@ impl ValidatorService {
.iter()
.map(|x| x.digest().to_string())
.collect::<Vec<_>>()
.join(", ")
.join(", "),
total_size_bytes
);

let span = error_span!("handle_soft_bundle_certificates_v3");
Expand Down
77 changes: 76 additions & 1 deletion crates/sui-core/src/unit_tests/transaction_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1781,7 +1781,7 @@ async fn test_handle_soft_bundle_certificates_errors() {
let mut senders = Vec::new();
let mut gas_objects = Vec::new();
let mut owned_objects = Vec::new();
for _i in 0..10 {
for _i in 0..15 {
let (sender, keypair): (_, AccountKeyPair) = get_key_pair();
let mut objects = create_gas_objects(2, sender);
senders.push((sender, keypair));
Expand All @@ -1793,6 +1793,7 @@ async fn test_handle_soft_bundle_certificates_errors() {
ProtocolConfig::get_for_version(ProtocolVersion::max(), Chain::Unknown);
protocol_config.set_enable_soft_bundle_for_testing(true);
protocol_config.set_max_soft_bundle_size_for_testing(3);
protocol_config.set_consensus_max_transactions_in_block_bytes_for_testing(10_000);
let authority = TestAuthorityBuilder::new()
.with_reference_gas_price(1000)
.with_protocol_config(protocol_config)
Expand Down Expand Up @@ -1863,6 +1864,7 @@ async fn test_handle_soft_bundle_certificates_errors() {
let rgp = authority.reference_gas_price_for_testing().unwrap();

// Case 0: submit an empty soft bundle.
println!("Case 0: submit an empty soft bundle.");
{
let response = client
.handle_soft_bundle_certificates_v3(
Expand All @@ -1886,6 +1888,7 @@ async fn test_handle_soft_bundle_certificates_errors() {

// Case 1: submit a soft bundle with more txs than the limit.
// The bundle should be rejected.
println!("Case 1: submit a soft bundle with more txs than the limit.");
{
let mut certificates: Vec<CertifiedTransaction> = vec![];
for i in 0..5 {
Expand Down Expand Up @@ -1934,6 +1937,7 @@ async fn test_handle_soft_bundle_certificates_errors() {

// Case 2: submit a soft bundle with tx containing no shared object.
// The bundle should be rejected.
println!("Case 2: submit a soft bundle with tx containing no shared object.");
{
let owned_object_ref = authority
.get_object(&owned_objects[5].id())
Expand Down Expand Up @@ -1978,6 +1982,7 @@ async fn test_handle_soft_bundle_certificates_errors() {

// Case 3: submit a soft bundle with txs of different gas prices.
// The bundle should be rejected.
println!("Case 3: submit a soft bundle with txs of different gas prices.");
{
let cert0 = {
let gas_object_ref = authority
Expand Down Expand Up @@ -2061,6 +2066,7 @@ async fn test_handle_soft_bundle_certificates_errors() {

// Case 4: submit a soft bundle with txs whose consensus message has been processed.
// The bundle should be rejected.
println!("Case 4: submit a soft bundle with txs whose consensus message has been processed.");
{
let cert0 = {
let gas_object_ref = authority
Expand Down Expand Up @@ -2143,6 +2149,75 @@ async fn test_handle_soft_bundle_certificates_errors() {
}
);
}

// Case 5: submit a soft bundle with total tx size exceeding the block size limit.
// The bundle should be rejected.
println!("Case 5: submit a soft bundle with total tx size exceeding the block size limit.");
{
let mut certificates: Vec<CertifiedTransaction> = vec![];

for i in 11..14 {
let owned_object_ref = authority
.get_object(&owned_objects[i].id())
.await
.unwrap()
.compute_object_reference();
let gas_object_ref = authority
.get_object(&gas_objects[i].id())
.await
.unwrap()
.compute_object_reference();
let sender = &senders[i];
let recipient = &senders[i + 1].0;

// Construct an oversized txn.
let pt = {
let mut builder = ProgrammableTransactionBuilder::new();
// Put a lot of commands in the txn so it's large.
for _ in 0..1000 {
builder
.transfer_object(*recipient, owned_object_ref)
.unwrap();
}
builder.finish()
};

let data = TransactionData::new_programmable(
sender.0,
vec![gas_object_ref],
pt,
rgp * TEST_ONLY_GAS_UNIT_FOR_TRANSFER,
rgp,
);

let signed = to_sender_signed_transaction(data, &sender.1);
certificates.push(signed_tx_into_certificate(signed).await.into());
}

let response = client
.handle_soft_bundle_certificates_v3(
HandleSoftBundleCertificatesRequestV3 {
certificates,
wait_for_effects: true,
include_events: false,
include_auxiliary_data: false,
include_input_objects: false,
include_output_objects: false,
},
None,
)
.await;
assert!(response.is_err());
assert_matches!(
response.unwrap_err(),
SuiError::UserInputError {
error: UserInputError::SoftBundleTooLarge {
size: 25116,
limit: 5000
},
}
);
}
}

#[test]
Expand Down
5 changes: 5 additions & 0 deletions crates/sui-types/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -267,6 +267,11 @@ pub enum UserInputError {
limit
)]
TooManyTransactionsInSoftBundle { limit: u64 },
#[error(
"Total transactions size ({:?})bytes exceeds the maximum allowed ({:?})bytes in a Soft Bundle",
size, limit
)]
SoftBundleTooLarge { size: u64, limit: u64 },
#[error("Transaction {:?} in Soft Bundle contains no shared objects", digest)]
NoSharedObjectError { digest: TransactionDigest },
#[error("Transaction {:?} in Soft Bundle has already been executed", digest)]
Expand Down

0 comments on commit f0ade5e

Please sign in to comment.