Improve joinsplit diagnostics #1797

Merged
merged 4 commits into from Nov 16, 2016

Conversation

Projects
None yet
4 participants
@ebfull
Contributor

ebfull commented Nov 5, 2016

I don't advocate merging this for the hotfix release (to fix #1779) but this PR can be used to diagnose the real issue and should be merged ASAP afterward.

I still need to add tests for last() and element() though. Done.

@str4d str4d added this to the 1.0.3 milestone Nov 5, 2016

@ebfull

This comment has been minimized.

Show comment
Hide comment
@ebfull

ebfull Nov 7, 2016

Contributor

These changes prevent the assertion from #1779, and confirm my diagnosis of the actual bug with randomization.

Contributor

ebfull commented Nov 7, 2016

These changes prevent the assertion from #1779, and confirm my diagnosis of the actual bug with randomization.

@bitcartel

This comment has been minimized.

Show comment
Hide comment
@bitcartel

bitcartel Nov 11, 2016

Contributor

@ebfull Is this ready for review?

Contributor

bitcartel commented Nov 11, 2016

@ebfull Is this ready for review?

Add tests for witness `element` and tree `last` methods. Strengthen t…
…esting by inserting a different commitment into the tree at each step.
@ebfull

This comment has been minimized.

Show comment
Hide comment
@ebfull

ebfull Nov 14, 2016

Contributor

@bitcartel This is ready for review.

Contributor

ebfull commented Nov 14, 2016

@bitcartel This is ready for review.

@ebfull

This comment has been minimized.

Show comment
Hide comment
@ebfull

ebfull Nov 14, 2016

Contributor

The old incremental merkle tree tests were appending the same commitment every time, which isn't exhaustive enough and insufficient for testing the changes we're making in this PR. I've changed it so that it appends a unique commitment and stresses the API changes at the same time.

Contributor

ebfull commented Nov 14, 2016

The old incremental merkle tree tests were appending the same commitment every time, which isn't exhaustive enough and insufficient for testing the changes we're making in this PR. I've changed it so that it appends a unique commitment and stresses the API changes at the same time.

@str4d

This comment has been minimized.

Show comment
Hide comment
@str4d

str4d Nov 15, 2016

Contributor

utACK (but someone else needs to review this too)

Contributor

str4d commented Nov 15, 2016

utACK (but someone else needs to review this too)

@ebfull

This comment has been minimized.

Show comment
Hide comment
@ebfull

ebfull Nov 15, 2016

Contributor

Added a test to solidify code coverage.

Contributor

ebfull commented Nov 15, 2016

Added a test to solidify code coverage.

Tree tree;
// The root of the tree at this point is expected to be the root of the
// empty tree.
ASSERT_TRUE(tree.root() == Tree::empty_root());
+ // The tree doesn't have a 'last' element added since it's blank.
+ ASSERT_THROW(tree.last(), std::runtime_error);
+
// We need to witness at every single point in the tree, so
// that the consistency of the tree and the merkle paths can
// be checked.
vector<Witness> witnesses;
for (size_t i = 0; i < 16; i++) {

This comment has been minimized.

@bitcartel

bitcartel Nov 15, 2016

Contributor

Why 16? Use constant for this magic number? (16 is also used elsewhere in this file)

@bitcartel

bitcartel Nov 15, 2016

Contributor

Why 16? Use constant for this magic number? (16 is also used elsewhere in this file)

This comment has been minimized.

@ebfull

ebfull Nov 15, 2016

Contributor

Since it's not involved in this PR I will leave it alone, but it's the number of elements that can fit in the testing tree (depth 4), so a constant already exists for calculating it (INCREMENTAL_MERKLE_TREE_DEPTH_TESTING).

@ebfull

ebfull Nov 15, 2016

Contributor

Since it's not involved in this PR I will leave it alone, but it's the number of elements that can fit in the testing tree (depth 4), so a constant already exists for calculating it (INCREMENTAL_MERKLE_TREE_DEPTH_TESTING).

This comment has been minimized.

@bitcartel

bitcartel Nov 15, 2016

Contributor

Doesn't block but I think it would be good to define a constant in this test file e.g. INCREMENTAL_MERKLE_TREE_MAX_ELEMENTS_TESTING = 2 ^ INCREMENTAL_MERKLE_TREE_DEPTH_TESTING just in case the testing tree depth ever changes.

@bitcartel

bitcartel Nov 15, 2016

Contributor

Doesn't block but I think it would be good to define a constant in this test file e.g. INCREMENTAL_MERKLE_TREE_MAX_ELEMENTS_TESTING = 2 ^ INCREMENTAL_MERKLE_TREE_DEPTH_TESTING just in case the testing tree depth ever changes.

+ throw std::invalid_argument("joinsplit not anchored to the correct root");
+ }
+
+ // The tree must witness the correct element

This comment has been minimized.

@bitcartel

bitcartel Nov 15, 2016

Contributor

Prior to this, was this ever checked before? Is there any risk that an existing wallet when upgrading will trigger the exception 'witness of wrong element...'?

@bitcartel

bitcartel Nov 15, 2016

Contributor

Prior to this, was this ever checked before? Is there any risk that an existing wallet when upgrading will trigger the exception 'witness of wrong element...'?

This comment has been minimized.

@bitcartel

bitcartel Nov 16, 2016

Contributor

@ebfull Ping --^

@bitcartel

bitcartel Nov 16, 2016

Contributor

@ebfull Ping --^

This comment has been minimized.

@bitcartel

bitcartel Nov 16, 2016

Contributor

Actually, even if it does, the user should launch with -reindex.

@bitcartel

bitcartel Nov 16, 2016

Contributor

Actually, even if it does, the user should launch with -reindex.

@bitcartel

This comment has been minimized.

Show comment
Hide comment
@bitcartel

bitcartel Nov 15, 2016

Contributor

Review finished. See comments above.

Contributor

bitcartel commented Nov 15, 2016

Review finished. See comments above.

@bitcartel

This comment has been minimized.

Show comment
Hide comment
@bitcartel

bitcartel Nov 16, 2016

Contributor

@ebfull Just that one comment above for your feedback. Otherwise ACK and ready to merge.

Contributor

bitcartel commented Nov 16, 2016

@ebfull Just that one comment above for your feedback. Otherwise ACK and ready to merge.

@bitcartel

This comment has been minimized.

Show comment
Hide comment
@bitcartel

bitcartel Nov 16, 2016

Contributor

@zkbot r+

Contributor

bitcartel commented Nov 16, 2016

@zkbot r+

@zkbot

This comment has been minimized.

Show comment
Hide comment
@zkbot

zkbot Nov 16, 2016

Contributor

📌 Commit 3e2e8b5 has been approved by bitcartel

Contributor

zkbot commented Nov 16, 2016

📌 Commit 3e2e8b5 has been approved by bitcartel

@zkbot

This comment has been minimized.

Show comment
Hide comment
@zkbot

zkbot Nov 16, 2016

Contributor

⌛️ Testing commit 3e2e8b5 with merge 54218ea...

Contributor

zkbot commented Nov 16, 2016

⌛️ Testing commit 3e2e8b5 with merge 54218ea...

zkbot pushed a commit that referenced this pull request Nov 16, 2016

zkbot
Auto merge of #1797 - ebfull:improve-joinsplit-diagnostics, r=bitcartel
Improve joinsplit diagnostics

I don't advocate merging this for the hotfix release (to fix #1779) but this PR can be used to diagnose the real issue and should be merged ASAP afterward.

~I still need to add tests for `last()` and `element()` though.~ Done.
@zkbot

This comment has been minimized.

Show comment
Hide comment
@zkbot

zkbot Nov 16, 2016

Contributor

☀️ Test successful - zcash

Contributor

zkbot commented Nov 16, 2016

☀️ Test successful - zcash

@zkbot zkbot merged commit 3e2e8b5 into zcash:master Nov 16, 2016

1 check passed

homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment