Favor bare binary-marshalling over length prefixed-marshalling #5799

jgimeno · 2020-03-12T23:01:19Z

Closes: #5748

Description

For contributor use:

Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Code follows the module structure standards.
Wrote unit and integration tests
Updated relevant documentation (docs/) or specification (x/<module>/spec/)
Added relevant godoc comments.
Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
Re-reviewed Files changed in the Github PR explorer

For admin use:

Added appropriate labels to PR (ex. WIP, R4R, docs, etc)
Reviewers assigned
Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

…n keeper store.

… store.

…n keeper store.

…per store.

…store.

…eper store.

codecov · 2020-03-12T23:23:19Z

Codecov Report

Merging #5799 into master will not change coverage by %.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #5799   +/-   ##
=======================================
  Coverage   36.03%   36.03%           
=======================================
  Files         342      342           
  Lines       34782    34782           
=======================================
  Hits        12534    12534           
  Misses      20998    20998           
  Partials     1250     1250

…om:cosmos/cosmos-sdk into jonathan/5748-marshal-unmashal-binare-bare

alexanderbez

LGTM. Thanks @jgimeno ☕️

Might be worth it to have another quick sign off from @cwgoes or @ValarDragon :)

CHANGELOG.md

Co-Authored-By: Alexander Bezobchuk <alexanderbez@users.noreply.github.com>

fedekunze

LGTM

fedekunze · 2020-03-13T19:54:29Z

CHANGELOG.md

@@ -148,6 +148,7 @@ Buffers for state serialization instead of Amino.
  to define their own concrete `MsgSubmitProposal` types.
  * The module now accepts a `Codec` interface which extends the `codec.Marshaler` interface by
  requiring a concrete codec to know how to serialize `Proposal` types.
