-
Notifications
You must be signed in to change notification settings - Fork 115
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
change(rpc): add validateaddress method #6086
Conversation
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 for this, I like the design here!
This method returns
isvalid: false
inzcashd
for shielded addresses, it currently checks Sapling addresses in Zebra but not Sprout, Orchard, or Unified addresses.
I think we might want to match zcashd
's behaviour for Sapling?
Otherwise nodes might try to build a transparent coinbase transaction with a Sapling address, which could cause them to fail or crash.
zebra-rpc/src/methods/get_block_template_rpcs/types/validate_address.rs
Outdated
Show resolved
Hide resolved
zebra-rpc/src/methods/tests/snapshot/get_block_template_rpcs.rs
Outdated
Show resolved
Hide resolved
Also there seems to be a merge conflict, can you rebase when you do your fixes? |
I'm not sure and that's safer, so, fixed in
Yep, I resolved those too (albeit with a merge). |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #6086 +/- ##
==========================================
- Coverage 78.06% 78.05% -0.01%
==========================================
Files 304 304
Lines 39027 39027
==========================================
- Hits 30465 30462 -3
- Misses 8562 8565 +3 |
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!
Can you please do a zcash-rpc-diff for this new RPC? (Maybe we should have a testing checklist somewhere.) |
Yep: Request:
validateaddress t3hgskquvKKoCtvxw86yN7q8bzwRxNgUZmc
Querying zebrad main chain at height >=1970386...
real 0m0.002s
user 0m0.002s
sys 0m0.000s
Querying zcashd main chain at height >=1830188...
real 0m0.003s
user 0m0.000s
sys 0m0.002s
Response diff between zebrad and zcashd:
--- /tmp/tmp.CZTvJsjeXg.rpc-diff/zebrad-main-1970386-validateaddress.json 2023-02-02 16:02:12.119542563 -0500
+++ /tmp/tmp.CZTvJsjeXg.rpc-diff/zcashd-main-1830188-validateaddress.json 2023-02-02 16:02:12.129542563 -0500
@@ -1,5 +1,8 @@
{
"isvalid": true,
"address": "t3hgskquvKKoCtvxw86yN7q8bzwRxNgUZmc",
+ "scriptPubKey": "a914fdaa2e848fa9813b6bbc7e5f8cbd9c0724dca7d087",
+ "ismine": false,
+ "iswatchonly": false,
"isscript": true
} Request:
validateaddress zs1knzgnu5l7h62qexv8mwv8dysr9qum9e5mtxju7hazz9qk8cpgw5uf3c3qd4n62qhdfy7q53z2c0
Querying zebrad main chain at height >=1970389...
real 0m0.002s
user 0m0.002s
sys 0m0.000s
Querying zcashd main chain at height >=1830504...
real 0m0.002s
user 0m0.002s
sys 0m0.000s
Response diff between zebrad and zcashd:
RPC responses were identical
/tmp/tmp.DmkpELZsdx.rpc-diff/zebrad-main-1970389-validateaddress.json:
{
"isvalid": false
} This brought my attention to a mismatch where Zebra was returning conversion errors. Fixed in |
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 for the testing!
Motivation
Mining pools may use this RPC to check addresses.
Closes #5722.
This method returns
isvalid: false
inzcashd
for shielded addresses, it currently checks Sapling addresses in Zebra but not Sprout, Orchard, or Unified addresses.Specifications
https://zcash.github.io/rpc/validateaddress.html
Solution
zcash_address
to parse raw addressAddress
enum in zebra-chain for convertingZcashAddress
Review
Part of regular scheduled work.
Reviewer Checklist
Follow Up Work
primitives::Address
pubkey
andscriptPubKey
fields