Spend transaction claims #1395
Draft
Spend transaction claims #1395
+414
−2
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
It's likely a better idea to create a |
AaronFeickert
force-pushed
the
spend-tx-claims
branch
3 times, most recently
from
f61c323
to
92501f6
Compare
reubenyap
assigned psolstice, levonpetrosyan93 and AaronFeickert and unassigned psolstice, levonpetrosyan93 and AaronFeickert
AaronFeickert
force-pushed
the
spend-tx-claims
branch
2 times, most recently
from
15a7ce9
to
16c63a4
Compare
|
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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.hdirectly 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
Claimclass 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 likeproveandverify. 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
verifymethod's signature is cleaner thanprove, but it still takes multiple parameters. Consistency in how data is passed (e.g., using structs for grouped data) betweenproveandverifycould improve the API's usability. Additionally, ensure that the method's purpose and the significance of each parameter are well-documented.- 34-43: The
challengemethod 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.hinspend_transaction.his 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.his included.- 64-65: The
proveClaimandverifyClaimmethods 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 theSpendTransactionclass and doesn't limit future extensibility.- 64-65: The method signatures for
proveClaimandverifyClaimare 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_verifyeffectively 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
provemethod 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
verifymethod'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
proveClaimmethod 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
proveClaimmethod, 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
proveClaimmethod correctly utilizes theClaimclass to generate the claim. It's important to ensure that theClaimclass is thoroughly tested, especially theprovemethod, 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
verifyClaimmethod is consistent with theproveClaimmethod, 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 inproveClaimare mirrored inverifyClaimto maintain this consistency.- 571-583: The claim verification step in the
verifyClaimmethod correctly utilizes theClaimclass to verify the claim against the spend transaction's data. It's important to ensure that theClaimclass'sverifymethod is robust and secure, as it plays a critical role in the claim verification process. Consider reviewing the implementation of theClaimclass and itsverifymethod for potential improvements in security and efficiency.src/Makefile.am (1)
- 679-680: The addition of
claim.handclaim.cppto thelibsparksources is correctly implemented. This ensures that the new functionality related to spend transaction claims will be compiled and linked as part of thelibsparklibrary. 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.
Comment on lines
+14
to
+23
| 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.
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.
Comment on lines
+46
to
+55
| // 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.
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.
AaronFeickert
force-pushed
the
spend-tx-claims
branch
from
c716d69
to
ab7c285
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
ChaumProofdata structure and serialization. It is generated and verified using the staticSpendTransaction::proveClaimandSpendTransaction::verifyClaimfunctions.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
ChaumProofdata structure representing the claim, it should be sent to the verifier in a serialized package containing:ChaumProofThe 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
ChaumProofwhile binding in the identifier and message.Summary by CodeRabbit
libsparklibrary to include functionality for creating, proving, and verifying claims in cryptographic protocols.