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

Minimal modifications to getblocktemplate #1602

Merged
merged 5 commits into from Oct 22, 2016

Conversation

Projects
None yet
6 participants
@str4d
Contributor

str4d commented Oct 22, 2016

A simpler alternative to #1435 that ensures Zcash GBT will remain compatible with BIP 22.

Closes #1424

str4d added some commits Sep 22, 2016

GBT: Support coinbasetxn instead of coinbasevalue
Once a Zcash GBT spec has been written, we can re-enable coinbasevalue.
@str4d

This comment has been minimized.

Show comment
Hide comment
@str4d

str4d Oct 22, 2016

Contributor

@luke-jr as the motivator for this simpler PR, please review.

Contributor

str4d commented Oct 22, 2016

@luke-jr as the motivator for this simpler PR, please review.

@str4d

This comment has been minimized.

Show comment
Hide comment
@str4d

str4d Oct 22, 2016

Contributor

@ampyx @artlav you will both want to review this too, as this may affect you. tl;dr Zcash won't launch with coinbasevalue support because doing so before publishing a ZIP would very likely make Zcash GBT incompatible with BIP 22.

Contributor

str4d commented Oct 22, 2016

@ampyx @artlav you will both want to review this too, as this may affect you. tl;dr Zcash won't launch with coinbasevalue support because doing so before publishing a ZIP would very likely make Zcash GBT incompatible with BIP 22.

@daira

This comment has been minimized.

Show comment
Hide comment
@daira

daira Oct 22, 2016

Contributor

ut(ACK+cov). We need to do the release within a few hours but if we get more feedback before then, we'll take it into account.

Contributor

daira commented Oct 22, 2016

ut(ACK+cov). We need to do the release within a few hours but if we get more feedback before then, we'll take it into account.

@@ -585,7 +593,17 @@ Value getblocktemplate(const Array& params, bool fHelp)
entry.push_back(Pair("fee", pblocktemplate->vTxFees[index_in_template]));
entry.push_back(Pair("sigops", pblocktemplate->vTxSigOps[index_in_template]));

This comment has been minimized.

@luke-jr

luke-jr Oct 22, 2016

Contributor

I don't think fee/sigops are populated for the coinbasetxn.

@luke-jr

luke-jr Oct 22, 2016

Contributor

I don't think fee/sigops are populated for the coinbasetxn.

This comment has been minimized.

@daira

daira Oct 22, 2016

Contributor

So these should just be commented out for now?

@daira

daira Oct 22, 2016

Contributor

So these should just be commented out for now?

This comment has been minimized.

@daira

daira Oct 22, 2016

Contributor

According to https://github.com/bitcoin/bips/blob/master/bip-0022.mediawiki#transactions-object-format , absent "fee" or "sigops" keys mean "unknown".

@daira

daira Oct 22, 2016

Contributor

According to https://github.com/bitcoin/bips/blob/master/bip-0022.mediawiki#transactions-object-format , absent "fee" or "sigops" keys mean "unknown".

This comment has been minimized.

@Ban44n

This comment has been minimized.

Show comment
Hide comment
@Ban44n

Ban44n Oct 22, 2016

I would agree with the current modifications, txCoinbase includes all necessary information to construct the coinbase: founders and miners reward txes. Great pull!

Ban44n commented Oct 22, 2016

I would agree with the current modifications, txCoinbase includes all necessary information to construct the coinbase: founders and miners reward txes. Great pull!

@daira

This comment has been minimized.

Show comment
Hide comment
@daira

daira Oct 22, 2016

Contributor

ut(ACK+cov). Thanks for everyone's review! @zkbot r+

Contributor

daira commented Oct 22, 2016

ut(ACK+cov). Thanks for everyone's review! @zkbot r+

@zkbot

This comment has been minimized.

Show comment
Hide comment
@zkbot

zkbot Oct 22, 2016

Contributor

📌 Commit 07064ea has been approved by daira

Contributor

zkbot commented Oct 22, 2016

📌 Commit 07064ea has been approved by daira

@zkbot

This comment has been minimized.

Show comment
Hide comment
@zkbot

zkbot Oct 22, 2016

Contributor

⌛️ Testing commit 07064ea with merge e44a61e...

Contributor

zkbot commented Oct 22, 2016

⌛️ Testing commit 07064ea with merge e44a61e...

zkbot pushed a commit that referenced this pull request Oct 22, 2016

zkbot
Auto merge of #1602 - str4d:1424-minimal-getblocktemplate, r=daira
Minimal modifications to getblocktemplate

A simpler alternative to #1435 that ensures Zcash GBT will remain compatible with BIP 22.

Closes #1424
@daira

This comment has been minimized.

Show comment
Hide comment
@daira

daira Oct 22, 2016

Contributor

@artlav wrote:

Already looked at 1602, tested it and adjusted my miner software to use it. Looks good to me, appears to adequately solve the problems that remained before.

Contributor

daira commented Oct 22, 2016

@artlav wrote:

Already looked at 1602, tested it and adjusted my miner software to use it. Looks good to me, appears to adequately solve the problems that remained before.

