Implement clone for some structs

[adityapk00]

Derive Clone for some structs to make it easier for light clients to move around transparent inputs

[str4d]

Do you actually require Eq to be implemented? I know that PartialEq is usually required for test comparisons.

[zancas]

The reason to include Eq (which is derived from PartialEq) is to enforce x == x (reflexivity), which is relevant for this type.

PartialEq's are not strict equivalence relations because they do not enforce x == x.

This counterintuitive property makes sense when you think about the concrete example of IEEE floating-point values. They are specified to be NaN in some cases (e.g. 0.0/0.0), but NaN != NaN, i.e. NaN is not reflexive.

e.g.:

assert_eq!(0.0/0.0 == 0.0/0.0, false)

Since f32 and f64 are the only types in the standard library that are PartialEq, but not Eq, it's generally best practice to implement Eq on any type that doesn't include them.

Most of what I am asserting here is rehashed from the Equality Tests section of this book:

http://shop.oreilly.com/product/0636920040385.do

[zancas]

Here's a lot of the same info, but readable!!
https://doc.rust-lang.org/std/cmp/trait.PartialEq.html

[zancas]

See also: https://www.reddit.com/r/rust/comments/3f9e7q/why_does_eq_require_partialeq/

[zancas]

NOTE: The reason this type can be used in equivalences without explicitly defining "fn eq" in either the PartialEq, or Eq trait (either as a default, or when implementing for the type), is because the PartialEq trait is being implemented with the #[derive( macro which is supplying an fn eq implementation.

At least... I think that's what's happening, when I drilled into the macro implementation I hit this:

https://doc.rust-lang.org/src/core/cmp.rs.html#209

When I replace the #[derive(PartialEq)] with an explicit:

impl PartialEq for OutPoint {} I get the compiler error I expect:

error[E0046]: not all trait items implemented, missing: `eq`
  --> src/main.rs:11:1
   |
11 | impl PartialEq for OutPoint {}
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ missing `eq` in implementation
   |
   = note: `eq` from trait: `fn(&Self, &Rhs) -> bool`

[zancas]

This is consistent with this documentation (and the fact that u8's are Eq):
https://doc.rust-lang.org/std/cmp/trait.PartialEq.html#derivable

[zancas]

TL;DR OutPoints should be Eq

Unless their use dictates that having op1.hash == op2.hash && op1.n == op2.n does not imply equivalence.. in that case they should be neither Eq nor #[derive(PartialEq)

[str4d]

Technically under the BIP 30 consensus rules we inherited from Bitcoin Core, OutPoints do not have full equivalence, because transaction IDs are allowed to repeat in the block chain as long as they do not repeat in the UTXO set. Thus an OutPoint is a unique reference in the context of a block chain (to the transaction with that ID in the most recent block), but not technically unique on its own.

In practice, wallet code assumes that transaction IDs will never repeat (which causes the overwriting problem that BIP 30 was written to solve). So maybe we're fine assuming that we can treat identical OutPoints as equivalent.

[str4d]

My current preference is that the Eq derive be removed if it is not needed by @adityapk00 (to unblock the higher-level goal of having ZecWallet depend on versioned crates), and then we can take more time to consider whether Eq makes sense (both for this and other types in the API). Removing derives later is a more problematic API change than adding missing derives.

[str4d]

ACK. I opened zcash#140 for the question about Eq.