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.

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

Add packing for varint/uvarint types #3081

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

iFrostizz
Copy link

@iFrostizz iFrostizz commented Jun 4, 2024

Why this should be merged

This PR adds capabilities of packing / unpacking varint/uvarint types in the packer. We wanted this in the hypersdk (ava-labs/hypersdk#989) and @patrick-ogrady figured that we should not handle low-level packer operations in the high-level code but rather in avalanchego since that's the package we are importing.

How this works

Create the varint in a temporary slice as we don't know the size before, and append it as a fixed bytes slice in the packer.
The unpacking is also pretty self-explanatory

How this was tested

Adding two unit test that does the packing/unpacking and making sure that the number matches in utils/wrappers/packing_test.go

if bytesRead <= 0 {
p.Add(errInvalidInput)
}
p.Bytes = p.Bytes[bytesRead:]
Copy link
Contributor

Choose a reason for hiding this comment

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

(No action required) According to the docstring for binary.Uvarint, a negative value will be returned if p.Bytes contains a value larger than 64 bits. The conditional on line 158 (<= 0) suggests that a negative value is expected. But wouldn't a negative index into this slice result in a panic? Or is there something preventing this (i.e. the value of p.Bytes is guaranteed not to overflow)?

Copy link
Author

Choose a reason for hiding this comment

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

I think that you're absolutely right ! We definitely should not merge this before solving this issue. It also doesn't really make sense to read the whole p.Bytes buffer, but I'm not sure on what to do here because we don't know how much bytes to decode

Copy link
Contributor

Choose a reason for hiding this comment

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

According to:

n == 0: buf too small
n  < 0: value larger than 64 bits (overflow)
        and -n is the number of bytes read

Seems if bytesRead is negative, we should advance the buffer by -bytesRead, so perhaps:

Suggested change
p.Bytes = p.Bytes[bytesRead:]
if bytesRead <= 0 {
p.Add(errInvalidInput)
bytesRead = -bytesRead // number of bytes read is returned negative in case of overflow
}

Copy link
Author

Choose a reason for hiding this comment

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

If the value that is read from the buffer is overflowing 64 bits or that the buffer is too small, do we really still want to read from it ?

Copy link
Contributor

Choose a reason for hiding this comment

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

it also seems acceptable to mark an error and return early instead as you point out

Copy link

github-actions bot commented Aug 4, 2024

This PR has become stale because it has been open for 30 days with no activity. Adding the lifecycle/frozen label will cause this PR to ignore lifecycle events.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Backlog 🗄️
Development

Successfully merging this pull request may close these issues.

None yet

3 participants