+* (codec) [\#5799](https://github.com/cosmos/cosmos-sdk/pull/5799) Now we favor the use of MarshalBinaryBare instead of LengthPrefixed in all cases that is not needed.


Suggested change

* (codec) [\#5799](https://github.com/cosmos/cosmos-sdk/pull/5799) Now we favor the use of MarshalBinaryBare instead of LengthPrefixed in all cases that is not needed.

* (codec) [\#5799](https://github.com/cosmos/cosmos-sdk/pull/5799) Now we favor the use of `(Un)MarshalBinaryBare` instead of `(Un)MarshalBinaryLengthPrefixed` in all cases that are not needed.

alessio · 2020-03-14T21:21:43Z

This brings a potentially massive ABI breaking change for clients, and as such it should be mentioned in the CHANGELOG. @jgimeno gaia's build is broken due to this PR, feel free to poke me when you spare some time, I'm happy to provide assistance and try to fix it together

alexanderbez · 2020-03-15T18:06:24Z

How so? I think the proper message was added to changelog. For any client dealing with raw amino binary encoding, the given entry should be clear enough.

That being said, @jgimeno can you update Gaia to the SDK's latest master by any chance?

alessio · 2020-03-15T18:37:57Z

Whether a change introduces ABI/client breakages can be determined with a simple test: do gaia cli_tests require change? If the answer yes, then clients will receive a different response than they used to and the changeset breaks ABI.

Don't get me wrong, the CHANGELOG line per se` looks good to me, I'm arguing that this change should be mentioned among the breaking ones as well so that clients will be informed.

alexanderbez · 2020-03-16T14:06:32Z

do gaia cli_tests require change? If the answer yes, then clients will receive a different response than they used to and the changeset breaks ABI.

Not necessarily. CLI/REST work in JSON, not binary. The test suite could easily fail if we're doing manual binary encoding/decoding, which I 100% bet is the case.

jgimeno · 2020-03-16T14:27:50Z

Yeah, there were only 2 cases that checked manually the encoding of a transaction.

The other failure were due to the changes we did for the proposal json encoding for amounts.

cosmos/gaia#323

alessio · 2020-03-16T15:49:54Z

Whether an interface is part of a program's ABI or API does not depend on encoding. It has nothing to do with encoding. Any interface between two binary programs is an ABI, e.g. HTTP server and client such as gaiacli rest-server and an HTTP REST-ful client. When an endpoint's JSON response's schema changes, that is an ABI breakage by definition - regardless of the encoding.
The other end just sees a different schema and may not be able to interpret it without external intervention, such as recompilation.
Same goes for command line programs. Their expected inputs and produced outputs are the ABI, why? Because on the other end there may be (quite certainly there are) automated clients that pilot the execution of the binary by supplying input and parsing outputs. If expected inputs or outputs schemas change, then the clients would quite surely require external intervention, such as recompilation.

alexanderbez · 2020-03-16T16:56:38Z

Right, I get that. I'm still not following you on what broke here WRT to ABI. We went from length-prefixing to non-length-prefixing. Clients in this model are only the servers (apps) which will understand how to talk to each other. Older clients won't and that is precisely what was stated in the changelog.

alexanderbez · 2020-03-16T23:53:33Z

crypto/keys/keybase_base.go

@@ -124,7 +124,7 @@ func (kb baseKeybase) DecodeSignature(info Info, msg []byte) (sig []byte, pub tm
 		return nil, nil, err
 	}

-	if err := CryptoCdc.UnmarshalBinaryLengthPrefixed([]byte(signed), sig); err != nil {


Sorry @jgimeno, I skimmed through and didn't realize you also updated the Keybase. The Keybase should not be updated because than that will break existing local wallets. Let's undo these.

alexanderbez · 2020-03-16T23:53:52Z

crypto/keys/types.go

@@ -337,12 +337,12 @@ func (i multiInfo) GetPath() (*hd.BIP44Params, error) {

 // encoding info
 func marshalInfo(i Info) []byte {
-	return CryptoCdc.MustMarshalBinaryLengthPrefixed(i)


https://github.com/cosmos/cosmos-sdk/pull/5799/files#r393373483

alexanderbez · 2020-03-16T23:53:58Z

crypto/keys/types.go

 }

 // decoding info
 func unmarshalInfo(bz []byte) (info Info, err error) {
-	err = CryptoCdc.UnmarshalBinaryLengthPrefixed(bz, &info)


https://github.com/cosmos/cosmos-sdk/pull/5799/files#r393373483

* change abci file to use BinaryBare * change all calls to EncodeLengthPrefixed to BinaryBare in distribution keeper store. * change all calls to EncodeLengthPrefixed to BinaryBare in mint keeper store. * change all calls to EncodeLengthPrefixed to BinaryBare in auth keeper store. * change all calls to EncodeLengthPrefixed to BinaryBare in distribution keeper store. * change all calls to EncodeLengthPrefixed to BinaryBare in staking keeper store. * change all calls to EncodeLengthPrefixed to BinaryBare in staking keeper store. * change all calls to EncodeLengthPrefixed to BinaryBare in gov keeper store. * change all calls to EncodeLengthPrefixed to BinaryBare in slashing keeper store. * update decoder test * migrate decoder * migrate gov simulation decoder * migrate baseapp_test * refactor QuerySubspace * refactor coedc std codec * migrate keybase * migrate iavl store * migrate root multi * migrate ante basic * migrate tx type to bare * migrate auth client * update auth types * update decoder * migrate supply decoder * migrate stake encoding * migrate staking simulation * migrate genutil * migrate simapp test helpers * migrate docs * upgrade changelog * Update CHANGELOG.md Co-Authored-By: Alexander Bezobchuk <alexanderbez@users.noreply.github.com> Co-authored-by: Alexander Bezobchuk <alexanderbez@users.noreply.github.com>

jgimeno added 11 commits March 12, 2020 22:18

change abci file to use BinaryBare

c47055c

change all calls to EncodeLengthPrefixed to BinaryBare in distributio…

0ba8426

…n keeper store.

change all calls to EncodeLengthPrefixed to BinaryBare in mint keeper…

3144b47

… store.

change all calls to EncodeLengthPrefixed to BinaryBare in auth keeper…

6af1f7e

… store.

change all calls to EncodeLengthPrefixed to BinaryBare in distributio…

12fce22

…n keeper store.

change all calls to EncodeLengthPrefixed to BinaryBare in staking kee…

81ecf2e

…per store.

change all calls to EncodeLengthPrefixed to BinaryBare in staking kee…

d967f26

…per store.

change all calls to EncodeLengthPrefixed to BinaryBare in gov keeper …

950402e

…store.

change all calls to EncodeLengthPrefixed to BinaryBare in slashing ke…

9b5f9a5

…eper store.

update decoder test

3b7aecf

migrate decoder

30add06

jgimeno added 18 commits March 13, 2020 00:28

migrate gov simulation decoder

3900e4e

Merge branch 'master' into jonathan/5748-marshal-unmashal-binare-bare

9907e62

migrate baseapp_test

f082fde

refactor QuerySubspace

fbefe8a

Merge branch 'jonathan/5748-marshal-unmashal-binare-bare' of github.c…

5d0f1e2

…om:cosmos/cosmos-sdk into jonathan/5748-marshal-unmashal-binare-bare

refactor coedc std codec

e419105

migrate keybase

67cbf19

migrate iavl store

57ad6ac

migrate root multi

35b2ae4

migrate ante basic

b740903

migrate tx type to bare

82848ef

migrate auth client

9ff06ed

update auth types

f10ac70

update decoder

98bd455

migrate supply decoder

d235280

migrate stake encoding

1f1cfac

migrate staking simulation

4b3221b

migrate genutil

e3e3605

jgimeno added the T: State Machine Breaking State machine breaking changes (impacts consensus). label Mar 13, 2020

jgimeno added 2 commits March 13, 2020 17:43

upgrade changelog

78f8b3d

Merge branch 'master' into jonathan/5748-marshal-unmashal-binare-bare

9e845b7

jgimeno marked this pull request as ready for review March 13, 2020 16:43

jgimeno requested review from alessio, alexanderbez, fedekunze, hschoenburg and nylira as code owners March 13, 2020 16:43

alexanderbez approved these changes Mar 13, 2020

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

alexanderbez added A:automerge Automatically merge PR once all prerequisites pass. C:Encoding labels Mar 13, 2020

Update CHANGELOG.md

ca25273

Co-Authored-By: Alexander Bezobchuk <alexanderbez@users.noreply.github.com>

fedekunze approved these changes Mar 13, 2020

View reviewed changes

mergify bot merged commit 2e42f9c into master Mar 13, 2020

mergify bot deleted the jonathan/5748-marshal-unmashal-binare-bare branch March 13, 2020 19:58

alexanderbez reviewed Mar 16, 2020

View reviewed changes

This was referenced Mar 17, 2020

Revert changes for keybase encoding. #5814

Merged

Comment client breaking #5811

Closed

alessio changed the title ~~Favor marshal unmashal binare bare~~ Favor bare binary-marshalling over length prefixed-marshalling Oct 1, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Favor bare binary-marshalling over length prefixed-marshalling #5799

Favor bare binary-marshalling over length prefixed-marshalling #5799

jgimeno commented Mar 12, 2020 •

edited

Loading

codecov bot commented Mar 12, 2020 •

edited

Loading

alexanderbez left a comment

fedekunze left a comment

fedekunze Mar 13, 2020

alessio commented Mar 14, 2020 •

edited

Loading

alexanderbez commented Mar 15, 2020 •

edited

Loading

alessio commented Mar 15, 2020

alexanderbez commented Mar 16, 2020

jgimeno commented Mar 16, 2020

alessio commented Mar 16, 2020

alexanderbez commented Mar 16, 2020

alexanderbez Mar 16, 2020

alexanderbez Mar 16, 2020

alexanderbez Mar 16, 2020

	* (codec) [\#5799](https://github.com/cosmos/cosmos-sdk/pull/5799) Now we favor the use of MarshalBinaryBare instead of LengthPrefixed in all cases that is not needed.
	* (codec) [\#5799](https://github.com/cosmos/cosmos-sdk/pull/5799) Now we favor the use of `(Un)MarshalBinaryBare` instead of `(Un)MarshalBinaryLengthPrefixed` in all cases that are not needed.

Favor bare binary-marshalling over length prefixed-marshalling #5799

Favor bare binary-marshalling over length prefixed-marshalling #5799

Conversation

jgimeno commented Mar 12, 2020 • edited Loading

Description

codecov bot commented Mar 12, 2020 • edited Loading

Codecov Report

alexanderbez left a comment

Choose a reason for hiding this comment

fedekunze left a comment

Choose a reason for hiding this comment

fedekunze Mar 13, 2020

Choose a reason for hiding this comment

alessio commented Mar 14, 2020 • edited Loading

alexanderbez commented Mar 15, 2020 • edited Loading

alessio commented Mar 15, 2020

alexanderbez commented Mar 16, 2020

jgimeno commented Mar 16, 2020

alessio commented Mar 16, 2020

alexanderbez commented Mar 16, 2020

alexanderbez Mar 16, 2020

Choose a reason for hiding this comment

alexanderbez Mar 16, 2020

Choose a reason for hiding this comment

alexanderbez Mar 16, 2020

Choose a reason for hiding this comment

jgimeno commented Mar 12, 2020 •

edited

Loading

codecov bot commented Mar 12, 2020 •

edited

Loading

alessio commented Mar 14, 2020 •

edited

Loading

alexanderbez commented Mar 15, 2020 •

edited

Loading