Skip to content
This repository has been archived by the owner on Nov 2, 2018. It is now read-only.

Add support for offline transaction signing #2907

Open
wants to merge 29 commits into
base: master
Choose a base branch
from

Conversation

lukechampine
Copy link
Member

@lukechampine lukechampine commented Mar 28, 2018

This mostly addresses #1927. I will make a follow-up PR shortly that adds support for watch-only wallets. In the meantime, you can simulate a watch-only wallet by creating a normal wallet and then locking it.

The basic process is:

  1. GET /wallet/unspent to receive a list of spendable outputs
  2. Construct an unsigned transaction using the outputs
  3. Use siac wallet sign to sign the transaction
  4. POST /tpool/raw to broadcast the transaction

wrt 3, siac wallet sign does not require siad; it's completely self-contained. The downside of this is that you need to regenerate a bunch of keys from the seed, and you pay that cost every time you sign a transaction. For wallets that have <10000 addresses, this cost should be barely noticeable, but for larger wallets, there is also a /wallet/sign endpoint that will perform the same signing operation using the keys already in memory.

Lastly, there's the question of how to encode the transactions. /tpool/raw expects base64-encoded binary, but that isn't a very convenient format for constructing transactions. I figured that JSON was a better choice for that, so siac wallet sign and /wallet/sign both expect the transaction to be JSON.

Oh, one more thing: the /sign endpoint expects the JSON to be in the request body, not the query params. This seemed natural given the complex structure of the arguments, but I'm open to changing it.

Copy link
Contributor

@ChrisSchinnerl ChrisSchinnerl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty good to me. I only have a few questions and would like to see a siatest integration test and support in our client package to allow users to easily take advantage of this new feature.

