Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(sequencer): make ABCI response for account balances deterministic #1574

Merged
merged 9 commits into from
Oct 21, 2024
214 changes: 214 additions & 0 deletions crates/astria-core/src/primitive/v1/asset/denom.rs
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the Ord impl here, can you provide exhaustive tests that demonstrate and document the order of the denoms?

Naively, I would expect a lexical ordering among trace-prefixed and ibc-prefixed denoms, so it would be great to have tests that also demonstrate that ordering.

I think I should also walk back my suggestion to order ibc-prefixed, trace-prefixed later: a naive implementation that keeps ibc denominations in string form (without parsing) would just use lexical ordering of the strings. I think the parsed variant here should also reflect that.

In addition to unit tests for IbcPrefixed and TracePrefixed, do a more complex example:

  1. create a "string list" of ibc denomination strings (mixing both trace-prefixed and ibc-prefixed).
  2. parse this list as a Denom list.
  3. sort the string list (this will give lexical order).
  4. sort the denom list by its Ord implementation and then use to_string to convert all elements to strings again
  5. assert that results of steps 3 and 4 are the same.

Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,29 @@ impl FromStr for Denom {
}
}

impl PartialOrd for Denom {
fn partial_cmp(&self, other: &Self) -> Option<std::cmp::Ordering> {
Some(self.cmp(other))
}
}

impl Ord for Denom {
/// Comparison meant to mirror the lexical ordering based on a [`Denom`]'s display-formatted
/// string, but without allocation. If both denoms have the same prefix, the prefix
/// comparison function is called. Otherwise, the [`TracePrefixed`] denom is compared with the
/// IBC prefix `ibc/`.
fn cmp(&self, other: &Self) -> std::cmp::Ordering {
match (self, other) {
(Self::TracePrefixed(lhs), Self::TracePrefixed(rhs)) => lhs.cmp(rhs),
(Self::TracePrefixed(trace), Self::IbcPrefixed(_)) => compare_trace_to_ibc(trace),
(Self::IbcPrefixed(lhs), Self::IbcPrefixed(rhs)) => lhs.cmp(rhs),
(Self::IbcPrefixed(_), Self::TracePrefixed(trace)) => {
compare_trace_to_ibc(trace).reverse()
}
}
}
}

#[derive(Debug, thiserror::Error)]
#[error(transparent)]
pub struct ParseDenomError(ParseDenomErrorKind);
Expand Down Expand Up @@ -350,6 +373,40 @@ impl TracePrefixed {
}
}

impl PartialOrd for TracePrefixed {
fn partial_cmp(&self, other: &Self) -> Option<std::cmp::Ordering> {
Some(self.cmp(other))
}
}

impl Ord for TracePrefixed {
/// This comparison is meant to mirror the lexical ordering of a [`TracePrefixed`]'s
/// display-formatted string without allocation. It returns the collowing comparisons:
/// - If both traces are empty, compares the base denoms.
/// - If one trace is empty, compares the base denom to the leading port of the other trace.
/// - If both traces are non-empty, compares the traces, then compares the base denoms.
fn cmp(&self, other: &Self) -> std::cmp::Ordering {
match (self.trace.is_empty(), other.trace.is_empty()) {
(true, true) => self.base_denom.cmp(&other.base_denom),
(true, false) => self.base_denom.as_str().cmp(
other
.trace
.leading_port()
.expect("leading port should be `Some` after checking for its existence"),
),
(false, true) => self
.trace
.leading_port()
.expect("leading port should be `Some` after checking for its existence")
.cmp(other.base_denom.as_str()),
(false, false) => self
.trace
.cmp(&other.trace)
.then_with(|| self.base_denom.cmp(&other.base_denom)),
}
}
}

#[derive(Debug, Clone, Hash, PartialEq, Eq)]
struct TraceSegments {
inner: VecDeque<PortAndChannel>,
Expand Down Expand Up @@ -420,6 +477,29 @@ impl FromStr for TraceSegments {
Ok(parsed_segments)
}
}

impl PartialOrd for TraceSegments {
fn partial_cmp(&self, other: &Self) -> Option<std::cmp::Ordering> {
Some(self.cmp(other))
}
}

impl Ord for TraceSegments {
/// Returns the first non-equal comparison between two trace segments. If one doesn't exist,
/// returns the shortest segment, and if they are entirely equal, returns
/// [`std::cmp::Ordering::Equal`].
fn cmp(&self, other: &Self) -> std::cmp::Ordering {
self.inner
.iter()
.zip(other.inner.iter())
.find_map(|(self_segment, other_segment)| {
Some(self_segment.cmp(other_segment))
.filter(|&cmp| cmp != std::cmp::Ordering::Equal)
})
.unwrap_or(self.inner.len().cmp(&other.inner.len()))
}
}

#[derive(Debug, Clone, Hash, PartialEq, Eq)]
pub struct PortAndChannel {
port: String,
Expand All @@ -438,6 +518,21 @@ impl PortAndChannel {
}
}

impl PartialOrd for PortAndChannel {
fn partial_cmp(&self, other: &Self) -> Option<std::cmp::Ordering> {
Some(self.cmp(other))
}
}

