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

Disallow v0 transactions as a consensus rule #1600

Merged
merged 2 commits into from Oct 22, 2016

Conversation

Projects
None yet
5 participants
@str4d
Contributor

str4d commented Oct 22, 2016

Closes #1557

@str4d str4d added this to the 1.0.0-rc2 milestone Oct 22, 2016

@str4d str4d added the review needed label Oct 22, 2016

@bitcartel

This comment has been minimized.

Show comment
Hide comment
@bitcartel

bitcartel Oct 22, 2016

Contributor

utACK

Contributor

bitcartel commented Oct 22, 2016

utACK

@str4d

This comment has been minimized.

Show comment
Hide comment
@str4d

str4d Oct 22, 2016

Contributor

Rebased on master so that I could base the block version consensus change off this branch to avoid merge conflicts.

Contributor

str4d commented Oct 22, 2016

Rebased on master so that I could base the block version consensus change off this branch to avoid merge conflicts.

// Check transaction version
if (tx.nVersion < MIN_TX_VERSION) {
return state.DoS(100, error("CheckTransaction(): version too low"),
REJECT_INVALID, "bad-version-too-low");

This comment has been minimized.

@daira

daira Oct 22, 2016

Contributor

bad-txns-version-too-low

@daira

daira Oct 22, 2016

Contributor

bad-txns-version-too-low

This comment has been minimized.

@daira

daira Oct 22, 2016

Contributor

I'll tack this change onto the end of #1603 to avoid disruption.

@daira

daira Oct 22, 2016

Contributor

I'll tack this change onto the end of #1603 to avoid disruption.

@daira

This comment has been minimized.

Show comment
Hide comment
@daira

daira Oct 22, 2016

Contributor

it(ACK+cov). @zkbot r+

Contributor

daira commented Oct 22, 2016

it(ACK+cov). @zkbot r+

@zkbot

This comment has been minimized.

Show comment
Hide comment
@zkbot

zkbot Oct 22, 2016

Contributor

📌 Commit 7ac924c has been approved by daira

Contributor

zkbot commented Oct 22, 2016

📌 Commit 7ac924c has been approved by daira

@daira

This comment has been minimized.

Show comment
Hide comment
@daira

daira Oct 22, 2016

Contributor

s/it/ut/

Contributor

daira commented Oct 22, 2016

s/it/ut/

daira added a commit to str4d/zcash that referenced this pull request Oct 22, 2016

Update the error message string for tx version too low. ref #1600
Signed-off-by: Daira Hopwood <daira@jacaranda.org>
@str4d

This comment has been minimized.

Show comment
Hide comment
@str4d

str4d Oct 22, 2016

Contributor

@zkbot force

Contributor

str4d commented Oct 22, 2016

@zkbot force

@ebfull

This comment has been minimized.

Show comment
Hide comment
@ebfull

ebfull Oct 22, 2016

Contributor

@zkbot r+

Contributor

ebfull commented Oct 22, 2016

@zkbot r+

@zkbot

This comment has been minimized.

Show comment
Hide comment
@zkbot

zkbot Oct 22, 2016

Contributor

💡 This pull request was already approved, no need to approve it again.

Contributor

zkbot commented Oct 22, 2016

💡 This pull request was already approved, no need to approve it again.

@zkbot

This comment has been minimized.

Show comment
Hide comment
@zkbot

zkbot Oct 22, 2016

Contributor

📌 Commit 7ac924c has been approved by ebfull

Contributor

zkbot commented Oct 22, 2016

📌 Commit 7ac924c has been approved by ebfull

@zkbot

This comment has been minimized.

Show comment
Hide comment
@zkbot

zkbot Oct 22, 2016

Contributor

⌛️ Testing commit 7ac924c with merge f2ba0aa...

Contributor

zkbot commented Oct 22, 2016

⌛️ Testing commit 7ac924c with merge f2ba0aa...

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

zkbot
Auto merge of #1600 - str4d:1557-consensus-rule-disallow-v0-txns, r=e…
…bfull

Disallow v0 transactions as a consensus rule

Closes #1557
@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

@str4d

This comment has been minimized.

Show comment
Hide comment
@str4d

str4d Oct 22, 2016

Contributor

Okay, the problem is this line in sighash_test generation:

void static RandomTransaction(CMutableTransaction &tx, bool fSingle) {
    tx.nVersion = insecure_rand();

So there are random transactions that have nVersion = 0. There are two options:

  • Regenerate the tests to only use nVersion >= 1
  • Modify the tests so that if nVersion < MIN_TX_VERSION, the test must fail.

I prefer the latter.

Contributor

str4d commented Oct 22, 2016

Okay, the problem is this line in sighash_test generation:

void static RandomTransaction(CMutableTransaction &tx, bool fSingle) {
    tx.nVersion = insecure_rand();

So there are random transactions that have nVersion = 0. There are two options:

  • Regenerate the tests to only use nVersion >= 1
  • Modify the tests so that if nVersion < MIN_TX_VERSION, the test must fail.

I prefer the latter.

@daira

This comment has been minimized.

Show comment
Hide comment
@daira

daira Oct 22, 2016

Contributor

Yes, I think the latter is more in the spirit of the test.

Contributor

daira commented Oct 22, 2016

Yes, I think the latter is more in the spirit of the test.

@str4d

This comment has been minimized.

Show comment
Hide comment
@str4d

str4d Oct 22, 2016

Contributor

@daira test fixed.

Contributor

str4d commented Oct 22, 2016

@daira test fixed.

@daira

This comment has been minimized.

Show comment
Hide comment
@daira

daira Oct 22, 2016

Contributor

ACK. @zkbot r+

Contributor

daira commented Oct 22, 2016

ACK. @zkbot r+

@zkbot

This comment has been minimized.

Show comment
Hide comment
@zkbot

zkbot Oct 22, 2016

Contributor

📌 Commit f4f1b4b has been approved by daira

Contributor

zkbot commented Oct 22, 2016

📌 Commit f4f1b4b has been approved by daira

@zkbot

This comment has been minimized.

Show comment
Hide comment
@zkbot

zkbot Oct 22, 2016

Contributor

⌛️ Testing commit f4f1b4b with merge f822738...

Contributor

zkbot commented Oct 22, 2016

⌛️ Testing commit f4f1b4b with merge f822738...

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

zkbot
Auto merge of #1600 - str4d:1557-consensus-rule-disallow-v0-txns, r=d…
…aira

Disallow v0 transactions as a consensus rule

Closes #1557
@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 f4f1b4b into zcash:master Oct 22, 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