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

Serialise bigint as idiomatic Filecoin form #1516

Merged
merged 1 commit into from
Feb 8, 2024

Conversation

rvagg
Copy link
Member

@rvagg rvagg commented Feb 8, 2024

Without this it naively serialises using the underlying Rust BigInt representation:

[sign,[data,data,...]]

BigInts are going to be a footgun for these events, any time we encounter a DataCap, TokenAmount or some other BigInt wrapper since we need this explicit serde callout. I'll follow up tomorrow with a PR to the FIP-0083 doc to add some clarification around what "bigint" means for serialising this field, then hopefully future FIP authors will take note of that.


Example field value from this event without this fix: 8201821a4876e80017 which is [1,[1215752192,23]], or specifically:

82                                                # array(2)
  01                                              #   uint(1)
  82                                              #   array(2)
    1a 4876e800                                   #     uint(1215752192)
    17                                            #     uint(23)

With this fix we get: 4600174876e800, or:

46                                                # bytes(6)
  00174876e800                                    #   "\x00\x17Hvè\x00"

Which decodes with github.com/filecoin-project/go-state-types/big.Big#UnmarshalCBOR as what I fed in to it: 100000000000.

Without this it naively serialises using the underlying Rust BigInt
representation:

	[sign,[data,data,...]]
@rvagg rvagg force-pushed the rvagg/verifreg-actor-event-bigint-ser branch from a3df645 to 740507c Compare February 8, 2024 11:06
@Stebalien
Copy link
Member

Well, that's really annoying. Ideally we'd:

  1. Move all this stuff out of fvm_shared. StoragePower doesn't belong there.
  2. Modify the type such that it serializes correclty.

Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

Code LGTM, but I didn't audit for other cases.

@Stebalien Stebalien added this pull request to the merge queue Feb 8, 2024
Merged via the queue into master with commit 58d30dc Feb 8, 2024
14 checks passed
@Stebalien Stebalien deleted the rvagg/verifreg-actor-event-bigint-ser branch February 8, 2024 19:06
rjan90 pushed a commit that referenced this pull request Feb 8, 2024
Without this it naively serialises using the underlying Rust BigInt
representation:

	[sign,[data,data,...]]
rjan90 added a commit that referenced this pull request Feb 8, 2024
* Make Prove{CommitSectors, ReplicaUpdates}3Params aggregate proof type optional (#1511)

* Make Prove{CommitSectors, ReplicaUpdates}3Params aggregate proof type optional.

* Enforce no agg proof type when not used

* Serialise bigint as idiomatic Filecoin form (#1516)

Without this it naively serialises using the underlying Rust BigInt
representation:

	[sign,[data,data,...]]

---------

Co-authored-by: Alex North <445306+anorth@users.noreply.github.com>
Co-authored-by: Rod Vagg <rod@vagg.org>
rvagg added a commit to rvagg/FIPs that referenced this pull request Feb 9, 2024
Came up in filecoin-project/builtin-actors#1516 because it's not obvious when doing doing serialisation that we have a bespoke bigint form that fits within the IPLD data model and is based off the Go representation. Letting a "bigint" pass through a system without special casing leads to surprise so I feel the call-out is warranted and hopefully a reminder for future events where we're going to be using these more.
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.

None yet

3 participants