Skip to content

trees: replace uint64 types with int64 - #9133

Open
jsha wants to merge 3 commits into
mainfrom
ints
Open

jsha wants to merge 3 commits into
mainfrom
ints

Conversation

@jsha

@jsha jsha commented Oct 29, 2026

Copy link
Copy Markdown
Contributor

Since start, end, and entryIndex will always fit in 48 bits, we can represent them either either uint64 or int64. We're consolidating on int64 to reduce the amount of casting we do (and the attendant nolint:gosec // G115 comments).

jsha added 2 commits October 27, 2026 12:07
Since start, end, and entryIndex will always fit in 48 bits, we can represent
them either either `uint64` or `int64`. We're consolidating on `int64` to reduce
the amount of casting we do (and the attendant `nolint:gosec // G115` comments).
Since start, end, and entryIndex will always fit in 48 bits, we can represent
them either either `uint64` or `int64`. We're consolidating on `int64` to reduce
the amount of casting we do (and the attendant `nolint:gosec // G115` comments).
@jsha
jsha requested a review from a team as a code owner October 29, 2026 00:11
@jsha
jsha requested a review from aarongable October 29, 2026 00:11
aarongable
aarongable previously approved these changes Oct 29, 2026

@aarongable aarongable 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 with one optional improvement

Comment thread trees/cosigned/message.go
if !s.ReadUint64(&start) {
return nil, errors.New("invalid start")
}
if start > math.MaxInt64 {

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.

Should this (and below) instead compare against 2^48?

@beautifulentropy beautifulentropy Oct 29, 2026 •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Agreed. Per Appendix C.2, "The largest possible Merkle Tree in this protocol is 2^48-1", and I think we already reject anything past that in proof.Marshal() and treedb. So we want > trees.MaxSize here and below.

@aarongable
aarongable requested review from a team and beautifulentropy and removed request for a team October 29, 2026 05:05
@aarongable
aarongable dismissed stale reviews from beautifulentropy and themself via de76877 October 29, 2026 15:21

@aarongable aarongable 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.

re-LGTM with same optional question

Sign up for free to 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.

3 participants