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

Add onchain address validation compatible with Taproot adresses #194

Closed
Reckless-Satoshi opened this issue Jul 30, 2022 · 18 comments · Fixed by #198
Closed

Add onchain address validation compatible with Taproot adresses #194

Reckless-Satoshi opened this issue Jul 30, 2022 · 18 comments · Fixed by #198
Assignees
Labels
⚡Eligible for Sats ⚡ This issue or pull request rewards bitcoin enhancement 🆙 New feature or request good first issue Good for newcomers

Comments

@Reckless-Satoshi
Copy link
Collaborator

Reckless-Satoshi commented Jul 30, 2022

Is your feature request related to a problem? Please describe.
Current on-chain payout (as on-the-fly swaps) does not accept taproot addressed. The RoboSats backend can pay to taproot. However, the function to validate an address submitted by the user does not support it.

Describe the solution you'd like
We are using pip package coinaddrvalidator to validate on-chain addresses. Ideally, we would prefer to validate addresses using the LND backend, but this functionality does not seem to be implemented for gRPC calls (I think?). In addition, coinaddrvalidator is not well maintained and bloated with shitcoin address validators. It would be great to instead find a replacement or write our own on-chain address validation.

This is the relevant util that performs validation using coinaddrvalidator. The task consist in a) making it compatible with taproot and b) making it dependency free (or a light dependency).
https://github.com/Reckless-Satoshi/robosats/blob/aad87e7d98be30bbbb814b91534e0709724c2bc4/api/utils.py#L14-L43
Alternatively, a call to Bitcoin RPC or LND gRPC would also be good solutions.

Additional context
This task is ⚡rewarded with 120K Sats⚡ . Drop a comment if you want to be assigned.

@Reckless-Satoshi Reckless-Satoshi added enhancement 🆙 New feature or request good first issue Good for newcomers ⚡Eligible for Sats ⚡ This issue or pull request rewards bitcoin labels Jul 30, 2022
@Reckless-Satoshi
Copy link
Collaborator Author

@zx9r and @satsbaba are interested on this task.

@ghost
Copy link

ghost commented Jul 30, 2022

When you say valid that means that it should follow general guidlines of how a taproot address should be ? @Reckless-Satoshi

Meaning a taproot address should start with 'bc1p.....' else if not it is not a taproot address?

@Reckless-Satoshi
Copy link
Collaborator Author

Reckless-Satoshi commented Jul 30, 2022

'bc1p.....' else if not it is not a taproot address?

Yes, but not only. It should have a valid checksum. More context here https://en.bitcoin.it/wiki/Base58Check_encoding#Creating_a_Base58Check_string there is probably some lightweight library to perform this check much better suited that coinaddrvalidator.

@ghost
Copy link

ghost commented Jul 30, 2022

ohh yes that makes more sense. I am looking into this task for seeing how it can be done. Realised I can use github web editor also for coding.

@zx9r
Copy link
Contributor

zx9r commented Jul 30, 2022

I will take a look at the different approachs that could be taken:

  • decode the address
  • grpc call to bitcoind/lnd

@Reckless-Satoshi
Copy link
Collaborator Author

ohh yes that makes more sense. I am looking into this task for seeing how it can be done. Realised I can use github web editor also for coding.

You will definitely need to run the function :)

@Reckless-Satoshi Reckless-Satoshi assigned zx9r and ghost Jul 30, 2022
@ghost
Copy link

ghost commented Jul 30, 2022

That I can do it definitely. @Reckless-Satoshi

@ghost
Copy link

ghost commented Jul 30, 2022

Also it should adhere to bech32 https://en.bitcoin.it/wiki/Bech32 also. @Reckless-Satoshi

@zx9r
Copy link
Contributor

zx9r commented Jul 30, 2022

I think that to avoid using external libraries, and to avoid reinventing the wheel writing new code to validate, which is prone to errors and needs maintenance, a good solution would be to make a rpc call to bitcoind validateaddress.

Example outputs:

./bitcoin-cli validateaddress 16p7Hhj1tTgAo4GdwQhyzb9gJdsivC1rFH
{
"isvalid": true,
"address": "16p7Hhj1tTgAo4GdwQhyzb9gJdsivC1rFH",
"scriptPubKey": "76a9143fc16d94f84236e46ede1a6cd61ab79d5a42eaae88ac",
"isscript": false,
"iswitness": false
}

./bitcoin-cli validateaddress 3Av2maSQWHmibBAdDaHQS7utMKzjp3JrnF
{
"isvalid": true,
"address": "3Av2maSQWHmibBAdDaHQS7utMKzjp3JrnF",
"scriptPubKey": "a914652f3ec9d3625e8fd7f579131a220b963ecb0a1f87",
"isscript": true,
"iswitness": false
}

./bitcoin-cli validateaddress bc1q4zmgw3dy7u5npxtnrweq3pt9hvpjppz6klpxvf
{
"isvalid": true,
"address": "bc1q4zmgw3dy7u5npxtnrweq3pt9hvpjppz6klpxvf",
"scriptPubKey": "0014a8b68745a4f7293099731bb2088565bb0320845a",
"isscript": false,
"iswitness": true,
"witness_version": 0,
"witness_program": "a8b68745a4f7293099731bb2088565bb0320845a"
}

With a TAPROOT address:
$ lncli newaddress p2tr
{
"address": "bc1pzvysgkm0c5q4qwjegme4csthynhrtznt4ye080ggxlz8ylaeaf2q4unkrj"
}
./bitcoin-cli validateaddress bc1pzvysgkm0c5q4qwjegme4csthynhrtznt4ye080ggxlz8ylaeaf2q4unkrj
{
"isvalid": true,
"address": "bc1pzvysgkm0c5q4qwjegme4csthynhrtznt4ye080ggxlz8ylaeaf2q4unkrj",
"scriptPubKey": "51201309045b6fc501503a5946f35c417724ee358a6ba932f3bd0837c4727fb9ea54",
"isscript": true,
"iswitness": true,
"witness_version": 1,
"witness_program": "1309045b6fc501503a5946f35c417724ee358a6ba932f3bd0837c4727fb9ea54"
}

With an invalid address (the same from last example changing the last character):

$ ./bitcoin-cli validateaddress bc1pzvysgkm0c5q4qwjegme4csthynhrtznt4ye080ggxlz8ylaeaf2q4unkrp
{
"isvalid": false,
"error": "Invalid address format"
}

@Reckless-Satoshi
Copy link
Collaborator Author

rpc call to bitcoind validateaddress.

Agree, more reliable and elegant. It might be that in order to implement/test this feature one might indeed needs to run the development stack?

@zx9r
Copy link
Contributor

zx9r commented Jul 30, 2022

A call with a testnet bitcoin address with a bitcoind running in mainnet returns invalid as expected:

$ ./bitcoin-cli validateaddress mfWxJ45yp2SFn7UciZyNpvDKrzbhyfKrY8
{
"isvalid": false,
"error": "Invalid prefix for Base58-encoded address"
}

I cannot test with a bitcoind running in testnet though

@zx9r
Copy link
Contributor

zx9r commented Jul 30, 2022

rpc call to bitcoind validateaddress.

Agree, more reliable and elegant. It might be that in order to implement/test this feature one might indeed needs to run the development stack?

I can test the function locally but i dont have a robosats development enviroment to make integration tests.

BTW, I dont know how to access the bitcoind rpc inside robosats. Could it be passed as a parameter to the function ?

@zx9r
Copy link
Contributor

zx9r commented Jul 30, 2022

Well if it is just a REST call to bitcoind I guess I can get the connection parameters from config. Right ?

@Reckless-Satoshi
Copy link
Collaborator Author

Reckless-Satoshi commented Jul 30, 2022

Well if it is just a REST call to bitcoind I guess I can get the connection parameters from config. Right ?

We are not calling bitcoind anywhere at the moment from robosats backend. So the wiring will have to be implemented in a similar fashion we do with LND.

Just add a environmental variables in .env & .env-sample . Example:
https://github.com/Reckless-Satoshi/robosats/blob/7645bb39f29ba55ec85d0e2b8caba8335eb57d04/.env-sample#L23
Will possibly also need RPCuser and RPCpassword as environmental variables

Then it could be retrieved in utils.py using config() and used in the validate_onchain_address() function.
https://github.com/Reckless-Satoshi/robosats/blob/7645bb39f29ba55ec85d0e2b8caba8335eb57d04/api/lightning/node.py#L32

@zx9r zx9r mentioned this issue Jul 30, 2022
@ghost
Copy link

ghost commented Jul 30, 2022

I am putting a pull request for this task please check. I have checked this function for this list of btc addresses:
address_list = ['bc1qw508d6qejxtdg4y5r3zarvary0c5xw7kv8f3t4', 'bc1pw508d6qejxtdg4y5r3zarvary0c5xw7kw508d6qejxtdg4y5r3zarvary0c5xw7kt5nd6y', 'bc1sw50qgdz25j', 'bc1zw508d6qejxtdg4y5r3zarvaryvaxxpcs', 'bc1p0xlxvlhemja6c4dqv22uapctqupfhlxm9h8z3k2e72q4k9hcz7vqzk5jj0', '3FLrWFTzcZ7LatttUM4NAKQsvZsL6eNrj3', '18udFJF9mX9txXrmPPVDeURUMWHxXZB2Np', 'bc1qgak3cnfjavwnfxnvf9sxchjnp5euuu9m36xe0d', 'bcsdfhuisdhfjksddkgjdkhgjdhgkdj', 'djflkdjgldfjiqjdigkijqiojgioqejr', 'tb1qw508d6qejxtdg4y5r3zarvary0c5xw7kxpjzsx', 'tb1pqqqqp399et2xygdj5xreqhjjvcmzhxw4aywxecjdzew6hylgvsesf3hn0c', 'mipcBbFg9gMiCh81Kj8tqqdgoZub1ZJRfn', 'mfWxJ45yp2SFn7UciZyNpvDKrzbhyfKrY8', 'n11112Lo13n4GvQhQpDtLY8KH7KNeCVmvw']

for i in address_list:
    info_about_address = check_if_valid_btc_address(i)
    print(i, '\n', info_about_address.valid_address)
    print('__________________')`

@ghost
Copy link

ghost commented Jul 30, 2022

link for the pull request : #200

@Reckless-Satoshi

@Reckless-Satoshi
Copy link
Collaborator Author

Reckless-Satoshi commented Jul 31, 2022

Thank you guys for the prompt action on this Issue! Both approaches look great (#198 & #200).

Just a note for future tasks eligible for Sats. The idea behind the rewarded tasks is to offload work from the lead developer. We also want to avoid duplicate work as much as possible. I believe there was some initial agreement (in telegram) that @zx9r would be the first one to give the task a try. But I understand it was a fun task and both of you wanted to give it a try :)

If the work is duplicated and the assignees require hints/help and testing from the lead dev for two different PRs, then we have failed our main mission: this does not offload work from the lead dev. In fact, it can potentially result in more work for me than if I had implemented myself :D

I will only fully test one of the PRs, the one of @zx9r as it is succinct with no new dependencies and we can be sure it will always work (as it relies directly on Bitcoin Core). We keep @satsbaba PR as a plan B in case we find some issue (e.g. when bitcoind service is down? To explore if this is ever a real issue)

@ghost
Copy link

ghost commented Jul 31, 2022

@Reckless-Satoshi 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⚡Eligible for Sats ⚡ This issue or pull request rewards bitcoin enhancement 🆙 New feature or request good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants