Serve cfcheckpt requests

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

Serve cfcheckpt messages if basic block filter index is enabled and -peercfilters is set.

NODE_COMPACT_FILTERS is not signaled to peers, but functionality can be used for testing and serving pre-configured clients.

fanquake added the label P2P on May 5, 2020

prusnak commented at 8:46 am on May 5, 2020: contributor

Seems the lint target fails in the Travis CI.

DrahtBot commented at 9:25 am on May 5, 2020: 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:

#18876 ([WIP] Serve BIP 157 compact filters by jnewbery)
#18698 (Make g_chainman internal to validation by MarcoFalke)
#18638 (net: Use mockable time for ping/pong, add tests by MarcoFalke)
#18242 (Add BIP324 encrypted p2p transport de-/serializer (only used in tests) by jonasschnelli)
#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.

jnewbery force-pushed on May 5, 2020

in src/init.cpp:992 in 493eb3bd47 outdated

986@@ -986,6 +987,16 @@ bool AppInitParameterInteraction()
987         }
988     }
989 
990+    // Signal NODE_COMPACT_FILTERS if peercfilters and required index are both enabled.
991+    if (gArgs.GetBoolArg("-peercfilters", DEFAULT_PEERCFILTERS)) {
992+        const bool index_enabled = std::find(g_enabled_filter_types.begin(),

promag commented at 8:42 pm on May 5, 2020:

g_enabled_filter_types.count(BlockFilterType::BASIC)? otherwise #include <algorithm>.

jnewbery commented at 3:52 am on May 6, 2020:

Good suggestion. Taken.

jnewbery force-pushed on May 5, 2020

jnewbery force-pushed on May 6, 2020

jnewbery commented at 3:52 am on May 6, 2020: member

Note to reviewers: the Travis failures are timeouts and can be ignored. This PR is ready for review.

jnewbery force-pushed on May 6, 2020

in src/protocol.h:250 in bd1a924b34 outdated

245+ * cfcheckpt is a response to a getcfcheckpt request containing a vector of
246+ * evenly spaced filter headers for blocks on the requested chain.
247+ * Only available with service bit NODE_COMPACT_FILTERS as described by
248+ * BIP 157 & 158.
249+ */
250+extern const char *CFCHECKPT;

jkczyz commented at 6:41 pm on May 6, 2020:

Update allNetMessageTypes as per namespace comment.

jnewbery commented at 4:18 pm on May 7, 2020:

done

in src/net_processing.cpp:1999 in bd1a924b34 outdated

1994+    if (!supported_filter_type) {
1995+        LogPrint(BCLog::NET, "peer %d requested unknown block filter type: %d\n",
1996+                 pfrom->GetId(), static_cast<uint8_t>(filter_type));
1997+        pfrom->fDisconnect = true;
1998+        return false;
1999+    }

jkczyz commented at 6:59 pm on May 6, 2020:

Might be worth separating these checks. Otherwise, the logging is not quite accurate when -peercfilters is not set.

jnewbery commented at 4:22 pm on May 7, 2020:

I think changing the log text to “peer x requested unsupported block filter type” addresses your concern (either -peercfilters is unset and they’ve requested any block filter type, or -peercfilters is set and they’ve requested a block filter type that isn’t basic).

Later on we’ll switch this logic to use GetLocalServices instead: https://github.com/bitcoin/bitcoin/pull/18876/commits/5c23d400fec93c1a52e42a6d0c08f7164bbb268b#diff-eff7adeaec73a769788bb78858815c91R2038.

in src/net_processing.cpp:1989 in bd1a924b34 outdated

1984+ */
1985+static bool PrepareBlockFilterRequest(CNode* pfrom, const CChainParams& chain_params,
1986+                                      BlockFilterType filter_type,
1987+                                      const uint256& stop_hash,
1988+                                      const CBlockIndex*& stop_index,
1989+                                      BlockFilterIndex*& filter_index)

jkczyz commented at 7:12 pm on May 6, 2020:

Also use const here for consistency with stop_index?

jnewbery commented at 4:23 pm on May 7, 2020:

done

in src/net_processing.cpp:2056 in bd1a924b34 outdated

2051+
2052+    // Populate headers.
2053+    const CBlockIndex* block_index = stop_index;
2054+    for (int i = headers.size() - 1; i >= 0; i--) {
2055+        uint32_t height = (i + 1) * CFCHECKPT_INTERVAL;
2056+        block_index = block_index->GetAncestor(height);

jkczyz commented at 7:36 pm on May 6, 2020:

Given block heights are of type int in CBlockIndex, perhaps height and CFCHECKPT_INTERVAL should be int as well instead of uint32_t.

jnewbery commented at 4:23 pm on May 7, 2020:

done

in src/net_processing.cpp:2024 in bd1a924b34 outdated

2019+
2020+    return true;
2021+}
2022+
2023+/**
2024+ * Handle a cfcheckpt request.

jkczyz commented at 8:01 pm on May 6, 2020:

s/cfcheckpt/getcfcheckpt

jnewbery commented at 4:23 pm on May 7, 2020:

done

jkczyz commented at 0:09 am on May 7, 2020: contributor

Concept ACK

in src/net_processing.cpp:2006 in bd1a924b34 outdated

2001+    {
2002+        LOCK(cs_main);
2003+        stop_index = LookupBlockIndex(stop_hash);
2004+
2005+        // Check that the stop block exists and was at some point connected to the active chain.
2006+        if (!stop_index || !BlockRequestAllowed(stop_index, chain_params.GetConsensus())) {

ariard commented at 8:29 am on May 7, 2020:

Is the following scenario plausible ? Client receives from peers block H1 as tip and goes offline. H1 is reorg’ed out of the active chain. After a month, Client goes back and try to start filter-sync through checkpoints. Sending H1 will be rejected as per BlockRequestAllowed we reject reorg’ed out block older than a month, but has been part of the active chain and dutifully announced.

It maybe a marginal issue, but at least comment should mention that stop block should have been connected to the active chain in the last month.

jnewbery commented at 4:51 pm on May 7, 2020:

Yes, that’s possible. A client should always request the filter headers from a recently sync’ed block headers chain.

I was going to update the comment to say something like “Check that the stop block exists and is either in the best chain or was re-orged out less than a month ago”, but I don’t like repeating program logic in a comment, especially if it’s located far from the code, since any changes in the logic would make the comment obsolete. Instead I’ve changed it to “Check that the stop block exists and the peer would be allowed to fetch it.” Anyone reading the code can look up BlockRequestAllowed() for the exact conditions.

in test/functional/p2p_cfilters.py:24 in fccc85728d outdated

19+    connect_nodes,
20+    disconnect_nodes,
21+    wait_until,
22+)
23+
24+class CompactFiltersTest(BitcoinTestFramework):

ariard commented at 8:51 am on May 7, 2020:

Is this test verify the following spec requirement “StopHash MUST be known to belong to a block accepted by the receiving peer. This is the case if the peer had previously sent a headers or inv message with any descendent blocks” ?

This point is unclear to me with regards to the receiver requirement, what should we do in case of peer asking for a StopHash non-yet-announced to them. Do we check StopHash inferior ou equal to its pindexBestHeaderSent somewhere ?

jnewbery commented at 4:48 pm on May 7, 2020:

I don’t think it matters. If a BIP 157 client pre-emptively requests a filter header for a block that the server hasn’t yet accepted, then they would be disconnected.

Generally we don’t consider block propagation timing to be a privacy/topology leak, since new block events are rare and too expensive to be controlled by an observer.

Requesting a block that the server hasn’t accepted yet is logically equivalent to requesting a non-existent block, which is tested in p2p_cfilters.

ariard commented at 7:25 am on May 8, 2020:

Generally we don’t consider block propagation timing to be a privacy/topology leak, since new block events are rare and too expensive to be controlled by an observer.

I think relying on event scarcity in case of block variance may not hold, so I would rather consider the heavily-optimized block propagation as an obstruction for an attacker to fingerprint topology with confidence.

Requesting a block that the server hasn’t accepted yet is logically equivalent to requesting a non-existent block, which is tested in p2p_cfilters.

I think there is logically a difference between accepting a block and header announcement, but AFAIK there is no types of connections for which we don’t announce headers so doesn’t make a difference in practice, I agree.

in src/net_processing.cpp:1993 in bd1a924b34 outdated

1988+                                      const CBlockIndex*& stop_index,
1989+                                      BlockFilterIndex*& filter_index)
1990+{
1991+    const bool supported_filter_type =
1992+        (filter_type == BlockFilterType::BASIC &&
1993+         gArgs.GetBoolArg("-peercfilters", DEFAULT_PEERCFILTERS));

fjahr commented at 3:53 pm on May 7, 2020:

If -peercfilters isn’t set, the resulting error message below would be confusing I think.

fjahr commented at 4:58 pm on May 7, 2020:

i.e. I agree with what @jkczyz was saying earlier I think but just saw you addressed it now

jnewbery force-pushed on May 7, 2020

jnewbery commented at 4:00 pm on May 7, 2020: member

Rebased on master to fix the spurious travis failures, and addressed @jkczyz’s review comments. Thanks for the review!

in src/init.cpp:998 in 48bc8ff2ca outdated

994@@ -994,6 +995,12 @@ bool AppInitParameterInteraction()
995         }
996     }
997 
998+    // Basic filters index must be enabled to serve compact filters

fjahr commented at 4:29 pm on May 7, 2020:

nit:

0    // Basic filters index must be enabled to serve compact filters
1    // because it is the only available filter option for now

jnewbery commented at 1:32 am on May 8, 2020:

done

jnewbery force-pushed on May 7, 2020

jnewbery commented at 4:51 pm on May 7, 2020: member

Addressed @ariard’s review comments.

fjahr commented at 4:56 pm on May 7, 2020: member

Concept ACK

in src/net_processing.cpp:2050 in 2a15f9943c outdated

2045+    if (!PrepareBlockFilterRequest(pfrom, chain_params, filter_type, stop_hash,
2046+                                   stop_index, filter_index)) {
2047+        return;
2048+    }
2049+
2050+    std::vector<uint256> headers(stop_index->nHeight / CFCHECKPT_INTERVAL);

jonatack commented at 5:39 pm on May 7, 2020:

CFCHECKPT_INTERVAL is set to 1000, but perhaps add a static assert sanity check against division by zero (as well as against a height of zero on line 2055).

0+    static_assert(CFCHECKPT_INTERVAL !=0);
1     std::vector<uint256> headers(stop_index->nHeight / CFCHECKPT_INTERVAL);

jnewbery commented at 1:34 am on May 8, 2020:

I’ll pass on this one. No-one is ever going to change the constant to be zero.

jonatack commented at 8:02 am on May 8, 2020:

I’ll pass on this one. No-one is ever going to change the constant to be zero.

Heh, this is 2020, I don’t count on anything to be sane anymore :)

in test/functional/p2p_cfilters.py:59 in 2a15f9943c outdated

54+        request = msg_getcfcheckpt(
55+            filter_type=FILTER_TYPE_BASIC,
56+            stop_hash=int(stale_block_hash, 16)
57+        )
58+        node0.send_message(request)
59+        node0.sync_with_ping(timeout=5)

jonatack commented at 5:48 pm on May 7, 2020:

Current idiom in the test suite:

0-        node0.send_message(request)
1-        node0.sync_with_ping(timeout=5)
2+        node0.send_and_ping(message=request, timeout=5)

jnewbery commented at 1:35 am on May 8, 2020:

done

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

in src/net_processing.cpp:1986 in 2a15f9943c outdated

1981+ * @param[out]  stop_index      The CBlockIndex for the stop_hash block, if the request can be serviced.
1982+ * @param[out]  filter_index    The filter index, if the request can be serviced.
1983+ * @return                      True if the request can be serviced.
1984+ */
1985+static bool PrepareBlockFilterRequest(CNode* pfrom, const CChainParams& chain_params,
1986+                                      BlockFilterType filter_type,

jonatack commented at 8:27 pm on May 7, 2020:

Would it be preferable to reference to const here?

0-                                      BlockFilterType filter_type,
1+                                      const BlockFilterType& filter_type,

jnewbery commented at 1:38 am on May 8, 2020:

No. The enum is a uint8_t. Passing an int is more efficient than passing a pointer (see eg https://stackoverflow.com/questions/3009543/passing-integers-as-constant-references-versus-copying)

jonatack commented at 7:54 am on May 8, 2020:

Ugh, sorry John, I’ve been indoors for too long, for some reason I misread the BlockFilterType. Yes, an integer should be passed by value.

in test/functional/p2p_cfilters.py:79 in 2a15f9943c outdated

74+        request = msg_getcfcheckpt(
75+            filter_type=FILTER_TYPE_BASIC,
76+            stop_hash=int(tip_hash, 16)
77+        )
78+        node0.send_message(request)
79+        node0.sync_with_ping()

jonatack commented at 8:58 pm on May 7, 2020:

idem

0-        node0.send_message(request)
1-        node0.sync_with_ping()
2+        node0.send_and_ping(request)

in test/functional/p2p_cfilters.py:97 in 2a15f9943c outdated

92+        request = msg_getcfcheckpt(
93+            filter_type=FILTER_TYPE_BASIC,
94+            stop_hash=int(stale_block_hash, 16)
95+        )
96+        node0.send_message(request)
97+        node0.sync_with_ping()

jonatack commented at 8:58 pm on May 7, 2020:

and here as well

0-        node0.send_message(request)
1-        node0.sync_with_ping()
2+        node0.send_and_ping(request)

jnewbery commented at 1:36 am on May 8, 2020:

done

jonatack commented at 9:15 pm on May 7, 2020: member

Code review, built/ran tests, and running a mainnet node with -peercfilters=1 -blockfilterindex=1… which took a bit more than an hour to build ~./bitcoin/indexes/blockfilter/basic for the first time, which at current tip height of 629407 takes 5.0 GB of disk space – not strictly related to this PR, but I wanted to test it. Verified the behavior for “Error: Cannot set -peercfilters without -blockfilterindex.” A few non-blocking comments follow below.

in test/functional/p2p_cfilters.py:80 in 2a15f9943c outdated

77+        )
78+        node0.send_message(request)
79+        node0.sync_with_ping()
80+        response = node0.last_message['cfcheckpt']
81+        assert_equal(response.filter_type, request.filter_type)
82+        assert_equal(response.stop_hash, request.stop_hash)

fjahr commented at 10:21 pm on May 7, 2020:

0        assert_equal(response.stop_hash, request.stop_hash)
1        assert_equal(len(response.headers), 2)

jnewbery commented at 1:41 am on May 8, 2020:

That’ll be caught by the assert_equal(response.headers, [int(header, 16) for header in (main_cfcheckpt, tip_cfcheckpt)]) below

fjahr approved

fjahr commented at 10:33 pm on May 7, 2020: member

ACK fccc85728deec37acfb3d325409a9397b9397834

Reviewed code, ran tests, and modified them to see them fail.

Had another nit. And this is also not a blocker for me but I would like to repeat my comment from the PR Review Club that I would prefer that we have a consistent name for this feature for user-facing communication. The connection between -blockfilterindex and -peercfilters does not seem obvious. I am happy with either naming scheme as long as it’s consistent.

jnewbery force-pushed on May 8, 2020

jnewbery commented at 2:33 am on May 8, 2020: member

Addressed all review comments from @fjahr and @jonatack . Thanks for the review!

ariard commented at 7:30 am on May 8, 2020: member

Code Review ACK 6691493, changes since last review are switch to int instead of uint32_t, renaming to peerblockfilters, test modifications, better comments.

in test/functional/p2p_cfilters.py:27 in 6691493340 outdated

22+)
23+
24+class CompactFiltersTest(BitcoinTestFramework):
25+    def set_test_params(self):
26+        self.setup_clean_chain = True
27+        self.rpc_timeout = 480

jonatack commented at 8:40 am on May 8, 2020:

Is this line needed nowadays? I don’t think any other functional p2p_* tests use rpc_timeout.

jnewbery commented at 12:56 pm on May 8, 2020:

This was needed to prevent the test from timing out when run under valgrind. Since yesterday, we no longer run functional tests under valgrind on every travis run, but I’d prefer not to keep changing this. If people run this test under valgrind locally, they’d need to add this back.

in src/init.cpp:1000 in 6691493340 outdated

 994@@ -994,6 +995,13 @@ bool AppInitParameterInteraction()
 995         }
 996     }
 997 
 998+    // Basic filters are the only supported filters. The basic filters index must be enabled
 999+    // to serve compact filters
1000+    if (gArgs.GetBoolArg("-peerblockfilters", DEFAULT_PEERCFILTERS) &&

jonatack commented at 8:47 am on May 8, 2020:

Per the change to -peerblockfilters, consider renaming DEFAULT_PEERCFILTERS to DEFAULT_PEERBLOCKFILTERS as well.

0src/init.cpp:449:    gArgs.AddArg("-peerblockfilters", strprintf("Serve compact block filters to peers per BIP 157 (default: %u)", DEFAULT_PEERCFILTERS), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
1src/init.cpp:1000:    if (gArgs.GetBoolArg("-peerblockfilters", DEFAULT_PEERCFILTERS) &&
2src/net_processing.cpp:1993:         gArgs.GetBoolArg("-peerblockfilters", DEFAULT_PEERCFILTERS));
3src/net_processing.h:24:static const bool DEFAULT_PEERCFILTERS = false;

jnewbery commented at 1:01 pm on May 8, 2020:

done

in test/functional/test_runner.py:228 in 6691493340 outdated

224@@ -225,6 +225,7 @@
225     'feature_loadblock.py',
226     'p2p_dos_header_tree.py',
227     'p2p_unrequested_blocks.py',
228+    'p2p_cfilters.py',

jonatack commented at 8:50 am on May 8, 2020:

After the change to -peerblockfilters, should the test be renamed as well? e.g. p2p_peerblockfilters.py

jnewbery commented at 1:01 pm on May 8, 2020:

done

jonatack commented at 8:54 am on May 8, 2020: member

Re-built and tested successfully. A few comments following the change from -peercfilters to -peerblockfilters.

jnewbery commented at 1:02 pm on May 8, 2020: member

Thanks for review @jonatack . I’ve addressed all your comments.

jkczyz commented at 2:59 pm on May 8, 2020: contributor

Thanks for review @jonatack . I’ve addressed all your comments.

Needs push. Also, please update the flag name in commit messages.

jnewbery force-pushed on May 8, 2020

jnewbery commented at 3:27 pm on May 8, 2020: member

Needs push. Also, please update the flag name in commit messages.

done and done!

fjahr commented at 5:01 pm on May 8, 2020: member

re-ACK 967e2b10a51b831e49abd9171e3f05843e00e76b

Changes addressed nits discussed in the comments. Tests are green for me locally.

jkczyz commented at 5:57 pm on May 8, 2020: contributor

ACK 967e2b10a51b831e49abd9171e3f05843e00e76b

Comments addressed and tests pass.

jonatack commented at 6:19 pm on May 8, 2020: member

Code review ACK 967e2b10a51b831e49abd9171e3f05843e00e76b. I’m still thinking about the long-term consequences of adding BIP157 support but so far these changes seem fine. Per git diff 6691493 967e2b1 the only change since my previous tested review was addressing my renaming comments.

There may be a update needed for #16224. Appveyor is signalling init.cpp(1001,97): error C2664: ‘bool InitError(const bilingual_str &)’: cannot convert argument 1 from ‘std::string’ to ‘const bilingual_str &’.

[init] Add -peerblockfilters option

When a node is configured with --blockfilterindex=basic and
-peerblockfilters it can serve compact block filters to its peers.

This commit adds the configuration option handling. Future commits
add compact block serving and service bits signaling.

9ccaaba11e

[net processing] Message handling for getcfcheckpt.

If -peerblockfilters is configured, handle requests for cfcheckpt.

f9e00bb25a

[test] Add test for cfcheckpt 23083856a5

jnewbery force-pushed on May 8, 2020

jonatack commented at 4:07 pm on May 9, 2020: member

Code review re-ACK 23083856a551ca13e8b142791c296ecb25cc4e7f the only change since my review @ 967e2b1 is an update required for #16224 that was merged yesterday.

0--- a/src/init.cpp
1+++ b/src/init.cpp
2@@ -999,22 +998,22 @@ bool AppInitParameterInteraction()
3     // to serve compact filters
4     if (gArgs.GetBoolArg("-peerblockfilters", DEFAULT_PEERBLOCKFILTERS) &&
5         g_enabled_filter_types.count(BlockFilterType::BASIC) != 1) {
6-        return InitError(_("Cannot set -peerblockfilters without -blockfilterindex.").translated);
7+        return InitError(_("Cannot set -peerblockfilters without -blockfilterindex."));
8     }

fjahr commented at 1:25 pm on May 10, 2020: member

re-ACK 23083856a551ca13e8b142791c296ecb25cc4e7f

Only change was fixing merge conflict in init.cpp:

0$ git range-diff f54753293fe7355e4280944d766f22054b560ba1..967e2b10a51b831e49abd9171e3f05843e00e76b 5b24f6084ede92d0f493ff416b4726245140b2c1..23083856a551ca13e8b142791c296ecb25cc4e7f

jkczyz commented at 6:03 pm on May 10, 2020: contributor

re-ACK 23083856a551ca13e8b142791c296ecb25cc4e7f

Only change required in init.cpp for #16224.

ariard commented at 8:11 am on May 11, 2020: member

re-Code Review ACK 2308385

luke-jr commented at 4:07 pm on May 11, 2020: member

This remains Concept NACK since it is harmful to Bitcoin and should not be merged, no matter how well-reviewed.

NicolasDorier commented at 4:27 pm on May 11, 2020: contributor

I also believe neutrino is harmful to bitcoin https://medium.com/@nicolasdorier/neutrino-is-dangerous-for-my-self-sovereignty-18fac5bcdc25

However, I believe it can be useful to whitelisted peers. (I think you should add a permission flag for that)

Concept NACK, except if protected by a permission flag to force its use only to specific trusted peers.

ThomasBucaioni commented at 5:03 pm on May 11, 2020: none

All three test suites passed here.

clarkmoody commented at 9:54 pm on May 11, 2020: none

Tested ACK 23083856a

theStack approved

theStack commented at 10:28 am on May 12, 2020: member

ACK https://github.com/bitcoin/bitcoin/commit/23083856a551ca13e8b142791c296ecb25cc4e7f

jnewbery commented at 12:45 pm on May 12, 2020: member

This PR now has 6 ACKs:

The PR that this is a subset of also has an ACK:

@MarcoFalke (https://github.com/bitcoin/bitcoin/pull/16442#issuecomment-623037890)

And there are 6 additional Concept ACKs for the change:

That’s 13 ACKs/Concept ACKs. There does appear to be widespread support to move forwards with this. @luke-jr @NicolasDorier - your principled objection to BIP 157 has been heard and understood. Thank you for your input!

MarcoFalke commented at 12:57 pm on May 12, 2020: member

re-ACK 23083856a5 🌳

Only change since my previous review #18876 (comment)

Rename setting
Small code and test refactoring

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3re-ACK 23083856a5 🌳
 4
 5Only change since my previous review [#18876 (comment)](/bitcoin-bitcoin/18876/#issuecomment-627322015)
 6
 7* Rename setting
 8* Small code and test refactoring
 9-----BEGIN PGP SIGNATURE-----
10
11iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
12pUgPkAv/fzn4E/8T8kcqmj3aMGV6rY1b0Kg0cqRZKVMrfapyWIJxuQ0+WiB7Leyl
13TbKLG8hXCWHy/GNdtkJjhXKtQDH4NA/m+jlIhTMIYCsi1YZ9IPzKLPm9NvuEQSp0
14MRmfkDFH/RMIgxSDROm96+StbWlunDDjXvy7recX7hLJ1pMAfufEiyVbqDHwuCjw
15K9lG2YbdBWwUxo6zP1iQkBV+DwTl0GNuEtF47e6oQUozAWNosBw0efBEDGNN41c6
16Li1aPBXnZUHd/Uwioxz0Ar0+N06VWz0b5Uz4NDOK//Bqu4sf3djZY4WEF/TdlrSs
17sXy8LIhyL3uCnSWKUL2L4zoKkYjImJthLk8MIdcFnFLotQO/Za1yDZ/ur/yKI7ee
18bYqLh7vbW8oupsKigJr25jtPZY1+p5EmdeFh+/4JYsSG/Da5gGryQ+AcWAi9Hk1/
197U4XhAwiwyLEvadJpPJGuCuFKZXl89KCCKKTY8x/nuNzWuvzSNqv183muNIhrCqk
20IIprWPI3
21=Dnd8
22-----END PGP SIGNATURE-----

Timestamp of file with hash 286f8e8c18188195dc7b2da1cd9ec5c45e6b4ff0cfa7173476f9af073c383815 -

MarcoFalke commented at 1:02 pm on May 12, 2020: member

Concept NACK, except if protected by a permission flag to force its use only to specific trusted peers.

The implementation here is incomplete. The follow-ups in #18876 need to be merged as well. And if another setting is seen to be needed for this to be safely shipped, this should also be done in a follow-up. However, I think we shouldn’t block progress here based on Bitcoin Core related implementation details that can easily be fixed up later. There is enough time until the release to hash this out. (See #18947) If by then the implementation is still incomplete, it should be reverted.

Given that, I think this is ready for merge, both code-wise and conceptually.

MarcoFalke merged this on May 12, 2020

MarcoFalke closed this on May 12, 2020

jnewbery deleted the branch on May 12, 2020

sidhujag referenced this in commit f21e1a5217 on May 12, 2020

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

jonasschnelli commented at 1:08 pm on May 13, 2020: contributor

post merge concept ACK.

However, I think this merge was too fast. A 6 day window for a first concrete step in filter serving after BIP157 is IMO too fast for a reasonable discussion. I also still miss conceptual reviews from long term p2p contributors. There is no rush IMO.

Edit: I probably underestimated the effort that went into #16442 (this PR is mostly recycled code from 16442) which could legitimate a quicker merge.

MarcoFalke commented at 4:36 pm on May 14, 2020: member

Edit: I probably underestimated the effort that went into #16442 (this PR is mostly recycled code from 16442) which could legitimate a quicker merge.

Thanks. Just wanted to reply that.

For reference, the permission flag pr is here, btw: #18972

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

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

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

jasonbcox referenced this in commit 3ca37803e0 on Nov 19, 2020

kittywhiskers referenced this in commit 3ad2028e25 on Aug 22, 2021

kittywhiskers referenced this in commit 7f683d9e9d on Aug 22, 2021

kittywhiskers referenced this in commit a82c6151ea on Sep 16, 2021

kittywhiskers referenced this in commit 8ce75439ab on Sep 18, 2021

kittywhiskers referenced this in commit 5b22d6d0ac on Sep 19, 2021

UdjinM6 referenced this in commit 4ffd42de63 on Sep 21, 2021

thelazier referenced this in commit de1edb58c7 on Sep 25, 2021

kittywhiskers referenced this in commit d34285e77f on Oct 12, 2021

gades referenced this in commit 3eaf66dfb3 on May 30, 2022

DrahtBot locked this on Aug 16, 2022

Serve cfcheckpt requests #18877

Conflicts