jimpo commented at 4:49 pm on July 23, 2019: contributor

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 force-pushed on Jul 23, 2019

fanquake added the label P2P on Jul 23, 2019

DrahtBot commented at 6:29 pm on July 23, 2019: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#18638 (net: Use mockable time for ping/pong, add tests by MarcoFalke)
#18450 (util: Use locale independent ToString(…) instead of locale dependent strprintf(…) for low-level string formatting by practicalswift)
#18242 (Add BIP324 encrypted p2p transport de-/serializer (only used in tests) by jonasschnelli)
#18165 (Consolidate service flag bit-to-name conversion to a shared serviceFlagToStr function by luke-jr)
#18000 (Index for UTXO Set Statistics by fjahr)
#17479 (Return BlockValidationState from ProcessNewBlock if CheckBlock/AcceptBlock fails by jnewbery)

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 commented at 6:41 pm on July 23, 2019: contributor

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 force-pushed on Jul 24, 2019

jamesob commented at 1:58 pm on July 24, 2019: member

Concept ACK - will review within the next few days.

DrahtBot added the label Needs rebase on Jul 24, 2019

in src/net.cpp:2651 in c6f73db3ab outdated

2598@@ -2597,7 +2599,18 @@ uint64_t CConnman::GetTotalBytesSent()
2599 
2600 ServiceFlags CConnman::GetLocalServices() const
2601 {
2602-    return nLocalServices;
2603+    uint64_t local_services = nLocalServices;
2604+    if (local_services & NODE_COMPACT_FILTERS) {

TheBlueMatt commented at 8:10 pm on July 29, 2019:

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.

jimpo commented at 12:53 pm on July 30, 2019:

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?

jamesob commented at 8:36 pm on August 6, 2019:

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?

jamesob commented at 8:37 pm on August 6, 2019:

(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.)

jimpo commented at 9:33 am on August 7, 2019:

@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.

jamesob commented at 6:29 pm on September 10, 2019:

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.

jimpo commented at 10:11 am on October 19, 2019:

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.

luke-jr commented at 1:47 am on November 14, 2019:

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?

jimpo commented at 8:53 pm on March 3, 2020:

I have removed the dynamic flipping in accordance with reviewers’ suggestions.

in src/net_processing.cpp:2018 in ac3a72a828 outdated

1917+    uint32_t start_height;
1918+    uint256 stop_hash;
1919+
1920+    vRecv >> filter_type_ser >> start_height >> stop_hash;
1921+
1922+    BlockFilterType filter_type = static_cast<BlockFilterType>(filter_type_ser);

TheBlueMatt commented at 8:37 pm on July 29, 2019:

Is this defined behavior?

jimpo commented at 12:54 pm on July 30, 2019:

Yes, BlockFilterType is enum class BlockFilterType : uint8_t.

TheBlueMatt commented at 8:13 pm on July 30, 2019:

Hmmm, actually I don’t think so? SO quotes:

C++ standard 5.2.9 Static cast [expr.static.cast] paragraph 7

A value of integral or enumeration type can be explicitly converted to an enumeration type. The value is unchanged if the original value is within the range of the enumeration values (7.2). Otherwise, the resulting enumeration value is unspecified / undefined (since C++17).

sipa commented at 8:20 pm on July 30, 2019:

Converting a uint8_t to a uint8_t-based enum should always result in something in range?

jimpo commented at 8:26 pm on July 30, 2019:

“For an enumeration whose underlying type is fixed, the values of the enumeration are the values of the underlying type.”

https://stackoverflow.com/questions/18195312/what-happens-if-you-static-cast-invalid-value-to-enum-class

in src/net_processing.cpp:1881 in ac3a72a828 outdated

1876+        LOCK(cs_main);
1877+        stop_index = LookupBlockIndex(stop_hash);
1878+
1879+        // Check that the stop block exists and was at some point connected to the active chain.
1880+        if (!stop_index ||
1881+            !stop_index->IsValid(BLOCK_VALID_SCRIPTS) ||

TheBlueMatt commented at 8:42 pm on July 29, 2019:

This is redundant with BlockRequestAllowed.

in src/net_processing.cpp:1890 in ac3a72a828 outdated

1885+            pfrom->fDisconnect = true;
1886+            return false;
1887+        }
1888+    }
1889+
1890+     uint32_t stop_height = stop_index->nHeight;

TheBlueMatt commented at 8:43 pm on July 29, 2019:

nit: bad indentation here.

in src/net_processing.cpp:1910 in ac3a72a828 outdated

1905+    filter_index = GetBlockFilterIndex(filter_type);
1906+    if (!filter_index) {
1907+        return error("Filter index for supported type %s not found", BlockFilterTypeName(filter_type));
1908+    }
1909+
1910+     return true;

TheBlueMatt commented at 8:43 pm on July 29, 2019:

nit: bad indentation here.

in src/net_processing.cpp:1882 in ac3a72a828 outdated

1877+        stop_index = LookupBlockIndex(stop_hash);
1878+
1879+        // Check that the stop block exists and was at some point connected to the active chain.
1880+        if (!stop_index ||
1881+            !stop_index->IsValid(BLOCK_VALID_SCRIPTS) ||
1882+            !BlockRequestAllowed(stop_index, chain_params.GetConsensus())) {

TheBlueMatt commented at 8:46 pm on July 29, 2019:

I don’t actually think this is sufficient. The stop block may be BlockRequestAllowed while the previous block(s) may not. I think it is sufficient to check BlockRequestAllowed both on the stop block and the start block, but you should also update the comment there to make that an invariant in the future.

jimpo commented at 12:47 pm on July 30, 2019:

Good point!

jimpo commented at 8:12 pm on July 30, 2019:

Upon further reflection, this seems to not be an issue. Discussed offline with @TheBlueMatt.

in src/net_processing.cpp:2066 in ac3a72a828 outdated

1902+        return false;
1903+    }
1904+
1905+    filter_index = GetBlockFilterIndex(filter_type);
1906+    if (!filter_index) {
1907+        return error("Filter index for supported type %s not found", BlockFilterTypeName(filter_type));

TheBlueMatt commented at 8:48 pm on July 29, 2019:

Because you haven’t checked filter_type is sane yet, this allows DoS by filling someone’s debug.log with garbage by repeatedly requesting undefined filter types.

jimpo commented at 12:49 pm on July 30, 2019:

Filter type is checked at the top of this function?

in src/net_processing.cpp:2022 in ac3a72a828 outdated

1856+ * request is valid and can be serviced, this returns the stop block index and the filter index
1857+ * as out parameters. May disconnect from the peer in the case of a bad request.
1858+ */
1859+static bool PrepareBlockFilterRequest(CNode* pfrom, const CChainParams& chain_params,
1860+                                      BlockFilterType filter_type, uint32_t start_height,
1861+                                      uint256& stop_hash, uint32_t max_height_diff,

TheBlueMatt commented at 8:49 pm on July 29, 2019:

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

jimpo commented at 12:47 pm on July 30, 2019:

It’ll be obvious in subsequent commits.

Empact commented at 8:13 pm on February 3, 2020:

nit: const stop_hash would make the role the arg plays more clear - absent that pass by reference could be misleading

in src/net_processing.cpp:1928 in ac3a72a828 outdated

1923+
1924+    const CBlockIndex* stop_index;
1925+    BlockFilterIndex* filter_index;
1926+    if (!PrepareBlockFilterRequest(pfrom, chain_params, filter_type, start_height, stop_hash,
1927+                                   MAX_GETCFILTERS_SIZE, stop_index, filter_index)) {
1928+        return false;

TheBlueMatt commented at 9:13 pm on July 29, 2019:

I actually don’t think we want to return false here and a few other places. All it does is change logging but no need to print twice when you just printed the error.

jimpo commented at 8:28 pm on July 30, 2019:

Makes sense. Changed to return true.

in src/net_processing.cpp:1939 in ac3a72a828 outdated

1934+        return error("Failed to find block filter in index: filter_type=%s, start_height=%d, stop_hash=%s",
1935+                     BlockFilterTypeName(filter_type), start_height, stop_hash.ToString());
1936+    }
1937+
1938+    for (const auto& filter : filters) {
1939+        CSerializedNetMsg msg = CNetMsgMaker(INIT_PROTO_VERSION)

TheBlueMatt commented at 9:15 pm on July 29, 2019:

This should use the peer protocol version, not that I think it matters.

TheBlueMatt commented at 9:16 pm on July 29, 2019: member

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

jimpo force-pushed on Jul 30, 2019

DrahtBot removed the label Needs rebase on Jul 30, 2019

in src/net_processing.cpp:2009 in 9da9daad60 outdated

2004+    const CBlockIndex* stop_index;
2005+    BlockFilterIndex* filter_index;
2006+    if (!PrepareBlockFilterRequest(pfrom, chain_params, filter_type, /*start_height=*/0, stop_hash,
2007+                                   /*max_height_diff=*/std::numeric_limits<uint32_t>::max(),
2008+                                   stop_index, filter_index)) {
2009+        return false;

jamesob commented at 7:46 pm on August 7, 2019:

https://github.com/bitcoin/bitcoin/pull/16442/commits/9da9daad6053934009448344bae77f4e18f36869

Why isn’t this true as in ProcessGetCFilters()? (Same question for ProcessGetCFHeaders().)

marcinja commented at 1:38 am on August 27, 2019:

Looks like @jimpo may have just forgotten to update the corresponding return statements when updating the PR earlier. See: #16442 (review) “All it does is change logging but no need to print twice when you just printed the error.”

It should be consistent though.

in src/net_processing.cpp:410 in 3499e8877b outdated

404@@ -403,6 +405,10 @@ limitedmap<uint256, int64_t> g_already_asked_for GUARDED_BY(cs_main)(MAX_INV_SZ)
405 /** Map maintaining per-node state. */
406 static std::map<NodeId, CNodeState> mapNodeState GUARDED_BY(cs_main);
407 
408+/** In-memory cache of all BIP157 compact filter checkpoints for the active chain. */
409+static std::vector<std::pair<const CBlockIndex*, uint256>> active_chain_cf_headers;
410+static boost::shared_mutex active_chain_cf_headers_mtx;

jamesob commented at 8:01 pm on August 7, 2019:

https://github.com/bitcoin/bitcoin/pull/16442/commits/3499e8877b3be1a629647353ddb8bb380c10a513

Might want to make a note here about why we’re adding more boost usage instead of using the utilities in sync (ie std::mutex). I assume this is because we want to allow multiple threads to be able to read the headers cache simultaneously?

jimpo commented at 11:52 am on August 8, 2019:

Yeah, the rationale is that std doesn’t get shared_mutex until C++17. I’ll add a comment.

in src/blockfilter.h:148 in bbb909cf12 outdated

142@@ -143,8 +143,8 @@ class BlockFilter
143 
144     template <typename Stream>
145     void Serialize(Stream& s) const {
146-        s << m_block_hash
147-          << static_cast<uint8_t>(m_filter_type)
148+        s << static_cast<uint8_t>(m_filter_type)
149+          << m_block_hash

jamesob commented at 2:48 pm on August 8, 2019:

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?

jimpo commented at 12:59 pm on August 9, 2019:

Right

in src/net_processing.cpp:1215 in 3499e8877b outdated

1206@@ -1201,6 +1207,40 @@ void PeerLogicValidation::NewPoWValidBlock(const CBlockIndex *pindex, const std:
1207     });
1208 }
1209 
1210+static bool UpdateCFHeadersCache(const BlockFilterIndex& filter_index)
1211+{
1212+    LOCK(cs_main);
1213+    boost::unique_lock<boost::shared_mutex> _lock(active_chain_cf_headers_mtx);
1214+
1215+    CChain& active_chain = ::ChainActive();

jamesob commented at 6:29 pm on August 8, 2019:

https://github.com/bitcoin/bitcoin/pull/16442/commits/3499e8877b3be1a629647353ddb8bb380c10a513

FWIW could make this const.

in src/net_processing.cpp:2092 in 3499e8877b outdated

2086@@ -2043,23 +2087,37 @@ static bool ProcessGetCFCheckPt(CNode* pfrom, CDataStream& vRecv, const CChainPa
2087         return false;
2088     }
2089 
2090-    bool index_in_sync = false;
2091     std::vector<uint256> headers(stop_index->nHeight / CFCHECKPT_INTERVAL);
2092+    {
2093+        boost::shared_lock<boost::shared_mutex> _lock(active_chain_cf_headers_mtx);

jamesob commented at 6:38 pm on August 8, 2019:

To be clear: at the moment, the shared_lock is more or less moot because this is only ever called into from a single thread (msghand). I’m not necessarily opposed to using the shared_lock before necessary, but might be worth falling back to our standard RecursiveMutex (which comes with lock diagnostics) until and if we actually hit this from multiple threads.

jimpo commented at 1:06 pm on August 9, 2019:

Oh, for some reason I thought there were multiple message handler threads. Since there’s just one and this doesn’t require recursive locking, I think going to a std::mutex makes sense. Agree?

jamesob commented at 1:45 pm on August 9, 2019:

@jimpo yep, that’d be my inclination.

jamesob commented at 6:46 pm on August 8, 2019: member

https://github.com/bitcoin/bitcoin/pull/16442/commits/3499e8877b3be1a629647353ddb8bb380c10a513

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

Review checklist

7da4f239e4 net: Signal NODE_COMPACT_FILTERS if the block filter index is on.
cfc30957b2 net: Define new BIP 157 protocol messages.
7bbf08cbba net: Message handling for GETCFILTERS.
abc58d6984 net: Message handling for GETCFHEADERS.

Inconsistent return type (false instead of true).
9da9daad60 net: Message handling for GETCFCHECKPT.

Inconsistent return type (false instead of true).
bbb909cf12 blockfilter: Fix default (de)?serialization of BlockFilter.
ab0ea72734 test: Functional tests for cfilters P2P interface.

Nice tests!
77653ec568 net: Synchronize cfilter request handling with block filter index.
f928bdd05f test: Exercise cache update code during getcfcheckpt handling.
3499e8877b 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:

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

After filter syncing:

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

The comprehensive functional test coverage is cause for optimism.

Sjors commented at 4:01 pm on August 13, 2019: member

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 commented at 0:38 am on August 20, 2019: member

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.

in test/functional/p2p_cfilters.py:55 in 3499e8877b outdated

50+    def run_test(self):
51+        # Node 0 supports COMPACT_FILTERS, node 1 does not.
52+        node0 = self.nodes[0].add_p2p_connection(CFiltersClient())
53+        node1 = self.nodes[1].add_p2p_connection(CFiltersClient())
54+
55+        # Nodes 0 & 1 share the same first 999 nodes in the chain.

marcinja commented at 1:06 am on August 27, 2019:

nit:

0        # Nodes 0 & 1 share the same first 999 blocks in the chain.

in test/functional/p2p_cfilters.py:75 in 3499e8877b outdated

67+        wait_until(lambda: self.nodes[1].getblockcount() == 2000)
68+
69+        # Fetch cfcheckpt on node 0. Since the implementation caches the checkpoints on the active
70+        # chain in memory, this checks that the cache is updated correctly upon subsequent queries
71+        # after the reorg.
72+        request = msg_getcfcheckpt(

marcinja commented at 1:27 am on August 27, 2019:

I was a bit confused reviewing ab0ea72734ee0bcf7fdf8201d611941ea86ed15f here before seeing the synchronization code added later. If you decide to remove synchronization between the net thread and the filter-index thread, you can use the syncwithvalidationinterface RPC here and after reorging node0 to keep the tests working consistently.

Otherwise (without 77653ec568628291f69ea1b5a31bee35a6492a4a) I believe it is possible that the block filter index might not be synced at this point and the request would fail.

in src/net_processing.cpp:1995 in 3499e8877b outdated

1959+/**
1960+ * Do a lookup on the block filter index. The lookup may fail erroneously if the filter index, which
1961+ * is updated asynchronously, has not been synchronized with the net processing thread. In that
1962+ * case, block for a short time until the filter index is updated, then retry the lookup.
1963+ */
1964+static bool QueryFilterIndexWithRetry(BaseIndex* index, bool& in_sync, std::function<bool()> fn)

marcinja commented at 2:11 am on August 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 77653ec568628291f69ea1b5a31bee35a6492a4a)

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.

jimpo commented at 10:18 am on October 19, 2019:

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.

marcinja commented at 2:20 am on August 27, 2019: contributor

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.

Sjors commented at 11:13 am on September 1, 2019: member

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

in test/functional/p2p_cfilters.py:186 in 3499e8877b outdated

181+            assert_equal(cfilter.filter_type, FILTER_TYPE_BASIC)
182+            assert_equal(cfilter.block_hash, int(block_hash, 16))
183+            computed_cfhash = uint256_from_str(hash256(cfilter.filter_data))
184+            assert_equal(computed_cfhash, cfhash)
185+
186+        # Check thet peers can fetch cfilters for stale blocks.

practicalswift commented at 9:32 am on September 7, 2019:

Should be “that”? :-)

jamesob referenced this in commit 5bee2ae6b2 on Sep 10, 2019

jamesob referenced this in commit 1d08108437 on Sep 10, 2019

jamesob referenced this in commit 82e53f37e1 on Sep 11, 2019

laanwj referenced this in commit eb812257a3 on Sep 16, 2019

sidhujag referenced this in commit 6208e34cd0 on Sep 16, 2019

jamesob commented at 3:33 pm on September 27, 2019: member

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

laanwj added the label Feature on Sep 30, 2019

DrahtBot added the label Needs rebase on Oct 9, 2019

PierreRochard referenced this in commit 2abefd3323 on Oct 12, 2019

jimpo force-pushed on Oct 19, 2019

jimpo commented at 10:32 am on October 19, 2019: contributor

@jamesob Sorry for the delay, just pushed up changes.

DrahtBot removed the label Needs rebase on Oct 19, 2019

Sjors commented at 9:52 am on October 23, 2019: member

I lightly tested 8d08df4 on macOS 10.15 using -bind=127.0.0.1 -debug=net -blockfilterindex=1 -peercfilters=1 -blocksonly against a locally running Lnd node (master @ 8ed75834480e7be3f42d77f5d96e2880c94156fb) with bitcoin.node=neutrino and neutrino.connect=127.0.0.1. It initially catches up to the tip nicely.

I then connect to the using Zap desktop, at which point Lnd starts complaining (maybe an Lnd / Zap bug):

02019-10-23 11:18:35.411 [INF] BTCN: New valid peer 127.0.0.1:8333 (outbound) (/Satoshi:0.19.99/)
12019-10-23 11:18:35.412 [INF] BTCN: Syncing to block height 600677 from peer 127.0.0.1:8333
22019-10-23 11:18:35.412 [INF] BTCN: Fetching set of headers from tip (height=600677) from peer 127.0.0.1:8333
32019-10-23 11:19:00.399 [ERR] BTCN: Query failed with 0 out of 435 filters received
42019-10-23 11:19:00.399 [ERR] LNWL: Neutrino rescan ended with error: unable to fetch cfilter

It seems that bitcoind is disconnecting the Lnd node for requesting too many filters:

 02019-10-23T09:18:30Z received: getcfilters (37 bytes) peer=13
 12019-10-23T09:18:30Z peer 13 requested too many cfilters/cfheaders: 435 / 100
 22019-10-23T09:18:30Z disconnecting peer=13
 3....
 42019-10-23T09:18:35Z Added connection peer=14
 52019-10-23T09:18:35Z connection from 127.0.0.1:54906 accepted
 62019-10-23T09:18:35Z received: version (121 bytes) peer=14
 72019-10-23T09:18:35Z sending version (103 bytes) peer=14
 82019-10-23T09:18:35Z send version message: version 70015, blocks=600677, us=[::]:0, peer=14
 92019-10-23T09:18:35Z sending verack (0 bytes) peer=14
102019-10-23T09:18:35Z receive version message: /btcwire:0.5.0/neutrino:0.0.4-beta/: version 70013, blocks=600677, us=127.0.0.1:8333, peer=14
112019-10-23T09:18:35Z received: verack (0 bytes) peer=14
122019-10-23T09:18:35Z sending sendheaders (0 bytes) peer=14
132019-10-23T09:18:35Z sending ping (8 bytes) peer=14
142019-10-23T09:18:35Z sending addr (31 bytes) peer=14
152019-10-23T09:18:35Z sending feefilter (8 bytes) peer=14
162019-10-23T09:18:35Z received: getaddr (0 bytes) peer=14
172019-10-23T09:18:35Z received: getheaders (997 bytes) peer=14
182019-10-23T09:18:35Z getheaders -1 to end from peer=14
192019-10-23T09:18:35Z sending headers (1 bytes) peer=14
202019-10-23T09:18:35Z received: pong (8 bytes) peer=14
212019-10-23T09:18:46Z sending addr (30003 bytes) peer=14

As a result it’s not detecting the confirmed coins I sent to the Lnd / Zap wallet. It seems unable to perform the required rescan:

0lncli newaddress p2wkh
1[lncli] rpc error: code = Unknown desc = Rescan is already done and cannot be updated. It returned error: unable to fetch cfilter`

There’s different maxima for getcfilters from getcfheaders: https://github.com/bitcoin/bips/blob/master/bip-0157.mediawiki#getcfilters

getcfilters:

The height of the block with hash StopHash MUST be greater than or equal to StartHeight, and the difference MUST be strictly less than 100.

getcfheaders:

The height of the block with hash StopHash MUST be greater than or equal to StartHeight, and the difference MUST be strictly less than 2,000.

Btcd uses the wrong value for getcfilters: https://github.com/btcsuite/btcd/blob/master/wire/msggetcfilters.go#L15

PR: https://github.com/btcsuite/btcd/pull/1482 (makes the error go away, and now it finds the UTXO)

Sjors commented at 11:38 am on October 23, 2019: member

Should there be a limit to how much filter data we give a peer? My Lnd is going berserk (https://github.com/lightningnetwork/lnd/issues/3630), and bitcoind happily sent it 15 GB of filters and counting:

 0{
 1    "id": 1,
 2    "addr": "127.0.0.1:56456",
 3    ...
 4    "servicesnames": [
 5      "WITNESS"
 6    ],
 7    "relaytxes": false,
 8    "lastsend": 1571830501,
 9    "lastrecv": 1571830501,
10    "bytessent": 15615598004,
11    "bytesrecv": 514550,
12    "conntime": 1571829847,
13    "version": 70013,
14    "subver": "/btcwire:0.5.0/neutrino:0.0.4-beta/",
15    "inbound": true,
16    "startingheight": 600689,
17    "banscore": 0,
18    "synced_headers": -1,
19    "synced_blocks": -1,
20    "inflight": [],
21    "whitelisted": false,
22    "permissions": [
23      "noban",
24      "relay",
25      "mempool"
26    ],
27    "minfeefilter": 0,
28    "bytessent_per_msg": {
29      "addr": 30192,
30      "block": 140862657,
31      "cfheaders": 854,
32      "cfilter": 15474702573,
33      "feefilter": 32,
34      "getdata": 61,
35      "headers": 742,
36      "inv": 366,
37      "ping": 192,
38      "pong": 160,
39      "sendheaders": 24,
40      "verack": 24,
41      "version": 127
42    },
43    "bytesrecv_per_msg": {
44      "*other*": 496906,
45      "getaddr": 24,
46      "getdata": 8723,
47      "getheaders": 7339,
48      "inv": 1037,
49      "ping": 160,
50      "pong": 192,
51      "verack": 24,
52      "version": 145
53    }
54  }

laanwj added this to the "Blockers" column in a project

DrahtBot added the label Needs rebase on Oct 30, 2019

jimpo commented at 1:46 pm on October 31, 2019: contributor

@Sjors Why would you limit filter data? We don’t limit the amount of block data served to a peer…

Sjors commented at 5:30 pm on November 2, 2019: member

@jimpo indeed, we discussed this during the IRC meeting. No need to rate limit this specific message.

jamesob commented at 9:03 pm on November 4, 2019: member

Ping @jimpo for a rebase. Looking to review shortly.

jimpo force-pushed on Nov 5, 2019

DrahtBot removed the label Needs rebase on Nov 5, 2019

jimpo force-pushed on Nov 7, 2019

barrystyle referenced this in commit 1dd464f8af on Nov 11, 2019

Sjors commented at 12:55 pm on November 11, 2019: member

There’s still some ongoing discussion about getcfilters limit in the BIP https://github.com/bitcoin/bips/pull/857 (cc @TheBlueMatt, @harding)

jimpo force-pushed on Nov 11, 2019

jimpo force-pushed on Nov 12, 2019

TheBlueMatt commented at 2:02 am on November 14, 2019: member

Note that if you want to use the new max limit, you’ll need to do something closer to what we do for getdatas - dumping the full set of requested filters into the send buffer opens us up to a massive dos attack (based purely on Harding’s estimated size I haven’t checked the match myself).

On Nov 11, 2019, at 07:55, Sjors Provoost notifications@github.com wrote:

There’s still some ongoing discussion about getcfilters limit in the BIP bitcoin/bips#857 (cc @TheBlueMatt, @harding)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

Sjors commented at 10:56 am on November 14, 2019: member

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 commented at 9:33 pm on November 15, 2019: member

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 referenced this in commit ae36367f10 on Nov 17, 2019

jnewbery commented at 11:21 pm on November 18, 2019: member

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?

in src/protocol.h:257 in 06cd88c6bc outdated

248+ * BIP 157 & 158.
249+ */
250+extern const char *GETCFHEADERS;
251+/**
252+ * cfheaders is a response to a getcfheaders request containing a vector of
253+ * filter headers for each block in the requested range.

jkczyz commented at 4:13 am on November 19, 2019:

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

in src/net_processing.cpp:2029 in aaceafd749 outdated

1895+                                      BlockFilterIndex*& filter_index)
1896+{
1897+    bool supported_filter_type =
1898+        (filter_type == BlockFilterType::BASIC &&
1899+         (pfrom->GetLocalServices() & NODE_COMPACT_FILTERS));
1900+    if (!supported_filter_type) {

jkczyz commented at 4:31 am on November 19, 2019:

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

jimpo commented at 5:43 pm on November 20, 2019:

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

jkczyz commented at 11:18 pm on November 21, 2019:

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

0if (!pfrom->SupportsBlockFilter(BlockFilterType::BASIC)) {
1    // ...
2}

in src/net_processing.cpp:2193 in 459aead0e6 outdated

2136+            if (static_cast<size_t>(i) < active_chain_cf_headers.size() &&
2137+                active_chain_cf_headers[i].first == block_index) {
2138+                break;
2139+            }
2140+
2141+            // Filter header requested for stale block.

pinheadmz commented at 10:27 pm on November 19, 2019:

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.

jkczyz commented at 0:16 am on November 20, 2019:

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.

in src/net_processing.cpp:1333 in 8788254526 outdated

1250+        uint256 filter_header;
1251+
1252+        if (!filter_index.LookupFilterHeader(block_index, filter_header)) {
1253+            return error("Failed to find block filter header in index: filter_type=%s, block_hash=%s",
1254+                         BlockFilterTypeName(filter_index.GetFilterType()),
1255+                         block_index->GetBlockHash().ToString());

jkczyz commented at 11:59 pm on November 19, 2019:

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?

jimpo commented at 5:39 pm on November 20, 2019:

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.

in src/net_processing.cpp:2025 in 7a05bb6cb9 outdated

2021@@ -1998,17 +2022,27 @@ static bool ProcessGetCFHeaders(CNode* pfrom, CDataStream& vRecv, const CChainPa
2022         return true;
2023     }
2024 
2025+    bool index_in_sync = false;

jkczyz commented at 1:02 am on November 20, 2019:

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).

jimpo commented at 5:46 pm on November 20, 2019:

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.

jkczyz commented at 11:43 pm on November 21, 2019:

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

https://github.com/bitcoin/bitcoin/blob/46d6930f8c7ba7cbcd7d86dd5d0117642fcbc819/src/index/base.h#L95-L100

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

https://github.com/bitcoin/bitcoin/blob/b2a6b0216192b6e8428f1a80b47f5178fccb961a/src/index/base.cpp#L282-L289

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 commented at 0:45 am on February 29, 2020:

Yes, you are right. I guess I wanted to avoid taking cs_main, but it already does in PrepareBlockFilterRequest so maybe that’s an unnecessary optimization.

jkczyz commented at 1:15 am on November 20, 2019: contributor

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

jimpo commented at 1:02 pm on November 20, 2019: contributor

@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 commented at 1:20 pm on November 20, 2019: contributor

@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.

in src/net.cpp:2657 in dd577bef3c outdated

2653+        BlockFilterIndex* basic_filter_index = GetBlockFilterIndex(BlockFilterType::BASIC);
2654+        if (!basic_filter_index) {
2655+            LogPrintf("WARNING: NODE_COMPACT_FILTERS is signaled, but filter index is not available\n");
2656+        }
2657+        if (!basic_filter_index || !basic_filter_index->IsSynced()) {
2658+            // If block filter index is still syncing, do not advertise the service bit.

fjahr commented at 5:01 pm on November 20, 2019:

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

fjahr commented at 5:21 pm on November 20, 2019: member

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 f927847519b5a5f8644dcd843af2249e89653a11 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.

benthecarman commented at 9:58 pm on November 20, 2019: contributor

NODE_COMPACT_FILTERS should be added to GetServicesNames

in src/index/base.h:96 in 459aead0e6 outdated

 95+    /// Get the name of the index for display in logs.
 96+    virtual const char* GetName() const = 0;
 97+
 98+    /// Returns whether index has completed the initial sync with the active chain.
 99+    /// After returning true once, this function will return true on all subsequent calls.
100+    bool IsSynced() const { return m_synced; }

Talkless commented at 6:53 pm on November 21, 2019:

nit: trivial, could be noexcept

in src/init.cpp:952 in 459aead0e6 outdated

946@@ -946,6 +947,18 @@ bool AppInitParameterInteraction()
947         }
948     }
949 
950+    // Signal NODE_COMPACT_FILTERS if peercfilters and required index are both enabled.
951+    if (gArgs.GetBoolArg("-peercfilters", DEFAULT_PEERCFILTERS)) {
952+        bool index_enabled = std::find(g_enabled_filter_types.begin(),

Talkless commented at 6:54 pm on November 21, 2019:

nit: could be const bool index_enabled

in src/net_processing.cpp:1310 in 459aead0e6 outdated

1225@@ -1214,6 +1226,40 @@ void PeerLogicValidation::NewPoWValidBlock(const CBlockIndex *pindex, const std:
1226     });
1227 }
1228 
1229+static bool UpdateCFHeadersCache(const BlockFilterIndex& filter_index)
1230+{
1231+    LOCK(cs_main);
1232+    std::lock_guard<std::mutex> _lock(active_chain_cf_headers_mtx);

Talkless commented at 7:01 pm on November 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.

jimpo commented at 0:49 am on February 29, 2020:

This doesn’t seem actionable here. Probably best to open an issue or discuss in IRC or something?

in src/net_processing.cpp:1315 in 459aead0e6 outdated

1232+    std::lock_guard<std::mutex> _lock(active_chain_cf_headers_mtx);
1233+
1234+    const CChain& active_chain = ::ChainActive();
1235+
1236+    // Drop any entries in the checkpoint cache that have been reorganized off the active chain.
1237+    int new_size = active_chain_cf_headers.size();

Talkless commented at 7:11 pm on November 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:

0const auto it = std::find_if(active_chain_cf_headers.crbegin(), active_chain_cf_headers.crend(), [&](const std::pair<const CBlockIndex*, uint256> &i) {
1    return active_chain.Contains(i.first);
2});
3active_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:

 0#include <algorithm>
 1#include <iostream>
 2#include <iterator>
 3#include <vector>
 4
 5int main()
 6{
 7    std::vector<int> vals{1, 1, 0, 0, 0};
 8
 9    const auto it = std::find_if(vals.crbegin(), vals.crend(), [](int v) -> bool {
10        return v > 0;
11    });
12
13    vals.erase(it.base(), vals.cend());
14
15    std::copy(vals.cbegin(),
16              vals.cend(),
17              std::ostream_iterator<int>(std::cout, " "));
18}

in src/net_processing.cpp:1249 in 459aead0e6 outdated

1244+
1245+    // Populate the checkpoint cache with headers for blocks on the active chain.
1246+    for (uint32_t height = (new_size + 1) * CFCHECKPT_INTERVAL;
1247+         height <= static_cast<uint32_t>(active_chain.Height());
1248+         height += CFCHECKPT_INTERVAL) {
1249+        const CBlockIndex* block_index = active_chain[height];

Talkless commented at 7:11 pm on November 21, 2019:

nit: could be const CBlockIndex* const block_index

in src/net_processing.cpp:1944 in 459aead0e6 outdated

1939+                                      BlockFilterType filter_type, uint32_t start_height,
1940+                                      uint256& stop_hash, uint32_t max_height_diff,
1941+                                      const CBlockIndex*& stop_index,
1942+                                      BlockFilterIndex*& filter_index)
1943+{
1944+    bool supported_filter_type =

Talkless commented at 7:13 pm on November 21, 2019:

nit: could be const bool supported_filter_type

in src/net_processing.cpp:2018 in 459aead0e6 outdated

2013+    uint32_t start_height;
2014+    uint256 stop_hash;
2015+
2016+    vRecv >> filter_type_ser >> start_height >> stop_hash;
2017+
2018+    BlockFilterType filter_type = static_cast<BlockFilterType>(filter_type_ser);

Talkless commented at 7:16 pm on November 21, 2019:

nit: could be const BlockFilterType filter_type

in src/net_processing.cpp:2031 in 459aead0e6 outdated

2026+    }
2027+
2028+    bool index_in_sync = false;
2029+
2030+    std::vector<BlockFilter> filters;
2031+    bool lookup_success = QueryFilterIndexWithRetry(

Talkless commented at 7:19 pm on November 21, 2019:

nit: could be const bool lookup_success

in src/net_processing.cpp:2058 in 459aead0e6 outdated

2053+    uint32_t start_height;
2054+    uint256 stop_hash;
2055+
2056+    vRecv >> filter_type_ser >> start_height >> stop_hash;
2057+
2058+    BlockFilterType filter_type = static_cast<BlockFilterType>(filter_type_ser);

Talkless commented at 7:20 pm on November 21, 2019:

nit: could be const BlockFilterType filter_type

in src/net_processing.cpp:2072 in 459aead0e6 outdated

2067+
2068+    bool index_in_sync = false;
2069+
2070+    uint256 prev_header;
2071+    if (start_height > 0) {
2072+        const CBlockIndex* prev_block = stop_index->GetAncestor(start_height - 1);

Talkless commented at 7:25 pm on November 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.

in src/net_processing.cpp:2073 in 459aead0e6 outdated

2068+    bool index_in_sync = false;
2069+
2070+    uint256 prev_header;
2071+    if (start_height > 0) {
2072+        const CBlockIndex* prev_block = stop_index->GetAncestor(start_height - 1);
2073+        bool lookup_success = QueryFilterIndexWithRetry(

Talkless commented at 7:25 pm on November 21, 2019:

nit: could be const bool lookup_success

in src/net_processing.cpp:2084 in 459aead0e6 outdated

2079+                         BlockFilterTypeName(filter_type), prev_block->GetBlockHash().ToString());
2080+        }
2081+    }
2082+
2083+    std::vector<uint256> filter_hashes;
2084+    bool lookup_success = QueryFilterIndexWithRetry(

Talkless commented at 7:26 pm on November 21, 2019:

nit: ditto, could be const bool lookup_success

in src/net_processing.cpp:2112 in 459aead0e6 outdated

2107+    uint8_t filter_type_ser;
2108+    uint256 stop_hash;
2109+
2110+    vRecv >> filter_type_ser >> stop_hash;
2111+
2112+    BlockFilterType filter_type = static_cast<BlockFilterType>(filter_type_ser);

Talkless commented at 7:27 pm on November 21, 2019:

nit: could be const BlockFilterType filter_type

in src/net_processing.cpp:2142 in 459aead0e6 outdated

2137+                active_chain_cf_headers[i].first == block_index) {
2138+                break;
2139+            }
2140+
2141+            // Filter header requested for stale block.
2142+            bool lookup_success = QueryFilterIndexWithRetry(

Talkless commented at 7:28 pm on November 21, 2019:

nit: could be const bool lookup_success

Talkless commented at 7:35 pm on November 21, 2019: none

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

Talkless commented at 7:50 pm on December 4, 2019: none

my getnetworkinfo shows:

0"localservicesnames": [
1    "NETWORK",
2    "WITNESS",
3    "NETWORK_LIMITED"
4  ],

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:

0 ./bitcoin-qt -chain=regtest

while my bitcoin.conf is:

0[regtest]
1blockfilterindex=basic
2peercfilters=1
3server=1
4listen=1
5bind=0.0.0.0

marcinja commented at 10:52 pm on December 4, 2019: contributor

Why I don’t see NODE_COMPACT_FILTERS? @Talkless I believe that is because GetServicesNames hasn’t been updated yet. See: #16442 (comment)

Talkless commented at 5:08 pm on December 5, 2019: none

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

DrahtBot added the label Needs rebase on Jan 2, 2020

in src/net_processing.cpp:2028 in aaceafd749 outdated

1894+                                      const CBlockIndex*& stop_index,
1895+                                      BlockFilterIndex*& filter_index)
1896+{
1897+    bool supported_filter_type =
1898+        (filter_type == BlockFilterType::BASIC &&
1899+         (pfrom->GetLocalServices() & NODE_COMPACT_FILTERS));

ariard commented at 3:28 am on January 7, 2020:

aaceafd

Is generic BIP 157 support can be checked at beginning of ProcessMessage like it’s done for NODE_BLOOM ? It would avoid to busy ThreadMessageHandler for nothing?

in src/net_processing.cpp:2041 in aaceafd749 outdated

1907+    {
1908+        LOCK(cs_main);
1909+        stop_index = LookupBlockIndex(stop_hash);
1910+
1911+        // Check that the stop block exists and was at some point connected to the active chain.
1912+        if (!stop_index || !BlockRequestAllowed(stop_index, chain_params.GetConsensus())) {

ariard commented at 3:33 am on January 7, 2020:

aaceafd

This seems to assume than both client and server blockchain views are in sync. In case of diverging tips seen, client may be disconnected for a non-reprehensible request, just for being too fast ? (BlockRequestAllowed is for block which has been validated at least once)

in src/net_processing.cpp:2064 in aaceafd749 outdated

1930+                 pfrom->GetId(), stop_height - start_height + 1, max_height_diff);
1931+        pfrom->fDisconnect = true;
1932+        return false;
1933+    }
1934+
1935+    filter_index = GetBlockFilterIndex(filter_type);

ariard commented at 3:36 am on January 7, 2020:

aaceafd

As you said, filter type support is already checked at first step. What does this check add ?

jimpo commented at 1:12 am on February 29, 2020:

This isn’t a check, this is a lookup? I agree that the check below this should never fail though. It’s just there for defensive purposes to prevent a segfault.

in src/net_processing.cpp:2130 in 07dd76272c outdated

1995+        // Return true because the issue with the invalid request has already been logged.
1996+        return true;
1997+    }
1998+
1999+    uint256 prev_header;
2000+    if (start_height > 0) {

ariard commented at 3:38 am on January 7, 2020:

07dd762

What the purpose of checking filters header availability before start of the range ? It seems like zeal on the bip

jimpo commented at 1:14 am on February 29, 2020:

What do you mean? Like fetching the filter at start_height - 1? The purpose is that the one filter header before the start of the range is required to compute the filter headers for all filters in the range given their hashes (it’s a hash chain).

in src/net_processing.cpp:2139 in 07dd76272c outdated

2003+            return error("Failed to find block filter header in index: filter_type=%s, block_hash=%s",
2004+                         BlockFilterTypeName(filter_type), prev_block->GetBlockHash().ToString());
2005+        }
2006+    }
2007+
2008+    std::vector<uint256> filter_hashes;

ariard commented at 3:38 am on January 7, 2020:

07dd762

nit: filter_headers, also log message

jimpo commented at 1:15 am on February 29, 2020:

No, I think filter_hashes is right.

in src/net_processing.cpp:441 in 8788254526 outdated

396@@ -396,6 +397,10 @@ limitedmap<uint256, std::chrono::microseconds> g_already_asked_for GUARDED_BY(cs
397 /** Map maintaining per-node state. */
398 static std::map<NodeId, CNodeState> mapNodeState GUARDED_BY(cs_main);
399 
400+/** In-memory cache of all BIP157 compact filter checkpoints for the active chain. */
401+static std::vector<std::pair<const CBlockIndex*, uint256>> active_chain_cf_headers;

ariard commented at 3:41 am on January 7, 2020:

8788254

What the perf improvement of caching checkpoints ? If clients are configured with trusted checkpoints, they are only going to use the higher range, or maybe a rolling checkpoints for last X headers/filters would be better as they are likely to be requested by everyone

jimpo commented at 1:16 am on February 29, 2020:

The GETCFCHECKPT request returns all filter checkpoints. There’s no parameter to request a subset from a different start height because the message is small and only requested once per catchup generally.

in src/net_processing.cpp:1952 in 7a05bb6cb9 outdated

1943@@ -1944,6 +1944,25 @@ static bool PrepareBlockFilterRequest(CNode* pfrom, const CChainParams& chain_pa
1944     return true;
1945 }
1946 
1947+/**
1948+ * Do a lookup on the block filter index. The lookup may fail erroneously if the filter index, which
1949+ * is updated asynchronously, has not been synchronized with the net processing thread. In that
1950+ * case, block for a short time until the filter index is updated, then retry the lookup.
1951+ */
1952+static bool QueryFilterIndexWithRetry(BaseIndex* index, bool& in_sync, std::function<bool()> fn)

ariard commented at 3:43 am on January 7, 2020:

But you may have race condition between client and server where block isn’t fully processed before request is handled, so client would have to bear this case anyway

jimpo commented at 7:37 pm on March 1, 2020:

The node is only guaranteed to serve a block filter/filter header if it has already advertised the block through a headers or block message to the requesting peer.

in src/init.cpp:991 in 1b7fbb57a2 outdated

946@@ -946,6 +947,18 @@ bool AppInitParameterInteraction()
947         }
948     }
949 
950+    // Signal NODE_COMPACT_FILTERS if peercfilters and required index are both enabled.
951+    if (gArgs.GetBoolArg("-peercfilters", DEFAULT_PEERCFILTERS)) {

ariard commented at 3:44 am on January 7, 2020:

1b7fbb5

IMO you can set -blockfilterindex by transivity if -peercfilters is already set, just mention it in the helper.

ariard commented at 3:57 am on January 7, 2020: member

I don’t think that both dynamically changing service flag and coupling index/server sync are great. A lot of PR complexity seems to be due to avoiding client complexity at all price. It’s not worth it because client would have to be robust anyway, like reliable peers over an unreliable connection or divergent tips for a short time period. Reading neutrino go backend for example, query timeout have already been implemented, it’s just peer error handling don’t dissociate between protocol misbehaves and unknown errors.
Lack of DoS protection for block servicing doesn’t mean we shouldn’t for filters. Encumbering block servicing may hamper IBD or normal block propagation, that’s not a concern here. Apart of DoS, I would favor at least extending -maxuploadtarget to cover bip 157, nodes operators should be able to do resources control if they want to service bip 157 with some bound. It may align incentives better.

in test/functional/p2p_cfilters.py:103 in 459aead0e6 outdated

 98+
 99+        # Check that the localservices is as expected.
100+        assert_equal(int(self.nodes[0].getnetworkinfo()['localservices'], 16),
101+                     default_services | NODE_COMPACT_FILTERS)
102+        assert_equal(int(self.nodes[1].getnetworkinfo()['localservices'], 16),
103+                     default_services)

luke-jr commented at 11:23 pm on January 16, 2020:

This doesn’t seem to be the right place to test default node services… Suggest initialising default_services to node1.nServices, and checking that it excludes NODE_COMPACT_FILTERS.

jonasschnelli commented at 7:04 pm on February 6, 2020: contributor

Needs rebase

in src/protocol.h:300 in 459aead0e6 outdated

289@@ -253,6 +290,9 @@ enum ServiceFlags : uint64_t {
290     // NODE_WITNESS indicates that a node can be asked for blocks and transactions including
291     // witness data.
292     NODE_WITNESS = (1 << 3),
293+    // NODE_COMPACT_FILTERS means the node will service basic block filter requests.
294+    // See BIP157 and BIP158 for details on how this is implemented.
295+    NODE_COMPACT_FILTERS = (1 << 6),

jonasnick commented at 5:26 pm on February 16, 2020:

NODE_COMPACT_FILTERS does not show up in the getnetworkinfo RPC’s "localservices" key, presumably because there’s no entry for it in GetServicesNames.

jimpo force-pushed on Mar 1, 2020

DrahtBot removed the label Needs rebase on Mar 1, 2020

jimpo force-pushed on Mar 2, 2020

luke-jr referenced this in commit 96b402aa40 on Mar 5, 2020

luke-jr referenced this in commit 498546fdc4 on Mar 5, 2020

luke-jr referenced this in commit 482126f3bc on Mar 5, 2020

DrahtBot added the label Needs rebase on Mar 11, 2020

jimpo force-pushed on Mar 22, 2020

DrahtBot removed the label Needs rebase on Mar 22, 2020

sipa commented at 9:04 pm on March 25, 2020: member

Looks like there is a data race now (see Travis output https://travis-ci.org/github/bitcoin/bitcoin/jobs/665594933).

jimpo force-pushed on Mar 28, 2020

in src/init.cpp:1791 in 7608b32fc2 outdated

1716+        if (!basic_filter_index) {
1717+            error("NODE_COMPACT_FILTERS is signaled, but filter index is not available");
1718+            return false;
1719+        }
1720+        if (!basic_filter_index->IsSynced()) {
1721+            InitError(strprintf(_("Cannot enable -peercfilters until basic block filter index is in sync. Please disable and reenable once filters have been indexed.").translated));

kilrau commented at 8:32 am on April 1, 2020:

I ran into this error when I had to restart a still-syncing node. So while I can add the -peercfilters flag when starting the node for the first time and syncing from scratch, I have to remove it when restarting a syncing node (otherwise it’d refuse to start with this error). I think it’s better to either always allow the -peercfilters flag or only allow it once the filter index is ready. Former being preferred, since if combined with https://github.com/bitcoin/bitcoin/pull/16442/files#r363580980, it allows for everything to work with the -peercfilters flag only.

Other than that: amazing work!

alejagapatrick commented at 0:46 am on January 6, 2021:

# # # # #

MarcoFalke commented at 4:20 pm on April 8, 2020: member

It looks like the test times out when run in valgrind. Maybe you could bump the timeout like some of the other tests do?

0self.rpc_timeout = 120

DrahtBot added the label Needs rebase on Apr 8, 2020

brakmic commented at 5:10 pm on April 30, 2020: contributor

Concept ACK.

Built, run and tested on macOS Catalina 10.15.4.

prusnak commented at 5:21 pm on April 30, 2020: contributor

What is the target release of bitcoin-core to have this included?

MarcoFalke removed this from the "Blockers" column in a project

MarcoFalke commented at 7:18 pm on April 30, 2020: member

Removed from high-prio for now because it needs rebase. Can be re-added right after the rebase.

jonatack commented at 7:34 pm on April 30, 2020: member

@prusnak afaik it depends on review; master currently targets v0.21

jnewbery commented at 10:05 pm on April 30, 2020: member

@jimpo would you like me to maintain this branch/pull request? Happy to rebase it and squash the fixup commits.

luke-jr referenced this in commit 076b956d4a on Apr 30, 2020

luke-jr referenced this in commit 86e3db85f7 on Apr 30, 2020

luke-jr referenced this in commit f8c299b3d4 on Apr 30, 2020

luke-jr referenced this in commit 732c02ec7f on Apr 30, 2020

luke-jr referenced this in commit d8fd9669a6 on Apr 30, 2020

luke-jr referenced this in commit ed6ce3fcbb on Apr 30, 2020

luke-jr referenced this in commit 373019fd81 on Apr 30, 2020

luke-jr referenced this in commit 16beef3510 on Apr 30, 2020

luke-jr referenced this in commit d5c365b4d6 on Apr 30, 2020

luke-jr referenced this in commit edc3eeca68 on Apr 30, 2020

luke-jr referenced this in commit fc278034da on Apr 30, 2020

luke-jr referenced this in commit 3f89d7f49c on Apr 30, 2020

luke-jr referenced this in commit 8d63eab427 on Apr 30, 2020

luke-jr referenced this in commit 448b218af5 on Apr 30, 2020

luke-jr referenced this in commit e53faa241a on Apr 30, 2020

luke-jr referenced this in commit 0dcb2910f9 on Apr 30, 2020

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

The node will enable the NODE_COMPACT_FILTERS service flag once the
block filter index catches up to the active chain.

1ca131c3d9

net: Define new BIP 157 protocol messages. e80db7c440

jimpo force-pushed on May 1, 2020

DrahtBot removed the label Needs rebase on May 1, 2020

jimpo commented at 3:16 am on May 2, 2020: contributor

Yes, that would be very appreciated. Thank you @jnewbery!

prusnak commented at 8:47 am on May 2, 2020: contributor

The current changeset seems to be failing the p2p_cfilters.py test on some configurations.

in src/net_processing.cpp:2091 in 14df7f7b17 outdated

2040+    const CBlockIndex* stop_index;
2041+    BlockFilterIndex* filter_index;
2042+    if (!PrepareBlockFilterRequest(pfrom, chain_params, filter_type, start_height, stop_hash,
2043+                                   MAX_GETCFILTERS_SIZE, stop_index, filter_index)) {
2044+        // Return true because the issue with the invalid request has already been logged.
2045+        return true;

MarcoFalke commented at 11:56 pm on May 2, 2020:

in commit 14df7f7b17: Seems a bit odd to return true here, but not in the error case (which is also) logged. I suggest making the return type here void

jimpo commented at 7:10 am on May 4, 2020:

It returns false in cases that are unexpected, our-fault errors (more like exceptions) and returns true and logs when it’s just an invalid request.

in src/net_processing.cpp:2022 in 72784c3dac outdated

2017+ * request is valid and can be serviced, this returns the stop block index and the filter index
2018+ * as out parameters. May disconnect from the peer in the case of a bad request.
2019+ */
2020+static bool PrepareBlockFilterRequest(CNode* pfrom, const CChainParams& chain_params,
2021+                                      BlockFilterType filter_type, uint32_t start_height,
2022+                                      uint256& stop_hash, uint32_t max_height_diff,

MarcoFalke commented at 1:12 am on May 3, 2020:

nit in commit 14df7f7b17f1936ae641c619ae95989090f5a063

0                                      const uint256& stop_hash, uint32_t max_height_diff,

MarcoFalke commented at 1:16 am on May 3, 2020: member

ACK 72784c3dac 📶

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3ACK 72784c3dac 📶
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUhRkwv9GYZx75PbE6Q6VqEuDomfW8amF89WneIQOES2IG69dMBT9QVDjwyBiAhW
 8JUZBEd5+QruLK+F7zaVSA0MWBTrytjbOPJooVW4zqwz6pvmCGDfvK5GgIq+rxpXi
 9b+RtNaJOozJOtjzKgYnXnjVMTIsXlikvHNRfPV78ut7rQesZFyvfs/KFEHNDZiQn
10tAmTpIWkjCSL+AzCPRHl1oM4btBQYo4U05zXz5R4mwgmfFiGIR9/zVyeGj+IZdjg
11iyzonjZxcgfkQliBxoKIiuMrZJpUREoxdtz7Bz3K6M/EvAE/uXs1UvO4nW6C9NPb
124ZxzHAHW0ZpFar7rokzAdrYKMol57tMzu4+Zy4+ORR7iFLEtM6/eusoaM926LtgR
137lHn6w7BI4e47WZUOv6B2blJ2PDhUbIMaGGJHcp0L0oq82g8G3+ifKrMQnBMx+t0
14vY0uiIqXux9qPmEzCBeTdrRcP9ttEx0dUa9J8CX8SB1nAkHn3Tqm4EI3AjwTqBv1
158rak+MEe
16=7kNY
17-----END PGP SIGNATURE-----

Timestamp of file with hash 1689fa73387e19ab3b3f9823a2aeee68cde8d5c8190a7f072880b6df2ab6a10f -

MarcoFalke added this to the milestone 0.21.0 on May 3, 2020

MarcoFalke added this to the "Blockers" column in a project

MarcoFalke commented at 1:23 am on May 3, 2020: member

Re-added to the high-prio project, per my previous comment.

Also assigned 0.21.0 milestone to get this merged hopefully early in the timeline, so that additional long-term testing can be conducted.

in src/net_processing.cpp:133 in 14df7f7b17 outdated

128@@ -127,6 +129,8 @@ static constexpr unsigned int INVENTORY_BROADCAST_MAX = 7 * INVENTORY_BROADCAST_
129 static constexpr unsigned int AVG_FEEFILTER_BROADCAST_INTERVAL = 10 * 60;
130 /** Maximum feefilter broadcast delay after significant change. */
131 static constexpr unsigned int MAX_FEEFILTER_CHANGE_DELAY = 5 * 60;
132+/** Maximum number of compact filters that may be requested with one getcfilters. See BIP 157. */
133+static constexpr uint32_t MAX_GETCFILTERS_SIZE = 100;

MarcoFalke commented at 1:25 am on May 3, 2020:

note to reviewers, in commit 14df7f7b17f1936ae641c619ae95989090f5a063

The BIP https://github.com/bitcoin/bips/blob/master/bip-0157.mediawiki#getcfilters says 1000. This is fixed up in a later commit 7f59550cb176ad2ea3ef6e9a75422bc8ffcc5c7e

in test/functional/p2p_cfilters.py:45 in a7b375a446 outdated

40+        """Store cfilters received in a list."""
41+        self.cfilters.append(message)
42+
43+class CompactFiltersTest(BitcoinTestFramework):
44+    def set_test_params(self):
45+        self.setup_clean_chain = True

MarcoFalke commented at 4:16 pm on May 3, 2020:

in commit a7b375a4469fcef823a274e3f05db0f9ab7aa035 :

The test still times out on valgrind. Please bump the timeout.

 0diff --git a/test/functional/p2p_cfilters.py b/test/functional/p2p_cfilters.py
 1index dca35f4e89..d16946510b 100755
 2--- a/test/functional/p2p_cfilters.py
 3+++ b/test/functional/p2p_cfilters.py
 4@@ -42,6 +42,7 @@ class CFiltersClient(P2PInterface):
 5 
 6 class CompactFiltersTest(BitcoinTestFramework):
 7     def set_test_params(self):
 8+        self.rpc_timeout = 480
 9         self.setup_clean_chain = True
10         self.num_nodes = 2
11         self.extra_args = [

(generating more than 1000 blocks in valgrind takes time)

https://travis-ci.org/github/bitcoin/bitcoin/jobs/682347754#L3147

0test_framework.authproxy.JSONRPCException: 'generatetoaddress' RPC took longer than 120.000000 seconds. Consider using larger timeout for calls that take longer to return. (-344)

in test/functional/test_framework/messages.py:1522 in a7b375a446 outdated

1514@@ -1512,3 +1515,154 @@ class msg_no_witness_blocktxn(msg_blocktxn):
1515 
1516     def serialize(self):
1517         return self.block_transactions.serialize(with_witness=False)
1518+
1519+
1520+class msg_getcfilters:
1521+    __slots__ = ("filter_type", "start_height", "stop_hash")
1522+    command = b"getcfilters"

MarcoFalke commented at 4:16 pm on May 3, 2020:

in commit a7b375a4469fcef823a274e3f05db0f9ab7aa035 :

Could you please squash 72784c3dac1e9d482cf5499170f689f483c1da51 into this commit?

MarcoFalke approved

MarcoFalke commented at 4:17 pm on May 3, 2020: member

Please bump the rpc_timeout in the test. Feel free to ignore the other stylistic-only nits.

net: Message handling for GETCFILTERS. 94a7cb785f

net: Message handling for GETCFHEADERS. 454f83729f

net: Message handling for GETCFCHECKPT. 2a83db2104

blockfilter: Fix default (de)?serialization of BlockFilter. 6b76adb4f8

test: Functional tests for cfilters P2P interface. 4a8ad724a1

net: Synchronize cfilter request handling with block filter index.

Since the block filter index is updated asynchronously by proxy of the
ValidationInterface queue, lazily synchronize the net thread with the
block filter index in case lookups fail.

6f846a4771

test: Exercise cache update code during getcfcheckpt handling. 763d2cee80

net: Cache compact filter checkpoints in memory. be5f7c5f8e

init: Separate CLI flag for block filter index and serving cfilters. 82713cd727

net: Raise MAX_GETCFILTERS_SIZE following change to BIP 157. 5d8a825173

util: Add NODE_COMPACT_FILTERS to GetServicesNames. 793ee7775d

const all the things. 94a662e269

net: Remove dynamic setting of local service flags. 0dcd363060

net: Fix concurrency issues with CF checkpts cache updates. 6782deb365

jimpo force-pushed on May 4, 2020

ariard commented at 9:22 am on May 4, 2020: member

@jimpo what’s the bandwidth consumption by month of a BIP157 client ? I guess you have done simulations but how many clients you can onboard for a given number of public peers servicing filters, these peers allocating half of their 350GB/month available bandwidth to light clients ?

MarcoFalke commented at 11:45 am on May 4, 2020: member

re-ACK 6782deb365cc5a9db763aad1bd0604f515ee9a68 , only change is bumping the test timeout 🦑

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3re-ACK 6782deb365cc5a9db763aad1bd0604f515ee9a68 , only change is bumping the test timeout 🦑
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUgd5wv/fOA6Wp1SOJL7RkcBPP6E2vCWfAHgO1Nkqm8IM0b3ni39GycRiGPo8byX
 8Lb4OJoBp1eYn16M3jQM/6p+qwHIaGtK8PF3dYAHE5ausfPbU08i7lSXXgiwLhLRT
 9yL80XA7GSfmndXRVlC4oT+gEZTiLmtHjRHPxncHBUwk0CsbDranHW26L5yWM837q
10IpEEvYmNGdY25EkdyomYPKdYubC1kg3KNfs2al/stHfnZOcc2qHER7TG1iae0uKS
11UR+LlZM8YC1m7YDNnfQax7YZha/7uO2mtWk4NRiMF0kKVc+5OMxYafYmG5TgsCyX
12/V6yf3PCsDljHqCVMLXIpq7yMa+HFkY0o5VHK/f/rzRHSN5O0by+jejhXqFuHbf4
133hSLWGDxPOZ7t0sJRmmsUXvpd+3a4InTIwYFPXUnMb+8MtoZ+s1y8lyXGeJ0n4LY
14hqU/z6KG7Jwco1Jw31/OuLQEj9xps5c5n5sXRo7/RRSsOFVbaPYAyHy+T3X1etrU
15+ZINQMb8
16=R4ah
17-----END PGP SIGNATURE-----

Timestamp of file with hash 12b25cde2d674e19d526f72520f81959ca1a59d56f42ab8c777377410a6d12f9 -

MarcoFalke commented at 11:47 am on May 4, 2020: member

@ariard I think maxuploadtarget documentation can be added in a follow-up. No need to solve all issues in one pull request.

jnewbery commented at 5:19 pm on May 4, 2020: member

Yes, that would be very appreciated. Thank you @jnewbery!

Thanks @jimpo . I plan to do the following:

squash all fixup commits into their respective commits
rebase on master
re-order commits and then open PRs for subsets of the functionality so we can make some progress towards merging this.

Thanks again for all your work on this :)

ariard commented at 8:34 pm on May 4, 2020: member

@MarcoFalke It’s not a question of node resources management, but a more broader concern about light clients current scalability model. Even with a low-bandwidth protocol such as BIP157/158, you may still have a huge discrepancy between the numbers of requesting clients and opt-in public peers to serve them.

At its heart, you will always have a incentives conflict between clients asking for resources and offering nothing back and full-node operators. Unless your light client protocol is so ridiculous cheap to rely on niceness of a subset of nodes operators offering free resources, it won’t scale for thousands or millions of clients. And it’s likely you will always have a ratio disequilibrium between numbers of clients and numbers of full-node, even worst their growth rate won’t be the same, first one are so much easier to setup.

I have been through mailing posts about BIP157/158 design but haven’t seen any realistic projections and specially what people do aim to achieve by integrating this at the p2p layer. I do understand people may be interested to use compact filters for their own Neutrino-style client and if so they should use the RPC interface. I’m worried about wallet vendors building such chain access backend for people not connecting to their own full-node but directly to the broader network, which maybe fine now because you have really few BIP157 clients, but going to hit a bandwidth scalability wall few years from now instead of pursing better solutions.

Maybe I’m completely wrong, missing some numbers and I would be super glad to read about what have been previously thought on this issue. In the meanwhile I’m Concept NACK on this and happy to bring it during next meeting and explained issues as I perceived it on the ml.

Note, that doesn’t restrain to get this PR ready for merge.

jnewbery commented at 8:53 pm on May 4, 2020: member

@ariard please take discussion of light-client scalability to the mailing list. Comments on this PR should be for review of the implementation.

luke-jr commented at 9:39 pm on May 4, 2020: member

@ariard This is an optional feature, not enabled by default. It might make some sense to support it for trusted peers only (missing from this PR), although protocols like Stratum are probably better for that purpose (but nobody has implemented it for Core to date). @jnewbery Concept NACKs, and the explanation therefore (required by dev notes!) do belong on PRs…

I think I’m going to mirror @ariard’s Concept NACK. We do have bloom already for trusted peers, and using this for public peers does indeed harm the network. We can’t stop anyone else from deploying it, but with Core’s monopoly, that hardly matters.

sipa commented at 9:56 pm on May 4, 2020: member

@ariard Of course there are always questions about incentives to provide public data for free by nodes, but that problem isn’t unique to BIP157 - you can also question if nodes have reason to provide blocks, transactions, to participate in relay, … etc. Perhaps there is a future where all these things eventually have to be paid for, but for now, trying to solve this perfectly would just lead you to the conclusion that all non-miner nodes should be turned off, and if someone wants to audit the chain they can connect directly to miners. The relevant question is how BIP157 costs compare to services already offered, which doesn’t seem very bad to me.

sipa commented at 10:03 pm on May 4, 2020: member

@luke-jr Certainly reasons to reject an implementation of a BIP should go on a PR, but you have principled objections to a BIP - which seems to be the case here - it is much more useful to have that criticism be visible more widely. It would be strange if there is apparent consensus that a BIP is a good idea, and then having it not be implemented in Core because of an argument only presented there.

jnewbery commented at 2:59 am on May 5, 2020: member

Github doesn’t allow PRs to be transferred, so I’ve opened #18876 for the new branch.

fanquake commented at 3:08 am on May 5, 2020: member

Closing this for #18876. Discussion / review should move to #18876 and the sub-PRs.

fanquake closed this on May 5, 2020

fanquake removed this from the "Blockers" column in a project

fanquake removed this from the milestone 0.21.0 on May 5, 2020

ariard commented at 10:26 am on May 5, 2020: member

@luke-jr Yes I noticed that’s an optional feature, used the term opt-in somehow IIRC. @sipa that’s the right but you may have an indirect incentive to relay blocks and likely-to-be-blocks to foster network robustness and make your transactions more censorship-resistant. Killing a light client doesn’t increase or decrease network security ?

Anyway, thanks for your comments, throw a lengthy argumentation on the mailing list on this.

luke-jr referenced this in commit b75ee9bcf6 on Jun 9, 2020

deadalnix referenced this in commit fbda81eaa4 on Oct 20, 2020

vijaydasmp referenced this in commit 6e2e4ce09e on Oct 29, 2021

vijaydasmp referenced this in commit 7e47d328e7 on Oct 30, 2021

vijaydasmp referenced this in commit e87bd6335c on Nov 2, 2021

vijaydasmp referenced this in commit bc29631666 on Nov 7, 2021

vijaydasmp referenced this in commit 295d642365 on Nov 7, 2021

vijaydasmp referenced this in commit 12ab004e7e on Nov 11, 2021

vijaydasmp referenced this in commit a478350315 on Nov 12, 2021

vijaydasmp referenced this in commit 228fa42768 on Nov 13, 2021

vijaydasmp referenced this in commit fb20b4e82e on Nov 14, 2021

vijaydasmp referenced this in commit 72beaa63b6 on Nov 14, 2021

vijaydasmp referenced this in commit 47714ab3b7 on Nov 16, 2021

gades referenced this in commit c7d3760455 on May 18, 2022

DrahtBot locked this on Aug 16, 2022

Serve BIP 157 compact filters #16442

Conflicts

Review checklist

Tests

# # # # #