Implement ldb database functionality for optional addr index. #11

Roasbeef · 2014-12-25T00:08:00Z

This pull request alters the btcdb.db interface, adding new functions for accessing/updating/deleting the optional address index. For now, only the ldb driver implements the newly defined functions. memdb has also been altered, but only to supply skeleton functions in order for the package to conform to the new btcdb interface (won't compile otherwise). Additionally this PR makes progress towards completion of btcsuite/btcd#190. Subsequent PR's dependant upon this one will be opened across: btcd + btcjson + btcrpcclient, ultimately fully implementing btcd's new optional addrindex.

In order to provide functionality for storage of a new address-based transaction index, the following functions have been added to the btcdb, Db interface:

  type Db interface {
      ....
      FetchAddrIndexTip() (sha *btcwire.ShaHash, height int64, err error)
      UpdateAddrIndexForBlock(blkSha *btcwire.ShaHash, blkHeight int64, addrIndex *BlockAddrIndex) error
      FetchTxsForAddr(addr *btcutil.Address, skip int, limit int) ([]*TxListReply, error)
      DeleteAddrIndex() error
  }

Along with a helper type-def:

type BlockAddrIndex map[[ripemd160.Size]byte][]*btcwire.TxLoc

A single entry in the new optional index takes up 38-bytes raw, and is layed out like the following:

       ----------------------------------------------------------------------
       | Prefix  | Hash160  | BlkHeight | Tx Offset | Tx Size |
       ----------------------------------------------------------------------
       | 2 bytes | 20 bytes |  8 bytes  |  4 bytes  | 4 bytes |
       ----------------------------------------------------------------------

Each index maps a hash160 to a transaction locator object (offset and size within block). Indexes are stored purely in the key (leaving blank bytes for the value), to make it easy to seek and limit the number of transactions returned via iterators. An ability to skip and limit the number of returned transactions is also required to properly implement the searchrawtransaction RPC.

coveralls · 2014-12-25T00:17:21Z

Coverage increased (+2.18%) when pulling 34c7350 on Roasbeef:addrindex into b2186ae on conformal:master.

davecgh · 2014-12-25T01:16:52Z

I don't expect btcwire will need to be touched.

Roasbeef · 2014-12-25T04:46:41Z

Ah, you're right! My mistake, updated current PR description to reflect this.

davecgh · 2015-01-19T04:21:15Z

Can you rebase this and update it for all the new paths please?

davecgh · 2015-01-19T04:33:31Z

db.go

+	// transaction which is commited before the function returns.
+	// Addresses are indexed by the raw bytes of their base58 decoded
+	// hash160.
+	UpdateAddrIndexForBlock(blkSha *btcwire.ShaHash, blkHeight int64,


Minor nit, but the rest of the parameters are called height int64, should probably be consistent.

coveralls · 2015-01-20T00:30:53Z

Coverage increased (+2.18%) when pulling 080fdb5 on Roasbeef:addrindex into c67a305 on btcsuite:master.

davecgh · 2015-01-20T01:03:17Z

@jrick is going to also review this. With the latest changes, it's looking pretty good for me. I'll give it another round of review in a bit.

jrick · 2015-01-20T14:04:59Z

db.go

 )

 // Errors that the various database functions may return.
 var (
+	ErrAddrIndexDoesNotExist  = errors.New("Address index hasn't been built up yet.")


Don't use capitals and punctuation in error strings.

golint should catch this but for some reason the travis build still succeeded.

jrick · 2015-01-20T14:16:43Z

ldb/block.go

+
+	data, err := db.lDb.Get(addrIndexMetaDataKey, db.ro)
+	if err != nil {
+		return &btcwire.ShaHash{}, -1, btcdb.ErrAddrIndexDoesNotExist


This answers my question above.. but why not return nil here instead? Otherwise we end up creating additional garbage for no good reason, for a value that shouldn't be touched to begin with.

Very good point, returning nil instead does seem to be better behavior. It currently returns a pointer to the btcwire zero hash simply to emulate the behavior of NewestSha, for sake of consistency.

If changed to return nil, then following the same logic, should we also change NewestSha and update all callers the react to the new behavior? I think that'd require changes in both btcd and btcchain, and maybe some other packages I can't name off-head.

Hmm true, I didn't realize NewestSha behaved the same way. I think it should be changed, but with the existing db interface being rewritten anyways, I wouldn't worry too much about it.

jrick · 2015-01-20T15:56:02Z

I think I would prefer having the address index methods on a "greater" interface then the existing db. Then, instead of needing to add noop methods to other database drivers (memdb for example) you can type assert your db variable against the interface to check for the existence of address index features.

So, something like this (in btcdb):

type AddressIndexDB interface {
    DB
    FetchAddrIndexTip() (sha *btcwire.ShaHash, height int32, err error)
    UpdateAddrIndexForBlock(blkSha *btcwire.ShaHash, height int32, addrIndex BlockAddrIndex) error
    // ...
}

And the caller:

switch db := db.(type) {
case btcdb.AddressIndexDB:
    // Call address index methods
default:
    // Address index not supported
}

// or...

adb, ok := db.(btcdb.AddressIndexDB)
if !ok {
    // not supported
}
// use address index methods from the adb instance

davecgh · 2015-01-20T16:33:51Z

I think the idea for the AddressIndexDB interface makes sense for longer term, but since btcdb is in the process of being redone completely and this will no longer be an issue, is it worth doing it now?

jrick · 2015-01-20T16:38:16Z

Nah, but it's something to consider when the db interface is rewritten.

Any other optional features could be added in the same manner.

coveralls · 2015-01-21T05:20:17Z

Coverage increased (+2.26%) when pulling ee94a3e on Roasbeef:addrindex into c67a305 on btcsuite:master.

Roasbeef · 2015-01-21T17:47:37Z

Fixed another linter warning, the Travis build scripts seem to be allowing builds through that fail golint?

coveralls · 2015-01-21T18:08:26Z

Coverage increased (+2.41%) to 65.51% when pulling 8d234ab on Roasbeef:addrindex into c67a305 on btcsuite:master.

davecgh · 2015-01-26T21:02:34Z

Ok, I think this is set now. Can you rebase it and squash it down into a single commit (or at least a few commits which address specific functionality) as I like to avoid having a bunch of extra commits on master that are simply addressing review items, lint issues, etc.

coveralls · 2015-01-26T22:08:38Z

Coverage increased (+2.41%) to 65.51% when pulling b284bf0 on Roasbeef:addrindex into c67a305 on btcsuite:master.

Roasbeef · 2015-01-26T22:09:33Z

Rebased+squashed into a single commit.

This was referenced Dec 29, 2014

Add searchrawtransaction btcsuite/btcjson#33

Merged

Add support for an optional address-based transaction index btcsuite/btcd#205

Merged

Roasbeef changed the title ~~[WIP - DO NOT MERGE] Implement ldb database functionality for optional addr index.~~ Implement ldb database functionality for optional addr index. Jan 4, 2015

davecgh reviewed Jan 19, 2015
View reviewed changes

Roasbeef force-pushed the addrindex branch from b89f62d to cd6e6d1 Compare January 19, 2015 23:51

jrick reviewed Jan 20, 2015
View reviewed changes

Implement ldb database functionality for optional addrindex.

b284bf0

Roasbeef force-pushed the addrindex branch from 8d234ab to b284bf0 Compare January 26, 2015 22:05

conformal-deploy merged commit b284bf0 into btcsuite:master Jan 26, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement ldb database functionality for optional addr index. #11

Implement ldb database functionality for optional addr index. #11

Roasbeef commented Dec 25, 2014

coveralls commented Dec 25, 2014

davecgh commented Dec 25, 2014

Roasbeef commented Dec 25, 2014

davecgh commented Jan 19, 2015

davecgh Jan 19, 2015

Roasbeef Jan 20, 2015

coveralls commented Jan 20, 2015

davecgh commented Jan 20, 2015

jrick Jan 20, 2015

Roasbeef Jan 21, 2015

jrick Jan 20, 2015

Roasbeef Jan 21, 2015

jrick Jan 21, 2015

jrick commented Jan 20, 2015

davecgh commented Jan 20, 2015

jrick commented Jan 20, 2015

coveralls commented Jan 21, 2015

Roasbeef commented Jan 21, 2015

coveralls commented Jan 21, 2015

davecgh commented Jan 26, 2015

coveralls commented Jan 26, 2015

Roasbeef commented Jan 26, 2015

Implement ldb database functionality for optional addr index. #11

Implement ldb database functionality for optional addr index. #11

Conversation

Roasbeef commented Dec 25, 2014

coveralls commented Dec 25, 2014

davecgh commented Dec 25, 2014

Roasbeef commented Dec 25, 2014

davecgh commented Jan 19, 2015

davecgh Jan 19, 2015

Choose a reason for hiding this comment

Roasbeef Jan 20, 2015

Choose a reason for hiding this comment

coveralls commented Jan 20, 2015

davecgh commented Jan 20, 2015

jrick Jan 20, 2015

Choose a reason for hiding this comment

Roasbeef Jan 21, 2015

Choose a reason for hiding this comment

jrick Jan 20, 2015

Choose a reason for hiding this comment

Roasbeef Jan 21, 2015

Choose a reason for hiding this comment

jrick Jan 21, 2015

Choose a reason for hiding this comment

jrick commented Jan 20, 2015

davecgh commented Jan 20, 2015

jrick commented Jan 20, 2015

coveralls commented Jan 21, 2015

Roasbeef commented Jan 21, 2015

coveralls commented Jan 21, 2015

davecgh commented Jan 26, 2015

coveralls commented Jan 26, 2015

Roasbeef commented Jan 26, 2015