relay/circuit-v2: Mark Reservation::expire as required

[mxinden]

The Go implementation of the circuit relay v2 implementation treats the Reservation::expire field as required:

result := &Reservation{}
result.Expiration = time.Unix(int64(rsvp.GetExpire()), 0)
if result.Expiration.Before(time.Now()) {
    return nil, fmt.Errorf("received reservation with expiration date in the past: %s", result.Expiration)
}

https://github.com/libp2p/go-libp2p/blob/bfee9f593553b3cdaecc351f04b98aac78b8d8af/p2p/protocol/circuitv2/client/reservation.go#L88-L92

Where rsvp.GetExpire returns 0 when Reservation::expire is not set:

func (m *Reservation) GetExpire() uint64 {
    if m != nil && m.Expire != nil {
        return *m.Expire
    }
    return 0
}

https://github.com/libp2p/go-libp2p/blob/bfee9f593553b3cdaecc351f04b98aac78b8d8af/p2p/protocol/circuitv2/pb/circuit.pb.go#L414-L419

While inexplicable to me why Go treats the non-set case equal to the default value (0), we unfortunately have to take the go implementation as the source of truth, given that it is already released.

With the above in mind and to prevent confusion for other implementations in languages which do not treat the non-set case equal to the default value (0), this commit marks the Reservation::expire field as required.

What do folks think? If I am not mistaken, the change below is non-breaking for the Go implementation, given that it effectlively already treats expire as required.

[mxinden]

How should we proceed with the optional fields in Limit?

message Limit {
  optional uint32 duration = 1; // seconds
  optional uint64 data = 2;     // bytes
}

func (m *Limit) GetDuration() uint32 {
    if m != nil && m.Duration != nil {
        return *m.Duration
    }
    return 0
}

https://github.com/libp2p/go-libp2p/blob/bfee9f593553b3cdaecc351f04b98aac78b8d8af/p2p/protocol/circuitv2/pb/circuit.pb.go#L476-L481

func (m *Limit) GetData() uint64 {
    if m != nil && m.Data != nil {
        return *m.Data
    }
    return 0
}

https://github.com/libp2p/go-libp2p/blob/bfee9f593553b3cdaecc351f04b98aac78b8d8af/p2p/protocol/circuitv2/pb/circuit.pb.go#L483-L488

---

How should we treat the-Golang-Protobuf-Compiler-Treating-Nil-and-Default-of-base-types-the-same more generally @marten-seemann @vyzo? Is there an alternative compiler go-libp2p could use?

[marten-seemann]

I don't see any problem here. If you care about the distinction, just do a nil check on m.Data, it's an exported field.

[vyzo]

they are both truly optional i think, so we shouldnt maul them.

[mxinden]

I don't see any problem here. If you care about the distinction, just do a nil check on m.Data, it's an exported field.

So according to the Protobuf definition a limit with a duration but no data field is valid. If I am not mistaken, the Go implementation would instead of a non-set data field in the protobuf set the LimitData to 0. Or is a LimitData of 0 equal to a non-set LimitData?

    limit := msg.GetLimit()
    if limit != nil {
        result.LimitDuration = time.Duration(limit.GetDuration()) * time.Second
        result.LimitData = limit.GetData()
    }

https://github.com/libp2p/go-libp2p/blob/bfee9f593553b3cdaecc351f04b98aac78b8d8af/p2p/protocol/circuitv2/client/reservation.go#L119-L123

they are both truly optional i think, so we shouldnt maul them.

@vyzo I am not following. Could you paraphrase the above?

[vyzo]

i meant that either limit could be unset, and a value of 0 is meaningless so i dont see a problem with treating it is unset.