portable serializer: unsigned/signed tx made portable, ignore archive version check, tests added #1515

Merged
merged 2 commits into from Jan 9, 2017

Projects

None yet

4 participants

@kenshi84
Contributor

Issue #1551

I realized that the error was caused when boost::archive::archive_exception::unsupported_version was thrown due to the version of the archiver reading the file being smaller than that of the archiver used to write the file. In other words, if my understanding is correct, the error was caused because a wallet built with an older version of Boost tried to de-serialize a file serialized by another wallet built with newer version of Boost.

I'm not sure if I'm doing the right thing, but removing the archiver version check allows me to read the wallet cache file in such cases. Will this change lead to any bad consequences in the future?

(I'm sorry to mess around with this relatively serious matter considering me being not such a skilled programmer. Help and wisdom from more experienced developers are highly appreciated.)

@moneromooo-monero
Contributor

Why were there two different versions in the first place, if it's using the boost code that you added to the monero tree ? Was it updated after addition ?

@kenshi84
Contributor

Similar to the official binary_*archive, portable ones also write/read the archiver's version in the beginning of the serialized file using a function BOOST_ARCHIVE_VERSION(), see portable_binary_iarchive.hpp#L326 and portable_binary_oarchive.hpp#L281

I can see the function declaration at boost/archive/basic_archive.hpp, but strangely I couldn't find its definition anywhere. In any case, the version number seems to be somewhat correlated to the entire Boost version. As far as I tested with my Mac (Boost 1.60), Windows (Boost 1.62) and Linux (Boost 1.58), their archiver versions are 14, 14 and 12, respectively.

@moneromooo-monero
Contributor

OK, that makes sense. And in this case, I think disabling that check is OK. I find it a bit odd it'd use the main boost serializer version though, since it's a separate one.

@NanoAkron
Contributor

Maybe we could look at their version testing code and/or feed this issue back to them?

@NanoAkron
Contributor

Possible to combine with #1507 so as to have a single working PR?

@moneromooo-monero
Contributor

Good catch. Always a bad idea to introduce known buggy stuff with a later fix you already have :)

@kenshi84
Contributor
kenshi84 commented Jan 2, 2017

@NanoAkron

Maybe we could look at their version testing code and/or feed this issue back to them?

Do you mean submitting an issue ticket to Boost? I'm not sure if that makes sense, since this behavior (i.e. rejecting files created by newer archive version) seems intentional.

@moneromooo-monero How about PR #1495?

@moneromooo-monero
Contributor

I can't test this 1495. Do you mean it'd be nice to test the serialization code on ARM ? If so, yes, it'd be nice :)

@kenshi84
Contributor
kenshi84 commented Jan 2, 2017

@moneromooo-monero I mean, PR #1495 fixes a linker error that only occurs with ARM.

@kenshi84 kenshi84 changed the title from portable serializer: isgnore archive version check to portable serializer: unsigned/signed tx made portable, ignore archive version check, tests added Jan 2, 2017
@kenshi84
Contributor
kenshi84 commented Jan 2, 2017 edited

I made unit tests for wallet cache, outputs, and unsigned/signed transactions. I left the testing of cryptonote::trasnaction_prefix as a future task since it seems much more complicated than other structs. The tests seem to pass on all platforms on buildbot except ARMs where the build doesn't succeed like all other PRs (due to undefined reference to abstract_variables_map).

I'm sorry to have to add the big wallet cache of 27.8 MB to the repository. I could have used a small offline wallet cache which doesn't store the entire block hashes, but then the file cannot make real transactions (I believe) and we can't test fields like payments and tx_keys.

@moneromooo-monero
Contributor

Unfortunate about the cache. Since the test doesn't actually make any transfers, you could just hack the wallet code to drop all but the genesis block hash before saving, just to save that cache.
Other than that, a pretty thorough test, nice one :)

@kenshi84 kenshi84 portable serializer: tests added
ada7c7d
@kenshi84
Contributor
kenshi84 commented Jan 3, 2017

@moneromooo-monero

you could just hack the wallet code to drop all but the genesis block hash before saving, just to save that cache.

Really smart idea, thanks! Now the wallet cache is only 2 KB :)

@NanoAkron
Contributor

Hi @kenshi84 do you know whether this warning message (repeated quite frequently during compilation) was introduced by your previous PR?

In file included from /home/x/monero/src/cryptonote_core/cryptonote_boost_serialization.h:41:0,
                 from /home/x/monero/src/cryptonote_core/blockchain.cpp:41:
/home/x/monero/external/boost/archive/portable_binary_iarchive.hpp: In member function ‘void boost::archive::portable_binary_iarchive::load_impl(intmax_t&, char)’:
/home/x/monero/external/boost/archive/portable_binary_iarchive.hpp:245:27: warning: comparison is always false due to limited range of data type [-Wtype-limits]
     bool negative = (size < 0);
@kenshi84
Contributor
kenshi84 commented Jan 5, 2017 edited

@NanoAkron Good point! Yes, the warning has existed since PR #1462 but only with ubuntu-arm7 and debian-armv8 builds as far as I can see on the buildbot. This warning is caused by line 237 of external/boost/archive/portable_binary_iarchive.hpp where char size is assumed to be signed, which is incorrect and compiler-dependent. This code was directly copied from the original Boost unofficial code, and I'm not so sure if this mistake has had any real impact on the system so far.

I'll post a PR about this. Thanks!

@fluffypony

Reviewed

@fluffypony fluffypony merged commit ada7c7d into monero-project:master Jan 9, 2017

7 of 10 checks passed

buildbot/monero-static-debian-armv8 Build done.
Details
buildbot/monero-static-ubuntu-arm7 Build done.
Details
buildbot/monero-static-win32 Build done.
Details
buildbot/monero-static-freebsd64 Build done.
Details
buildbot/monero-static-osx-10.10 Build done.
Details
buildbot/monero-static-osx-10.11 Build done.
Details
buildbot/monero-static-osx-10.12 Build done.
Details
buildbot/monero-static-ubuntu-amd64 Build done.
Details
buildbot/monero-static-ubuntu-i686 Build done.
Details
buildbot/monero-static-win64 Build done.
Details
@kenshi84 kenshi84 deleted the kenshi84:boost-portable-archive branch Jan 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment