Skip to content
This repository has been archived by the owner on Aug 2, 2022. It is now read-only.

A SIG_K1 generated by eosiopy library is not accepted anymore on EOSIO 2.0 #8534

Closed
maoueh opened this issue Jan 29, 2020 · 12 comments
Closed
Labels

Comments

@maoueh
Copy link
Contributor

maoueh commented Jan 29, 2020

Here a packed transaction that fails on EOSIO 2.0 node:

$ curl -v -X POST -d '{"compression": "none", "packed_context_free_data": "", "packed_trx": "b6b9315e24ababf4fb2b000000000100a6823403ea3055000000572d3ccdcd0180a68967ba6aa28a00000000a8ed32322180a68967ba6aa28a00118d87321531550a0000000000000004454f53000000000000", "signatures": ["SIG_K1_Aq4qpUtK3ar5CWcisdgCkMHrBm6oXgWw3FsVGwFuNTfuzuGTGV5XjGm7mtbfcxXokgJu2gh15hhC7u5fHvhPJwVNduDXxdAg9aZCSzjA6CpNJr54VUbJ"]}' https://mainnet.eoscanada.com/v1/chain/push_transaction

{"code":500,"message":"Internal Service Error","error":{"code":3010010,"name":"packed_transaction_type_exception","what":"Invalid packed transaction","details":[{"message":"Invalid packed transaction","file":"chain_plugin.cpp","line_number":2084,"method":"push_transaction"},{"message":"!unpacker.remaining(): decoded base58 length too long","file":"common.hpp","line_number":48,"method":"apply"},{"message":"error parsing signature","file":"signature.cpp","line_number":32,"method":"parse_base58"},{"message":"Failed to deserialize variant","file":"abi_serializer.hpp","line_number":786,"method":"from_variant"}]}}

But properly decode itself using a EOSIO 1.8 node:

$ curl -v -X POST -d '{"compression": "none", "packed_context_free_data": "", "packed_trx": "b6b9315e24ababf4fb2b000000000100a6823403ea3055000000572d3ccdcd0180a68967ba6aa28a00000000a8ed32322180a68967ba6aa28a00118d87321531550a0000000000000004454f53000000000000", "signatures": ["SIG_K1_Aq4qpUtK3ar5CWcisdgCkMHrBm6oXgWw3FsVGwFuNTfuzuGTGV5XjGm7mtbfcxXokgJu2gh15hhC7u5fHvhPJwVNduDXxdAg9aZCSzjA6CpNJr54VUbJ"]}' https://api.eosnewyork.io/v1/chain/push_transaction

<Expired transaction>, so it was decoded correctly
  • mainnet.eoscanada.com peering node are on EOSIO 2.0
  • api.eosnewyork.io peering node reports 1.8.9

My current hypothesis is that the signature SIG_K1_Aq4qpUtK3ar5CWcisdgCkMHrBm6oXgWw3FsVGwFuNTfuzuGTGV5XjGm7mtbfcxXokgJu2gh15hhC7u5fHvhPJwVNduDXxdAg9aZCSzjA6CpNJr54VUbJ contains more data than just the signature payload (85 bytes here in the decodable part of the string).

I think this signature was accepted before when dealing with EOSIO 1.8, but now, it's not accepted anymore.

This was reported by one of our dfuse user. Personally, I'm not sure it warrant a bug fix in EOSIO.

Wanted to report the issue nonetheless since it still some kind of breaking change between EOSIO 1.8 and 2.0 and gather feedback.

Maybe when transforming the string to a binary format, maybe we could just truncate straight the extra bytes of the signature, so the rest could still work like in EOSIO 1.8.

@tbfleming tbfleming added the bug label Jan 29, 2020
@tbfleming
Copy link
Contributor

Verified behavior change between 1.8 and 2.0

@spoonincode
Copy link
Contributor

The string parser for keys & sigs in 2.0 is more strict and will not accept a base58 string with more data than expected.

This change was unintentional.

However, this may actually be good behavior. If an impl is buggy enough to include extra junk in a signature, maybe that extra junk is sensitive information.

@maoueh
Copy link
Contributor Author

maoueh commented Jan 29, 2020

And to add a bit on that, for other people that might get here. The extra length in the signature generated by the eosiopy was a bug fixed 17 months ago, the checksum bytes were fully appended to the signature instead of the only first 4 ones, creating a signature too big.

The thing is that the maintainer never released a new version of his library. And all was good until EOSIO 2.0 since it's less lenient.

From my point of view, this indeed does not deserve a fix. Maybe just a note in the upgrade guide about this that EOSIO 2.0 is stricter now seems enough.

@tbfleming tbfleming removed the bug label Jan 29, 2020
@tbfleming
Copy link
Contributor

It looks like there are 2 issues:

  • eosjs 21 needs to pad r & s. (too short)
  • eosiopy added extra data

So far, it looks like eosjs 20 doesn't have an issue.

@tbfleming
Copy link
Contributor

EOSIO/eosjs#663

@spoonincode
Copy link
Contributor

Once the release documentation reflects this change, going to close this one as wontfix. As mentioned before, there is concern that if an implementation leaks extra data in to signatures it may be leaking something sensitive. It's prudent to discourage such behavior.

@yiailongsimon
Copy link

How to solve this problem in the end?

@yiailongsimon
Copy link

And to add a bit on that, for other people that might get here. The extra length in the signature generated by the eosiopy was a bug fixed 17 months ago, the checksum bytes were fully appended to the signature instead of the only first 4 ones, creating a signature too big.

The thing is that the maintainer never released a new version of his library. And all was good until EOSIO 2.0 since it's less lenient.

From my point of view, this indeed does not deserve a fix. Maybe just a note in the upgrade guide about this that EOSIO 2.0 is stricter now seems enough.

How to solve this problem in the end?

@maoueh
Copy link
Contributor Author

maoueh commented Apr 9, 2020

@yiailongsimon Use master branch of eosiopy repository instead of the released version. If using pip, search in Google for pip install master from github: https://www.google.com/search?q=pip+install+master+from+github&oq=pip+install+master&aqs=chrome.1.69i57j0l2.3936j0j7&sourceid=chrome&ie=UTF-8 for references on how to do that.

@yiailongsimon
Copy link

yiailongsimon commented Apr 10, 2020

eosiopy

@maoueh hello,eosiopy git has only one master branch, https://github.com/eosmoto/eosiopy
I try first, thanks!

@maoueh
Copy link
Contributor Author

maoueh commented Apr 10, 2020

Yeah ok...

The master branch contains the fix. The default pip install eosiopy does not install a version with the fix in it.

Just ensure you are using the master branch and you are good to go.

Check the last commit on master branch and validate against your local copy that you have the bug fix in there.

If present and still have an issue, it’s not related to this bug report.

@yiailongsimon
Copy link

@maoueh ok,I try first, thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

4 participants