chore(sequencer)!: change `Deposit` byte length calculation #1507

ethanoroshiba · 2024-09-16T15:49:07Z

Summary

Changed Deposit byte length calculation to use constant base value plus the lengths ofasset and destination_chain_address.

Background

The get_deposit_byte_length() function uses prost::message::encoded_len() to calculate the on-wire size of raw deposit events for fee calculation. Due to protobuf using variable length integers, however, this would mean that fees for BridgeLock actions would increase with larger deposit amounts or higher action indices, which should not be the case. This PR aims to address this issue and provide more fair fee calculation.

Offline discussion resulted in the decision to calculate a "fair" base length and then add the lengths of asset and destination_chain_address

Changes

Calculate get_deposit_byte_length() based on a constant base length plus the lengths of asset and destination_chain_address. The base length is determined by a unit test with "reasonable" values for all fields except for asset and destination_chain_address.

Testing

Added unit test to ensure calculated byte length doesn't change for any other changes than to asset and destination_chain_address.
Added snapshot test to ensure the method for calculating byte length is not changed.

Breaking Changelist

Changing the byte length calculation changes fees for BridgeLock actions, breaking the app hash.

Related Issues

closes #1503

…ress

noot

codewise this looks fine, arguably values that encode to a smaller length should be cheaper, as the fee should be proportional to what's written to DA. however this will return a larger value rather than smaller, which seems okay

SuperFluffy

Blocking this because the bridge address construction cannot assume a fixed prefix.

SuperFluffy · 2024-09-17T20:53:32Z

Suggestions for unblocking this while we redesign our fee handling.

Make a "base case" of a deposit with destination_chain_address and asset empty and an arbitrary prefix set (take the prefix as the max number of bytes permitted in a bech32m encoding, which would be 83 ASCII chars)
define an constant from the Message::encoded_len of this base case scenario
define the fee as the constant from 2. and add the lengths of asset and destination_chain_address to that

Support 2. with a unit test, and provide a few unit tests to support the implementation's claim that the calculated fee/byte length be never smaller than the actual proto len.

ethanoroshiba · 2024-09-18T14:43:49Z

Should we also change get_deposit_byte_length() to be called something more reflective of its true functionality?

SuperFluffy

Should we also change get_deposit_byte_length() to be called something more reflective of its true functionality?

Yeah, we should make give this a more meaningful name in its context. It's extremely rescriptive of what it's doing right now but doesn't give extra meaning.

crates/astria-sequencer/src/bridge/bridge_lock_action.rs

SuperFluffy · 2024-09-20T12:22:27Z

crates/astria-sequencer/src/bridge/bridge_lock_action.rs

