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.

Sign up for GitHub

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

ibc: add withdrawal timestamp rounding chain rule #4065

Merged
merged 6 commits into from
Mar 26, 2024
Merged

ibc: add withdrawal timestamp rounding chain rule #4065

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
fd5d494
pcli: round withdrawal timestamp
avahowell Mar 21, 2024
24c161d
add chain rule: enforce nearest-even-second timestamps for withdrawals
avahowell Mar 21, 2024
abf66d9
round to nearest single second
avahowell Mar 21, 2024
fb6bf89
enforce timestamp resolution in ics20withdrawl to minutes, round to h…
avahowell Mar 21, 2024
58cf12f
change pcli default to round timestamps to 10 minutes
avahowell Mar 22, 2024
1441a97
fix error text for timeout timestamp
avahowell Mar 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions crates/bin/pcli/src/command/tx.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1052,17 +1052,20 @@ impl TxCmd {
};

// get the current time on the local machine
let current_time_u64_ms = SystemTime::now()
let current_time_ns = SystemTime::now()
.duration_since(UNIX_EPOCH)
.expect("Time went backwards")
.as_nanos() as u64;

let mut timeout_timestamp = *timeout_timestamp;
if timeout_timestamp == 0u64 {
// add 2 days to current time
timeout_timestamp = current_time_u64_ms + 1.728e14 as u64;
timeout_timestamp = current_time_ns + 1.728e14 as u64;
}

// round to the nearest even second
timeout_timestamp += timeout_timestamp % 2_000_000_000;

fn parse_denom_and_amount(value_str: &str) -> anyhow::Result<(Amount, Metadata)> {
let denom_re = Regex::new(r"^([0-9.]+)(.+)$").context("denom regex invalid")?;
if let Some(captures) = denom_re.captures(value_str) {
Expand Down
11 changes: 10 additions & 1 deletion crates/core/component/shielded-pool/src/ics20_withdrawal.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ pub struct Ics20Withdrawal {
// prevent relayer censorship attacks. The core IBC implementation does this
// in its handling of validation of timeouts.
pub timeout_height: IbcHeight,
// the timestamp at which this transfer expires.
// the timestamp at which this transfer expires, in nanoseconds after unix epoch.
pub timeout_time: u64,
// the source channel used for the withdrawal
pub source_channel: ChannelId,
Expand Down Expand Up @@ -77,6 +77,15 @@ impl Ics20Withdrawal {
anyhow::bail!("timeout time must be non-zero");
}

// in order to prevent clients from inadvertantly identifying themselves by their clock
// skew, enforce that timeout time is rounded to the nearest even second
if self.timeout_time % 2_000_000_000 != 0 {
Copy link
Member

Choose a reason for hiding this comment

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

What's the motivation for rounding to 2-second intervals rather than 1-second ones?

I might have miscommunicated in Discord, I was thinking of "even second" as meaning "on the second boundary" -- but looking at this I'm wondering what the right resolution should be. 1s is an "easy" answer, but what about 10 or 60, or greater?

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 don't have a good intuition for 'what amount of rounding covers the skew of >99% of machines', but I think single seconds might not be enough. From a user's standpoint quantizing withdrawal timeouts to minutes seems fine, so maybe 1 minute would be better

Copy link
Member

Choose a reason for hiding this comment

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

Let's set two rules:

  • in consensus, require that timeout timestamps are truncated to one-minute boundaries
  • in our clients, set the default timeout timestamp to be truncated to 1h boundaries

This leaves the possibility for someone who has a pressing need for finer grained timestamps to do so while setting a good default

Copy link
Contributor

Choose a reason for hiding this comment

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

For posterity, what's the reasoning behind baking a privacy setting like this into consensus? Is it to prevent mistakes with alternative clients?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, the "obvious" thing to do is set the timestamp as Utc.now() + timeout, and the existence of this rule means that that code will immediately break.

anyhow::bail!(
"withdrawal timeout timestamp {} is not an even number of seconds",
self.timeout_time
);
}

// NOTE: we could validate the destination chain address as bech32 to prevent mistyped
// addresses, but this would preclude sending to chains that don't use bech32 addresses.

Expand Down
Loading