bbrtj / btcpaywall

Self-hosted bitcoin payment system
BSD 2-Clause "Simplified" License
7 stars 3 forks source link

unable to use master.key contents to create spendable wallet in Electrum #4

Open artvogel opened 2 years ago

artvogel commented 2 years ago

Hello,

I'm testing btcpaywall with live (not testnet) bitcoin payments.

I have the watch-only wallet set up with bitcoind, and it shows about $70 over three btcpaywall payments being in the watch-only wallet.

However when I try to spend those BTC by setting up a new Electrum wallet using the contents of master.key as a BIP39 passphrase, electrum shows:

Has using master.key been tested using Electrum? And/or what btc wallets is the master.key contents expected to work with? Edit: or what am I doing wrong?

bbrtj commented 2 years ago

Hello!

Thank you for reporting. You're not doing anything wrong, it is me who overlooked some things. While I took extra care to check that derivation paths are correct and matches what other tools like https://iancoleman.io/bip39/ are returning, I did not actually check it with a HD wallet. I was not aware that the standard is to use a bit different derivation paths (44 for old addresses, 49 for compat addresses and 84 for segwit addresses, while I use 44 in all cases).

Your coins are safe, so don't worry about that. But to restore them you will have to do a bit more work than what I initially wanted (which was just to import it into a HD wallet).

I have created a new git branch named coins-restore. This branch contains a new CLI action called restore-keys, which will print out all the private keys of your payment requests which have been completed. The format of those private keys is WIF (Wallet Import Format) and you can use some old-style wallet or a HD wallet 'sweep' feature to redeem them. You can invoke it with carton exec script/btcpaywall restore-keys.

Now that you know how to redeem your $70, I will have to spend next days fixing the HD wallet issue by:

I'm sorry for any inconvenience that caused you. Not knowing about different derivation paths is totally on me - I should've tried importing it myself, but somehow I didn't. I did not expect somebody would use "real" bitcoins with it without testing it thoroughly on testnet first.

bbrtj commented 2 years ago

From https://en.bitcoin.it/wiki/BIP_0049:

Two generally different approaches are possible for current BIP44 capable wallets: 
1) Allow the user to use the same account(s) that they already use, but add segregated witness encoded addresses to it. 
2) Create dedicated accounts used only for segregated witness addresses. 

BtcPaywall used 1. up to this point, and the two approaches are incompatible

bbrtj commented 2 years ago

I might also change how bitcoind is handled - currently, as you found out, only addresses are added to it in watch-only mode to allow for checking of the balance. I think I could technically add private keys there instead, which would enable redeeming coins without the HD wallet. Needs more investigation Edit: But as always, I am a bit wary storing private keys in too many places

bbrtj commented 2 years ago

Hey @artvogel, good news.

Its quite easy to access the funds on a pruned node. Importing the private key immediately associates the previously watched addresses with newly imported private keys, so the funds become spendable and the addresses are no longer watch-only - no rescan needed (which is not available on a pruned node).

I have leveraged that to create a CLI command that immediately imports all private keys that have seen some transactions into the bitcoind wallet and sends a transaction to a given adddress. The usage is: carton exec script/btcpaywall withdraw <address>. Be careful though - it immediately sends the transaction with all available coins to that address. Tested on testnet - works very well. No control over transaction fees though - I've set it to economical.

In your case, you want to run that command from the coins-restore branch. Master is getting changed to make it work well with HD wallets, as this is still the goal. Having only one option to withdraw funds is not exactly a good idea, which is why withdraw command is also going to be available and documented, on top of HD wallet compatibility.

bbrtj commented 2 years ago

Tested with coinomi. With the latest changes on the master branch, paywall transactions are visible in the wallet. Paywall requests created before are no longer in the same address space. Any coins contained in them should be sent to a safe wallet with withdraw action on the coins-restore branch.

Sadly, full interoperability between BtcPaywall and HD wallets cannot be achieved because of limitations defined in BIP44 document:

So the initial idea of full compatibility with HD wallets was flawed. As things stand now, it has partial compatibility - it might work in a specific setting, but there can't be any guarantees. However, with the addition of the withdraw action, it is now possible to move funds out of the system with the tooling it already uses, and possibly also automate it.

TODO: document coin withdrawal options

artvogel commented 2 years ago

hello, @brtastic

Thank you for your code changes related to this, and for the detailed write-up above.

I tried what you suggested:

I do see the coins in the bitcoin wallet using bitcoin-cli (note that I changed the exact amounts a little, but they are close to the actual values)

% bitcoin-cli  getbalances
{
  "mine": {
    "trusted": 0.00097320,
    "untrusted_pending": 0.00000000,
    "immature": 0.00000000
  },
  "watchonly": {
    "trusted": 0.00064200,
    "untrusted_pending": 0.00000000,
    "immature": 0.00000000
  }
}

Also I don't understand why the 'mine' and 'watchonly' 'trusted' values are different.

Advice on how to proceed?

bbrtj commented 2 years ago

Hey @artvogel

so what happens when you run withdraw command is that all private keys from payment requests which are complete or callback_failed are imported into your bitcoind wallet. Then all the possible balance is moved into the address you specified.

Since the balance is taken from both watchonly and normal addresses, it can fail like it did in your case, which means that not all watched addresses were imported correctly.

Since you're already on coins_restore branch, this should not have happened... unless you used the master branch with the same master.key after this commit: 241036792a14e29f2d651b8733f069712c9b4123

The thing is, there was a bug which caused trailing newline in master.key to create incorrect (different than expected) private keys / addresses. So basically master and coins_restore branches are using completely different private keys, even though the mnemonic is still the same.

If you were testing after I changed master 12 days ago and generated / paid to new addresses, then you will also have to run the withdraw command on master branch - which will import all the other (new and proper) private keys. It should work out of the box, since you already have private keys from the old address space imported.

Another possibility is that you might've wiped the database of btcpaywall at some point- which would make withdraw command not see all complete addresses. If that's the case, more custom code will be needed, which will search the address space and import all the private keys in a given range - basically much harder than just importing the private keys we know contain funds. If you think that this is your issue, let me know and I'll try to write a proper command that will recover that.

bbrtj commented 2 years ago

A little correction to the above - current master is using even different address space (bip49 and bip84) so you might also want to use master branch on commit b325d562cc99e3ff646c1422b31a4d55243920da to withdraw. I know that's messed up, so that's understandable that you're having some problems. It all depends on when you were using / updating the code - its either coins_restore, current master, or master on the commit mentioned above

bbrtj commented 2 years ago

Hello @artvogel, you might want to try the withdraw command on the updated coins_restore branch - I've added commit 8c96d6027eb3dcbb97aa09f58120f9d79d90061c, which uses all types of derivation schemes for the withdraw. It should send all your coins no matter at which point in time you sent them