New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add test to verify final sapling root in block header is updated. #3588

Merged
merged 2 commits into from Oct 19, 2018

Conversation

4 participants
@bitcartel
Copy link
Contributor

bitcartel commented Oct 11, 2018

No description provided.

@bitcartel bitcartel added this to the v2.0.2 milestone Oct 11, 2018

@bitcartel bitcartel requested review from arielgabizon and Eirik0 Oct 11, 2018

result = self.nodes[0].getrawtransaction(mytxid, 1)
assert_equal(len(result["vShieldedOutput"]), 2) # there is Sapling shielded change

if __name__ == '__main__':

This comment has been minimized.

@arielgabizon

arielgabizon Oct 11, 2018

Contributor

Nice job.
Might be nice to add a test where the sender is a shielded spend but not recipient
and see the root doesn't change. Perhaps I'll add a commit for that.

This comment has been minimized.

@arielgabizon

This comment has been minimized.

@bitcartel

bitcartel Oct 18, 2018

Contributor

ACK

@str4d str4d added this to Review Backlog in Zcashd Team Oct 16, 2018

@LarryRuane
Copy link
Contributor

LarryRuane left a comment

Nice test!

initialize_chain_clean,
start_nodes,
wait_and_assert_operationid_status,
)

This comment has been minimized.

@LarryRuane

LarryRuane Oct 17, 2018

Contributor

This is great, a much cleaner way to list all these imports (than most of our other tests have)! I like the comma after the last element, too.


# Verfify genesis block contains null field for what is now called the final sapling root field.
blk = self.nodes[0].getblock("0")
assert_equal(blk["finalsaplingroot"], NULL_FIELD)

This comment has been minimized.

@LarryRuane

LarryRuane Oct 17, 2018

Contributor

Super minor (suggestion only) but the expected is the first argument. (But I know many of our existing tests have that backward.) Fixing this just makes failure output a little clearer. I actually do like it as you've done it here because in English it's more natural to say "assert X is equal to 5" than "assert 5 is equal to X." Maybe we should reverse assert_equal()'s convention. Or maybe don't worry about it. :)

This comment has been minimized.

@bitcartel

bitcartel Oct 18, 2018

Contributor

Yes, this was mentioned on another ticket too...

self.sync_all()

# Verify the final Sapling root has changed
blk = self.nodes[0].getblock("201")

This comment has been minimized.

@LarryRuane

LarryRuane Oct 17, 2018

Contributor

Maybe index using the return from getblockcount()? That way (1) if more steps are added later, the test automatically adjusts, and (2) the reader is sure the test is referencing the most recent block.

This comment has been minimized.

@bitcartel

bitcartel Oct 18, 2018

Contributor

I considered it, but this test is small and it makes it easy to visually check what block we are checking.

@bitcartel bitcartel moved this from Review Backlog to In Review in Zcashd Team Oct 18, 2018

@bitcartel

This comment has been minimized.

Copy link
Contributor

bitcartel commented Oct 18, 2018

@zkbot r+

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Oct 18, 2018

📌 Commit eb0bf23 has been approved by bitcartel

zkbot added a commit that referenced this pull request Oct 19, 2018

Auto merge of #3588 - bitcartel:hash_final_sapling_root_verification,…
… r=bitcartel

Add test to verify final sapling root in block header is updated.
@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Oct 19, 2018

⌛️ Testing commit eb0bf23 with merge d810703...

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Oct 19, 2018

☀️ Test successful - pr-merge
Approved by: bitcartel
Pushing d810703 to master...

@zkbot zkbot merged commit eb0bf23 into zcash:master Oct 19, 2018

1 check passed

homu Test successful
Details

Zcashd Team automation moved this from In Review to Released (Merged in Master) Oct 19, 2018

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