-
Notifications
You must be signed in to change notification settings - Fork 16
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 method to arbitrary msg using bip322 #184
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.
LGTM! 🚀
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.
Great work!
walletcontroller/client.go
Outdated
} | ||
|
||
if !txscript.IsPayToWitnessPubKeyHash(toSpend.TxOut[0].PkScript) { | ||
return nil, fmt.Errorf("") |
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.
empty error?
// To work properly: | ||
// - wallet must be unlocked | ||
// - address must be under wallet control | ||
// - address must be native segwit address |
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.
Our system should also support taproot address, right? How can we deal with that?
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.
so for now, from what I understand, we cannot support them now as we are forced to use old version of bitcoind with legacy wallets. This is due to using DumpPrivateKey
operation which is unsupported in later version.
So the road to support bip86 taproot addresses would be:
- remove
DumpPrivateKey
- bump bitcoind to latest versions (v26+)
- start experimenting how to add support for those addresses
One note though, this is not super criticial as:
- native segwit addresses are most popular ones
- in my mind staker progam in the future will by default generate new address for new staking operation (new public key) and default generated address by
getnewaddress
function is native segwit one.
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.
added issue to track this: #185
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 explanation! It makes sense
ref: https://github.com/babylonchain/pm/issues/48
One of the places where staker program dumps private key is when creating pop for Babylon:
btc-staker/staker/stakerapp.go
Line 1460 in 5ab83e6
To remove
DumPrivateKey
function pop needs to be created in some other way.Unfortunately, we cannot use https://developer.bitcoin.org/reference/rpc/signmessage.html bitcoind api as this does not work for native segwit addresses (only legacy ones which should be avoided) . See: bitcoin/bitcoin#10542
Fortunately, Babylon supports pop with bip322 signatures. This pr add necessary api to create such signature along with the test. Followup pr will switch creating pop to this functions