-
Notifications
You must be signed in to change notification settings - Fork 467
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
there's a funky smell and it's coming from command output encoding #599
Comments
note, JSON does not support the full u64 range so we will have to use sth not number there always. This is not the case for cbor though. |
We can use json numbers for a long time.
…On Tue, Jun 26, 2018, 5:05 AM Friedel Ziegelmayer ***@***.***> wrote:
note, JSON does not support the full u64 range so we will have to use sth
not number there always. This is not the case for cbor though.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#599 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAJl4c4kMkN4XOtcOPJNC5_pgyqKtdwOks5uAiORgaJpZM4U0owK>
.
|
That’s not really a viable approach, we should never marshal into JSON numbers unless they are safe to fit by their type. so anything <= u32, but nothing above like u64.
…On 26. Jun 2018, 19:03 +0200, Fritz Schneider ***@***.***>, wrote:
We can use json numbers for a long time.
On Tue, Jun 26, 2018, 5:05 AM Friedel Ziegelmayer ***@***.***>
wrote:
> note, JSON does not support the full u64 range so we will have to use sth
> not number there always. This is not the case for cbor though.
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <#599 (comment)>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AAJl4c4kMkN4XOtcOPJNC5_pgyqKtdwOks5uAiORgaJpZM4U0owK>
> .
>
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or mute the thread.
|
In taking about for output to humans. It will work fine for the uint64 use
cases for a long while.
On Tue, Jun 26, 2018, 10:10 AM Friedel Ziegelmayer <[email protected]>
wrote:
… That’s not really a viable approach, we should never marshal into JSON
numbers unless they are safe to fit by their type. so anything <= u32, but
nothing above like u64.
On 26. Jun 2018, 19:03 +0200, Fritz Schneider ***@***.***>,
wrote:
> We can use json numbers for a long time.
>
> On Tue, Jun 26, 2018, 5:05 AM Friedel Ziegelmayer <
***@***.***>
> wrote:
>
> > note, JSON does not support the full u64 range so we will have to use
sth
> > not number there always. This is not the case for cbor though.
> >
> > —
> > You are receiving this because you authored the thread.
> > Reply to this email directly, view it on GitHub
> > <
#599 (comment)
>,
> > or mute the thread
> > <
https://github.com/notifications/unsubscribe-auth/AAJl4c4kMkN4XOtcOPJNC5_pgyqKtdwOks5uAiORgaJpZM4U0owK
>
> > .
> >
> —
> You are receiving this because you commented.
> Reply to this email directly, view it on GitHub, or mute the thread.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#599 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAJl4YiFLZ7uof2nkYNNrLmmUdSpKfrdks5uAmsegaJpZM4U0owK>
.
|
But yes marshalling to a string would be better long term. Probably
dovetail with #559...
…On Tue, Jun 26, 2018, 10:43 AM Fritz Schneider ***@***.***> wrote:
In taking about for output to humans. It will work fine for the uint64 use
cases for a long while.
On Tue, Jun 26, 2018, 10:10 AM Friedel Ziegelmayer <
***@***.***> wrote:
> That’s not really a viable approach, we should never marshal into JSON
> numbers unless they are safe to fit by their type. so anything <= u32, but
> nothing above like u64.
>
> On 26. Jun 2018, 19:03 +0200, Fritz Schneider ***@***.***>,
> wrote:
> > We can use json numbers for a long time.
> >
> > On Tue, Jun 26, 2018, 5:05 AM Friedel Ziegelmayer <
> ***@***.***>
> > wrote:
> >
> > > note, JSON does not support the full u64 range so we will have to use
> sth
> > > not number there always. This is not the case for cbor though.
> > >
> > > —
> > > You are receiving this because you authored the thread.
> > > Reply to this email directly, view it on GitHub
> > > <
> #599 (comment)
> >,
> > > or mute the thread
> > > <
> https://github.com/notifications/unsubscribe-auth/AAJl4c4kMkN4XOtcOPJNC5_pgyqKtdwOks5uAiORgaJpZM4U0owK
> >
> > > .
> > >
> > —
> > You are receiving this because you commented.
> > Reply to this email directly, view it on GitHub, or mute the thread.
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <#599 (comment)>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AAJl4YiFLZ7uof2nkYNNrLmmUdSpKfrdks5uAmsegaJpZM4U0owK>
> .
>
|
I don't consider JSON output human-readable, and I think that is why in some cases we have a third output encoding, text. Is it possible that: a) we can ditch the json encoding entirely, or |
When converting a Block to an IPLD Node, it does not encode the cids under 0001-test-Block-CID-IPLD-Encoding.patch.txt (running off of master, bc46155)
A bit of digging it appears there is something weird happening when the ipld node is marshaled it self. The The https://github.com/ipfs/go-cid/blob/master/cid.go#L413-L415 Does this all seam related? If the ipld node can't be encoded to json it makes sense that it can't be decoded. Which I believe might explain #288 not working with the new cbor package. |
The error is/was something along the lines of This is because This can be demonstrated by the following playground code |
With regard to:
This really only has to do with going from an IPLD Node to JSON. (Probably will effect point 2) This issue is due to a quirk of reflect in golang I believe. In Example of quirk https://play.golang.org/p/sOeVJYiv84x |
I believe this branch (https://github.com/filecoin-project/go-filecoin/tree/test/dag-daemon-test-cbor) will resolve #266. It makes It fails with The big issue is that |
I updated the |
@travisperson thanks for jumping in, really helpful. There are multiple overlapping issues here I think I'm close to wrapping my head around. However could you clarify for me what you mean by:
Is there a fundamental reason that we couldn't try to make the float64's fit into the thing they're unmarshaled into eg in https://github.com/polydawn/refmt/blob/0ceb50a9198a7771378f4d712cfdf1b99121dfbb/obj/unmarshalBuiltins.go#L62, or is it just that that work hasn't been done yet, or is it something else? |
OK, this boils down to at least the following set of issues, many of which were discovered by @travisperson above. I'll update this comment as I address them:
STATUS July'18: waiting for ipfs/go-ipld-cbor#29 to land which brings go-ipfs to our version of cbor. |
Immediately I can't really think of one. It might be sane to attempt to check for the precision of that floating values in this step (https://github.com/polydawn/refmt/blob/0ceb50a9198a7771378f4d712cfdf1b99121dfbb/obj/unmarshalBuiltins.go#L80), and attempt to fit the value. I think it would definitely work, but not sure if it's a great idea for the library. Might be worth opening an issue and get the authors thoughts. |
I hope that this issue becomes redundant with well-defined encoding in #3098 and the CLI using that API directly. |
OK I'm sorry / not sorry to have to hand this off as I go on vacation. As part of #587 which you can see in #598 I uncovered some funky smells in our output encoding. See in particular https://github.com/filecoin-project/go-filecoin/pull/598/files#diff-e77b7ad9d6713425351aae9703d0d36bR16
First, take note of #266 which is almost certainly related to these problems. The issues specifically are:
Block.Miner doesn't make it through the nutty series of encodings in dag_daemon_test. The only reason this test passes is that the genesis block doesn't have a miner address set. Anyway, this call returns an error that is presently ignored, if you capture and assert no error on it you can see it: https://github.com/filecoin-project/go-filecoin/blob/master/commands/dag_daemon_test.go#L45
This test also embodies the following pattern. It gets a block two ways and then checks that they are equal. It gets the block first via
Block => json => Block
: it calls chain ls which json encodes and outputs a block from memory. Then it dag gets the cid from that block which results inBlock => IPLD node => CBOR => IPLD node => json => IPLD node => Block
. Walking you through that: the block gets turned into an ipld node which is written as cbor in the block store (Block => IPLD node => CBOR
) and then dag get CBOR decodes it into an IPLD node and json encodes it and outputs it (Block => IPLD node => CBOR => IPLD node => json
). Then the test json decodes back to the ipld node (Block => IPLD node => CBOR => IPLD node => json => IPLD node
) and then decodes the ipld node as a block (bringing us all the way toBlock => IPLD node => CBOR => IPLD node => json => IPLD node => Block
). There was an issue with this test when implemented Uint64 so I wanted to write a test in block_test that specifically checked whether the block survived this sequence of transformations. I wrote the test, but it fails with the same error as above. However! The dag_daemon_test which does the should-be-logically-identical transformation passes. So, that's mystery number 2, possibly related to mystery 1.I mentioned we json encode Uint64's to strings. We have to do this because of the way that dag_daemon_test's (old, outdated) cbor implementation works: https://github.com/ipfs/go-ipld-cbor/blob/master/node.go#L82. It uses json for a reason I don't understand, it was probably expedient. The ipld node contains bytes for Uint64's so unless we define custom json marshaling it will fail because without custom Uint64 marshaling it expects that Uint64's are marshaled as numbers (not bytes). It's a bummer to have to json encode Uint64's to a string (the LEB128 bytes are base64-encoded and js-quoted) because that makes the json output of Uint64's in commands opaque. Ick.
I'd like to understand what we're shooting for output encoding wise. The sensible thing to me and i think(?) what we have been shooting for in general is to have exactly two types of output encoding for commands.
Is that right? If so we need a pass over the code to make sure it is what we're doing.
OK, whew, popping back up. I think the place to start is with #266, now that it is clear that things are not well in this area of the code. Note that everything that is not dag_daemon_test uses this fork of go-ipld-cbor: https://github.com/filecoin-project/go-ipld-cbor. Once dag_daemon_test has been moved to use QmRiRJhn427YVuufBEHofLreKWNw7P7BWNq86Sb9kzqdbd like the rest of the codebase then tackling 1 and 2 above makes sense. Then once they are fixed or understood it would be amazing to look at 3, whether we can json marshal to a number (sees like we should since the new version of go-ipld-cbor uses refmt and not some hacky series of json marshals).
cc @whyrusleeping @laser @aboodman
Edit: clarified 2 based on feedback from @travisperson in #598 (review)
The text was updated successfully, but these errors were encountered: