Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Introduce type common.ExtHash #1852

Merged
merged 1 commit into from
Jun 26, 2023
Merged

Introduce type common.ExtHash #1852

merged 1 commit into from
Jun 26, 2023

Conversation

blukat29
Copy link
Contributor

@blukat29 blukat29 commented May 25, 2023 •
edited
Loading

Proposed changes

  • Introduce ExtHash type for the state-pruning MPT node hashes.
  • See KIP-111 for background.

Types of changes

  • Bugfix
  • New feature or enhancement
  • Others

Checklist

  • I have read the CONTRIBUTING GUIDELINES doc
  • I have signed the CLA
  • Lint and unit tests pass locally with my changes ($ make test)
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
  • Any dependent changes have been merged and published in downstream modules

Related issues

Further comments

@blukat29 blukat29 requested a review from ethan-kr May 25, 2023 01:46
@blukat29 blukat29 self-assigned this May 26, 2023
aidan-kwon
aidan-kwon reviewed May 29, 2023
View reviewed changes
common/types.go Outdated Show resolved Hide resolved
aidan-kwon
aidan-kwon reviewed May 29, 2023
View reviewed changes
common/types.go Outdated Show resolved Hide resolved
@blukat29 blukat29 force-pushed the ollie/pruning-exthash-type branch from 332db2d to 7f16b16 Compare June 8, 2023 09:21
@blukat29
Copy link
Contributor Author

blukat29 commented Jun 8, 2023

Update:

  • rebased to latest dev
  • strict length check in BytesToExtHash
  • nonce is now 7 bytes

@blukat29
Copy link
Contributor Author

I removed RootNonce which was deemed unnecessary in the KIP-111 implementation.
@ethan-kr @aidan-kwon PTAL.

ian0371
ian0371 reviewed Jun 19, 2023
View reviewed changes
Copy link
Contributor

@ian0371 ian0371 left a comment

Choose a reason for hiding this comment

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

I think we need to test BytesTo* functions separately (like TestBytesConversion)

common/types.go Outdated Show resolved Hide resolved
common/types.go Show resolved Hide resolved
common/types.go Outdated Show resolved Hide resolved
common/types.go Outdated Show resolved Hide resolved
common/types.go Show resolved Hide resolved
@blukat29
Copy link
Contributor Author

I think we need to test BytesTo* functions separately (like TestBytesConversion)

Added 050d2d8. Tell me if you are thinking of specific test cases other than this commit.

aidan-kwon
aidan-kwon reviewed Jun 19, 2023
View reviewed changes
common/types.go Outdated

extHashCounterMin = uint64(0x100) // Any counter below extHashCounterMin are reserved.
extHashCounter = extHashCounterMin
extHashLegacyNonce = ExtHashNonce{0, 0, 0, 0, 0, 0, 0x45}
Copy link
Member

Choose a reason for hiding this comment

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

The value doesn't have any meaning, right? Please comment about this for the later changes

Copy link
Member

Choose a reason for hiding this comment

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

@ethan-kr Can you explain the meaning of the value? How about changing this to all-zero?

Copy link
Contributor

Choose a reason for hiding this comment

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

When ExtHash was first developed, there was a magic number in a certain location that determined that it was ExtHash. 0x45 is a remnant of that. It would be possible to just set it to all-zero, but I wanted to leave something to distinguish it, so I left it. However, there is no logic to specifically check ExtHashNonce. It is left in the sense that it is better than nothing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added comments as to why we have nonzero number.

aidan-kwon
aidan-kwon previously approved these changes Jun 19, 2023
View reviewed changes
Copy link
Member

@aidan-kwon aidan-kwon left a comment

Choose a reason for hiding this comment

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

ExtHash requires many memory copies. Let's have a performance test about this in QA process

ian0371
ian0371 previously approved these changes Jun 20, 2023
View reviewed changes
ethan-kr
ethan-kr reviewed Jun 22, 2023
View reviewed changes
common/types.go Outdated Show resolved Hide resolved
common/types.go Show resolved Hide resolved
common/types.go Outdated Show resolved Hide resolved
common/types.go Outdated

extHashCounterMin = uint64(0x100) // Any counter below extHashCounterMin are reserved.
extHashCounter = extHashCounterMin
extHashLegacyNonce = ExtHashNonce{0, 0, 0, 0, 0, 0, 0x45}
Copy link
Contributor

Choose a reason for hiding this comment

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

When ExtHash was first developed, there was a magic number in a certain location that determined that it was ExtHash. 0x45 is a remnant of that. It would be possible to just set it to all-zero, but I wanted to leave something to distinguish it, so I left it. However, there is no logic to specifically check ExtHashNonce. It is left in the sense that it is better than nothing.

@blukat29 blukat29 dismissed stale reviews from ian0371 and aidan-kwon via 5ab7872 June 22, 2023 07:06
@blukat29 blukat29 force-pushed the ollie/pruning-exthash-type branch 2 times, most recently from 357d6a5 to 4e5ffc8 Compare June 22, 2023 08:23
@blukat29
Copy link
Contributor Author

Update:

  • extHashCounter is initialized with UnixNano as-is, not shifted by 8 bits.
  • BytesToExtHashNonce strictly checks the byte length
  • Add helper function HexToExtHash

@blukat29
Copy link
Contributor Author

Cleaned up and squashed. In a conversation with @ethan-kr and @aidan-kwon, we decided:

  • LegacyNonce is all-zero. If we ever need to change it later, we can do it because LegacyNonce only exists in memory and not persisted.
  • extHashCounter increments by 1. Added comment explaining the logic.
  • No need to compare the initial extHashCounter against a specific value. Only check for zero to make sure any timestamp has returned.

@jeongkyun-oh @ian0371 PTAL.

@blukat29 blukat29 merged commit 17b8a0a into klaytn:dev Jun 26, 2023
11 checks passed
@blukat29 blukat29 deleted the ollie/pruning-exthash-type branch June 26, 2023 05:48
@JayChoi1736 JayChoi1736 mentioned this pull request Jul 24, 2023
Closed
20 tasks
@aidan-kwon aidan-kwon mentioned this pull request Jul 30, 2023
Closed
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ian0371 ian0371 ian0371 approved these changes

@aidan-kwon aidan-kwon aidan-kwon approved these changes

@ethan-kr ethan-kr ethan-kr approved these changes

@kjeom kjeom Awaiting requested review from kjeom

@jeongkyun-oh jeongkyun-oh Awaiting requested review from jeongkyun-oh

Assignees

@blukat29 blukat29

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@blukat29 @ian0371 @aidan-kwon @ethan-kr