@zkbot

This comment has been minimized.

Show comment
Hide comment
@zkbot

zkbot Oct 22, 2016

Contributor

💔 Test failed - zcash

Contributor

zkbot commented Oct 22, 2016

💔 Test failed - zcash

@daira

This comment has been minimized.

Show comment
Hide comment
@daira

daira Oct 22, 2016

Contributor
=== Running testscript getblocktemplate.py ===
  File "/home/admin/bbs/zcash/build/qa/rpc-tests/test_framework/test_framework.py", line 118, in main
    self.run_test()
  File "/home/admin/bbs/zcash/build/qa/rpc-tests/getblocktemplate.py", line 51, in run_test
    assert(len(tmpl['noncerange']) == 8)
  Initializing test directory /tmp/test4uGnA_
  Assertion failed: 
  Stopping nodes
  Cleaning up
  Failed
!!! FAIL: getblocktemplate.py !!!

Investigating.

Contributor

daira commented Oct 22, 2016

=== Running testscript getblocktemplate.py ===
  File "/home/admin/bbs/zcash/build/qa/rpc-tests/test_framework/test_framework.py", line 118, in main
    self.run_test()
  File "/home/admin/bbs/zcash/build/qa/rpc-tests/getblocktemplate.py", line 51, in run_test
    assert(len(tmpl['noncerange']) == 8)
  Initializing test directory /tmp/test4uGnA_
  Assertion failed: 
  Stopping nodes
  Cleaning up
  Failed
!!! FAIL: getblocktemplate.py !!!

Investigating.

@artlav

This comment has been minimized.

Show comment
Hide comment
@artlav

artlav Oct 22, 2016

On the test fail:
I don't know python, so this might be a stupid question, but does len(tmpl['noncerange']) return the length of the represented integer value, or a length of the hex string from JSON?

I've had a few brain farts to that effect while making a miner, of assuming a binary length of a hex string and vice versa, and that line reminded me of them.

artlav commented Oct 22, 2016

On the test fail:
I don't know python, so this might be a stupid question, but does len(tmpl['noncerange']) return the length of the represented integer value, or a length of the hex string from JSON?

I've had a few brain farts to that effect while making a miner, of assuming a binary length of a hex string and vice versa, and that line reminded me of them.

@str4d

This comment has been minimized.

Show comment
Hide comment
@str4d

str4d Oct 22, 2016

Contributor

Yeah, the problem is it should count in hex, so it's 2 32-bit integers in hex, which is 16 bytes.

Contributor

str4d commented Oct 22, 2016

Yeah, the problem is it should count in hex, so it's 2 32-bit integers in hex, which is 16 bytes.

@daira

This comment has been minimized.

Show comment
Hide comment
@daira

daira Oct 22, 2016

Contributor

Checking whether that fixes it locally.

Contributor

daira commented Oct 22, 2016

Checking whether that fixes it locally.

@str4d

This comment has been minimized.

Show comment
Hide comment
@str4d

str4d Oct 22, 2016

Contributor

Pushed that as a fix, so once you've verified locally you can merge.

Contributor

str4d commented Oct 22, 2016

Pushed that as a fix, so once you've verified locally you can merge.

@daira

This comment has been minimized.

Show comment
Hide comment
@daira

daira Oct 22, 2016

Contributor

I'll just @zkbot r+ it now to save time.

Contributor

daira commented Oct 22, 2016

I'll just @zkbot r+ it now to save time.

@zkbot

This comment has been minimized.

Show comment
Hide comment
@zkbot

zkbot Oct 22, 2016

Contributor

📌 Commit bc54cf0 has been approved by daira

Contributor

zkbot commented Oct 22, 2016

📌 Commit bc54cf0 has been approved by daira

@daira

This comment has been minimized.

Show comment
Hide comment
@daira

daira Oct 22, 2016

Contributor

ACK bc54cf0.

Contributor

daira commented Oct 22, 2016

ACK bc54cf0.

@zkbot

This comment has been minimized.

Show comment
Hide comment
@zkbot

zkbot Oct 22, 2016

Contributor

⌛️ Testing commit bc54cf0 with merge 026c3f7...

Contributor

zkbot commented Oct 22, 2016

⌛️ Testing commit bc54cf0 with merge 026c3f7...

zkbot pushed a commit that referenced this pull request Oct 22, 2016

zkbot
Auto merge of #1602 - str4d:1424-minimal-getblocktemplate, r=daira
Minimal modifications to getblocktemplate

A simpler alternative to #1435 that ensures Zcash GBT will remain compatible with BIP 22.

Closes #1424
@zkbot

This comment has been minimized.

Show comment
Hide comment
@zkbot

zkbot Oct 22, 2016

Contributor

☀️ Test successful - zcash

Contributor

zkbot commented Oct 22, 2016

☀️ Test successful - zcash

@zkbot zkbot merged commit bc54cf0 into zcash:master Oct 22, 2016

1 check was pending

homu Testing commit bc54cf0 with merge 026c3f7...
Details

@str4d str4d deleted the str4d:1424-minimal-getblocktemplate branch Feb 13, 2017

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