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

Throw more descriptive exceptions when the constraint system is violated #1752

Merged
merged 1 commit into from Nov 2, 2016

Conversation

Projects
None yet
5 participants
@ebfull
Contributor

ebfull commented Nov 1, 2016

Closes #1668.

@ebfull

This comment has been minimized.

Show comment
Hide comment
@ebfull

ebfull Nov 1, 2016

Contributor

This is ready for review! :)

Contributor

ebfull commented Nov 1, 2016

This is ready for review! :)

@str4d

This comment has been minimized.

Show comment
Hide comment
@str4d

str4d Nov 1, 2016

Contributor

Reviewing.

Contributor

str4d commented Nov 1, 2016

Reviewing.

// Balance must be sensical
if (inputs[i].note.value > MAX_MONEY) {
throw std::invalid_argument("nonsensical input note value");

This comment has been minimized.

@bitcartel

bitcartel Nov 1, 2016

Contributor

input note value too big (greater than MAX_MONEY) would be more descriptive than nonsensical.

@bitcartel

bitcartel Nov 1, 2016

Contributor

input note value too big (greater than MAX_MONEY) would be more descriptive than nonsensical.

@str4d

str4d approved these changes Nov 1, 2016

utACK modulo comment.

tree.root(),
"nonsensical right hand side of joinsplit balance");
// Absurd total output value

This comment has been minimized.

@str4d

str4d Nov 1, 2016

Contributor

Copy-pasta comment.

@str4d

str4d Nov 1, 2016

Contributor

Copy-pasta comment.

@@ -99,7 +99,7 @@ CWalletTx GetValidReceive(const libzcash::SpendingKey& sk, CAmount value, bool r
// Prepare JoinSplits
uint256 rt;
JSDescription jsdesc {*params, mtx.joinSplitPubKey, rt,
inputs, outputs, value, 0, false};
inputs, outputs, 2*value, 0, false};

This comment has been minimized.

@str4d

str4d Nov 1, 2016

Contributor

Is this (and subsequent changes in this file) for fixing issues that are now properly caught? If so, nice!

@str4d

str4d Nov 1, 2016

Contributor

Is this (and subsequent changes in this file) for fixing issues that are now properly caught? If so, nice!

This comment has been minimized.

@ebfull

ebfull Nov 1, 2016

Contributor

Yes.

@ebfull

ebfull Nov 1, 2016

Contributor

Yes.

lhs_value += inputs[i].note.value;
if (lhs_value > MAX_MONEY) {
throw std::invalid_argument("nonsensical left hand size of joinsplit balance");

This comment has been minimized.

@bitcartel

bitcartel Nov 1, 2016

Contributor

input value too big (greater than MAX_MONEY)

@bitcartel

bitcartel Nov 1, 2016

Contributor

input value too big (greater than MAX_MONEY)

rhs_value += outputs[i].value;
if (rhs_value > MAX_MONEY) {
throw std::invalid_argument("nonsensical right hand side of joinsplit balance");

This comment has been minimized.

@bitcartel

bitcartel Nov 1, 2016

Contributor

Ditto above, I prefer referring to MAX_MONEY rather than just say nonsensical.

@bitcartel

bitcartel Nov 1, 2016

Contributor

Ditto above, I prefer referring to MAX_MONEY rather than just say nonsensical.

// Sanity checks of output
{
if (outputs[i].value > MAX_MONEY) {
throw std::invalid_argument("nonsensical output value");

This comment has been minimized.

@bitcartel

bitcartel Nov 1, 2016

Contributor

Ditto avove

@bitcartel

bitcartel Nov 1, 2016

Contributor

Ditto avove

@@ -181,8 +182,44 @@ class JoinSplitCircuit : public JoinSplit<NumInputs, NumOutputs> {
throw std::runtime_error("JoinSplit proving key not loaded");
}
// Compute nullifiers of inputs
if (vpub_old > MAX_MONEY) {
throw std::invalid_argument("nonsensical vpub_old value");

This comment has been minimized.

@bitcartel

bitcartel Nov 1, 2016

Contributor

vpub_old is invalid / too big, greater than MAX_MONEY

@bitcartel

bitcartel Nov 1, 2016

Contributor

vpub_old is invalid / too big, greater than MAX_MONEY

}
if (vpub_new > MAX_MONEY) {
throw std::invalid_argument("nonsensical vpub_new value");

This comment has been minimized.

@bitcartel

bitcartel Nov 1, 2016

Contributor

ditto abive

@bitcartel

bitcartel Nov 1, 2016

Contributor

ditto abive

JSOutput(),
JSOutput()
},
2100000000000001,

This comment has been minimized.

@bitcartel

bitcartel Nov 1, 2016

Contributor

How about MAX_MONEY + 1 ?

@bitcartel

bitcartel Nov 1, 2016

Contributor

How about MAX_MONEY + 1 ?

2100000000000001,
0,
tree.root(),
"nonsensical vpub_old value");

This comment has been minimized.

@bitcartel

bitcartel Nov 1, 2016

Contributor

vpub_old value is too big, > MAX_MONEY

@bitcartel

bitcartel Nov 1, 2016

Contributor

vpub_old value is too big, > MAX_MONEY

JSOutput()
},
0,
2100000000000001,

This comment has been minimized.

@bitcartel

bitcartel Nov 1, 2016

Contributor

Ditto above

@bitcartel

bitcartel Nov 1, 2016

Contributor

Ditto above

0,
2100000000000001,
tree.root(),
"nonsensical vpub_new value");

This comment has been minimized.

@bitcartel

bitcartel Nov 1, 2016

Contributor

Ditto above

@bitcartel

bitcartel Nov 1, 2016

Contributor

Ditto above

0,
0,
tree.root(),
"nonsensical input note value");

This comment has been minimized.

@bitcartel

bitcartel Nov 1, 2016

Contributor

ditto above

@bitcartel

bitcartel Nov 1, 2016

Contributor

ditto above

increment_note_witnesses(note1.cm(), witnesses, tree);
Note note2(addr.a_pk, 100, random_uint256(), random_uint256());
increment_note_witnesses(note2.cm(), witnesses, tree);
Note note3(addr.a_pk, 2100000000000001, random_uint256(), random_uint256());

This comment has been minimized.

@bitcartel

bitcartel Nov 1, 2016

Contributor

MAX_MONEY + 1

@bitcartel

bitcartel Nov 1, 2016

Contributor

MAX_MONEY + 1

0,
0,
tree.root(),
"nonsensical left hand size of joinsplit balance");

This comment has been minimized.

@bitcartel

bitcartel Nov 1, 2016

Contributor

ditto above

@bitcartel

bitcartel Nov 1, 2016

Contributor

ditto above

JSInput()
},
{
JSOutput(addr, 2100000000000001),

This comment has been minimized.

@bitcartel

bitcartel Nov 1, 2016

Contributor

MAX_MONEY + 1

@bitcartel

bitcartel Nov 1, 2016

Contributor

MAX_MONEY + 1

0,
0,
tree.root(),
"nonsensical output value");

This comment has been minimized.

@bitcartel

bitcartel Nov 1, 2016

Contributor

ditto above

@bitcartel

bitcartel Nov 1, 2016

Contributor

ditto above

0,
0,
tree.root(),
"nonsensical right hand side of joinsplit balance");

This comment has been minimized.

@bitcartel

bitcartel Nov 1, 2016

Contributor

MAX_MONEY

@bitcartel

bitcartel Nov 1, 2016

Contributor

MAX_MONEY

@bitcartel

This comment has been minimized.

Show comment
Hide comment
@bitcartel

bitcartel Nov 1, 2016

Contributor

ACK but see comments (which are none blocking)

Contributor

bitcartel commented Nov 1, 2016

ACK but see comments (which are none blocking)

@bitcartel bitcartel added this to the 1.0.1 milestone Nov 1, 2016

@ebfull

This comment has been minimized.

Show comment
Hide comment
@ebfull

ebfull Nov 2, 2016

Contributor

posted on wrong pr

Contributor

ebfull commented Nov 2, 2016

posted on wrong pr

@ebfull

This comment has been minimized.

Show comment
Hide comment
@ebfull

ebfull Nov 2, 2016

Contributor

@zkbot r+

Contributor

ebfull commented Nov 2, 2016

@zkbot r+

@zkbot

This comment has been minimized.

Show comment
Hide comment
@zkbot

zkbot Nov 2, 2016

Contributor

📌 Commit c4643bd has been approved by ebfull

Contributor

zkbot commented Nov 2, 2016

📌 Commit c4643bd has been approved by ebfull

@zkbot

This comment has been minimized.

Show comment
Hide comment
@zkbot

zkbot Nov 2, 2016

Contributor

⌛️ Testing commit c4643bd with merge 624b5f3...

Contributor

zkbot commented Nov 2, 2016

⌛️ Testing commit c4643bd with merge 624b5f3...

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

zkbot
Auto merge of #1752 - ebfull:diagnostics-of-constraint-system-violati…
…ons, r=ebfull

Throw more descriptive exceptions when the constraint system is violated

Closes #1668.
@zkbot

This comment has been minimized.

Show comment
Hide comment
@zkbot

zkbot Nov 2, 2016

Contributor

☀️ Test successful - zcash

Contributor

zkbot commented Nov 2, 2016

☀️ Test successful - zcash

@zkbot zkbot merged commit c4643bd into zcash:master Nov 2, 2016

1 check passed

homu Test successful
Details
@daira

This comment has been minimized.

Show comment
Hide comment
@daira

daira Nov 3, 2016

Contributor

[commented on wrong PR]

Contributor

daira commented Nov 3, 2016

[commented on wrong PR]

@daira

This comment has been minimized.

Show comment
Hide comment
@daira

daira Nov 3, 2016

Contributor

utACK

Contributor

daira commented Nov 3, 2016

utACK

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