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
Payment disclosure (experimental feature) #2159
Conversation
ab8870f
to
3a698ac
Compare
src/wallet/rpcdisclosure.cpp
Outdated
"3. \"output_index\" (string, required) \n" | ||
"4. \"message\" (string, optional) \n" | ||
"\nResult:\n" | ||
"\"path\" (string) The full path of the destination file\n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should return the disclosure rather than writing a file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR is not ready for review, it's just a placeholder so we don't forget to complete proof of concept. The file path in the help string is left-over text copied from another rpc call...
3a698ac
to
d9dd359
Compare
Bumped from 1.0.9 which is focused on stability/safety/testing improvements, not new big features. |
src/wallet/rpcdisclosure.cpp
Outdated
ZCNoteEncryption::Ciphertext ciphertext = jsdesc.ciphertexts[pd.payload.n]; | ||
|
||
uint256 pk_enc = zaddr.pk_enc; | ||
auto plaintext = decrypter.decryptWithEsk(ciphertext, pk_enc, pd.payload.esk, h_sig, pd.payload.n); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't verify the note commitment. See https://github.com/zcash/zips/pull/119/files#r139325414.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has now been fixed in latest update. Thanks.
c55d446
to
f2acb72
Compare
b7f706f
to
96b426c
Compare
@daira @str4d Have addressed review comments and updated PR. I have posted a diff of the changes: https://gist.github.com/bitcartel/8497746779de286e3feae2689007ea3c |
src/gtest/test_paymentdisclosure.cpp
Outdated
|
||
// Test payment disclosure constructors | ||
PaymentDisclosure pd2(payload, arrayPayloadSig); | ||
assert(pd == pd2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ASSERT_EQ(pd, pd2);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
src/gtest/test_paymentdisclosure.cpp
Outdated
PaymentDisclosure pd2(payload, arrayPayloadSig); | ||
assert(pd == pd2); | ||
PaymentDisclosure pd3(joinSplitPubKey, key, info, payload.message); | ||
assert(pd == pd3); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ASSERT_EQ(pd, pd3);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly for the other asserts below in this test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
qa/rpc-tests/paymentdisclosure.py
Outdated
message = "Here is proof of my payment!" | ||
pd = self.nodes[0].z_getpaymentdisclosure(txid, 0, 0, message) | ||
result = self.nodes[0].z_validatepaymentdisclosure(pd) | ||
assert( result["valid"] == True ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use assert_equal
here and below in this test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
qa/rpc-tests/paymentdisclosure.py
Outdated
assert(False) | ||
except JSONRPCException as e: | ||
errorString = e.error['message'] | ||
assert_equal("Transaction does not belong to the wallet" in errorString, True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These assert_equal(... in errorString, True)
can just be assert(... in errorString)
since the expression is guaranteed to be a boolean.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
src/paymentdisclosure.h
Outdated
|
||
|
||
struct PaymentDisclosurePayload { | ||
int32_t marker = PAYMENT_DISCLOSURE_PAYLOAD_MAGIC_BYTES; // to be disjoint from trasaction encoding |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/trasaction/transaction/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
9a55bc1
to
c35632b
Compare
@daira @str4d Updated PR. Here are the changes: https://gist.github.com/bitcartel/11c8fdc653c2da3d2585937cb408b7fb |
src/paymentdisclosure.h
Outdated
// which are signed with the same key, joinSplitPrivKey, have disjoint encodings such that an | ||
// encoding from one context will be rejected in the other. We know that the set of valid | ||
// transaction versions ({1..INT32_MAX}) so we will use a negative value for payment disclosure. | ||
// The negative value -9411486 in hex is 0xFF706462 and in ascii ÿpdb. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's not strictly speaking ASCII, it's ISO-8859-1.
Also the {1..INT32_MAX} is only the set of currently valid transaction versions; it will likely change in NU0, but the spec will reserve -224..-1 for magic values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updating comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait, -9411486 is hex FF 70 64 62 in big endian (in little-endian it would be "bdpÿ" as ISO-8859-1).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment has now been addressed.
cdcde78
to
8c341be
Compare
src/paymentdisclosure.h
Outdated
// which are signed with the same key, joinSplitPrivKey, have disjoint encodings such that an | ||
// encoding from one context will be rejected in the other. We know that the set of valid | ||
// transaction versions is currently ({1..INT32_MAX}) so we will use a negative value for | ||
// payment disclosure of -9411486, which in hex is 0xFF706462 and in ISO-8859-1 is ÿpdb. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you make the constant 0xFF626470 = -10328976, that will be "pdbÿ" in ISO-8859-1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, thats what I changed it too. I used this website to help: https://www.scadacore.com/tools/programming-calculators/online-hex-converter/
} catch (const std::exception &e) { | ||
throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter, payment disclosure data is malformed."); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (pd.payload.marker != PAYMENT_DISCLOSURE_PAYLOAD_MAGIC_BYTES) {
throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter, payment disclosure marker not found.");
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok added
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. There's no test that triggers this error.
8c341be
to
b9d12cd
Compare
b9d12cd
to
45232b1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is lacking test coverage for some errors, but I don't consider this to be blocking for the RC.
string hexInput = params[0].get_str(); | ||
if (!IsHex(hexInput)) | ||
{ | ||
throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter, expected payment disclosure data in hexadecimal format."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no test that triggers this error.
// too much data is ignored, but if not enough data, exception of type ios_base::failure is thrown, | ||
// CBaseDataStream::read(): end of data: iostream error | ||
} catch (const std::exception &e) { | ||
throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter, payment disclosure data is malformed."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no test that triggers this error.
} catch (const std::exception &e) { | ||
throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter, payment disclosure data is malformed."); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. There's no test that triggers this error.
} | ||
|
||
if (pd.payload.version != PAYMENT_DISCLOSURE_VERSION_EXPERIMENTAL) { | ||
throw JSONRPCError(RPC_INVALID_PARAMETER, "Payment disclosure version is unsupported."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no test that triggers this error.
|
||
// Check js_index | ||
if (pd.payload.js >= tx.vjoinsplit.size()) { | ||
errs.push_back("Payment disclosure refers to an invalid joinsplit index"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no test that triggers this error.
o.push_back(Pair("jsIndex", pd.payload.js)); | ||
|
||
if (pd.payload.n < 0 || pd.payload.n >= ZC_NUM_JS_OUTPUTS) { | ||
errs.push_back("Payment disclosure refers to an invalid output index"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no test that triggers this error.
tx.joinSplitPubKey.begin()) == 0); | ||
o.push_back(Pair("signatureVerified", sigVerified)); | ||
if (!sigVerified) { | ||
errs.push_back("Payment disclosure signature does not match transaction signature"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no test that triggers this error.
PaymentAddress zaddr = pd.payload.zaddr; | ||
CZCPaymentAddress address; | ||
if (!address.Set(zaddr)) { | ||
errs.push_back("Payment disclosure refers to an invalid payment address"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no test that triggers this error.
bool cm_match = (cm_decrypted == cm_blockchain); | ||
o.push_back(Pair("commitmentMatch", cm_match)); | ||
if (!cm_match) { | ||
errs.push_back("Commitment derived from payment disclosure does not match blockchain commitment"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no test that triggers this error.
errs.push_back("Commitment derived from payment disclosure does not match blockchain commitment"); | ||
} | ||
} catch (const std::exception &e) { | ||
errs.push_back(string("Error while decrypting payment disclosure note: ") + string(e.what()) ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no test that triggers this error.
UX-manual-testing-ACK: I have not reviewed this code, but I did do interactive testing of the feature on a single node and did not notice any UX-facing requests for change. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK, the blocking issue I found above has been resolved.
📌 Commit 45232b1 has been approved by |
Payment disclosure (experimental feature)
No description provided.