CurrentAddress - mark first address used and return new address #238

tuxcanfly · 2015-04-10T05:56:32Z

Fixed Last(In/Ex)ternalAddress to make sure CurrentAddress returns a new address when the first and subsequent addresses are flagged as used.

manan19 · 2015-04-10T19:47:56Z

This solves part of the problem I was facing.

I think the first generated external address should be also be added to first rescan job.

Right now, despite calling .CurrentAddress, the first address isn't included in the rescan job.
Since it's not included in that list, it won't be marked as used, even though it has a balance on it. .CurrentAddress will keep returning the same address.

This issue might need to be fixed in the wallet package.

Also, IMO .NewAddress should be changed to .newAddress (un-exported)

jrick · 2015-04-13T21:10:26Z

What about erroring on last in/external address if none have been created yet? It should never have been creating new addresses to begin with.

Other option here is to keep the api similar but write the address to the db, but this changes an otherwise read-only call to one that may require a db update.

manan19 · 2015-04-13T22:05:35Z

@jrick That will work too. It means that .CurrentAddress can implement the error handling for .LastExternalAddress and call .NewAddress.

Although, this approach would mean that the first rescan job will have 0 addresses. It would scan for the first address only after a call to .CurrentAddress (Not ideal, I'd much rather have the first address be included on the first rescan job. But that is something that can be modified by wallet developers)

jrick · 2015-04-13T23:21:01Z

The rescan with 0 addresses is actually intentional because the response that btcwallet gets after the finished rescan is the block that the wallet is marked in sync with.

However, btcd should detect the 0 address 0 outpoint rescan and not actually load blocks and compare addresses against empty maps in that case.

One thing that appears broken with the current waddrmgr is that when a full rescan is performed (I noticed after explicitly dropping all transaction history on my intwtxmgr branch) the rescan starts from the genesis block and not the latest seen block just before the first address was created. If this is fixed, then the rescan can be avoided, and when there are still no addresses, that initial starting sync block can be continuously updated, until the first address is created. I also like this solution since addresses never exist before the user requests them (external) or they're needed for change (internal), and it will help avoid address reuse.

Another thought: since the rescan for the 0 address case is only being used to mark what block the wallet is in sync with, and we can continuously update this block from the websocket notifications as new blocks are attached, the rescan can be avoided. But we still need an RPC round trip to fetch the current block height/hash, so I don't think it really matters either way, and leaving it in the rescan means less code in wallet.

This has gotten a little rambly, but does that make sense?

jrick · 2015-04-14T15:20:42Z

waddrmgr/manager.go

+		return acctInfo.lastExternalAddr, nil
+	}
+
+	return nil, managerError(ErrAddressNotFound, "address not found", nil)


Since this isn't looking up a particular address by string, hash160, etc., can you change the error description string to something like "no previous external address"?

Also, need a similar error for LastInternalAddress below.

jrick · 2015-04-14T15:21:26Z

Please update the PR title and description, since this problem is being fixed by a different method.

tuxcanfly · 2015-04-14T16:38:15Z

Yup, I agree about not creating addresses before they are required.

Updated Last(In/Ex)ternalAddress to err on first address. Also had to update loadAccountInfo to query whether the addr is used and clear the cache after it's flagged.

jrick · 2015-04-14T21:20:00Z

waddrmgr/db.go

@@ -1031,6 +1019,18 @@ func fetchAddressByHash(tx walletdb.Tx, addrHash []byte) (interface{}, error) {
 	return nil, managerError(ErrDatabase, str, nil)
 }

