Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Lenient bytes deserialization #2036

Merged
merged 3 commits into from
Sep 26, 2016
Merged

Lenient bytes deserialization #2036

merged 3 commits into from
Sep 26, 2016

Conversation

tomusdrw
Copy link
Collaborator

@tomusdrw tomusdrw commented Sep 1, 2016

Fixes #2023
Possibly related: #2004

@tomusdrw tomusdrw added the A0-pleasereview 🤓 Pull request needs code review. label Sep 1, 2016
@rphmeier
Copy link
Contributor

rphmeier commented Sep 1, 2016

I don't think we should stop adhering to the spec explicitly. If the mist/geth teams diverge from it then they have a bug in their software. We could do this in --geth mode though.

@tomusdrw
Copy link
Collaborator Author

tomusdrw commented Sep 1, 2016

Spec here defines how Output values should be formatted.
We also permit 0-padded quantities as inputs (currently we even output such https://github.com/ethcore/parity/issues/2032)

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.004%) to 85.384% when pulling 1906c8b on lenient-data into 9a5668f on master.

@rphmeier
Copy link
Contributor

rphmeier commented Sep 2, 2016

So the issue here (spec-wise) is that it actually never defined input parameter format, only output. So technically any input format is acceptable. IMO this was an oversight in the spec and I have made an edit to it accordingly.

@fjl
Copy link

fjl commented Sep 12, 2016

Please note that Geth will start being stricter in the 1.5 release.

@rphmeier rphmeier added the M4-core ⛓ Core client code / Rust. label Sep 13, 2016
@gavofyork
Copy link
Contributor

gavofyork commented Sep 22, 2016

@rphmeier :

it actually never defined input parameter format

It is perfectly well defined. From https://github.com/ethcore/parity/wiki/JSONRPC :

When encoding UNFORMATTED DATA (byte arrays, account addresses, hashes, bytecode arrays): encode as hex, prefix with "0x", two hex digits per byte
...
WRONG: 004200 (must be prefixed 0x)

This is not specific to one side or the other; encoding/decoding refers to both sides of the RPC call, since we're talking about all data which is transported.

I'm happy to have this under --geth but agree with @rphmeier 's comment that it shouldn't be in by standard.

@gavofyork gavofyork added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Sep 22, 2016
@rphmeier
Copy link
Contributor

It's well defined in our fork of the spec but not in the Ethereum/wiki version, although I made a tiny patch there to clarify as well.

@gavofyork gavofyork added A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Sep 26, 2016
@gavofyork
Copy link
Contributor

this should be reverted ASAP, but seems that for now, it's the least bad option

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.007%) to 85.207% when pulling 55b804a on lenient-data into abcdc81 on master.

@gavofyork gavofyork merged commit 92451ef into master Sep 26, 2016
@gavofyork gavofyork deleted the lenient-data branch September 26, 2016 13:55
"Deserializing empty string as empty bytes. This is a non-standard behaviour that will be removed in future versions. Please update your code to send `0x` instead!"
);
Ok(Bytes::new(Vec::new()))
} else if value.len() >= 2 && &value[0..2] == "0x" && value.len() & 1 == 0 {
Ok(Bytes::new(FromHex::from_hex(&value[2..]).unwrap_or_else(|_| vec![])))
Copy link
Collaborator

Choose a reason for hiding this comment

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

unrelated

unwrap_or_else(|_| vec![]) part is really interesting here. Is it really what geth is doing? It should be an error imo

@gavofyork gavofyork modified the milestone: 1.3.2 Sep 26, 2016
jacogr added a commit that referenced this pull request Sep 26, 2016
* 'master' of https://github.com/ethcore/parity:
  user defaults (#2014)
  Fixing jit feature compilation (#2310)
  Lenient bytes deserialization (#2036)
  Fixing tests
  saturating add
  Remove crufty code
  saturating not overflowing
  Peek transaction queue via RPC (#2270)
  Avoid penalizing legit transactions
  Penalize transactions with gas above gas limit
  Improving txqueue logs
jacogr added a commit that referenced this pull request Sep 26, 2016
* js:
  user defaults (#2014)
  Fixing jit feature compilation (#2310)
  Lenient bytes deserialization (#2036)
  Fixing tests
  saturating add
  Remove crufty code
  saturating not overflowing
  Peek transaction queue via RPC (#2270)
  Avoid penalizing legit transactions
  Penalize transactions with gas above gas limit
  Improving txqueue logs
@gavofyork gavofyork removed this from the 1.3.2 milestone Sep 27, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants