Skip to content

Commit

Permalink
Fix padding truncation bug in deserialization
Browse files Browse the repository at this point in the history
  • Loading branch information
ebfull committed Jan 7, 2016
1 parent 4cb85cc commit d60128e
Show file tree
Hide file tree
Showing 2 changed files with 21 additions and 5 deletions.
4 changes: 3 additions & 1 deletion libzerocash/IncrementalMerkleTree.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,9 @@ namespace libzerocash {
currentPos += hashListBytesLength;
convertBytesVectorToVector(hashListBytes, deserialized.hashList);
/* Remove the multiple-of-8-bits padding. */
deserialized.hashList.resize(deserialized.treeHeight);
deserialized.hashList.erase(deserialized.hashList.begin(),
deserialized.hashList.end() - deserialized.treeHeight
);

/* hashVec */
size_t hashVecSize = countOnes(deserialized.hashList);
Expand Down
22 changes: 18 additions & 4 deletions tests/merkleTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -113,11 +113,23 @@ BOOST_AUTO_TEST_CASE( testRootOfTreeOfNonZeroIsNonZero ) {
BOOST_CHECK( expected_root != actual_root );
}

BOOST_AUTO_TEST_CASE( testSerializationEdgeCase ) {

This comment has been minimized.

Copy link
@nathan-at-least

nathan-at-least Jan 14, 2016

Eh… Not the best test. ;-)

More explicitly: nuke dead code (if a later commit doesn't place something meaningful here).

This comment has been minimized.

Copy link
@ebfull

ebfull Jan 15, 2016

Author

Yup! This test is an ugly hack. ;)


}

BOOST_AUTO_TEST_CASE( testCompactRepresentation ) {
for (uint32_t num_entries = 0; num_entries < 100; num_entries++) {
size_t test_depth = 64;

if (num_entries == 2) {
// This is a particular failure I'm testing with weird
// padding caused by this depth.
test_depth = 20;
}

This comment has been minimized.

Copy link
@defuse

defuse Jan 7, 2016

Maybe we should put this whole loop inside another loop that tries different values of tree_depth?

This comment has been minimized.

Copy link
@ebfull

ebfull Jan 7, 2016

Author

Absolutely. I actually did try that at first but the tests are too specialized to 64 so I just hacked in a test for now. Most of this PR is just temporary until I go back and clean it up.


std::vector< std::vector<bool> > values;
std::vector<bool> root1, root2;
IncrementalMerkleTree incTree(64);
IncrementalMerkleTree incTree(test_depth);

constructNonzeroTestVector(values, num_entries);

Expand All @@ -126,16 +138,18 @@ BOOST_AUTO_TEST_CASE( testCompactRepresentation ) {

IncrementalMerkleTreeCompact compact = incTree.getCompactRepresentation();

BOOST_REQUIRE( compact.getTreeHeight() == 64 );
BOOST_REQUIRE( compact.getTreeHeight() == test_depth );

// Calculate what the path to the next-added element should be.
std::vector<unsigned char> path_bytes(8);
std::vector<bool> path_bits;
libzerocash::convertIntToBytesVector(num_entries, path_bytes);
libzerocash::convertBytesVectorToVector(path_bytes, path_bits);

// Make sure the paths match.
BOOST_REQUIRE( compact.getHashList() == path_bits );
if (test_depth == 64) {
// Make sure the paths match.
BOOST_REQUIRE( compact.getHashList() == path_bits );

This comment has been minimized.

Copy link
@nathan-at-least

nathan-at-least Jan 15, 2016

If this assertion is not true, isn't this a bug? We use non-64-bit depth all over the place, including protentially in production some day.

}

// Make sure there's a hash for every '1' bit down the path.
BOOST_REQUIRE( compact.getHashVec().size() == libzerocash::countOnes(path_bits) );
Expand Down

1 comment on commit d60128e

@nathan-at-least
Copy link

Choose a reason for hiding this comment

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

LGTM, except I want to improve the test coverage for all depths.

Please sign in to comment.