+// fetchAddressUsed returns true if the provided address id was flagged as used.
+func fetchAddressUsed(tx walletdb.Tx, addressID []byte) bool {


As discussed on IRC, I'd like to see all usage of the phrase "address ID" replaced with something like "Hash160", since it is the ripemd160 after sha256 of the pubkey, or the script. This is ok for now, but please fix this in another pr.

OK. I'm going to keep any new vars in this PR as addressID so I can find and replace all of them in the other PR.

manan19 · 2015-04-15T00:48:47Z

The way .CurrentAddress is implemented it would generate only 1 .NewAddress and stop. I don't think it will work well, when a wallet is being restored from a seed.

1st address, 0 balance, 2 transactions
change address, 0.1 balance, 1 transaction
2nd ext address, 0.2 balance, 1 transaction

.CurrentAddress in this scenario would be expected to return the 3rd ext address. It should recursively generate a new address until it comes across an address with 0 balance and 0 transactions.

(I've assumed change addresses are generated from a different branch and hence the numbers)

jrick · 2015-04-15T02:14:03Z

CurrentAddress only exists to implement the getaccountaddress rpc. It's ok if it's not terribly useful for recovering from a seed, since it wasn't designed for that use case.

In fact I wouldn't mind moving the CurrentAddress implementation from the wallet package into rpcserver.go in main, since it's only to be used for that. Doing so would also help to discourage address reuse, since a previously shown address will never be re-shown in case nothing was received by it.

tuxcanfly · 2015-04-15T04:20:17Z

waddrmgr/manager.go

+
+// MarkUsed updates the used flag for the provided address.
+func (m *Manager) MarkUsed(address btcutil.Address) error {
+	addressID := address.ScriptAddress()


@jrick I think this was the external API call you mentioned which takes addressID so I hope you're fine with changing the signature. We might need to notify any callers but since this was added only recently I don't think anyone would be using it.

as long as it's a different type, finding and unbreaking callers is easy.

tuxcanfly · 2015-04-15T11:30:12Z

@jrick @davecgh I think the larger problem is that we're storing the mutable used in an otherwise immutable address and this leads to issues like the above where we need to be careful about clearing the caches. I just started looking at wtxmgr but if we could index transactions by address, it might be easier to check that we have no transactions belonging to the address and hence determine it's used or not.

jrick · 2015-04-15T13:02:42Z

I think the used flag is still correct. If a transaction is sent to some address, and the address is marked "used", then if that transaction is removed from wtxmgr (perhaps it was double spent), the address shouldn't become unused. Once used, the address should never become unused again.

tuxcanfly · 2015-04-15T13:26:08Z

Yeah, just checked wtxmgr and looks like it is ranging over all transactions.

OK, let me test this out and prep for review. Thanks.

jrick · 2015-04-15T13:42:39Z

Where is it ranging transactions? Only place I can remember it doing that and filtering by address is Wallet.ListAddressTransactions, which I want to remove (along with the RPC). See #231

jrick · 2015-04-15T13:54:34Z

Well, let's keep things simple. Until waddrmgr and wtxmgr are properly combined (this will happen after the current wtxmgr integration is merged), we'll keep the transaction iterating to detect used-ness. Once combined, we'll introduce this flag again.

tuxcanfly · 2015-04-15T14:54:44Z

Yeah, I was checking if ListAddressTransactions was filtering by address or if it was using an index.

jrick · 2015-04-15T17:38:37Z

waddrmgr/manager_test.go

@@ -1028,28 +1028,32 @@ func testMarkUsed(tc *testContext) bool {
 	addrHash := expectedAddr1.addressHash
 	addr, err := btcutil.NewAddressPubKeyHash(addrHash, chainParams)

+	maddr, err := tc.manager.Address(addr)
+	if err != nil {
+		tc.t.Errorf("%s: unexpected error: %v", prefix, err)


Can't continue with the rest of this test, either return here of make this fatal

jrick · 2015-04-15T17:57:34Z

All of the waddrmgr changes look ok now but we need handling in Wallet.CurrentAddress so that if the error is because no addresses exist yet, it handles it by creating the first external address.

tuxcanfly · 2015-04-15T18:13:48Z

Updated CurrentAddress to handle ErrAddressNotFound and generate a new address. Assumption is LastExternalAddress only returns this err for missing the first address.

jrick · 2015-04-15T18:18:17Z

Thanks, going to test this out but the diff looks ok now. Can you squash?

tuxcanfly · 2015-04-15T18:24:28Z

Sure, done.

jrick · 2015-04-15T18:29:09Z

sorry, rebase on master too

tuxcanfly · 2015-04-16T01:07:01Z

OK, rebased.

tuxcanfly force-pushed the put-first-addr branch from 7852baf to f68e78a Compare April 14, 2015 14:17

jrick reviewed Apr 14, 2015
View reviewed changes

tuxcanfly force-pushed the put-first-addr branch 2 times, most recently from 3570331 to 7f7903f Compare April 14, 2015 16:27

tuxcanfly changed the title ~~Generate first addresses at account creation~~ CurrentAddress - mark first address used and return new address Apr 14, 2015

tuxcanfly force-pushed the put-first-addr branch 2 times, most recently from dcea4e0 to 3818ac7 Compare April 14, 2015 17:26

jrick reviewed Apr 14, 2015
View reviewed changes

tuxcanfly force-pushed the put-first-addr branch 5 times, most recently from 859721f to 4775040 Compare April 15, 2015 04:18

tuxcanfly reviewed Apr 15, 2015
View reviewed changes

tuxcanfly mentioned this pull request Apr 15, 2015

waddrmgr: optimize using each style functions #216

Merged

jrick reviewed Apr 15, 2015
View reviewed changes

tuxcanfly force-pushed the put-first-addr branch 2 times, most recently from c90346f to 1f90d3d Compare April 15, 2015 17:50

tuxcanfly force-pushed the put-first-addr branch from 1f90d3d to 4157111 Compare April 15, 2015 18:11

tuxcanfly force-pushed the put-first-addr branch 2 times, most recently from 9f8fb3b to 96b20e4 Compare April 15, 2015 18:22

tuxcanfly force-pushed the put-first-addr branch from 96b20e4 to 74208f9 Compare April 16, 2015 01:04

CurrentAddress: subsequently return new address

74208f9

conformal-deploy merged commit 74208f9 into btcsuite:master Apr 16, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CurrentAddress - mark first address used and return new address #238

CurrentAddress - mark first address used and return new address #238

tuxcanfly commented Apr 10, 2015

manan19 commented Apr 10, 2015

jrick commented Apr 13, 2015

manan19 commented Apr 13, 2015

jrick commented Apr 13, 2015

jrick Apr 14, 2015

jrick commented Apr 14, 2015

tuxcanfly commented Apr 14, 2015

jrick Apr 14, 2015

tuxcanfly Apr 15, 2015

manan19 commented Apr 15, 2015

jrick commented Apr 15, 2015

tuxcanfly Apr 15, 2015

jrick Apr 15, 2015

tuxcanfly commented Apr 15, 2015

jrick commented Apr 15, 2015

tuxcanfly commented Apr 15, 2015

jrick commented Apr 15, 2015

jrick commented Apr 15, 2015

tuxcanfly commented Apr 15, 2015

jrick Apr 15, 2015

jrick commented Apr 15, 2015

tuxcanfly commented Apr 15, 2015

jrick commented Apr 15, 2015

tuxcanfly commented Apr 15, 2015

jrick commented Apr 15, 2015

tuxcanfly commented Apr 16, 2015

CurrentAddress - mark first address used and return new address #238

CurrentAddress - mark first address used and return new address #238

Conversation

tuxcanfly commented Apr 10, 2015

manan19 commented Apr 10, 2015

jrick commented Apr 13, 2015

manan19 commented Apr 13, 2015

jrick commented Apr 13, 2015

jrick Apr 14, 2015

Choose a reason for hiding this comment

jrick commented Apr 14, 2015

tuxcanfly commented Apr 14, 2015

jrick Apr 14, 2015

Choose a reason for hiding this comment

tuxcanfly Apr 15, 2015

Choose a reason for hiding this comment

manan19 commented Apr 15, 2015

jrick commented Apr 15, 2015

tuxcanfly Apr 15, 2015

Choose a reason for hiding this comment

jrick Apr 15, 2015

Choose a reason for hiding this comment

tuxcanfly commented Apr 15, 2015

jrick commented Apr 15, 2015

tuxcanfly commented Apr 15, 2015

jrick commented Apr 15, 2015

jrick commented Apr 15, 2015

tuxcanfly commented Apr 15, 2015

jrick Apr 15, 2015

Choose a reason for hiding this comment

jrick commented Apr 15, 2015

tuxcanfly commented Apr 15, 2015

jrick commented Apr 15, 2015

tuxcanfly commented Apr 15, 2015

jrick commented Apr 15, 2015

tuxcanfly commented Apr 16, 2015