impl Ord for PortAndChannel {
/// Returns port comparison if not equal, otherwise returns channel comparison.
fn cmp(&self, other: &Self) -> std::cmp::Ordering {
self.port
.cmp(&other.port)
.then_with(|| self.channel.cmp(&other.channel))
}
}

impl std::fmt::Display for TracePrefixed {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
for segment in &self.trace.inner {
Expand Down Expand Up @@ -617,6 +712,18 @@ impl FromStr for IbcPrefixed {
}
}

impl PartialOrd for IbcPrefixed {
fn partial_cmp(&self, other: &Self) -> Option<std::cmp::Ordering> {
Some(self.cmp(other))
}
}

impl Ord for IbcPrefixed {
fn cmp(&self, other: &Self) -> std::cmp::Ordering {
self.id.cmp(&other.id)
}
}

#[cfg(feature = "serde")]
mod serde_impl {
use serde::{
Expand Down Expand Up @@ -683,6 +790,19 @@ mod serde_impl {
}
}

/// Compares a trace prefixed denom to an IBC prefixed denom. This is meant to mirror the lexical
/// ordering of [`TracePrefixed`] and [`IbcPrefixed`] display-formatted strings without allocation.
/// If the trace prefixed denom has a leading port, it is compared to the IBC prefix `ibc/`.
/// Otherwise, the trace's base denom is compared to the IBC prefix.
fn compare_trace_to_ibc(trace: &TracePrefixed) -> std::cmp::Ordering {
// A trace prefixed denom should never begin with "ibc/", so we can compare direclty to the ibc
// prefix as opposed to the entire ibc-prefixed denomination.
match trace.trace.leading_port() {
Some(port) => port.cmp("ibc/"),
None => trace.base_denom.as_str().cmp("ibc/"),
}
}

#[cfg(test)]
mod tests {
use super::{
Expand Down Expand Up @@ -812,4 +932,98 @@ mod tests {
let denom = denom_str.parse::<Denom>().unwrap();
assert_eq!(denom_str.len(), denom.display_len());
}

#[test]
fn ibc_prefixed_ord_matches_lexical_sort() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to have these be more atomic and not sort a vec (it's pretty difficult to read).

Instead do something like:

#[track_caller]
fn assert_denom_order<T: FromStr>(left: &str, right: &str]) {
    let left_denom: T = left.parse().unwrap();
    let right_denom: T = right.parse().unwrap();
    assert_eq!(left_denom.cmp(&right_denom), left.cmp(right));
}

#[test]
fn denoms_are_lexically_ordered() {
    assert_denom_order::<Denom>("ibc/etc", "trace/etc");
    todo!("and all the rest");
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did something similar in 0e5eeb5, let me know if it works for you!

let mut ibc_prefixed = vec![
format!("ibc/{}", hex::encode([135u8; 32]))
.parse::<IbcPrefixed>()
.unwrap(),
format!("ibc/{}", hex::encode([4u8; 32]))
.parse::<IbcPrefixed>()
.unwrap(),
format!("ibc/{}", hex::encode([0u8; 32]))
.parse::<IbcPrefixed>()
.unwrap(),
format!("ibc/{}", hex::encode([240u8; 32]))
.parse::<IbcPrefixed>()
.unwrap(),
format!("ibc/{}", hex::encode([60u8; 32]))
.parse::<IbcPrefixed>()
.unwrap(),
];
let mut ibc_prefixed_lexical = ibc_prefixed.clone();
ibc_prefixed.sort_unstable();
ibc_prefixed_lexical.sort_unstable_by_key(ToString::to_string);
assert_eq!(ibc_prefixed, ibc_prefixed_lexical);
}

#[test]
fn trace_prefixed_ord_matches_lexical_sort() {
let mut trace_prefixed = vec![
"ethan/was/here".parse::<TracePrefixed>().unwrap(),
"nria".parse::<TracePrefixed>().unwrap(),
"pretty/long/trace/prefixed/denom"
.parse::<TracePrefixed>()
.unwrap(),
"_using/underscore/here".parse::<TracePrefixed>().unwrap(),
"astria/test/asset".parse::<TracePrefixed>().unwrap(),
];
let mut trace_prefixed_lexical = trace_prefixed.clone();
trace_prefixed.sort_unstable();
trace_prefixed_lexical.sort_unstable_by_key(ToString::to_string);
assert_eq!(trace_prefixed, trace_prefixed_lexical);
}

#[test]
fn trace_and_ibc_prefixed_ord_matches_lexical_sort() {
let ibc_1 = format!("ibc/{}", hex::encode([135u8; 32]));
let ibc_2 = format!("ibc/{}", hex::encode([4u8; 32]));
let ibc_3 = format!("ibc/{}", hex::encode([0u8; 32]));
let ibc_4 = format!("ibc/{}", hex::encode([240u8; 32]));
let ibc_5 = format!("ibc/{}", hex::encode([60u8; 32]));
let trace_1 = "ethan/was/here";
let trace_2 = "nria";
let trace_3 = "pretty/long/trace/prefixed/denom";
let trace_4 = "_using/underscore/here";
let trace_5 = "astria/test/asset";

assert_ord_matches_lexical(&ibc_1, &ibc_1);
assert_ord_matches_lexical(&ibc_1, &ibc_2);
assert_ord_matches_lexical(&ibc_1, &ibc_3);
assert_ord_matches_lexical(&ibc_1, &ibc_4);
assert_ord_matches_lexical(&ibc_1, &ibc_5);

assert_ord_matches_lexical(&ibc_2, &ibc_3);
assert_ord_matches_lexical(&ibc_2, &ibc_4);
assert_ord_matches_lexical(&ibc_2, &ibc_5);

assert_ord_matches_lexical(&ibc_3, &ibc_4);
assert_ord_matches_lexical(&ibc_3, &ibc_5);

assert_ord_matches_lexical(&ibc_4, &ibc_5);

assert_ord_matches_lexical(trace_1, trace_1);
assert_ord_matches_lexical(trace_1, trace_2);
assert_ord_matches_lexical(trace_1, trace_2);
assert_ord_matches_lexical(trace_1, trace_3);
assert_ord_matches_lexical(trace_1, trace_4);
assert_ord_matches_lexical(trace_1, trace_5);

assert_ord_matches_lexical(trace_2, trace_3);
assert_ord_matches_lexical(trace_2, trace_4);
assert_ord_matches_lexical(trace_2, trace_5);

assert_ord_matches_lexical(trace_3, trace_4);
assert_ord_matches_lexical(trace_3, trace_5);

assert_ord_matches_lexical(trace_4, trace_5);
}

#[track_caller]
fn assert_ord_matches_lexical(left: &str, right: &str) {
let left_denom = left.parse::<Denom>().unwrap();
let right_denom = right.parse::<Denom>().unwrap();
assert_eq!(left_denom.cmp(&right_denom), left.cmp(right));
}
}
23 changes: 22 additions & 1 deletion crates/astria-core/src/protocol/account/v1alpha1/mod.rs
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are any of the PartialEq, Eq, Ord, and PartialOrd impls even necessary at this point?

If they are not necessary, I'd rather remove them.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've removed the impls for Ord and PartialOrd from AssetBalance, but aside from that all of them are necessary. Denom::cmp calls either IbcPrefixed::cmp or TracePrefixed::cmp, and trace calls TraceSegments::cmp, etc.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, those make sense. I just thought of AssetBalance :)

Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ enum AssetBalanceErrorKind {
InvalidDenom { source: ParseDenomError },
}

#[derive(Clone, Debug, PartialEq)]
#[derive(Clone, Debug, Eq)]
pub struct AssetBalance {
pub denom: Denom,
pub balance: u128,
Expand Down Expand Up @@ -55,6 +55,27 @@ impl AssetBalance {
}
}

impl PartialOrd for AssetBalance {
ethanoroshiba marked this conversation as resolved.
Show resolved Hide resolved
fn partial_cmp(&self, other: &Self) -> Option<std::cmp::Ordering> {
Some(self.cmp(other))
}
}

impl Ord for AssetBalance {
fn cmp(&self, other: &Self) -> std::cmp::Ordering {
match self.denom.cmp(&other.denom) {
std::cmp::Ordering::Equal => self.balance.cmp(&other.balance),
other => other,
}
}
}

impl PartialEq for AssetBalance {
fn eq(&self, other: &Self) -> bool {
self.denom == other.denom && self.balance == other.balance
}
}

impl raw::BalanceResponse {
/// Converts an astria native [`BalanceResponse`] to a
/// protobuf [`raw::BalanceResponse`].
Expand Down
11 changes: 10 additions & 1 deletion crates/astria-sequencer/src/accounts/query.rs
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,8 @@ async fn get_trace_prefixed_account_balances<S: StateRead>(
stream.try_collect::<Vec<_>>().await
}

/// Returns a list of [`AssetBalance`]s for the provided address. `AssetBalance`s are sorted
/// alphabetically by [`asset::Denom`].
pub(crate) async fn balance_request(
storage: Storage,
request: request::Query,
Expand All @@ -88,7 +90,7 @@ pub(crate) async fn balance_request(
Err(err_rsp) => return err_rsp,
ethanoroshiba marked this conversation as resolved.
Show resolved Hide resolved
};

let balances = match get_trace_prefixed_account_balances(&snapshot, &address).await {
let mut balances = match get_trace_prefixed_account_balances(&snapshot, &address).await {
Ok(balance) => balance,
Err(err) => {
return response::Query {
Expand All @@ -100,6 +102,13 @@ pub(crate) async fn balance_request(
};
}
};

// Unstable sort does not allocate auxillary memory and is typically faster. This utilizes a
ethanoroshiba marked this conversation as resolved.
Show resolved Hide resolved
// custom `Ord` implementation which sorts by `Denom` in alphabetical order. The comparison
// should never return `Ordering::Equal` since `Denom` is a unique identifier, hence the
// unstable sort will be deterministic.
balances.sort_unstable();
ethanoroshiba marked this conversation as resolved.
Show resolved Hide resolved

let payload = BalanceResponse {
height: height.value(),
balances,
Expand Down
Loading