SyncBucket cleanup #407
SyncBucket cleanup #407
Conversation
timvermeulen
requested review from
ec2,
RajarupanSampanthan,
austinabell,
flodesi,
StaticallyTypedAnxiety and
dutterbutter
| impl PartialOrd for BlockHeader { | ||
| fn partial_cmp(&self, other: &Self) -> Option<Ordering> { | ||
| self.cached_bytes.partial_cmp(&other.cached_bytes) | ||
| } | ||
| } | ||
|
|
||
| impl Ord for BlockHeader { | ||
| fn cmp(&self, other: &Self) -> Ordering { | ||
| self.cached_bytes.cmp(&other.cached_bytes) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
@dutterbutter Wasn't there a specific need for this? I remember vaguely it being written into the spec
There was a problem hiding this comment.
I vaguely remember us needing this at some point as well. Clearly not anymore?
There was a problem hiding this comment.
I don't recall the specific need for this, I think its fine to be removed.
| @@ -43,7 +42,7 @@ pub struct Prefix { | |||
| } | |||
|
|
|||
| /// Representation of a IPLD CID. | |||
| #[derive(Eq, Clone)] | |||
| #[derive(PartialEq, Eq, Hash, Clone)] | |||
There was a problem hiding this comment.
It was intentional to keep those separate before (this was a fork of existing implementations initially) and the specific details of comparing and hashing bytes was necessary. I can't see at this point for that specific comparison (cids aren't used as Hamt keys and that was going to be the only potential need to be specific).
There was a problem hiding this comment.
Hmm, could two CIDs ever be considered equal in one way but not in the other?
There was a problem hiding this comment.
Equality, no, but ordering potentially yes because of the variant encoding. Just something to keep in mind in the future, that would be a very nuanced bug to find if there was an inconsistency (I mean, even without varints because the bytes are lexicographically sorted and not all codes are the same byte length)
There was a problem hiding this comment.
So are you talking about ordering and not about Eq and Hash? I agree that changing the ordering can change behavior but here I'm getting rid of Cid's Ord implementation altogether, rather than changing it. Deriving Eq and Hash should not result in observable changes as long as the implementation is correct
There was a problem hiding this comment.
hash will be different too, only PartialEq/Eq would be the same (sorry for bringing up order, was just looking on my phone)
There was a problem hiding this comment.
But I don't think we use the cid hash for any consensus related things so should be fine. (Might be safe to keep that one until we have those specifics tested more in detail)
There was a problem hiding this comment.
Ah yeah, the hash will be different, but as far as I understand you would never look at the resulting hash value anyway, or somehow rely on what it is – i.e. previously the hash of a cid was the hash of its bytes which as far as we're concerned could be anything
There was a problem hiding this comment.
Yeah, I mean it could matter if you use a cid as the key to a hamt, but that shouldn't be a need in the near future because the go implementation statically uses bytes for their keys.
I double checked, doesn't seem like there is an actual use for a consistent hash here, so good on my end sorry to sidetrack with this. I just wanted to make sure it didn't go quietly unconsidered
There was a problem hiding this comment.
I will definitely keep this in mind for if we're ever going to use the CID as the key to a hamt
Summary of changes
Changes introduced in this pull request:
Several loosely related changes:
SyncBucketSet::heaviestwouldn't necessarily return the heaviest tipsetsame_chain_as->is_same_chain_asaccording to Rust's naming conventionsSyncBucketandSyncBucketSettypesPartialOrd/Ordimplementations that weren't usefulCid's manualPartialEqandHashimplementations in favor of derives that (should) have the same behavior but without allocating