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 hashFinalSaplingRoot to getblocktemplate #3299

Merged
merged 1 commit into from Jun 6, 2018

Conversation

Projects
None yet
5 participants
@Eirik0
Copy link
Contributor

Eirik0 commented May 23, 2018

Closes #3248

@Eirik0 Eirik0 requested review from gtank , str4d and ebfull May 23, 2018

@Eirik0 Eirik0 added this to In progress in Protocol Team May 23, 2018

@gtank

gtank approved these changes May 24, 2018

@@ -49,11 +49,15 @@ def run_test(self):

# Test 5: General checks
tmpl = node.getblocktemplate()
assert(len(tmpl['noncerange']) == 16)
assert_equal(16, len(tmpl['noncerange']))

This comment has been minimized.

@gtank

gtank May 24, 2018

Contributor

What's the difference between these two asserts?

This comment has been minimized.

@Eirik0

Eirik0 May 25, 2018

Contributor

assert_equals is essentially just a wrapper for assert. I think it reads a bit better when we are not just checking a boolean value.

@Eirik0 Eirik0 moved this from In progress to In Review in Protocol Team May 25, 2018

@str4d str4d added the RPC interface label May 31, 2018

@str4d str4d added this to the v2.0.0 milestone May 31, 2018


# Test 6: coinbasetxn checks
assert('foundersreward' in tmpl['coinbasetxn'])
assert(tmpl['coinbasetxn']['required'])

# Test 7: hashFinalSaplingRoot checks
assert('finalsaplingroothash' in tmpl)
assert_equal(64, len(tmpl['finalsaplingroothash']))

This comment has been minimized.

@str4d

str4d May 31, 2018

Contributor

This should be the empty Sapling root, so we should be able to compare them directly.

This comment has been minimized.

@Eirik0

Eirik0 Jun 1, 2018

Contributor

What do you mean by the empty Sapling root? If I print out tmpl['finalsaplingroothash'] I get some seemingly random hash value (3e49b5f954aa9d3545bc6c37744661eea48d7c34e3000d82b7f0010c30f4c2fb).

This comment has been minimized.

@ebfull

ebfull Jun 1, 2018

Contributor
    uint256 expected = uint256S("3e49b5f954aa9d3545bc6c37744661eea48d7c34e3000d82b7f0010c30f4c2fb");

    ASSERT_TRUE(ZCSaplingIncrementalMerkleTree::empty_root() == expected);

I guess that's good!

@ebfull
Copy link
Contributor

ebfull left a comment

So, this looks fine, but what should the hex output really look like? Is GetHex() returning the right endianness? Should it not be returning the exact data that needs to be serialized and placed in the block?


# Test 6: coinbasetxn checks
assert('foundersreward' in tmpl['coinbasetxn'])
assert(tmpl['coinbasetxn']['required'])

# Test 7: hashFinalSaplingRoot checks
assert('finalsaplingroothash' in tmpl)
assert_equal(64, len(tmpl['finalsaplingroothash']))

This comment has been minimized.

@ebfull

ebfull Jun 1, 2018

Contributor
    uint256 expected = uint256S("3e49b5f954aa9d3545bc6c37744661eea48d7c34e3000d82b7f0010c30f4c2fb");

    ASSERT_TRUE(ZCSaplingIncrementalMerkleTree::empty_root() == expected);

I guess that's good!

@Eirik0 Eirik0 force-pushed the Eirik0:3248-update-getblocktemplate branch from 73b764b to 25c13ef Jun 1, 2018

@Eirik0

This comment has been minimized.

Copy link
Contributor

Eirik0 commented Jun 1, 2018

Changed the assert to compare the final sapling root to '3e49b5f954aa9d3545bc6c37744661eea48d7c34e3000d82b7f0010c30f4c2fb', and force pushed that as the commit.

@ebfull

This comment has been minimized.

Copy link
Contributor

ebfull commented Jun 4, 2018

We still need to discuss endianness. What will pool operators need/expect from this output?

@Eirik0

This comment has been minimized.

Copy link
Contributor

Eirik0 commented Jun 4, 2018

@ebfull We are doing finalsaplingroothash the same way as previousblockhash:

result.push_back(Pair("previousblockhash", pblock->hashPrevBlock.GetHex()));
@str4d

str4d approved these changes Jun 5, 2018

@str4d

This comment has been minimized.

Copy link
Contributor

str4d commented Jun 6, 2018

@zkbot r+

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Jun 6, 2018

📌 Commit 25c13ef has been approved by str4d

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Jun 6, 2018

⌛️ Testing commit 25c13ef with merge d18212b...

zkbot added a commit that referenced this pull request Jun 6, 2018

Auto merge of #3299 - Eirik0:3248-update-getblocktemplate, r=str4d
Add hashFinalSaplingRoot to getblocktemplate

Closes #3248
@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Jun 6, 2018

☀️ Test successful - pr-merge
Approved by: str4d
Pushing d18212b to master...

@zkbot zkbot merged commit 25c13ef into zcash:master Jun 6, 2018

1 check passed

homu Test successful
Details

Protocol Team automation moved this from In Review to Complete PRs (Ignore) Jun 6, 2018

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