Add support for offline transaction signing #2907

lukechampine · 2018-03-28T03:46:29Z

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:

GET /wallet/unspent to receive a list of spendable outputs
Construct an unsigned transaction using the outputs
Use siac wallet sign to sign the transaction
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.

ChrisSchinnerl

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.

ChrisSchinnerl · 2018-03-28T14:15:17Z

doc/API.md

+{
+  "transaction": { }, // types.Transaction
+  "tosign": {
+    "3689bd3489679aabcde02e01345abcde": "138950f0129d74acd4eade3453b45678",


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

ChrisSchinnerl · 2018-03-28T15:47:07Z

modules/wallet/transactionbuilder.go

+	w.mu.Lock()
+	defer w.mu.Unlock()
+	// ensure durability of reported outputs
+	w.syncDB()


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.

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."

ChrisSchinnerl · 2018-03-28T15:51:38Z

modules/wallet/transactionbuilder.go

+	}
+	return signTransaction(txn, keys, toSign)
+}
+


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

ChrisSchinnerl · 2018-03-28T15:53:37Z

modules/wallet/transactionbuilder.go

+// 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 {


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?

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

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.

ChrisSchinnerl · 2018-03-28T15:55:53Z

modules/wallet/transactionbuilder_test.go

@@ -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) {


missing comment

ChrisSchinnerl · 2018-03-28T15:57:00Z

modules/wallet/wallet.go

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


Why do we need to sync here?

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

ChrisSchinnerl · 2018-03-28T15:58:31Z

node/api/routes.go

@@ -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))


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.

Add siatest and client integration for offline signing

leevlad · 2018-03-30T21:54:15Z

siatest/wallet/wallet_test.go

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


would be good to report err here, too

lukechampine · 2018-04-12T05:49:18Z

modules/wallet/transactionbuilder.go

+					ID:                 o.ID,
+					UnlockHash:         o.RelatedAddress,
+					Value:              o.Value,
+					ConfirmationHeight: types.BlockHeight(math.MaxUint64), // unconfirmed


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.

lukechampine · 2018-04-12T05:50:07Z

modules/wallet/transactionbuilder.go

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

+	// don't include outputs marked as spent in pending transactions


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

lukechampine · 2018-04-12T05:50:25Z

modules/wallet/transactionbuilder.go

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

+	// add unconfirmed outputs


lukechampine · 2018-04-18T00:02:50Z

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...)

This reverts commit c5098c8.

fatherOfLegends · 2018-05-15T18:06:34Z

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 · 2018-05-25T11:18:34Z

is there any update about this PR?

DavidVorick · 2018-05-25T14:40:51Z

@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 · 2018-05-25T19:08:19Z

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

DavidVorick · 2018-05-25T21:24:09Z

@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 · 2018-05-26T00:38:54Z

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 · 2018-05-28T00:09:44Z

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 · 2018-05-28T03:22:31Z

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 · 2018-07-12T20:33:59Z

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.

lukechampine added 9 commits March 27, 2018 23:10

add offline signing functionality

d39e94f

sync before reporting wallet height

1c7efd4

add api routes for unspent+sign

4fa416a

add api docs for unspent+sign

e959025

change sign semantics

6332d01

add wallet sign command

b3741e7

generate keys incrementally

41c5410

use new SpendableOutput type for /unspent

32059e7

sign SiafundInputs as well

277d93a

ChrisSchinnerl reviewed Mar 28, 2018

View reviewed changes

ChrisSchinnerl and others added 6 commits March 29, 2018 12:29

Add siatest and client integration for offline signing

873500b

Merge pull request #2913 from NebulousLabs/offline-signing-siatest

043674b

Add siatest and client integration for offline signing

Merge branch 'master' into offline-signing

a2bcb24

decode directly into toSign map

90566ab

add docstrings

d408cc1

document tosign types

4050676

leevlad reviewed Mar 30, 2018

View reviewed changes

account for unconfirmed txns in SpendableOutputs

64ff690

lukechampine commented Apr 12, 2018

View reviewed changes

