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

rpcserver: Prevent SendCoins to Pubkey #2922

Merged
merged 1 commit into from Apr 22, 2019

Conversation

Projects
None yet
5 participants
@ccdle12
Copy link
Contributor

commented Apr 10, 2019

fixes #2920

This an early PR, this updates SendCoins in rpcserver.go to prevent an onchain transaction to a users own Pubkey.

Updated to check all use of pubkeys in SendCoins.

@OfficialBigAl

This comment has been minimized.

Copy link

commented Apr 11, 2019

Hello - When I originally raised this issue, I was sending from one LN node's On-Chain Btc Address and accidentally sent it to a physically separate LN node's On-Chain Address.

Is your code checking and trying to prevent this? (Sorry if it is - I'm a newbie at coding....)

@ccdle12

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2019

Hello - When I originally raised this issue, I was sending from one LN node's On-Chain Btc Address and accidentally sent it to a physically separate LN node's On-Chain Address.

Is your code checking and trying to prevent this? (Sorry if it is - I'm a newbie at coding....)

@OfficialBigAl,

No worries! If I'm understanding your description, you sent on chain BTC.

From:

                                                              
LN Wallet On Chain Address -------------------> LND Public Key

Is that correct?

Then this update will prevent any on chain BTC to be sent to LND Public Keys via gRPC SendCoins.

@molxyz

This comment has been minimized.

Copy link

commented Apr 11, 2019

Test results from my testnet node:

testnet c-lightning pubkey
>tlncli sendcoins 02aab1cd8a209da31e937ff70c150b5e1cdda96c0725c89cde1ea97b103fbefcbf 500000
[lncli] rpc error: code = Unknown desc = cannot send coins to pubkeys

testnet eclair node:
>tlncli sendcoins 03933884aaf1d6b108397e5efe5c86bcf2d8ca8d2f700eda99db9214fc2712b134 800000
[lncli] rpc error: code = Unknown desc = cannot send coins to pubkeys

testnet lnd node:
>tlncli sendcoins 03ecd95e86e780ae82139c3217b527073622d28c4d8a53b76142a7b7b1d1e36975 900000
[lncli] rpc error: code = Unknown desc = cannot send coins to pubkeys

mainnet c-lightning node:
>tlncli sendcoins 024655b768ef40951b20053a5c4b951606d4d86085d51238f2c67c7dec29c792ca 600000
[lncli] rpc error: code = Unknown desc = cannot send coins to pubkeys

mainnet eclair node: 
>tlncli sendcoins 03864ef025fde8fb587d989186ce6a4a186895ee44a926bfc370e2c366597a3f8f 500000
[lncli] rpc error: code = Unknown desc = cannot send coins to pubkeys

mainnet lnd pubkey:
>tlncli sendcoins 030c3f19d742ca294a55c00376b3b355c3c90d61c6b6b39554dbc7ac19b141c14f 700000
[lncli] rpc error: code = Unknown desc = cannot send coins to pubkeys

@Roasbeef Roasbeef added this to the 0.6.1 milestone Apr 12, 2019

@ccdle12 ccdle12 force-pushed the ccdle12:prevent-sendcoins-pubkey branch from 3ae5998 to d92b6e3 Apr 15, 2019

@cfromknecht

This comment has been minimized.

Copy link
Collaborator

commented Apr 18, 2019

@ccdle12 nice fix! ready for squash and rebase

@ccdle12 ccdle12 force-pushed the ccdle12:prevent-sendcoins-pubkey branch 2 times, most recently from 4e1a049 to a6e7940 Apr 19, 2019

@ccdle12

This comment has been minimized.

Copy link
Contributor Author

commented Apr 19, 2019

@cfromknecht thanks! squashed and rebased 😄

@cfromknecht
Copy link
Collaborator

left a comment

thanks @ccdle12, one small nit that I missed before

Show resolved Hide resolved rpcserver.go Outdated
@cfromknecht
Copy link
Collaborator

left a comment

LGTM 💾 just needs squash

@ccdle12 ccdle12 force-pushed the ccdle12:prevent-sendcoins-pubkey branch 2 times, most recently from 5366964 to c46457f Apr 20, 2019

@ccdle12

This comment has been minimized.

Copy link
Contributor Author

commented Apr 20, 2019

@cfromknecht thanks! all squashed and rebased

@Roasbeef
Copy link
Member

left a comment

LGTM 🛸

@Roasbeef Roasbeef merged commit 6c60caa into lightningnetwork:master Apr 22, 2019

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
coverage/coveralls Coverage decreased (-0.04%) to 59.86%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.