bitpay / bitcore-p2p

Interface to the bitcoin P2P network for bitcore
MIT License
80 stars 276 forks source link

Add FilterLoad, FilterAdd, and FilterClear Messages #39

Closed throughnothing closed 9 years ago

throughnothing commented 9 years ago

This relates to issue #10

throughnothing commented 9 years ago

Huh, I didn't think this one would reduce coverage.

throughnothing commented 9 years ago

Updated the interface for FilterLoad() to just take a filter, got rid of the constants, and used bloom-filter for them, and got rid of fromBloomFilter().

braydonf commented 9 years ago

We're going to need some additional methods on BloomFilter to implement isRelevantAndUpdate as well as toBuffer and fromBuffer as part of having a standard interface for bitcore objects, so I think it makes sense to have the buffer conversion there.

For example we can extend bloom-filter in bitcore-p2p:

lib/bloomfilter.js:


var BloomFilter = require('bloom-filter');

BloomFilter.prototype.fromBuffer = function fromBuffer() {
  ...
};
BloomFilter.prototype.toBuffer = function toBuffer() {
  ...
};

BloomFilter.prototype.isRelevantAndUpdate = function isRelevantAndUpdate(transaction) {
  var hash = transaction.hash;
  // we can implement this later
};

// etc.

And then require it in messages:

lib/messages.js:


var BloomFilter = require('./bloomfilter');

FilterLoad.prototype.fromBuffer = function fromBuffer(buffer) {
  this.filter = BloomFilter.fromBuffer(buffer);
  return this;
}

Additionally the checks, such as MAX_HASH_FUNCS are already made in BloomFilter, so we don't need to check it twice.

braydonf commented 9 years ago

Looks good, we can implement an extended bloomfilter later if its needed.

throughnothing commented 9 years ago

The last commit makes filter required on FilterLoad, but I'm not sure I like it.

braydonf commented 9 years ago

Okay, so I think it makes sense to instantiate where filter would be empty. And that way fromBuffer can instantiate an instance of BloomFilter.

throughnothing commented 9 years ago

I also noticed this isn't actually working on my client, even though the tests pass. It's currently sending an invalid message. Will figure out why.

braydonf commented 9 years ago

A few more tests: https://github.com/throughnothing/bitcore-p2p/pull/1

coveralls commented 9 years ago

Coverage Status

Coverage increased (+0.82%) to 92.75% when pulling 51871a033d520ef6d6ac1751b2d1f286cfdc9d74 on throughnothing:filter-messages into e19734593e1df6c4edc6d73603666eaaf9a9ba98 on bitpay:master.

braydonf commented 9 years ago

LGTM

maraoz commented 9 years ago

generally LGTM. Will review in depth tomorrow. Thanks a lot for the awesome contrib! :)

maraoz commented 9 years ago

ACK, sans minor comment

coveralls commented 9 years ago

Coverage Status

Coverage increased (+2.05%) to 93.98% when pulling 0ac40a9c47305fed76c4ae910d94e681a7d914a6 on throughnothing:filter-messages into e19734593e1df6c4edc6d73603666eaaf9a9ba98 on bitpay:master.

throughnothing commented 9 years ago

Fixed the _.isUndefined() issue, I think this is good to go now.