Skip to content
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

Verify p2 delegated conditions signatures #17054

Merged
merged 12 commits into from Dec 18, 2023
Merged

Verify p2 delegated conditions signatures #17054

merged 12 commits into from Dec 18, 2023

Conversation

MarvinQuevedo
Copy link
Contributor

  • Refactor the/verify_signatureAPI endpoint to support the verification process for signatures generated with Tangem Cards. Tangem Cards utilize a distinct puzzle. When crafting a signature with this hardware wallet, the synthetic_public_key is not employed. Consequently, when a developer attempts to verify the signature, the initial BLS signature verification step is successful. However, challenges arise when attempting to recreate the puzzle for comparing the PuzzleHash, as they are not equal. Now, if it is a Tangem Signature, the code recreates the Puzzle with the PublicKey using the p2_delegated_conditions puzzle, which is equivalent to the TangemCard puzzle, It allow verify correctly the signature maked with a Tangem Card.

  • Introduce a new SigningMode to identify signatures produced with Tangem Cards using CHIP_002:

CHIP_0002_P2_DELEGATED_CONDITIONS = "BLS_SIG_BLS12381G2_XMD:SHA-256_SSWU_RO_AUG:CHIP-0002_P2_DELEGATED_PUZZLE"

@MarvinQuevedo MarvinQuevedo requested a review from a team as a code owner December 13, 2023 16:54
@paninaro paninaro added the Added Required label for PR that categorizes merge commit message as "Added" for changelog label Dec 13, 2023
@paninaro
Copy link
Contributor

Thanks @MarvinQuevedo, the change looks good to me. We'll need a test added for this case. I think test_verify_signature is the correct place for that: https://github.com/Chia-Network/chia-blockchain/blob/main/tests/wallet/rpc/test_wallet_rpc.py#L2193

Note that the test has simulated requests and responses starting at https://github.com/Chia-Network/chia-blockchain/blob/main/tests/wallet/rpc/test_wallet_rpc.py#L2097, so it should be fairly simple to add another request/response case for the p2 delegated conditions signing scheme.

Thanks for the addition!

Copy link

coveralls-official bot commented Dec 13, 2023

Pull Request Test Coverage Report for Build 7224443800

Warning: This coverage report may be inaccurate.

We've detected an issue with your CI configuration that might affect the accuracy of this pull request's coverage report.
To ensure accuracy in future PRs, please see these guidelines.
A quick fix for this PR: rebase it; your next report should be accurate.

  • 9 of 9 (100.0%) changed or added relevant lines in 2 files are covered.
  • 99 unchanged lines in 16 files lost coverage.
  • Overall coverage increased (+0.2%) to 90.444%

Files with Coverage Reduction New Missed Lines %
chia/full_node/full_node.py 1 85.03%
chia/server/node_discovery.py 1 79.55%
chia/types/blockchain_format/serialized_program.py 1 98.77%
chia/types/coin_spend.py 1 98.63%
chia/wallet/wallet_node.py 1 88.09%
tests/core/data_layer/conftest.py 1 86.15%
tests/wallet/cat_wallet/test_offer_lifecycle.py 1 99.33%
chia/rpc/rpc_server.py 2 88.05%
chia/timelord/timelord.py 2 73.67%
chia/cmds/wallet_funcs.py 3 79.55%
Totals Coverage Status
Change from base Build 7197386215: 0.2%
Covered Lines: 93832
Relevant Lines: 103705

💛 - Coveralls

@MarvinQuevedo
Copy link
Contributor Author

Hi @paninaro I just added the test data, I test using the wallet UI and everything seems to be going well

@MarvinQuevedo
Copy link
Contributor Author

@paninaro I fixed the error message in the unit test for the invalid signature

@MarvinQuevedo
Copy link
Contributor Author

I just refactor the param type because it break the build process

@MarvinQuevedo
Copy link
Contributor Author

I just reformatter the files with back formatter, If It need any other change, please tell me about, thanks!

@MarvinQuevedo
Copy link
Contributor Author

Imports sorted

@MarvinQuevedo
Copy link
Contributor Author

I will to continue checking why in some cases don't pass the test

@MarvinQuevedo
Copy link
Contributor Author

@paninaro I found the error, I fixed the test data and It would be ok now with the signatures test, thanks!

@MarvinQuevedo
Copy link
Contributor Author

@paninaro only has a error with the core test, It's ok or need to pull the main to try to obtain all test passed?

@paninaro
Copy link
Contributor

@MarvinQuevedo thanks for working through the test errors. This last issue doesn't appear to be related to your PR so I'll just re-run it to see if it passes. This looks good to me now.

I did spend a bit of time yesterday trying to work through the test failures. The challenge I ran into is that it's not trivial to create a signature using this scheme. It would be nice if /sign_message_by_address and /sign_message_by_id supported the p2 delegated conditions scheme, but there's also the challenge of creating an XCH address using that puzzle. I'll check with some folks to see if this is something we'd want to take on.

Copy link
Member

@hoffmang9 hoffmang9 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm and thanks!

@MarvinQuevedo
Copy link
Contributor Author

@MarvinQuevedo thanks for working through the test errors. This last issue doesn't appear to be related to your PR so I'll just re-run it to see if it passes. This looks good to me now.

I did spend a bit of time yesterday trying to work through the test failures. The challenge I ran into is that it's not trivial to create a signature using this scheme. It would be nice if /sign_message_by_address and /sign_message_by_id supported the p2 delegated conditions scheme, but there's also the challenge of creating an XCH address using that puzzle. I'll check with some folks to see if this is something we'd want to take on.

Yes, the problem It's that for default ChiaWallet are not using this puzzle, the idea of have at least the verification it's can have a oficial way to check this signatures, for these dAPP can verify the Tangem Signatures and with it the Tangem Card can join the most completely posible in the Chia Blockchain ecosystem an can verify theirs NFT, DID and new features that can develop later.

Thanks for all your time too!

@cmmarslender cmmarslender merged commit 99ba21a into Chia-Network:main Dec 18, 2023
260 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Added Required label for PR that categorizes merge commit message as "Added" for changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants