multi: Add initial committed filter (CF) support #1151

jrick · 2018-03-13T14:24:11Z

This change begins the work of bringing committed filters to the
network consensus daemon. Committed filters are designed to enable
light wallets without many of the privacy issues associated with
server-side bloom filtering.

The new gcs package provides the primitives for creating and matching
against Golomb-coded sets (GCS) filters while the blockcf package
provides creation of filters and filter entries for data structures
found in blocks.

The wire package has been updated to define a new protocol version and
service flag for advertising CF support and includes types for the
following new messages: cfheaders, cfilter, cftypes, getcfheaders,
getcfilter, getcftypes. The peer package and server implementation
have been updated to include support for the new protocol version and
messages.

Filters are created using a collision probability of 2^-20 and are
saved to a new optional database index when running with committed
filter support enabled (the default). At first startup, if support is
not disabled, the index will be created and populated with filters and
filter headers for all preexisting blocks, and new filters will be
recorded for processed blocks.

Multiple filter types are supported. The regular filter commits to
output scripts and previous outpoints that any non-voting wallet will
require access to. Scripts and previous outpoints that can only be
spent by votes and revocations are not committed to the filter. The
extended filter is a supplementary filter which commits to all
transaction hashes and script data pushes from the input scripts of
non-coinbase regular and ticket purchase transactions. Creating these
filters is based on the algorithm defined by BIP0158 but is modified
to only commit "regular" data in stake transactions to prevent
committed filters from being used to create SPV voting wallets.

davecgh

This is just a very quick first pass. In addition to the inline comments, the ProtocolVersion in wire/protocol.go needs to be bumped to 6 and a new variable CFilterVersion uint32 = 6 since the wire protocol is being changed.

davecgh · 2018-03-15T04:35:39Z

gcs/gcs.go

+
+	"github.com/aead/siphash"
+	"github.com/decred/dcrd/chaincfg/chainhash"
+	"github.com/kkdai/bstream" // TODO: can probably remove this dep


Yes, let's address this TODO and remove this please. I really would prefer to avoid yet another dependency for something this trivial.

davecgh · 2018-03-15T04:40:29Z

blockchain/indexers/cfindex.go

@@ -0,0 +1,308 @@
+// Copyright (c) 2017 The btcsuite developers
+// Copyright (c) 2017 The Decred developers


davecgh · 2018-03-15T04:40:36Z

LICENSE

-Copyright (c) 2013-2016 The btcsuite developers
-Copyright (c) 2015-2016 The Decred developers
+Copyright (c) 2013-2017 The btcsuite developers
+Copyright (c) 2015-2017 The Decred developers


davecgh · 2018-03-15T04:47:22Z

blockchain/indexers/cfindex.go