modules/wallet/transactionbuilder.go Outdated

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

}

}

// add unconfirmed outputs

Copy link

Member Author

lukechampine Apr 12, 2018

Choose a reason for hiding this comment

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

ditto here

lukechampine added 3 commits April 17, 2018 18:17

add wallet sign -raw flag, JSON by default

78c2a13

try /wallet/sign before doing keygen

d2c89fc

include UnlockConditions in SpendableOutput

c5098c8

lukechampine force-pushed the offline-signing branch from 1d1a944 to c5098c8 Compare April 18, 2018 00:02

Revert "include UnlockConditions in SpendableOutput"

c1c14d7

This reverts commit c5098c8.

lukechampine force-pushed the offline-signing branch from 52a2d32 to 3adbd2f Compare April 25, 2018 20:26

lukechampine force-pushed the offline-signing branch 2 times, most recently from e108d18 to 4732acc Compare May 14, 2018 04:35

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

Merge branch 'master' into offline-signing

0514348

lukechampine force-pushed the offline-signing branch from 6c75c12 to 0514348 Compare May 15, 2018 18:10

don't include unconfirmed outputs that may be spent

6b22c87

lukechampine added 2 commits May 30, 2018 19:37

add UnlockConditions to SpendableOutput

477a497

Merge branch 'master' into offline-signing

c36c14e

lukechampine mentioned this pull request Jun 5, 2018

Ledger Integration #3081

Open

lukechampine added 4 commits July 11, 2018 12:15

Merge branch 'master' into offline-signing

30a5854

fix TransactionPoolRawPost signature

83967e1

add wallet broadcast cmd

4fccafd

more helpful signature decoding error

c97a9d7

lukechampine force-pushed the offline-signing branch from 13df66f to 2038081 Compare July 12, 2018 19:16

lukechampine force-pushed the offline-signing branch from 2038081 to f174e41 Compare July 12, 2018 19:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for offline transaction signing #2907

Add support for offline transaction signing #2907

lukechampine commented Mar 28, 2018 •

edited

Loading

ChrisSchinnerl left a comment

ChrisSchinnerl Mar 28, 2018

ChrisSchinnerl Mar 28, 2018

lukechampine Mar 28, 2018

ChrisSchinnerl Mar 28, 2018

ChrisSchinnerl Mar 28, 2018

ChrisSchinnerl Mar 28, 2018

lukechampine Mar 28, 2018

DavidVorick May 2, 2018

ChrisSchinnerl Mar 28, 2018

ChrisSchinnerl Mar 28, 2018

lukechampine Mar 28, 2018

ChrisSchinnerl Mar 28, 2018

leevlad Mar 30, 2018

lukechampine Apr 12, 2018

lukechampine Apr 12, 2018

lukechampine Apr 12, 2018

lukechampine commented Apr 18, 2018 •

edited

Loading

fatherOfLegends commented May 15, 2018

magmel48 commented May 25, 2018

DavidVorick commented May 25, 2018

fatherOfLegends commented May 25, 2018

DavidVorick commented May 25, 2018

lukechampine commented May 26, 2018

DavidVorick commented May 28, 2018

lukechampine commented May 28, 2018

lukechampine commented Jul 12, 2018 •

edited

Loading

Add support for offline transaction signing #2907

Are you sure you want to change the base?

Add support for offline transaction signing #2907

Conversation

lukechampine commented Mar 28, 2018 • edited Loading

ChrisSchinnerl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukechampine commented Apr 18, 2018 • edited Loading

fatherOfLegends commented May 15, 2018

magmel48 commented May 25, 2018

DavidVorick commented May 25, 2018

fatherOfLegends commented May 25, 2018

DavidVorick commented May 25, 2018

lukechampine commented May 26, 2018

DavidVorick commented May 28, 2018

lukechampine commented May 28, 2018

lukechampine commented Jul 12, 2018 • edited Loading

lukechampine commented Mar 28, 2018 •

edited

Loading

lukechampine commented Apr 18, 2018 •

edited

Loading

lukechampine commented Jul 12, 2018 •

edited

Loading