-
-
Notifications
You must be signed in to change notification settings - Fork 355
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
Spend transaction claims #1395
base: master
Are you sure you want to change the base?
Spend transaction claims #1395
Conversation
It's likely a better idea to create a |
f61c323
to
92501f6
Compare
15a7ce9
to
16c63a4
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.
Looks good to me,
Please add unittests too.
Added basic tests. |
@coderabbitai review |
Important Auto Review SkippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe recent updates introduce a new Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration 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.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (7)
- src/Makefile.am (1 hunks)
- src/libspark/claim.cpp (1 hunks)
- src/libspark/claim.h (1 hunks)
- src/libspark/spend_transaction.cpp (1 hunks)
- src/libspark/spend_transaction.h (2 hunks)
- src/libspark/test/spend_transaction_test.cpp (1 hunks)
- src/libspark/util.h (1 hunks)
Files skipped from review due to trivial changes (1)
- src/libspark/util.h
Additional comments: 16
src/libspark/claim.h (4)
- 4-5: Including
secp256k1/include/MultiExponent.h
directly might not be the best practice, especially if it's part of an external library. Consider whether this include is necessary in the header file or if it could be moved to the implementation file (claim.cpp
) to reduce coupling and improve compilation times.- 9-48: The
Claim
class is well-structured and encapsulates the functionality related to spend transaction claims. However, it's important to ensure that the documentation is thorough, especially for public methods likeprove
andverify
. These methods perform critical operations, and detailed comments explaining the parameters, the process, and any side effects or requirements would greatly enhance maintainability and clarity.- 25-31: The
verify
method's signature is cleaner thanprove
, but it still takes multiple parameters. Consistency in how data is passed (e.g., using structs for grouped data) betweenprove
andverify
could improve the API's usability. Additionally, ensure that the method's purpose and the significance of each parameter are well-documented.- 34-43: The
challenge
method is private and serves as a utility function for generating challenges based on various inputs. It's good practice to keep such utility functions private to encapsulate the class's internal logic. However, ensure that there's sufficient internal documentation explaining the challenge generation process, as it's a critical part of the cryptographic operations.src/libspark/spend_transaction.h (3)
- 10-10: Including
claim.h
inspend_transaction.h
is necessary for the new claim-related functionality. Ensure that any dependencies introduced by this inclusion are managed appropriately, and consider forward declarations if the full class definition is not needed in all cases wherespend_transaction.h
is included.- 64-65: The
proveClaim
andverifyClaim
methods are static, which is appropriate given their utility nature and the fact that they don't need to modify any instance state. However, ensure that the decision to make these methods static is consistent with the overall design philosophy of theSpendTransaction
class and doesn't limit future extensibility.- 64-65: The method signatures for
proveClaim
andverifyClaim
are clear and concise. However, given the complexity of the operations they perform, detailed documentation explaining each parameter, the method's purpose, and any side effects or requirements would be beneficial for maintainability and clarity.src/libspark/test/spend_transaction_test.cpp (1)
- 133-175: The test case
generate_verify
effectively demonstrates the process of generating and verifying claims, including negative test scenarios where verification should fail. This is crucial for ensuring the robustness of the claim functionality. However, consider adding more detailed comments explaining each step of the process and the expected outcomes, especially for the negative test scenarios. This will improve the readability and maintainability of the test code.src/libspark/claim.cpp (2)
- 57-88: The implementation of the
prove
method demonstrates a thorough approach to generating the proof. However, the use of randomization within this method introduces unpredictability that should be carefully managed, especially in a cryptographic context. Ensure that the source of randomness is secure and appropriate for the cryptographic operations being performed.- 91-182: The
verify
method's implementation is complex, involving multiple steps and calculations. While the method appears to be correctly implemented, consider adding more internal documentation to explain the verification process in detail, including the purpose of each calculation and how it contributes to the overall verification. This will greatly aid in understanding and maintaining the code.src/libspark/spend_transaction.cpp (5)
- 483-495: The input validation in
proveClaim
method ensures that the input coin data corresponds to the prover statement data from the spend transaction. This is crucial for the integrity of the claim generation process. However, consider adding more detailed error messages to distinguish between different types of input data mismatches. This can improve the debugging experience and make it easier for developers to identify the exact cause of an error.- 507-519: In the
proveClaim
method, the binding hash computation is a critical step that ensures the claim is securely bound to the spend transaction's data. The method correctly computes the binding hash by incorporating various elements of the transaction. However, it's important to ensure that all elements involved in the hash computation are immutable and correctly represent the transaction's state to prevent any potential vulnerabilities.- 522-538: The claim production step in the
proveClaim
method correctly utilizes theClaim
class to generate the claim. It's important to ensure that theClaim
class is thoroughly tested, especially theprove
method, to guarantee the security and correctness of the claim generation process. Consider adding more comprehensive tests covering various edge cases and potential error conditions.- 556-568: The binding hash computation in the
verifyClaim
method is consistent with theproveClaim
method, ensuring that the verification process matches the claim generation process. This consistency is crucial for the security and integrity of the claim verification. Ensure that any changes to the hash computation inproveClaim
are mirrored inverifyClaim
to maintain this consistency.- 571-583: The claim verification step in the
verifyClaim
method correctly utilizes theClaim
class to verify the claim against the spend transaction's data. It's important to ensure that theClaim
class'sverify
method is robust and secure, as it plays a critical role in the claim verification process. Consider reviewing the implementation of theClaim
class and itsverify
method for potential improvements in security and efficiency.src/Makefile.am (1)
- 679-680: The addition of
claim.h
andclaim.cpp
to thelibspark
sources is correctly implemented. This ensures that the new functionality related to spend transaction claims will be compiled and linked as part of thelibspark
library. It's important to verify that these files are located in the correct directory (libspark
) and that any dependencies they have are also included in the build process if necessary.
void prove( | ||
const Scalar& mu, | ||
const std::vector<unsigned char>& identifier, | ||
const std::vector<unsigned char>& message, | ||
const std::vector<Scalar>& x, | ||
const std::vector<Scalar>& y, | ||
const std::vector<Scalar>& z, | ||
const std::vector<GroupElement>& S, | ||
const std::vector<GroupElement>& T, | ||
ChaumProof& proof |
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.
The prove
method takes a significant number of parameters, which might indicate that the method is doing too much or that some of these parameters could be encapsulated into a higher-level object. Consider whether some of these parameters are related closely enough to be grouped into a struct or class to simplify the method signature and improve code readability.
// Check statement validity | ||
std::size_t n = x.size(); | ||
if (!(y.size() == n && z.size() == n && S.size() == n && T.size() == n)) { | ||
throw std::invalid_argument("Bad claim statement!"); | ||
} | ||
for (std::size_t i = 0; i < n; i++) { | ||
if (!(F*x[i] + G*y[i] + H*z[i] == S[i] && T[i]*x[i] + G*y[i] == U)) { | ||
throw std::invalid_argument("Bad claim statement!"); | ||
} | ||
} |
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.
The prove
method includes checks for the validity of the statement being proven. This is a good practice for ensuring the integrity of the input data. However, consider extracting these checks into a separate method to improve code readability and maintainability. Additionally, ensure that error messages provided in exceptions are descriptive enough to help diagnose issues.
c716d69
to
ab7c285
Compare
This PR is an initial draft of a design for spend transaction claims.
A claim is a proof against an existing spend transaction that asserts the prover knew the spend key used to authorize the transaction. It also binds an arbitrary message into the proof, which can be useful to avoid replay. It has the same structure as a
ChaumProof
, which is how it is internally represented (but it uses domain separation to ensure it can't be replayed for transaction authorization).How this should be structured in the codebase is up for debate. Because a claim is structured like a Chaum authorizing proof, it uses the same
ChaumProof
data structure and serialization. It is generated and verified using the staticSpendTransaction::proveClaim
andSpendTransaction::verifyClaim
functions.The prover must provide the spend transaction, the (secret) input coin data representing the coins that were consumed in the spend transaction, its full view and spend keys, an arbitrary message, and an identifier that the verifier can use to obtain its own view of the spend transaction.
The verifier must provide the proof, spend transaction, arbitrary message, and identifier. It is very important that the verifier use its own view of the spend transaction! Otherwise, the prover could lie about its contents.
Once the prover produces the
ChaumProof
data structure representing the claim, it should be sent to the verifier in a serialized package containing:ChaumProof
The verifier then uses its view of the ledger to look up the spend transaction using the identifier. It checks that the message is as expected. Then, it verifies the claim by checking the
ChaumProof
while binding in the identifier and message.Summary by CodeRabbit
libspark
library to include functionality for creating, proving, and verifying claims in cryptographic protocols.