ibc: add withdrawal timestamp rounding chain rule #4065
Conversation
avahowell
added
A-IBC
avahowell
force-pushed
the
ibc-timestamps-fix
branch
from
af17be2 to
24c161d
Compare
| @@ -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 { | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
For posterity, what's the reasoning behind baking a privacy setting like this into consensus? Is it to prevent mistakes with alternative clients?
There was a problem hiding this comment.
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.
|
Updated the chain rule to require rounding to 1 minute, updated pcli to round to 1 hour |
|
@ValarDragon comments:
|
|
Changed the pcli default to round to nearest 10 minute, per suggestion |
this is a consensus-breaking change
Fixes #4064