doc/API.md Outdated
{
"transaction": { }, // types.Transaction
"tosign": {
"3689bd3489679aabcde02e01345abcde": "138950f0129d74acd4eade3453b45678",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably mention that those are SiacoinOutputID: UnlockHash/Address pairs.

w.mu.Lock()
defer w.mu.Unlock()
// ensure durability of reported outputs
w.syncDB()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to sync here? Once you got the outputs and created a transaction, the transaction pool will decide if the transaction is valid. Therefore I think it is not really an issue if the output disappears from the wallet due to a crash.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's more about the principle. If the wallet reports that it has an output, and then it crashes, it should still have that output next time you ask it. The general rule is: "don't give the user any data that won't survive a crash."

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see 👍

}
return signTransaction(txn, keys, toSign)
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know that it is not required for unexported functions but I like to have comments everywhere :P

// SignTransaction must derive all of the keys from scratch, so it is
// appreciably slower than calling the Wallet.SignTransaction method. Only the
// first 1 million keys are derived.
func SignTransaction(txn *types.Transaction, seed modules.Seed, toSign map[types.OutputID]types.UnlockHash) error {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it makes sense to create a standalone utility too? Something like siasign instead of siac that keeps the keys in memory once generated and can be used to rapidly sign transactions without having to have a full locked full node installed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if someone wants to, it certainly wouldn't be very difficult. In practice I think most people will just use siac.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

at one point we had siag which was essentially a key generator. I extended it to support signing and multisig as well, but I don't think that we ever released that extension.

I think having siac do everything is fine.

@@ -450,3 +450,64 @@ func TestParallelBuilders(t *testing.T) {
t.Fatal("did not get the expected ending balance", expected, endingSCConfirmed, startingSCConfirmed)
}
}

func TestSignTransaction(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing comment

@@ -111,6 +111,7 @@ type Wallet struct {
func (w *Wallet) Height() types.BlockHeight {
w.mu.Lock()
defer w.mu.Unlock()
w.syncDB()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to sync here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see above -- don't want to report information that won't survive a crash.

@@ -124,6 +124,8 @@ func (api *API) buildHTTPRoutes(requiredUserAgent string, requiredPassword strin
router.GET("/wallet/verify/address/:addr", api.walletVerifyAddressHandler)
router.POST("/wallet/unlock", RequirePassword(api.walletUnlockHandler, requiredPassword))
router.POST("/wallet/changepassword", RequirePassword(api.walletChangePasswordHandler, requiredPassword))
router.GET("/wallet/unspent", RequirePassword(api.walletUnspentHandler, requiredPassword))
router.POST("/wallet/sign", RequirePassword(api.walletSignHandler, requiredPassword))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add those to the client package as well?
It would also be cool to extend the siatest package right away to support this and have a siatest test as well.

// get an output to spend
unspentResp, err := testNode.WalletUnspentGet()
if err != nil {
t.Fatal("failed to get spendable outputs")
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would be good to report err here, too

ID: o.ID,
UnlockHash: o.RelatedAddress,
Value: o.Value,
ConfirmationHeight: types.BlockHeight(math.MaxUint64), // unconfirmed
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure what the best approach is here. I guess this is an argument for making the field Confirmations instead of ConfirmationHeight; then we could set Confirmations: 0. If we instead set ConfirmationHeight to some special value, the API client has to do special processing to calculate the number of confirmations.

@@ -660,6 +662,23 @@ func (w *Wallet) SpendableOutputs() []modules.SpendableOutput {
})
})

// don't include outputs marked as spent in pending transactions
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DavidVorick this seemed like a reasonable thing to do, but would like to hear your thoughts

@@ -678,6 +697,21 @@ outer:
}
}

// add unconfirmed outputs
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto here

@lukechampine
Copy link
Member Author

lukechampine commented Apr 18, 2018

I've updated the /wallet/unspent API so that modules.SpendableOutput includes the UnlockConditions instead of the UnlockHash. Originally I thought that the UnlockConditions should not be revealed, but @DavidVorick assured me that leaking these is not a big concern (they are revealed anyway when you spend them). This makes the /wallet/sign API much nicer, because you can construct the full input beforehand (no need for /wallet/sign to fill in the UnlockConditions), and you only need to specify a list of OutputIDs to sign, rather than a map from OutputID to UnlockHash.

EDIT: Unfortunately, this breaks watch-only wallets, because they don't know what the UnlockConditions are. I've reverted the change. (I had a feeling it was too good to be true...)

@lukechampine lukechampine force-pushed the offline-signing branch 4 times, most recently from 615bd63 to 6c75c12 Compare May 15, 2018 03:41
@fatherOfLegends
Copy link

Is this the PR that HitBTC wants before allowing deposits/withdraws to its Sia wallets? Just curious because I would like to do what it takes to get my coin back.

@magmel48
Copy link

is there any update about this PR?

@DavidVorick
Copy link
Member

@magmel48 we're currently working with the ledger hardware and development kit to make sure that these API endpoints are compatible with hardware wallets. We take compatibility pretty seriously at Sia, and we don't want to merge anything that we aren't completely confident in. Since offline signing has the most obvious use case of hardware wallets, we want to make sure that our first API support is actually sufficient to create a smooth hardware wallet experience.

This is not likely to be ready until v1.3.4, but when we merge it we'll probably also have support for the ledger hardware wallet ready to be released as well.

@fatherOfLegends
Copy link

@magmel48 so I should expect my Sia wallet on HitBTC to be unavailable for 2-3 more months?

@DavidVorick
Copy link
Member

@lmckenzie that might not be legal. HitBTC is custodian to your money, they have to respond to requests to withdraw it. Hardware wallets have never been a feature for Sia, and any hardware wallet that was created previously would still work today (because we aggressively preserve compatibility), so there's really not any excuse.

HitBTC was able to do Sia withdrawals before, they should be able to continue using the same process today to honor new withdrawal requests. I could understand them disabling deposits, but as a custodian it's legally very risky for them to keep withdrawals disabled simply on grounds of no hardware wallet support.

@lukechampine
Copy link
Member Author

I'm gonna think out loud here for a bit, Mostly as a way to clarify my own thoughts, but maybe this will be of interest to others as well.

The wallet generates keys by hashing its seed with a monotonically-incremented counter, called the index. So private key 0 is H(seed|0), private key 1 is H(seed|1), etc. Hashing is one-way, so if you're just given a private key, you can't figure out which seed and index was used to generate it. However, if you already know the seed, you can try hashing successive indices until you get one that matches the key. This works for public keys too: you can hash successive indices to get private keys, compute the public key from the private key, and then compare it to what you're searching for.

This is what the PR currently does. You give SignTransaction a list of UnlockHashs to look up, and it checks each index to see if there's a match, giving up after some threshold. This is feasible on a desktop CPU, because you can check many thousands of indices per second. But on a less powerful processor, this search process can take much longer. On a hardware wallet, it could take up to a minute. Clearly we need a different approach.

We can remove the need for this search process if we have a lookup table from UnlockHash to index. Then we can just compute the private key directly, no searching required. To facilitate this, all we really need to do is add an index field to spendableKey. When signing a transaction with an unlocked siad wallet, you'll call SignTransaction with a map[types.OutputID]types.UnlockHash to specify which inputs you'd like to sign. The wallet will lookup the spendableKey for each UnlockHash, fill in the UnlockConditions, then use the index to generate the keypair and sign.

The workflow for a hardware wallet is a bit different. When the device is first set up, a seed is generated and stored in persistent memory. (The seed is device-specific, not a standard Sia wallet seed, and should never leave the device.) To get an address, siad will ask the device to derive a public key using a supplied index. siad will then create a set of UnlockConditions that use the public key, pair these with the index, and store the result in the wallet db.
When signing, you'll provide the same map[types.OutputID]types.UnlockHash as before. siad will look up the corresponding UnlockConditions and index in the wallet db, and send the index and hash to the device. The device will derive the key from the index, sign the hash, and return the signature, which siad will use to construct the full, signed transaction. This approach seems to match how Bitcoin hardware wallets work. In Go terms, the hardware wallet is just:

type HardwareWallet interface {
    DeriveKey(index uint64) (pubkey [32]byte)
    SignHash(hash [32]byte, index uint64) (signature [64]byte)
}

I like this approach because it requires minimal code and storage on the hardware wallet itself. Smaller code means less bug surface, and the two-function interface is quite flexible. For example, you could use it to sign an arbitrary message with the same private key that controls an address.

This will require modifying the wallet db slightly to add a bucket that stores UnlockConditions+index pairs. We may also want to create a new type, distinct from spendableKey, since that type has historically been used for siag keys (which aren't derived from an index). Not too difficult.

Final note: David and I also discussed storing the UnlockHash -> index lookup table on the device itself (instead of in the wallet db). This saves some code in siad, but it quickly becomes infeasible due to the highly constrained storage space of such devices. The Ledger Nano S, for example, has only 320kb of persistent storage, so it could only store a few thousand addresses. We could use the brute-force strategy to trade time for storage space, but this isn't great either; no one wants to wait 30 seconds for their signature. These approaches also fail if you want to use special UnlockConditions, like a multi-sig or timelocked output.

@DavidVorick
Copy link
Member

Looks pretty good to me. I'm wondering if we should do a little extra work here to make sure that we can support HD keys in the future. I'm not sure what that would entail entirely, but if the hardware wallet code and interface is able to do HD keys, then we won't have to upgrade a bunch of legacy hardware wallets to add it to siad later.

@lukechampine
Copy link
Member Author

I think at this point it's probably best to keep things simple. If we support HD keys on the hardware wallet but not in siad, it would probably confuse users. One thing we could do is, instead of storing the derivation index as a uint64, store a generic string. Then we'd be able to support HD key derivation paths down the line.

In a nutshell, SignTransaction now does less work: it requires the
user to fill out the UnlockConditions and TransactionSignatures of
the transaction, whereas before it would fill them in itself. This
is a better approach because it makes the most common operation --
signing all the inputs that the wallet controls -- dead simple. And
requiring the user to fill out the unlock conditions isn't a big
deal, because they can get those from /wallet/unspent. Lastly, if
the user is responsible for filling out the TransactionSignature
fields, they can control precisely what gets signed.
@lukechampine
Copy link
Member Author

lukechampine commented Jul 12, 2018

I'm reversing what I said earlier about unlock conditions: it's ok to require them. At the end of the day, if you want to spend an output, you need to know what its unlock conditions are. If you're using a watch-only wallet, you may or may not know the unlock conditions of the addresses you're tracking; that's okay. You might be using it as part of an offline-signing scheme, or you could just be interested in tracking the balances of a set of addresses. If you're in the latter group, /wallet/unspent just won't contain the unlock conditions.

The really nice thing about this decision is that it greatly simplifies the standard signing process. Since the user will have to fill in the unlock conditions themselves, SignTransaction can now calculate the unlock hashes itself. That means that it can automatically sign any inputs that it has keys for; you don't have to provide the toSign argument at all. I think this is how bitcoind does it: you just provide the partially-completed transaction, and SignTransaction automagically signs it. Of course, if you only want to sign a specific set of inputs, that's possible too. The automagic version is just a convenience.

There's still two missing pieces of the offline-signing puzzle. First, we need a true watch-only wallet. At bare minimum this means methods/api calls for loading addresses to watch (and optionally the unlock conditions along with them). Second, SpendableOutput needs to include the information used to derive the associated signing key, whether that ends up being a BIP44 path or a simple index. This is required by the Ledger app and other hardware wallets. Once we have those two things, it will be feasible to conduct a proper offline-signing scheme with a hardware wallet.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants