Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Serve BIP 157 compact filters #16442

Open
wants to merge 12 commits into
base: master
from

Conversation

@jimpo
Copy link
Contributor

jimpo commented Jul 23, 2019

This enables nodes to serve block filters compliant with BIP 157. This is the last PR required for BIP 157/158 compatibility.

If the node has the block filter index enabled, once the index is in sync, the node will begin signaling the BIP 158 service bit and responding to BIP 157 requests. On invalid requests, the node disconnects the offending peer.

This is tested using functional tests. I have also synced an lnd testnet node using the neutrino backend connected to a Core node running this code.

Questions:

  • Should there be a separate CLI flag other than -blockfilterindex to enable serving of filters from the index?
  • Is the mechanism to only signal the service flag once the index is in sync OK?
  • Is the use of boost::shared_lock around the checkpoint cache acceptable?
@jimpo jimpo force-pushed the jimpo:bip157-net branch from 6dfdd94 to cdbf8ac Jul 23, 2019
@fanquake fanquake added the P2P label Jul 23, 2019
@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Jul 23, 2019

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #17485 (net processing: Don't reach into CBlockIndex to check for block mutation by jnewbery)
  • #17479 (Return BlockValidationState from ProcessNewBlock if CheckBlock/AcceptBlock fails by jnewbery)
  • #17383 (Refactor: Move consts to their correct translation units by jnewbery)
  • #15443 (qa: Add getdescriptorinfo functional test by promag)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented Jul 23, 2019

Nice! Thanks for working on this.

Should there be a separate CLI flag other than -blockfilterindex to enable serving of filters from the index?

I think so,... because some users may want to use the blockfilters only "internal", more specific, to rescan wallets (especially in prune mode), though functionality hasn't been added to Bitcoin Core yet

Is the mechanism to only signal the service flag once the index is in sync OK?

I'd say yes. But unsure since I think we also signal NODE_NETWORK when in IBD.

@jimpo jimpo force-pushed the jimpo:bip157-net branch from 98331d7 to 3d398a8 Jul 24, 2019
@jamesob

This comment has been minimized.

Copy link
Member

jamesob commented Jul 24, 2019

Concept ACK - will review within the next few days.

Copy link
Contributor

TheBlueMatt left a comment

Just reviewed the first bit but I presume some of these comments will apply later too.

@@ -2597,7 +2599,18 @@ uint64_t CConnman::GetTotalBytesSent()

ServiceFlags CConnman::GetLocalServices() const
{
return nLocalServices;
uint64_t local_services = nLocalServices;
if (local_services & NODE_COMPACT_FILTERS) {

This comment has been minimized.

Copy link
@TheBlueMatt

TheBlueMatt Jul 29, 2019

Contributor

Please dont add more node logic in net.cpp - if you want node logic put it in net_processing. But, honestly, this should be handled either by init checking these conditions or by the filtering code itself.

This comment has been minimized.

Copy link
@jimpo

jimpo Jul 30, 2019

Author Contributor

The point of this code is that the filter index may be building in the background and we don't want to signal NODE_COMPACT_FILTERS to peers until it is in sync. Then as soon as it does finish syncing, turn it on.

I'm not sure this is the best way to do this so, I'm open to suggestions (this is the subject of two of the questions in the PR description). Another option would be to have a separate CLI flag for signalling NODE_COMPACT_FILTERS, which also blocks the net threads until the filter indices are in sync. That way the user can sync the filter index in the background, then shut down the node and restart with the BIP 157 net stuff enabled. Thoughts?

This comment has been minimized.

Copy link
@jamesob

jamesob Aug 6, 2019

Member

I spent a few minutes writing an indignant comment about how we should be okay with taking this small hack on as technical debt, but it actually looks like it might be pretty easy to address @TheBlueMatt's concerns by adding a method to CConman (eg AddLocalService(ServiceFlags flag)) that just modifies nLocalServices. The indexing code could call it on sync completion. Any reason not to do that?

This comment has been minimized.

Copy link
@jamesob

jamesob Aug 6, 2019

Member

(FWIW I hate the idea of blocking net threads while waiting for filter indices to sync, and slightly less hate requiring a restart to serve the filters.)

This comment has been minimized.

Copy link
@jimpo

jimpo Aug 7, 2019

Author Contributor

@jamesob Yes, that would work too, but I wouldn't want to index to call CConmann directly. It should do it through some sort of callback to avoid coupling them, which just creates more indirection. I think this approach is a bit easier, but I'd be OK with a callback that updates nLocalServices as well.

This comment has been minimized.

Copy link
@jamesob

jamesob Sep 10, 2019

Member

Just had an offline conversation with @sdaftuar here (with regard to #16847) which I think is relevant. Dynamically flipping nLocalServices bits doesn't make a ton of sense because we'd need to disconnect and reconnect to all of our peers in order for the flip to have any effect.

When thinking about this, it's instructive to look at how IBD works: we don't flip NODE_NETWORK off and on once we've started and finished IBD; instead net_processing just behaves differently during IBD and peers are able to gracefully handle nodes that signal NODE_NETWORK but haven't completed the sync yet and therefore drop getheaders messages.

I think the way forward in this case is to do something analogous: signal NODE_COMPACT_FILTERS from startup when appropriate and ensure that nodes seeking block filters account for the fact that a peer they request filters from may be in the process of building them.

This comment has been minimized.

Copy link
@jimpo

jimpo Oct 19, 2019

Author Contributor

Dynamically flipping nLocalServices bits doesn't make a ton of sense because we'd need to disconnect and reconnect to all of our peers in order for the flip to have any effect.

I disagree with this. New inbound connections will see that the service bit has been flipped. And clients who need the filters wouldn't connect until it is on, so it's not like any inbound connections need to periodically disconnect/reconnect. And if I understand correctly, we re-advertise our local address with service bits to each peer every day on average, so the network should see the change eventually without the node operator having to do anything.

instead net_processing just behaves differently during IBD and peers are able to gracefully handle nodes that signal NODE_NETWORK but haven't completed the sync yet and therefore drop getheaders messages.

This feels a bit different because the version message includes a start height indicating how far behind the node is. I suppose clients could also just request checkpoints and determine how far behind a node's filter index is at least to within 1,000 blocks though.

This comment has been minimized.

Copy link
@luke-jr

luke-jr Nov 14, 2019

Member

In any case, can you simply modify nLocalServices directly when the index has finished syncing? Or does other code that accesses it directly need to see the bit set even before sync is done?

src/net_processing.cpp Show resolved Hide resolved
src/net_processing.cpp Outdated Show resolved Hide resolved
src/net_processing.cpp Outdated Show resolved Hide resolved
src/net_processing.cpp Outdated Show resolved Hide resolved
src/net_processing.cpp Outdated Show resolved Hide resolved
src/net_processing.cpp Show resolved Hide resolved
*/
static bool PrepareBlockFilterRequest(CNode* pfrom, const CChainParams& chain_params,
BlockFilterType filter_type, uint32_t start_height,
uint256& stop_hash, uint32_t max_height_diff,

This comment has been minimized.

Copy link
@TheBlueMatt

TheBlueMatt Jul 29, 2019

Contributor

Why is max_height_diff a parameter? Why not just use the constant directly?

This comment has been minimized.

Copy link
@jimpo

jimpo Jul 30, 2019

Author Contributor

It'll be obvious in subsequent commits.

src/net_processing.cpp Outdated Show resolved Hide resolved
src/net_processing.cpp Outdated Show resolved Hide resolved
@jimpo jimpo force-pushed the jimpo:bip157-net branch from 3d398a8 to 3499e88 Jul 30, 2019
@DrahtBot DrahtBot removed the Needs rebase label Jul 30, 2019
@torkelrogstad torkelrogstad mentioned this pull request Aug 1, 2019
2 of 5 tasks complete
src/net_processing.cpp Outdated Show resolved Hide resolved
src/net_processing.cpp Outdated Show resolved Hide resolved
Copy link
Member

jamesob left a comment

3499e88

Code looks great! Especially the tests. A few small comments, but this is very close to an ACK from me.

Review checklist

  • 7da4f23 net: Signal NODE_COMPACT_FILTERS if the block filter index is on.

  • cfc3095 net: Define new BIP 157 protocol messages.

  • 7bbf08c net: Message handling for GETCFILTERS.

  • abc58d6 net: Message handling for GETCFHEADERS.

    Inconsistent return type (false instead of true).

  • 9da9daa net: Message handling for GETCFCHECKPT.

    Inconsistent return type (false instead of true).

  • bbb909c blockfilter: Fix default (de)?serialization of BlockFilter.

  • ab0ea72 test: Functional tests for cfilters P2P interface.

    Nice tests!

  • 77653ec net: Synchronize cfilter request handling with block filter index.

  • f928bdd test: Exercise cache update code during getcfcheckpt handling.

  • 3499e88 net: Cache compact filter checkpoints in memory.

Tests

Built and run locally for about a day so far with no problems. Synced filters
from somewhere in the mid 500ks.

Confirmed that the service bit flips appropriately: During filter syncing:

$ ./src/bitcoin-cli -datadir=/data/bitcoin getnetworkinfo
  ...
  "localservices": "0000000000000409",

After filter syncing:

$ ./src/bitcoin-cli -datadir=/data/bitcoin getnetworkinfo
  ...
  "localservices": "0000000000000449",

The comprehensive functional test coverage is cause for optimism.

s << m_block_hash
<< static_cast<uint8_t>(m_filter_type)
s << static_cast<uint8_t>(m_filter_type)
<< m_block_hash

This comment has been minimized.

Copy link
@jamesob

jamesob Aug 8, 2019

Member

These changes don't require reindexing because we manually spell out the serialization format when writing to disk (and same with reads) instead of using these serialization ops, right?

This comment has been minimized.

Copy link
@jimpo

jimpo Aug 9, 2019

Author Contributor

Right

src/net_processing.cpp Outdated Show resolved Hide resolved
src/net_processing.cpp Outdated Show resolved Hide resolved
@Sjors

This comment has been minimized.

Copy link
Member

Sjors commented Aug 13, 2019

Concept ACK

Should there be a separate CLI flag other than -blockfilterindex to enable serving of filters from the index?

Yes, suggest defaulting to true. It can wait until #15845.

Is the mechanism to only signal the service flag once the index is in sync OK?

I guess it's already built, but I'd be fine with moving that to a followup (and kick the src/net.cpp discussion can down the road). For fresh nodes the index is synced along with IBD. For existing nodes which add the index there's a slight lag between when this setting is enabled and when it's available. These flags take time to get gossiped around. The odds of another node wasting time because of that seems orders of magnitude smaller than the odds of the node just being offline.

On macOS 10.14.6 when configured with --enable-debug the newly added p2p_cfilters.py ignites my fan and failed the first time with Block sync timed out: (passed the second time).

@luke-jr

This comment has been minimized.

Copy link
Member

luke-jr commented Aug 20, 2019

Should there be a separate CLI flag other than -blockfilterindex to enable serving of filters from the index?

BIP 157 is controversial, so I think yes.

Copy link

marcinja left a comment

Concept ACK. Reviewed each commit and ran tests for each of them. I also did a full IBD and verified that the NODE_COMPACT_FILTERS flag is flipped on.

test/functional/p2p_cfilters.py Outdated Show resolved Hide resolved
test/functional/p2p_cfilters.py Show resolved Hide resolved
src/net_processing.cpp Outdated Show resolved Hide resolved
* is updated asynchronously, has not been synchronized with the net processing thread. In that
* case, block for a short time until the filter index is updated, then retry the lookup.
*/
static bool QueryFilterIndexWithRetry(BaseIndex* index, bool& in_sync, std::function<bool()> fn)

This comment has been minimized.

Copy link
@marcinja

marcinja Aug 27, 2019

Rather than block the message handler thread until the filter index has synced, is there much downside to ignoring messages when a filter query fails because it is not yet synced? (i.e. not sending anything back to the peer as implemented before 77653ec)

By the spec in BIP 157, clients should be downloading from multiple peers so they wouldn't be wasting much bandwidth because of dropped messages caused by this. Blocking the message handler thread just seems to have more downsides IMO.

This comment has been minimized.

Copy link
@jimpo

jimpo Oct 19, 2019

Author Contributor

It just makes the client logic more complicated as they need to have timeouts and if the index is active (like it has gotten in sync once since the node has been running), it should catch up to the tip pretty fast.

@Sjors

This comment has been minimized.

Copy link
Member

Sjors commented Sep 1, 2019

I spun up a temporary testnet node in case anyone wants to test the p2p behavior: uskjxnd7tud7qkeg.onion:18333

computed_cfhash = uint256_from_str(hash256(cfilter.filter_data))
assert_equal(computed_cfhash, cfhash)

# Check thet peers can fetch cfilters for stale blocks.

This comment has been minimized.

Copy link
@practicalswift

practicalswift Sep 7, 2019

Member

Should be "that"? :-)

jamesob added a commit to jamesob/bitcoin that referenced this pull request Sep 10, 2019
Recent questions have come up regarding dynamic service registration
(see bitcoin#16442 (comment)
and the assumeutxo project, which needs to dynamically flip NODE_NETWORK).

While investigating how dynamic service registration might work, I was
confused about how we convey local services to peers. This adds some
documentation that hopefully clarifies this process.
jamesob added a commit to jamesob/bitcoin that referenced this pull request Sep 10, 2019
Recent questions have come up regarding dynamic service registration
(see bitcoin#16442 (comment)
and the assumeutxo project, which needs to dynamically flip NODE_NETWORK).

While investigating how dynamic service registration might work, I was
confused about how we convey local services to peers. This adds some
documentation that hopefully clarifies this process.
jamesob added a commit to jamesob/bitcoin that referenced this pull request Sep 11, 2019
Recent questions have come up regarding dynamic service registration
(see bitcoin#16442 (comment)
and the assumeutxo project, which needs to dynamically flip NODE_NETWORK).

While investigating how dynamic service registration might work, I was
confused about how we convey local services to peers. This adds some
documentation that hopefully clarifies this process.
laanwj added a commit that referenced this pull request Sep 16, 2019
…ertised

82e53f3 doc: add comments clarifying how local services are advertised (James O'Beirne)

Pull request description:

  Recent questions have come up regarding dynamic service registration
  (see #16442 (comment)
  and the assumeutxo project ~~which needs to dynamically flip NODE_NETWORK~~).

  While investigating how dynamic service registration might work, I was
  confused about how we convey local services to peers. This adds some
  documentation that hopefully clarifies this process.

ACKs for top commit:
  laanwj:
    ACK 82e53f3
  darosior:
    ACK 82e53f3

Tree-SHA512: a30c1020387d7a75b5b3cdde45f8b7f2ae46293da97e6227b2ee17e290b93deb5b16c0bbc2b1676972300e5c3c2ad74eb8b3910d6b93e028dac1ae2700468ef9
sidhujag added a commit to syscoin/syscoin that referenced this pull request Sep 16, 2019
…are advertised

82e53f3 doc: add comments clarifying how local services are advertised (James O'Beirne)

Pull request description:

  Recent questions have come up regarding dynamic service registration
  (see bitcoin#16442 (comment)
  and the assumeutxo project ~~which needs to dynamically flip NODE_NETWORK~~).

  While investigating how dynamic service registration might work, I was
  confused about how we convey local services to peers. This adds some
  documentation that hopefully clarifies this process.

ACKs for top commit:
  laanwj:
    ACK 82e53f3
  darosior:
    ACK 82e53f3

Tree-SHA512: a30c1020387d7a75b5b3cdde45f8b7f2ae46293da97e6227b2ee17e290b93deb5b16c0bbc2b1676972300e5c3c2ad74eb8b3910d6b93e028dac1ae2700468ef9
@jamesob

This comment has been minimized.

Copy link
Member

jamesob commented Sep 27, 2019

@jimpo any plans to address feedback here? It'd be great to see this PR move towards merge.

@laanwj laanwj added the Feature label Sep 30, 2019
@jimpo jimpo force-pushed the jimpo:bip157-net branch from 14c0caa to c88df2c Nov 11, 2019
@TheBlueMatt

This comment has been minimized.

Copy link
Contributor

TheBlueMatt commented Nov 14, 2019

@Sjors

This comment has been minimized.

Copy link
Member

Sjors commented Nov 14, 2019

Should net_processing maintain a queue of filter messages (20 kb each) we want to send, and then drip them out 10(?) at a time each time SendMessages is called? Or not even the messages themselves, but instead construct on the fly?

@TheBlueMatt

This comment has been minimized.

Copy link
Contributor

TheBlueMatt commented Nov 15, 2019

See ProcessGetData - we'll want to do that, yea, keep a queue of what was requested and, just like ProcessGetData, hit that every time we get to ProcessMessage for the peer or SendMessages for the peer, and do nothing else until afterwards. In fact, why is there a new message for fetching these things anyway? Is there any reason to not literally just do it via invs? Would be a smaller diff and pretty in-line with the point of getdata anyway.

HashUnlimited pushed a commit to HashUnlimited/chaincoin that referenced this pull request Nov 17, 2019
Recent questions have come up regarding dynamic service registration
(see bitcoin#16442 (comment)
and the assumeutxo project, which needs to dynamically flip NODE_NETWORK).

While investigating how dynamic service registration might work, I was
confused about how we convey local services to peers. This adds some
documentation that hopefully clarifies this process.
@jnewbery

This comment has been minimized.

Copy link
Member

jnewbery commented Nov 18, 2019

Concept ACK. This looks really good. The commits are nicely laid out and the change set is very easy to follow. I haven't yet done a in-depth review, but I've skimmed through the code and have some high-level feedback:

  • some of the later commits could be squashed into earlier commits to avoid code churn (eg net: Raise MAX_GETCFILTERS_SIZE following change to BIP 158. into net: Message handling for GETCFILTERS. and init: Separate CLI flag for block filter index and serving cfilters. into net: Define new BIP 157 protocol messages.)
  • I don't like the code in net.cpp that accesses the indexer to set the service flags. As well as introducing an unpleasant coupling between net and the indexer, it seems to go against the common understanding of what service flags represent (capabilities rather than current node state)
  • Can the compact filter checkpoints be cached in the indexer rather than net_processing? It seems strange to have net_processing burdened with storing index data and its UpdatedBlockTip() callback being used to update that cache. The index is already a validationinterface client. Can't it handle updating and serving data from its own cache?
break;
}

// Filter header requested for stale block.

This comment has been minimized.

Copy link
@pinheadmz

pinheadmz Nov 19, 2019

Contributor

What do you mean by this comment? Looking at the tests, it seems like we should be able to retrieve filters, headers and checkpoint headers on stale chains.

This comment has been minimized.

Copy link
@jkczyz

jkczyz Nov 20, 2019

Contributor

The preceding condition is checking if the cfheader (i.e. filter header) is cached and thus for a block on the active chain. It breaks early and populates the remaining results from the cache. The comment here is for stale blocks where an index lookup is required since the filter header won't be in the cache. Once the active chain is reached (on a later iteration) the loop breaks and the cache is used.

Copy link
Contributor

jkczyz left a comment

Concept ACK. Nicely done! Left some comments but haven't made it through the tests yet.

* cfheaders is a response to a getcfheaders request containing a vector of
* filter headers for each block in the requested range.
Comment on lines +252 to +253

This comment has been minimized.

Copy link
@jkczyz

jkczyz Nov 20, 2019

Contributor

Should be the previous filter header and a vector of filter hashes.

bool supported_filter_type =
(filter_type == BlockFilterType::BASIC &&
(pfrom->GetLocalServices() & NODE_COMPACT_FILTERS));
if (!supported_filter_type) {
Comment on lines +1897 to +1900

This comment has been minimized.

Copy link
@jkczyz

jkczyz Nov 20, 2019

Contributor

Would this logic be more suitable encapsulated as a method of CNode?

This comment has been minimized.

Copy link
@jimpo

jimpo Nov 20, 2019

Author Contributor

I don't see any places where it's duplicated, so I don't feel the need to move it.

This comment has been minimized.

Copy link
@jkczyz

jkczyz Nov 21, 2019

Contributor

Duplication is one reason to refactor code, but there are others such as improving readability by hiding complexity. Compare:

if (!pfrom->SupportsBlockFilter(BlockFilterType::BASIC)) {
    // ...
}
return error("Failed to find block filter header in index: filter_type=%s, block_hash=%s",
BlockFilterTypeName(filter_index.GetFilterType()),
block_index->GetBlockHash().ToString());
Comment on lines +1253 to +1255

This comment has been minimized.

Copy link
@jkczyz

jkczyz Nov 20, 2019

Contributor

Looks like you are swallowing the return value below. Can this function be void instead? Or is this a common pattern when you want to log and return early?

This comment has been minimized.

Copy link
@jimpo

jimpo Nov 20, 2019

Author Contributor

Yes, the return value of this function is ignored, but returning a success indicator is a pretty common pattern and I don't see any downside.

break;
}

// Filter header requested for stale block.

This comment has been minimized.

Copy link
@jkczyz

jkczyz Nov 20, 2019

Contributor

The preceding condition is checking if the cfheader (i.e. filter header) is cached and thus for a block on the active chain. It breaks early and populates the remaining results from the cache. The comment here is for stale blocks where an index lookup is required since the filter header won't be in the cache. Once the active chain is reached (on a later iteration) the loop breaks and the cache is used.

@@ -1998,17 +2022,27 @@ static bool ProcessGetCFHeaders(CNode* pfrom, CDataStream& vRecv, const CChainPa
return true;
}

bool index_in_sync = false;

This comment has been minimized.

Copy link
@jkczyz

jkczyz Nov 20, 2019

Contributor

Both the interface and implementation of QueryFilterIndexWithRetry are quite complex and a bit difficult to reason about, especially when the output parameter in_sync is not used by the caller.

Would it be detrimental to first unconditionally call index->BlockUntilSyncedToCurrentChain() at each call site instead of only calling it before retrying? It would make the code more readable by eliminating the retry function and all the complexities around it (e.g. output parameter dependency).

This comment has been minimized.

Copy link
@jimpo

jimpo Nov 20, 2019

Author Contributor

Yes, I agree that QueryFilterIndexWithRetry is quite ugly, but I really don't like the idea of blocking the net thread on background processing unless necessary. In fact, I don't like the idea of doing it at all, but there's not really infrastructure for doing proper async logic here.

This comment has been minimized.

Copy link
@jkczyz

jkczyz Nov 21, 2019

Contributor

That seems like an orthogonal concern to my point. From what I can tell, BlockUntilSyncedToCurrentChain won't block needlessly:

bitcoin/src/index/base.h

Lines 95 to 100 in 46d6930

/// Blocks the current thread until the index is caught up to the current
/// state of the block chain. This only blocks if the index has gotten in
/// sync once and only needs to process blocks in the ValidationInterface
/// queue. If the index is catching up from far behind, this method does
/// not block and immediately returns false.
bool BlockUntilSyncedToCurrentChain();

That is, it is short-circuited if the index is current:

bitcoin/src/index/base.cpp

Lines 282 to 289 in b2a6b02

// Skip the queue-draining stuff if we know we're caught up with
// ::ChainActive().Tip().
LOCK(cs_main);
const CBlockIndex* chain_tip = ::ChainActive().Tip();
const CBlockIndex* best_block_index = m_best_block_index.load();
if (best_block_index->GetAncestor(chain_tip->nHeight) == chain_tip) {
return true;
}

So in best case you would just verify that the index is caught up before querying it. And in worse case, you would block until caught up just as you're already doing in QueryFilterIndexWithRetry.

Would also obliviate the need to address some of @Talkless's nits. ;)

@jimpo

This comment has been minimized.

Copy link
Contributor Author

jimpo commented Nov 20, 2019

@jnewbery @jkczyz Thanks for the reviews!

On point 2, I've heard the feedback that dynamically switching the service flags is a bad idea a few times. I also don't like the idea of leaving the service flag on before the index is in sync as it complicates client logic. Maybe it's OK to just not allow the -peercfilters flag until the index is in sync. So the user experience is that a fresh node can turn on all the flags, but a node that is in sync without the index will first need to run with -blockfilterindex=basic until in sync, then the node operator restarts with the -peercfilters flag.

On point 3, my reasoning is that the concept of checkpoints, including the checkpoint interval parameter, is specific to the net layer. But I don't think there's too much harm in moving that to the BlockFilterIndex and probably is a bit cleaner, so I'd be happy to make that change.

@jimpo

This comment has been minimized.

Copy link
Contributor Author

jimpo commented Nov 20, 2019

@TheBlueMatt

See ProcessGetData - we'll want to do that, yea, keep a queue of what was requested and, just like ProcessGetData, hit that every time we get to ProcessMessage for the peer or SendMessages for the peer, and do nothing else until afterwards. In fact, why is there a new message for fetching these things anyway? Is there any reason to not literally just do it via invs? Would be a smaller diff and pretty in-line with the point of getdata anyway.

The rationale is that filters are much smaller than blocks so it's not worthwhile to send a getdata for each individual filter. And unlike transactions, which may be of a similar size if not smaller, block filters can be queried more efficiently than individually by specifying a range of blocks.

A pretty hacky idea might be to getdata with MSG_BLOCK_FILTER where the inv is the first 30 bytes of the block hash and the last 2 bytes (which should be 0 due to min difficulty) are replaced by the number of preceding block filters, allowing one to effectively encode a range of blocks into an inv message.

Copy link
Contributor

fjahr left a comment

Concept ACK

Will try to do proper testing in the next days.

I am not sure why there is a ? in the commit message of f927847 and I agree with @jnewbery that some of the later commits could be squashed.

I don't know if switching the service flag while running is that bad but it seems to me like a good idea to go with a simpler approach first, even if users have to do a little more manual work. A better solution would be great as a follow-up.

LogPrintf("WARNING: NODE_COMPACT_FILTERS is signaled, but filter index is not available\n");
}
if (!basic_filter_index || !basic_filter_index->IsSynced()) {
// If block filter index is still syncing, do not advertise the service bit.

This comment has been minimized.

Copy link
@fjahr

fjahr Nov 20, 2019

Contributor

nit: could print a message to logs in this case too IMO.

@benthecarman

This comment has been minimized.

Copy link
Contributor

benthecarman commented Nov 20, 2019

NODE_COMPACT_FILTERS should be added to GetServicesNames

Copy link

Talkless left a comment

Sorry if nit's being annoying, but these const's does help reviewing, when you can "drop" that variable from the "brain cache" immediately :) .


/// Returns whether index has completed the initial sync with the active chain.
/// After returning true once, this function will return true on all subsequent calls.
bool IsSynced() const { return m_synced; }

This comment has been minimized.

Copy link
@Talkless

Talkless Nov 21, 2019

nit: trivial, could be noexcept

@@ -946,6 +947,18 @@ bool AppInitParameterInteraction()
}
}

// Signal NODE_COMPACT_FILTERS if peercfilters and required index are both enabled.
if (gArgs.GetBoolArg("-peercfilters", DEFAULT_PEERCFILTERS)) {
bool index_enabled = std::find(g_enabled_filter_types.begin(),

This comment has been minimized.

Copy link
@Talkless

Talkless Nov 21, 2019

nit: could be const bool index_enabled

static bool UpdateCFHeadersCache(const BlockFilterIndex& filter_index)
{
LOCK(cs_main);
std::lock_guard<std::mutex> _lock(active_chain_cf_headers_mtx);

This comment has been minimized.

Copy link
@Talkless

Talkless Nov 21, 2019

Kinda off-topic I guess, bug locking two mutex'es makes you think, maybe bitcoin codebase would need something like std::lock, or better std::scoped_lock (C++17) to lock multiple locks in deadlock-free way, with same LOCK() debugging features.

const CChain& active_chain = ::ChainActive();

// Drop any entries in the checkpoint cache that have been reorganized off the active chain.
int new_size = active_chain_cf_headers.size();

This comment has been minimized.

Copy link
@Talkless

Talkless Nov 21, 2019

Suggestion: consider simplifying this bit to remove these manual "off-by-one'ness-ness" like >, -1, etc, using reverse iterators and find_if, with something like this:

const auto it = std::find_if(active_chain_cf_headers.crbegin(), active_chain_cf_headers.crend(), [&](const std::pair<const CBlockIndex*, uint256> &i) {
    return active_chain.Contains(i.first);
});
active_chain_cf_headers.erase(it.base(), active_chain_cf_headers.cend());

later (new_size + 1) obviously can be changed into active_chain_cf_headers.size().

This method be toyed-with using this trivial example:

#include <algorithm>
#include <iostream>
#include <iterator>
#include <vector>

int main()
{
    std::vector<int> vals{1, 1, 0, 0, 0};

    const auto it = std::find_if(vals.crbegin(), vals.crend(), [](int v) -> bool {
        return v > 0;
    });

    vals.erase(it.base(), vals.cend());

    std::copy(vals.cbegin(),
              vals.cend(),
              std::ostream_iterator<int>(std::cout, " "));
}
for (uint32_t height = (new_size + 1) * CFCHECKPT_INTERVAL;
height <= static_cast<uint32_t>(active_chain.Height());
height += CFCHECKPT_INTERVAL) {
const CBlockIndex* block_index = active_chain[height];

This comment has been minimized.

Copy link
@Talkless

Talkless Nov 21, 2019

nit: could be const CBlockIndex* const block_index


uint256 prev_header;
if (start_height > 0) {
const CBlockIndex* prev_block = stop_index->GetAncestor(start_height - 1);

This comment has been minimized.

Copy link
@Talkless

Talkless Nov 21, 2019

nit: could be const CBlockIndex* const prev_block

Also, IDE notes about start_height - 1 being implicitly converted to int (GetAncestor() takes int), maybe worth "documenting"/"warning" by using static_cast? Though maybe overkill, idk.

uint256 prev_header;
if (start_height > 0) {
const CBlockIndex* prev_block = stop_index->GetAncestor(start_height - 1);
bool lookup_success = QueryFilterIndexWithRetry(

This comment has been minimized.

Copy link
@Talkless

Talkless Nov 21, 2019

nit: could be const bool lookup_success

}

std::vector<uint256> filter_hashes;
bool lookup_success = QueryFilterIndexWithRetry(

This comment has been minimized.

Copy link
@Talkless

Talkless Nov 21, 2019

nit: ditto, could be const bool lookup_success


vRecv >> filter_type_ser >> stop_hash;

BlockFilterType filter_type = static_cast<BlockFilterType>(filter_type_ser);

This comment has been minimized.

Copy link
@Talkless

Talkless Nov 21, 2019

nit: could be const BlockFilterType filter_type

}

// Filter header requested for stale block.
bool lookup_success = QueryFilterIndexWithRetry(

This comment has been minimized.

Copy link
@Talkless

Talkless Nov 21, 2019

nit: could be const bool lookup_success

@Talkless

This comment has been minimized.

Copy link

Talkless commented Dec 4, 2019

my getnetworkinfo shows:

"localservicesnames": [
    "NETWORK",
    "WITNESS",
    "NETWORK_LIMITED"
  ],

Why I don't see NODE_COMPACT_FILTERS? lnd is working fine in my regtest setup, using bitcoin.node=neutrino, so it seems working (received mined coins into lnd wallet), just curious about that service flag.

I've launched:

 ./bitcoin-qt -chain=regtest

while my bitcoin.conf is:

[regtest]
blockfilterindex=basic
peercfilters=1
server=1
listen=1
bind=0.0.0.0
@marcinja

This comment has been minimized.

Copy link

marcinja commented Dec 4, 2019

Why I don't see NODE_COMPACT_FILTERS?

@Talkless I believe that is because GetServicesNames hasn't been updated yet. See: #16442 (comment)

@Talkless

This comment has been minimized.

Copy link

Talkless commented Dec 5, 2019

Uhg, I've missed that part. Thanks @marcinja .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.