Skip to content

Use consistent hash type across headers - #698

Merged
tzdybal merged 10 commits into
mainfrom
manav/use-consistent-type-hash-header
Jan 22, 2023
Merged

Use consistent hash type across headers#698
tzdybal merged 10 commits into
mainfrom
manav/use-consistent-type-hash-header

Conversation

@Manav-Aggarwal

@Manav-AggarwalManav-Aggarwal commented Jan 18, 2023

Copy link
Copy Markdown
Member

Overview

Closes#689

Checklist

  • New and updated code has appropriate documentation
  • New and updated code has new and/or updated testing
  • Required CI checks are passing
  • Visual proof for any user facing features like CLI or documentation updates
  • Linked issues closed with keywords

@Manav-Aggarwal
Manav-Aggarwalforce-pushed the manav/use-consistent-type-hash-header branch from a26bb4f to bbb737bCompareJanuary 19, 2023 22:33
@Manav-AggarwalManav-Aggarwal linked an issue Jan 19, 2023 that may be closed by this pull request
@Manav-Aggarwal
Manav-Aggarwal marked this pull request as ready for review January 19, 2023 22:34
@Manav-Aggarwal
Manav-Aggarwal requested review from S1nus, gupadhyaya, nashqueue and tuxcanfly and removed request for nashqueueJanuary 19, 2023 22:35
Comment threadnode/full_client.go Outdated
Comment threadtypes/serialization.go Outdated
Comment threadtypes/pb/rollmint/rollmint.pb.go Outdated
Comment threadtypes/header.go Outdated
S1nus
S1nus previously approved these changes Jan 20, 2023
@codecov-commenter

codecov-commenter commented Jan 20, 2023

Copy link
Copy Markdown

Codecov Report

Merging #698 (bc29dce) into main (018e42a) will increase coverage by 0.16%.
The diff coverage is 91.54%.

@@ Coverage Diff @@## main #698 +/- ##
==========================================
+ Coverage 54.59% 54.76% +0.16% 
==========================================
Files 52 52 Lines 10372 10356 -16 ==========================================
+ Hits 5663 5671 +8 + Misses 3840 3824 -16 + Partials 869 861 -8 
Impacted FilesCoverage Δ
types/header.go6.89% <0.00%> (ø)
block/manager.go66.08% <66.66%> (+0.74%)⬆️
node/full_client.go54.65% <84.61%> (ø)
conv/abci/block.go94.33% <100.00%> (ø)
state/executor.go69.90% <100.00%> (ø)
store/store.go60.89% <100.00%> (ø)
types/hashing.go100.00% <100.00%> (ø)
types/serialization.go75.49% <100.00%> (+5.55%)⬆️
types/state.go92.10% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@nashqueuenashqueue left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only types [nit]

Comment threadnode/full_client.go Outdated
tzdybal
tzdybal previously approved these changes Jan 20, 2023

@tzdybaltzdybal left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

nashqueue
nashqueue previously approved these changes Jan 20, 2023

@tuxcanflytuxcanfly left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious why header.Hash is not [32]byte, it seems we are changing code which was expecting an array to now expect slice instead. Would loosening this restriction have any unknown side effects, not covered by test?

Comment threadnode/full_client.go Outdated
Comment threadstate/executor.go Outdated
Comment threadtypes/state.go Outdated
gupadhyaya
gupadhyaya previously approved these changes Jan 21, 2023

@gupadhyayagupadhyaya left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@Manav-Aggarwal

Copy link
Copy Markdown
MemberAuthor

I'm curious why header.Hash is not [32]byte, it seems we are changing code which was expecting an array to now expect slice instead. Would loosening this restriction have any unknown side effects, not covered by test?

Do we intend to support non-32 byte hashes in the future? @tzdybal

@tzdybal

Copy link
Copy Markdown
Contributor

I'm curious why header.Hash is not [32]byte, it seems we are changing code which was expecting an array to now expect slice instead. Would loosening this restriction have any unknown side effects, not covered by test?

Most projects use []byte as it's just easier to use. Added benefit is, that this enable to use hashes of arbitrary length - and we're planning to provide some interface for custom hashing of blcks/headers.

@tzdybal
tzdybal merged commit 9bf4b1d into mainJan 22, 2023
@tzdybal
tzdybal deleted the manav/use-consistent-type-hash-header branch January 22, 2023 11:49
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Use consistent type for all hash fields in Header

7 participants

@Manav-Aggarwal@codecov-commenter@tzdybal@tuxcanfly@S1nus@gupadhyaya@nashqueue