+pub(crate) fn calculate_base_deposit_fee(deposit: &Deposit) -> Option<u128> {
+    let variable_length = deposit
+        .asset
+        .to_string()


Can you extend Deposit to provide a function like display_len() that calculates this using its formatted representation? Also provide tests in astria-core to test that.

We can avoid this allocation.

Done in c46fa8a, let me know if this implementation works for you!

crates/astria-sequencer/src/bridge/bridge_lock_action.rs

SuperFluffy · 2024-09-20T20:19:56Z

crates/astria-core/src/primitive/v1/asset/denom.rs

@@ -691,4 +709,18 @@ mod tests {

        assert_eq!(None, denom.pop_leading_port_and_channel());
    }
+
+    #[test]
+    fn display_len_ok() {


Provide better test names:

Suggested change

fn display_len_ok() {

fn display_len_returns_expected_number() {

Yeah, thats what I had in mind. But in addition provide more exhaustive tests for some other scenarios. Use #[track_caller] to define the actual assertion and call it repeatedly from a test, similar to what's done here:

astria/crates/astria-core/src/primitive/v1/mod.rs

Lines 771 to 796 in f11f900

#[track_caller]

fn assert_wrong_address_bytes(bad_account: &[u8]) {

let error = Address::<Bech32m>::builder()

.slice(bad_account)

.prefix(ASTRIA_ADDRESS_PREFIX)

.try_build()

.expect_err(

"converting from an incorrectly sized byte slice succeeded where it should have \

failed",

);

let AddressError(AddressErrorKind::IncorrectAddressLength {

received,

}) = error

else {

panic!("expected AddressErrorKind::IncorrectAddressLength, got {error:?}");

};

assert_eq!(bad_account.len(), received);

}

#[test]

fn account_of_incorrect_length_gives_error() {

assert_wrong_address_bytes(&[42; 0]);

assert_wrong_address_bytes(&[42; 19]);

assert_wrong_address_bytes(&[42; 21]);

assert_wrong_address_bytes(&[42; 100]);

}

Here is some background reading on this: https://matklad.github.io/2021/05/31/how-to-test.html#Test-Driven-Design-Ossification

SuperFluffy · 2024-09-20T20:20:14Z

crates/astria-core/src/primitive/v1/asset/denom.rs

+
+    #[test]
+    fn display_len_ok() {
+        let trace_denom = Denom::from("a/long/path/to/denom".parse::<TracePrefixed>().unwrap());


Parse directly into a Denom. This is unnecessarily noisy.

SuperFluffy · 2024-09-20T20:21:04Z

crates/astria-core/src/primitive/v1/asset/denom.rs

+    fn display_len_ok() {
+        let trace_denom = Denom::from("a/long/path/to/denom".parse::<TracePrefixed>().unwrap());
+        assert_eq!(
+            trace_denom.to_string().len(),


Use the length of the input string (expressing it with the track_caller check I suggested above will make that neater).

SuperFluffy · 2024-09-20T20:24:51Z

crates/astria-core/src/primitive/v1/asset/denom.rs

+
+    /// Calculates the length of the display formatted [Denom] without allocating a String.
+    #[must_use]
+    pub fn display_len(&self) -> Option<usize> {


In general we use checked addition for doing math on externally provided data (specifically to calculate balances), but in this case returning usize is more than acceptable.

On any currently available machine you will not blow through 2^64-1 bytes of RAM of allocated strings data.

SuperFluffy · 2024-09-20T20:26:22Z

crates/astria-core/src/primitive/v1/asset/denom.rs

+                let mut len: usize = 0;
+                for segment in &trace.trace.inner {
+                    len = len
+                        .checked_add(segment.port.len())


As mentioned above: checked addition is unnecessary.

SuperFluffy · 2024-09-20T20:27:06Z

crates/astria-core/src/primitive/v1/asset/denom.rs

+    #[must_use]
+    pub fn display_len(&self) -> Option<usize> {
+        match self {
+            Denom::TracePrefixed(trace) => {


Define these on the trace-prefixed and ibc-prefixed types. Then delegate to them in this enum.

SuperFluffy · 2024-09-20T20:29:14Z

crates/astria-sequencer/src/bridge/bridge_lock_action.rs

@@ -99,7 +103,10 @@ impl ActionHandler for BridgeLockAction {
            .await
            .wrap_err("failed to get byte cost multiplier")?;
        let fee = byte_cost_multiplier
-            .saturating_mul(get_deposit_byte_len(&deposit))
+            .saturating_mul(
+                calculate_base_deposit_fee(&deposit)


Because we are doing saturating arithmetic here I would just keep doing that. Instead of returning early just replace it with with u128::MAX

SuperFluffy · 2024-09-20T20:31:36Z

crates/astria-sequencer/src/bridge/bridge_lock_action.rs

@@ -126,7 +133,10 @@ impl ActionHandler for BridgeLockAction {
            .get_bridge_lock_byte_cost_multiplier()
            .await
            .wrap_err("failed to get byte cost multiplier")?;
-        let fee = byte_cost_multiplier.saturating_mul(get_deposit_byte_len(&deposit));
+        let fee = byte_cost_multiplier.saturating_mul(
+            calculate_base_deposit_fee(&deposit)


As above, replace with u128::MAX in this context.

SuperFluffy · 2024-09-20T20:33:41Z

crates/astria-sequencer/src/bridge/bridge_lock_action.rs

+    /// is to allow for more flexibility in overall fees (we have more flexibility multiplying by a
+    /// lower number, and if we want fees to be higher we can just raise the multiplier).
+    #[test]
+    fn get_base_deposit_fee() {


Suggested change

fn get_base_deposit_fee() {

fn calculate_base_deposit_fee() {

SuperFluffy · 2024-09-20T20:36:02Z

crates/astria-sequencer/src/bridge/bridge_lock_action.rs

        state
            .put_account_balance(from_address, &asset, 100 + expected_deposit_fee)
            .unwrap();
        bridge_lock.check_and_execute(&mut state).await.unwrap();
    }
+
+    #[test]
+    fn test_calculate_base_deposit_fee() {


You are in a test so don't call the test a test:

Suggested change

fn test_calculate_base_deposit_fee() {

fn calculated_fee_matches_expected_value() {

I'd also refactor the test to use #[track_caller]. See my comment for testing display_len in astria_core.

SuperFluffy · 2024-09-20T20:37:01Z

crates/astria-sequencer/src/bridge/bridge_lock_action.rs

+            source_action_index: 0,
+        };
+        let calculated_len_2 = calculate_base_deposit_fee(&deposit).unwrap();
+        assert!(calculated_len_2 >= calculated_len_1);


Try to avoid assert!. If this fails the reader will not know what exactly failed. It's better to use assert_eq!. IMO that's also fine here because all of the values are deterministic.

SuperFluffy · 2024-09-20T20:37:48Z

crates/astria-sequencer/src/bridge/bridge_lock_action.rs

@@ -34,6 +34,10 @@ use crate::{
    utils::create_deposit_event,
 };

+// The base byte length of a deposit, as determined by the unit test


Make this a doc comment, i.e. start with ///

SuperFluffy · 2024-09-20T20:37:59Z

crates/astria-sequencer/src/bridge/bridge_lock_action.rs

@@ -34,6 +34,10 @@ use crate::{
    utils::create_deposit_event,
 };

+// The base byte length of a deposit, as determined by the unit test
+// `get_base_deposit_byte_length()` below.


Suggested change

// `get_base_deposit_byte_length()` below.

/// `tests::get_base_deposit_byte_length()` below.

Calculate deposit byte length based only on asset and dest. chain add…

3b6e795

…ress

github-actions bot added the sequencer pertaining to the astria-sequencer crate label Sep 16, 2024

ethanoroshiba added 2 commits September 16, 2024 11:03

Fix compile?

df2f701

Fix slice issue for rust 1.76

b1c855b

ethanoroshiba marked this pull request as ready for review September 16, 2024 16:29

ethanoroshiba requested review from a team, joroshiba, SuperFluffy and noot as code owners September 16, 2024 16:29

noot approved these changes Sep 17, 2024

View reviewed changes

SuperFluffy requested changes Sep 17, 2024

View reviewed changes

requested changes

9c7090b

ethanoroshiba requested a review from SuperFluffy September 18, 2024 14:45

Changes from offline discussion

00da52a

SuperFluffy reviewed Sep 19, 2024

View reviewed changes

ethanoroshiba added 3 commits September 19, 2024 15:31

Requested changes

10d1007

Merge branch 'main' into ENG-815/deposit_fixed_ints

e06ad1a

Fix changes from merge

c9e3cdf

ethanoroshiba requested a review from SuperFluffy September 19, 2024 20:46

SuperFluffy requested changes Sep 20, 2024

View reviewed changes

ethanoroshiba added 3 commits September 20, 2024 11:12

Requested changes

c46fa8a

minor clarification

e7a9820

Delete unused snapshot

40d28dc

SuperFluffy reviewed Sep 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(sequencer)!: change `Deposit` byte length calculation #1507

chore(sequencer)!: change `Deposit` byte length calculation #1507

ethanoroshiba commented Sep 16, 2024 •

edited

Loading

noot left a comment

SuperFluffy left a comment

SuperFluffy commented Sep 17, 2024

ethanoroshiba commented Sep 18, 2024

SuperFluffy left a comment

SuperFluffy Sep 20, 2024

ethanoroshiba Sep 20, 2024

SuperFluffy Sep 20, 2024 •

edited

Loading

SuperFluffy Sep 20, 2024

SuperFluffy Sep 20, 2024

SuperFluffy Sep 20, 2024 •

edited

Loading

SuperFluffy Sep 20, 2024 •

edited

Loading

SuperFluffy Sep 20, 2024

SuperFluffy Sep 20, 2024

SuperFluffy Sep 20, 2024

SuperFluffy Sep 20, 2024

SuperFluffy Sep 20, 2024

SuperFluffy Sep 20, 2024

SuperFluffy Sep 20, 2024

SuperFluffy Sep 20, 2024

	fn display_len_ok() {
	fn display_len_returns_expected_number() {

	#[track_caller]
	fn assert_wrong_address_bytes(bad_account: &[u8]) {
	let error = Address::<Bech32m>::builder()
	.slice(bad_account)
	.prefix(ASTRIA_ADDRESS_PREFIX)
	.try_build()
	.expect_err(
	"converting from an incorrectly sized byte slice succeeded where it should have \
	failed",
	);
	let AddressError(AddressErrorKind::IncorrectAddressLength {
	received,
	}) = error
	else {
	panic!("expected AddressErrorKind::IncorrectAddressLength, got {error:?}");
	};
	assert_eq!(bad_account.len(), received);
	}

	#[test]
	fn account_of_incorrect_length_gives_error() {
	assert_wrong_address_bytes(&[42; 0]);
	assert_wrong_address_bytes(&[42; 19]);
	assert_wrong_address_bytes(&[42; 21]);
	assert_wrong_address_bytes(&[42; 100]);
	}

	fn get_base_deposit_fee() {
	fn calculate_base_deposit_fee() {

	fn test_calculate_base_deposit_fee() {
	fn calculated_fee_matches_expected_value() {

	// `get_base_deposit_byte_length()` below.
	/// `tests::get_base_deposit_byte_length()` below.

chore(sequencer)!: change Deposit byte length calculation #1507

Are you sure you want to change the base?

chore(sequencer)!: change Deposit byte length calculation #1507

Conversation

ethanoroshiba commented Sep 16, 2024 • edited Loading

Summary

Background

Changes

Testing

Breaking Changelist

Related Issues

noot left a comment

Choose a reason for hiding this comment

SuperFluffy left a comment

Choose a reason for hiding this comment

SuperFluffy commented Sep 17, 2024

ethanoroshiba commented Sep 18, 2024

SuperFluffy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SuperFluffy Sep 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SuperFluffy Sep 20, 2024 • edited Loading

Choose a reason for hiding this comment

SuperFluffy Sep 20, 2024 • edited Loading

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

chore(sequencer)!: change `Deposit` byte length calculation #1507

chore(sequencer)!: change `Deposit` byte length calculation #1507

ethanoroshiba commented Sep 16, 2024 •

edited

Loading

SuperFluffy Sep 20, 2024 •

edited

Loading

SuperFluffy Sep 20, 2024 •

edited

Loading

SuperFluffy Sep 20, 2024 •

edited

Loading