Remove the concept of a "zero Height" #2346

plafer · 2022-06-27T21:31:35Z

Description

PR author checklist:

Added changelog entry, using unclog.
Added tests: integration (for Hermes) or unit/mock tests (for modules).
Linked to GitHub issue.
Updated code comments and documentation (e.g., docs/).

Reviewer checklist:

Reviewed Files changed in the GitHub PR explorer.
Manually tested (in case integration/unit/mock tests are absent).

mzabaluev

Have you thought of making the revision_height member of Height a NonZeroU64?
And maybe making both members non-public while we are at it.
This will enforce avoidance of 0, and ensure that std::mem::size_of::<Option<Height>>() is 16.

mzabaluev · 2022-06-28T13:36:11Z

modules/src/core/ics03_connection/msgs/conn_open_ack.rs

@@ -29,11 +29,8 @@ pub struct MsgConnectionOpenAck {
 impl MsgConnectionOpenAck {
    /// Getter for accessing the `consensus_height` field from this message. Returns the special
    /// value `Height(0)` if this field is not set.


The comments like this need to be updated too.

I didn't find any other comments that referred to "Height zero"

4a9dca7

…zero-complete

plafer · 2022-06-28T19:02:41Z

Have you thought of making the revision_height member of Height a NonZeroU64? And maybe making both members non-public while we are at it. This will enforce avoidance of 0, and ensure that std::mem::size_of::<Option<Height>>() is 16.

Yes, my original plan was to use NonZeroU64, but then creating a new Height looks like:

// e.g. when building from a tendermint header
Height::new(chain_id(), header_height_revision_number.try_into().map_err(|_| ...)?)

Basically since all the other types use u64, building a NonZeroU64 from a u64 requires a try_into().map_err(), and so I found it more elegant to have new() return a Result instead. We also can't use them internally because they currently don't have a stable API for addition, which we need.

I am also planning on making the members private.

modules/src/core/ics04_channel/packet.rs

modules/src/core/ics04_channel/timeout.rs

modules/src/core/ics04_channel/packet.rs

modules/src/core/ics04_channel/timeout.rs

hu55a1n1 · 2022-06-30T09:46:06Z

Great work @plafer! 👍 Just left some comments and questions.

…zero-complete

…ght`

romac

Awesome work @plafer, thanks so much!

Prior to this PR, the `Height` type used a special `Height::zero()` value to denote the absence of height value. This caused ad hoc checks of whether a height is zero in various places. From now on, we instead use `Option<Height>` on places where the height value may be optional, and require non-zero value in the `Height` constructor. - Optional height parameters should use `Option<Height>` instead of `Height`. - Checking of `height == Height::zero()` is replaced with Option matching. --- * From<Height> for String fix * Use Option<Height> instead of Height::zero for consensus_height * recv_packet still had Height::zero() in events * TryFrom<RawHeight> for Height * pre height parse changes * Packet.timeout_height is now an Option<Height> * rustdoc * Remove Height::with_revision_height * commit pre-modifying cli * Finish Height::new() returns Result * remove Height::is_zero() * Remove `Height::zero()` * Remove Height::default() * Height fields accessors * use revision_height accessor * use revision_number accessor * make Height fields private (WIP) * compile error fixes * FIXME in transfer.rs addressed * Use existing constants instead of hardcoded strings * changelog * TimeoutHeight newtype * Use TimeoutHeight everywhere * Fixes after merging with master * has_expired() fix * doc url * fix send_packet test * Remove timeout_height == 0 && timestamp == 0 check * Remove `has_timeout()` * Change TimeoutHeight impl * docstrings * tests fix * fix history manipulation test * transfer test: no timeout * msgtransfer test * packet no timeout or timestamp test * send_packet timeout height equal destination chain height * send_packet test with timeout = height - 1 * test invalid timeout height * fix docstring * clippy * Fix clippy warnings introduced in Rust 1.62 * Fix serialization of `TimeoutHeight` to match the format used by `Height` Co-authored-by: Romain Ruetschi <romain@informal.systems>

plafer added 8 commits June 24, 2022 09:29

From<Height> for String fix

d20ac5b

Use Option<Height> instead of Height::zero for consensus_height

aae757b

recv_packet still had Height::zero() in events

00957d2

TryFrom<RawHeight> for Height

047727f

pre height parse changes

444f2c3

Packet.timeout_height is now an Option<Height>

0b19d58

rustdoc

bf6192d

Remove Height::with_revision_height

2e47253

mzabaluev reviewed Jun 28, 2022

View reviewed changes

plafer added 5 commits June 28, 2022 13:39

commit pre-modifying cli

56378b5

Merge remote-tracking branch 'origin/master' into plafer/1009-height-…

0816c29

…zero-complete

Finish Height::new() returns Result

036da49

remove Height::is_zero()

362dab3

Remove Height::zero()

d1f9e45

plafer added 8 commits June 28, 2022 17:05

Remove Height::default()

52941d4

Height fields accessors

7507bfd

use revision_height accessor

2004958

use revision_number accessor

c2418c1

make Height fields private (WIP)

97fd832

compile error fixes

fca5c9e

FIXME in transfer.rs addressed

7730de8

Use existing constants instead of hardcoded strings

b10a703

plafer marked this pull request as ready for review June 28, 2022 21:43

plafer requested review from adizere, romac, seanchen1991, ancazamfir and hu55a1n1 as code owners June 28, 2022 21:43

changelog

1bc18b6

plafer added 3 commits June 29, 2022 16:40

Fixes after merging with master

bb8192e

has_expired() fix

78eb9f1

doc url

1178a2e

hu55a1n1 reviewed Jun 30, 2022

View reviewed changes

modules/src/core/ics04_channel/packet.rs Outdated Show resolved Hide resolved

hu55a1n1 reviewed Jun 30, 2022

View reviewed changes

modules/src/core/ics04_channel/timeout.rs Outdated Show resolved Hide resolved

hu55a1n1 reviewed Jun 30, 2022

View reviewed changes

modules/src/core/ics04_channel/packet.rs Show resolved Hide resolved

hu55a1n1 reviewed Jun 30, 2022

View reviewed changes

modules/src/core/ics04_channel/timeout.rs Show resolved Hide resolved

plafer and others added 18 commits June 30, 2022 12:50

fix send_packet test

a1bd379

Remove timeout_height == 0 && timestamp == 0 check

6e2cf23

Remove has_timeout()

87cbfed

Change TimeoutHeight impl

95eb992

docstrings

b3d4248

tests fix

6ed145b

fix history manipulation test

a2ec5aa

Merge remote-tracking branch 'origin/master' into plafer/1009-height-…

c958e62

…zero-complete

transfer test: no timeout

8eff6d6

msgtransfer test

62cad52

packet no timeout or timestamp test

0d3d2f2

send_packet timeout height equal destination chain height

c1a2a86

send_packet test with timeout = height - 1

a8a958c

test invalid timeout height

c0fafdb

fix docstring

4a9dca7

clippy

6a58582

Fix clippy warnings introduced in Rust 1.62

e262c3c

Fix serialization of TimeoutHeight to match the format used by `Hei…

f5c3f67

…ght`

romac approved these changes Jul 4, 2022

View reviewed changes

romac merged commit 16c356a into master Jul 4, 2022

romac deleted the plafer/1009-height-zero-complete branch July 4, 2022 08:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove the concept of a "zero Height" #2346

Remove the concept of a "zero Height" #2346

plafer commented Jun 27, 2022 •

edited by romac

Loading

mzabaluev left a comment •

edited

Loading

mzabaluev Jun 28, 2022

plafer Jun 30, 2022

plafer commented Jun 28, 2022 •

edited

Loading

hu55a1n1 commented Jun 30, 2022

romac left a comment

Remove the concept of a "zero Height" #2346

Remove the concept of a "zero Height" #2346

Conversation

plafer commented Jun 27, 2022 • edited by romac Loading

Description

PR author checklist:

Reviewer checklist:

mzabaluev left a comment • edited Loading

Choose a reason for hiding this comment

mzabaluev Jun 28, 2022

Choose a reason for hiding this comment

plafer Jun 30, 2022

Choose a reason for hiding this comment

plafer commented Jun 28, 2022 • edited Loading

hu55a1n1 commented Jun 30, 2022

romac left a comment

Choose a reason for hiding this comment

plafer commented Jun 27, 2022 •

edited by romac

Loading

mzabaluev left a comment •

edited

Loading

plafer commented Jun 28, 2022 •

edited

Loading