+	}
+
+	firstHeader := make([]byte, chainhash.HashSize)
+	err = dbStoreFilterHeader(


Please make the arguments match the normal style.

davecgh · 2018-03-15T04:47:48Z

blockchain/indexers/cfindex.go

+
+// storeFilter stores a given filter, and performs the steps needed to
+// generate the filter's header.
+func storeFilter(dbTx database.Tx, block *dcrutil.Block, f *gcs.Filter,


Func definitions on one line.

davecgh · 2018-03-15T04:50:07Z

config.go

@@ -901,6 +905,14 @@ func loadConfig() (*config, []string, error) {
 		return nil, nil, err
 	}

+	// !--nocfilters and --dropcfindex do not mix.


Not sure why the previous one has an exclamation point. None of the others do. Please remove it and the other one above too.

It's because this is an index enabled by default. The previous check for the addrexistsindex also uses the !, because keeping the index enabled while also dropping the index doesn't make sense.

jrick · 2018-03-22T20:51:50Z

There currently exists a bug where the CF index cannot be dropped. The index dropping code assumes there are no nested buckets in the index and uses a cursor to iterate over the bucket to delete all entries (which itself is a hack to reduce memory usage vs deleting the index bucket itself).

jrick · 2018-03-26T16:31:25Z

Plan is to merge this after #1158 is in, which will solve the problem of being unable to drop the CF index.

davecgh · 2018-03-27T23:29:55Z

wire/msgcfheaders.go

+// BtcDecode decodes r using the wire protocol encoding into the receiver.
+// This is part of the Message interface implementation.
+func (msg *MsgCFHeaders) BtcDecode(r io.Reader, pver uint32) error {
+	// Read stop hash


All of these BtcDecode and BtcEncode functions for the new messages should be checking the protocol version and returning an error if it's before the minimum supported one.

e.g.

if pver < NodeCFVersion { str := fmt.Sprintf("getcftypes message invalid for protocol "+ "version %d", pver) return messageError("MsgCFHeaders.BtcDecode", str) } ...

Then, in the tests for all of these new messages, there should be a cross protocol test function (e.g. TestCFHeadersCrossProtocol for this case) which ensures attempting to decode a message encoded with the new protocol version fails as expected when attempting to decode it with an older protocol version.

See msgfilteradd.go and msgfilteradd_test.go in the upstream btcd code for examples.

There appears to be variation in the meaning of the cross protocol tests. In dcrd's TestSendHeadersCrossProtocol test, the two protocols used both support the message being tested and serialization/deserialization is tested to not error on either. This same test is also present in btcd.

davecgh · 2018-03-27T23:56:52Z

wire/msggetcfilter.go

+// NewMsgGetCFilter returns a new getcfilter message that conforms to the
+// Message interface using the passed parameters and defaults for the remaining
+// fields.
+func NewMsgGetCFilter(blockHash *chainhash.Hash,


Func definitions on the same line, please.

davecgh · 2018-03-28T01:02:24Z

wire/msggetcfilter.go

+	if err != nil {
+		return err
+	}
+	return readElement(r, &msg.FilterType)


This should be cast to (*uint8)(&msg.FilterType) to avoid invoking the slow reflection path in readElement. Either that, or readElement could have *FilterType added as one of the cases. I personally would just do the cast here though as it's less code.

davecgh · 2018-03-28T01:05:36Z

wire/msggetcfilter.go

+	if err != nil {
+		return err
+	}
+	return writeElement(w, msg.FilterType)


This should either be return binarySerializer.PutUint8(w, msg.FilterType) or writeElement would need to have a FilterType case added to avoid the slow reflection-based path. I would suggest just using binarySerializer directly since that matches what is done elsewhere in msgtx.go.

davecgh · 2018-03-28T01:06:54Z

wire/msggetcfilter.go

@@ -0,0 +1,62 @@
+// Copyright (c) 2017 The btcsuite developers
+// Use of this source code is governed by an ISC


Don't forget to add the 2018 Decred copyright when making the modifications.

davecgh · 2018-03-28T02:04:57Z

blockchain/indexers/cfindex.go

+
+// dbFetchFilter retrieves a block's basic or extended filter. A filter's
+// absence is not considered an error.
+func dbFetchFilter(dbTx database.Tx, key []byte, h *chainhash.Hash) ([]byte, error) {


The error return seems unnecessary since nothing returns one.

davecgh · 2018-03-28T04:30:31Z

blockchain/indexers/cfindex.go

+func (idx *CFIndex) FilterHeaderByBlockHash(h *chainhash.Hash, filterType wire.FilterType) ([]byte, error) {
+	var fh []byte
+	err := idx.db.View(func(dbTx database.Tx) error {
+		if uint8(filterType) > maxFilterType {


Same here regarding performing the checkout outside of the db transaction.

davecgh · 2018-03-28T04:38:08Z

gcs/builder/builder.go

+// Use of this source code is governed by an ISC
+// license that can be found in the LICENSE file.
+
+package builder


Any reason this is in a separate package and not just the gcs package? It results in a generic and nondescriptive name as it is in a separate package.

It would be much clearer for the code to read like gcs.BuildExtFilter(...) instead of builder.BuildExtFilter.

I agree with unifying the packages. The builder package came from @Roasbeef's original implementation. Unsure if there was a technical reason for the separation.

There are many other functions exposed by the builder package that would need to be renamed if this change was made. For example, WithKeyPN and the rest of the With... functions. Currently these are called as builder.With... to return a *builder.GCSBuilder, but these function names wouldn't make much sense if they were qualified with the gcs name.

After looking into this some more, it appears that the gcs package is intended to be a general implementation of golomb coded sets while builder is specific to building filters from decred blocks. We can and probably also should remove the extra unused builder methods (everything but BuildBasicFilter and BuildExtFilter).

davecgh · 2018-03-28T04:40:00Z

peer/peer.go

@@ -147,6 +157,18 @@ type MessageListeners struct {
 	// message.
 	OnGetHeaders func(p *Peer, msg *wire.MsgGetHeaders)

+	// OnGetCFilter is invoked when a peer receives a getcfilter decred


s/decred/wire/

davecgh · 2018-03-28T04:40:07Z

peer/peer.go

+	OnGetCFilter func(p *Peer, msg *wire.MsgGetCFilter)
+
+	// OnGetCFHeaders is invoked when a peer receives a getcfheaders
+	// decred message.


s/decred/wire/

davecgh · 2018-03-28T04:40:12Z

peer/peer.go

+	// decred message.
+	OnGetCFHeaders func(p *Peer, msg *wire.MsgGetCFHeaders)
+
+	// OnGetCFTypes is invoked when a peer receives a getcftypes decred


s/decred/wire/

davecgh · 2018-03-28T05:45:59Z

gcs/builder/builder.go

+
+// RandomKey is a utility function that returns a cryptographically random
+// [gcs.KeySize]byte usable as a key for a GCS filter.
+func RandomKey() (key [gcs.KeySize]byte, err error) {


This package is using named returns in various places. Please remove them all.

davecgh · 2018-03-28T05:50:55Z

gcs/builder/builder_test.go

+			0x3b, 0x8b, 0x0e, 0x26, 0x64, 0x8d, 0x4a, 0x15,
+			0x3b, 0x8b, 0x0e, 0x26, 0x64, 0x8d, 0x4a, 0x15,
+		},
+


extra blank line

davecgh · 2018-03-28T08:36:13Z

gcs/builder/builder.go

+	b := WithKeyHash(&blockHash)
+
+	// For each stake transaction, commit the transaction hash.
+	for _, tx := range block.STransactions {


Based on the intent described by the comment of the function, this appears to be missing the signature scripts for all of the stake transactions.

In the case of votes and revocations, it's the output of the ticket that is being spent (.TxIn[1] for votes, and .TxIn[0] for revocations), and in the case of tickets, it is all inputs.

The function comment says it includes the witness/sigscripts of regular transactions. No where does it say it commits to the input scripts of stake transactions.

I'm not sure if it would be worthwhile modifying the extended filter to include this data, or if a 3rd filter type should be added specifically for extra commitments for stake transactions.

davecgh · 2018-03-28T08:39:47Z

wire/msgcfheaders.go

+	}
+
+	// Read filter type
+	err = readElement(r, &msg.FilterType)


As mentioned elsewhere, this needs to be a cast to avoid the slow path (or change readElement).

davecgh · 2018-03-28T08:40:22Z

wire/msgcfheaders.go

+	}
+
+	// Write filter type
+	err = writeElement(w, msg.FilterType)


Same here regarding use binarySerializer.PutUint8.

davecgh · 2018-03-28T08:44:24Z

wire/msgcfilter.go

+		return err
+	}
+	// Read filter type
+	err = readElement(r, &msg.FilterType)


davecgh · 2018-03-28T08:44:34Z

wire/msgcfilter.go

+		return err
+	}
+
+	err = writeElement(w, msg.FilterType)


davecgh · 2018-03-28T09:34:23Z

wire/msgcfilter.go

+	if err != nil {
+		return err
+	}
+	// Read filter type


All the other decode funcs have blank lines between each field, but this one doesn't.

davecgh · 2018-03-28T09:59:27Z

wire/msgcftypes.go

+	if err != nil {
+		return err
+	}
+


The maximum allowed number of filter types is 256 due to it being a uint8. This needs to ensure the value read does not exceed it. Otherwise, it would be trivial to send a message with a massive count and panic the process remotely.

davecgh · 2018-03-28T10:00:02Z

wire/msgcftypes.go

+	msg.SupportedFilters = make([]FilterType, count)
+	for i := uint64(0); i < count; i++ {
+		var filterType uint8
+		err = readElement(r, &filterType)


Same as above regarding the cast or addition of *FilterType to readElement.

Also, to cut down on the number of allocations, please read it directly into the array element.

davecgh · 2018-03-28T10:01:22Z

wire/msgcftypes.go

+	}
+
+	for i := range msg.SupportedFilters {
+		err = writeElement(w, msg.SupportedFilters[i])


Same regarding binarySerializer.PutUint8.

davecgh · 2018-03-28T10:03:08Z

wire/msggetcfheaders.go

+		return err
+	}
+
+	return readElement(r, &msg.FilterType)


davecgh · 2018-03-28T10:03:29Z

wire/msggetcfheaders.go

+		return err
+	}
+
+	return writeElement(w, msg.FilterType)


binarySerializer.PutUint8...

jrick · 2018-03-28T22:11:00Z

The regular filter has been changed to commit to output scripts themselves instead of all data pushes within the script. This significantly reduces filter bloat at the cost of needing to know the exact script used (instead of, say, a hash160).

This introduces a new problem that is specific to Decred. Output scripts of stake transactions include an opcode tag in the scripts to distinguish between a ticket purchase, ticket purchase change, vote coin generation, and revocation coin generation. This now means that SPV wallets that search the filter for address usage must now check 4 (!) possible output scripts instead of 1: P2PKH, OP_SSTXCHANGE P2PKH, OP_SSGEN P2PKH, and OP_SSRTX P2PKH. Obviously this is not ideal, and I'm considering stripping the tag before committing these scripts.

davecgh · 2018-03-28T21:52:25Z

gcs/gcs.go

+	return false
+}
+
+// MatchAny returns checks whether any []byte value is likely (within collision


s/returns checks/checks/

davecgh · 2018-03-28T22:30:50Z

gcs/gcs.go

+	return v*f.modulusP + rem, nil
+}
+
+// Hash returns the double-BLAKE256 hash of the filter.


We should change this to be a single hash. There is no reason to double hash in BLAKE256 since the hash function isn't vulnerable to length extension attacks which is why double sha256 is used in the upstream code.

davecgh · 2018-03-28T22:32:46Z

gcs/gcs.go

+	copy(filterTip, filterHash[:])
+	copy(filterTip[chainhash.HashSize:], prevHeader[:])
+
+	// The final filter hash is the double-blake256 of the hash computed


Same here. No need to double hash.

jrick · 2018-03-29T00:02:56Z

Stake opcode tags are now being stripped before committing the output script.

This change begins the work of bringing committed filters to the network consensus daemon. Committed filters are designed to enable light wallets without many of the privacy issues associated with server-side bloom filtering. The new gcs package provides the primitives for creating and matching against Golomb-coded sets (GCS) filters while the blockcf package provides creation of filters and filter entries for data structures found in blocks. The wire package has been updated to define a new protocol version and service flag for advertising CF support and includes types for the following new messages: cfheaders, cfilter, cftypes, getcfheaders, getcfilter, getcftypes. The peer package and server implementation have been updated to include support for the new protocol version and messages. Filters are created using a collision probability of 2^-20 and are saved to a new optional database index when running with committed filter support enabled (the default). At first startup, if support is not disabled, the index will be created and populated with filters and filter headers for all preexisting blocks, and new filters will be recorded for processed blocks. Multiple filter types are supported. The regular filter commits to output scripts and previous outpoints that any non-voting wallet will require access to. Scripts and previous outpoints that can only be spent by votes and revocations are not committed to the filter. The extended filter is a supplementary filter which commits to all transaction hashes and script data pushes from the input scripts of non-coinbase regular and ticket purchase transactions. Creating these filters is based on the algorithm defined by BIP0158 but is modified to only commit "regular" data in stake transactions to prevent committed filters from being used to create SPV voting wallets.

jrick force-pushed the cf branch from 4e3706d to b43e5b4 Compare March 14, 2018 17:49

davecgh requested changes Mar 15, 2018

View reviewed changes

jrick force-pushed the cf branch 2 times, most recently from 7c50501 to c2914b8 Compare March 16, 2018 17:25

jrick force-pushed the cf branch from f0f98a1 to f30c670 Compare March 22, 2018 17:10

jrick force-pushed the cf branch from d1c224e to 13e9ccc Compare March 27, 2018 23:02

davecgh requested changes Mar 27, 2018

View reviewed changes

davecgh requested changes Mar 28, 2018

View reviewed changes

jrick force-pushed the cf branch from 1afdcec to 4d3ae0f Compare March 28, 2018 21:31

davecgh requested changes Mar 28, 2018

View reviewed changes

jrick force-pushed the cf branch 2 times, most recently from cde78ec to 624ed55 Compare March 30, 2018 17:51

jrick force-pushed the cf branch from 624ed55 to 71500c8 Compare March 30, 2018 17:52

davecgh approved these changes Mar 30, 2018

View reviewed changes

davecgh merged commit 71500c8 into decred:master Mar 30, 2018

jrick deleted the cf branch March 30, 2018 19:57

		@@ -0,0 +1,308 @@
		// Copyright (c) 2017 The btcsuite developers
		// Copyright (c) 2017 The Decred developers

		@@ -0,0 +1,62 @@
		// Copyright (c) 2017 The btcsuite developers
		// Use of this source code is governed by an ISC

multi: Add initial committed filter (CF) support #1151

multi: Add initial committed filter (CF) support #1151

Conversation

jrick commented Mar 13, 2018 • edited Loading

davecgh left a comment • edited Loading

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

jrick commented Mar 22, 2018

jrick commented Mar 26, 2018

Choose a reason for hiding this comment

jrick Mar 28, 2018 • edited Loading

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

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

jrick commented Mar 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrick commented Mar 29, 2018 • edited Loading

jrick commented Mar 13, 2018 •

edited

Loading

davecgh left a comment •

edited

Loading

jrick Mar 28, 2018 •

edited

Loading

jrick commented Mar 29, 2018 •

edited

Loading