Feat/mempool sync #2884

jcnelson · 2021-10-16T02:41:15Z

This PR implements an anti-entropy protocol for querying transactions from other nodes' mempools, based on https://docs.google.com/document/d/1uHLUZEkzJJA8HtKfKVZmZIn5n7sY0X9j6gg90IyixYM/edit?usp=sharing.

It does the following:

Adds a bloom filter and counting bloom filter implementation. Importantly, the counting bloom filter is (a) disk-backed -- specifically, backed by a Sqlite blob -- and (b) can be converted into a bloom filter for issuing queries to other nodes.
Adds POST /v2/mempool/query, which takes a MemPoolSyncData structure as input and streams back zero or more recently-arrived transactions from the mempool (as determined by block height).
Adds a state machine to the main P2P state machine, which will cause a node to periodically ask a randomly-chosen outbound neighbor for any transactions in its mempool that it does not have locally.

The protocol in this PR is a simple set-reconciliation protocol. A client node will periodically ask a peer node for the list of recent transactions that it does not have in its own mempool. It does this by sending the remote peer a sketch of its mempool, which the remote peer uses to iterate through its set of recent transactions and stream back transactions it has that are not in the sketch.

A MemPoolSyncData structure encodes this sketch. Specifically, it encodes either a list of 8-byte transaction ID prefixes ("tags"), or a bloom filter with a protocol-defined error rate and size. The /v2/mempool/query handler will handle either variant; the client uses the variant that is cheaper for it to generate with an acceptable error rate.

The MemPoolSyncData represents a sketch calculated from node-specific transaction tags. These are calculated as siphash(node_seed, txid), which is an 8-byte value. The node_seed is a node-specific random 32-byte value. The reason for doing this is to ensure that a transaction has a different identifier for each node, so that even if a transaction is "masked" in one node's sketch via a false positive, it will almost certainly not be masked in other nodes' sketches. It also ensures that no one can create a "malicious" transaction whose txid masks another txid in all nodes' mempools. Note that this is an important anti-censorship tactic, because if the remote peer sees a positive match for a transaction in the client peer's MemPoolSyncData, it will not serve that transaction (even if it's a false positive).

Nodes can individually decide how many transactions they'll reply in a mempool query (the maximum is 8192, which is a protocol constant). To make sure that different nodes serve different transactions even though they can serve different numbers of transactions, a node iterates through transactions in the sketch in a deterministic random order by using its own sketch seed to calculate a random permutation of transactions to consider. Specifically, each txid in the mempool has a randomized_txid (i.e. derived as siphash(node_seed, txid)), and transactions are iterated through in randomized_txid order. This way, a heavily-loaded node can get away with serving fewer than the maximum transactions, and downstream client peers are still just as likely to receive a transaction as any other.

Given that everyone's attention is currently on SIP-012, I'm leaving this in draft status for now. I'd like to leave it running in production for a while to verify that a set of nodes reliably keep their mempools in sync.

…ementation, for use in mempool synchronization. Importantly, the disk-backed counting bloom filter lives in a sqlite3 table and maintains its buckets efficiently (i.e. in a blob), and can be converted into a bloom filter.

…h functions in the bloom filter. It used to be in std::hash, and still is, but is deprecated. So, just require it directly.

…he mempool

…last few blocks' worth of transactions. Whenever a transaction is accepted, it's added to the bloom filter. Whenever a transaction arrives for a never-before-seen chain height, it removes the last-recent transactions from the counting bloom filter. In addition, this patch extends the mempool to paginate through the mempool's recent transactions in a deterministic but random order (that is node-specific), as part of the ability to stream transactions out of the mempool in response to a mempool query. Due to the size of a bloom filter query, this code also handles direct requests for transactions as a list of 8-byte prefixes. In both cases, transactions are queried by taking the siphash over the node-specific seed and the txid, so that different nodes will put the same transaction into different buckets (so someone who wanted to induce false positives in one node's bloom filter would be unable to do so for other nodes)

…data

… supporting the new mempool query interface

…a list of transaction tags (8-byte prefixes), or a bloom filter with a node-specific initial hash state

…ynchronize the node's mempool, and how long it's allowed to last

…ort mempool sync

…e mempool

…empool/query

…ol-downloaded transactions are propagated to the relayer through the NetworkResult

…he p2p state machine

…lready evicted

…ed and also shorten it

…oom_state() helper method to safely run code on the inner bloom counter state; use better SQL queries; fix bug in tx streaming code to avoid sending more than `count` bytes; move tests to their own module

…ync request codecs

jcnelson · 2021-12-17T16:26:51Z

@kantai Thanks for all your comments! I think I've addressed them all now.

…pool state transition loop for advancing the state machine and recording data obtained by state transition functions; refactor state-transition functions to simply return new data instead of trying to store it in the PeerNetwork themselves; put all mempool_sync_reset() calls into the top-level mempool state machine loop; run mempool state machine in parallel to the main state machine so we don't stall the latter.

…ions

gregorycoppola

Again, empirical data looks good.

Are there any integration tests being checked in for this?

gregorycoppola · 2021-12-17T21:20:53Z

src/net/rpc.rs

+
+    #[test]
+    #[ignore]
+    fn test_rpc_mempool_query_bloom() {


what is this testing?

gregorycoppola · 2021-12-17T21:21:05Z

src/net/rpc.rs

+                let req_md = http_request.metadata().clone();
+                println!("{:?}", http_response);
+                match http_response {
+                    HttpResponseType::MemPoolTxs(_, txs) => {


why is this the answer?

gregorycoppola · 2021-12-17T21:23:41Z

src/util/bloom.rs

@@ -0,0 +1,1016 @@
+// Copyright (C) 2013-2020 Blockstack PBC, a public benefit corporation


https://app.codecov.io/gh/blockstack/stacks-blockchain/compare/2884/tree/src/util/bloom.rs

Code coverage is pretty good for this file.

BitField::clear isn't tested and some error conditions aren't tested. Might as well add a test for clear while you're in the area, up to you about error conditions.

kantai

This looks good to me, my only remaining comment is (possibly bikeshedding?) about the name of stream_transactions. I do feel pretty strongly that it needs to have more details in its rustdoc at a minimum, and really should be called something other than stream_transactions.

pavitthrap

lgtm, just a few minor comments

pavitthrap · 2021-12-23T19:59:56Z

src/net/rpc.rs

@@ -3489,6 +3543,63 @@ mod test {
        peer_1.sortdb = Some(sortdb1);
        peer_2.sortdb = Some(sortdb2);

+        // stuff some transactions into peer_2's mempool


Could add a comment that this relates to the test test_rpc_mempool_query_txtags and test_rpc_mempool_query_bloom

pavitthrap · 2022-01-04T18:40:11Z

src/net/p2p.rs

@@ -2109,6 +2142,42 @@ impl PeerNetwork {
        Ok(done)
    }

+    /// Do a mempool sync. Return any transactions we might receive.
+    /// Return true if we finish the sync.


Update comment here perhaps - function does not return true

pavitthrap · 2022-01-04T20:07:56Z

src/net/p2p.rs

+                        (false, Some(url)) => {
+                            // success! can advance
+                            self.mempool_sync_data_url = Some(url);
+                            self.mempool_state = MempoolSyncState::ResolveURL;


Would it make sense to store the url with the resolve URL state itself? Like MempoolSyncState::ResolveURL(url). That way we can also remove the logic in the ResolveURL state for the case were mempool_sync_data_url is None.
Similar suggestion for the SendQuery and RecvResponse state.

Sounds like a good idea. Done.

pavitthrap · 2022-01-04T23:13:42Z

src/core/mempool.rs

+    /// Make a mempool sync request.
+    /// If sufficiently sparse, use a MemPoolSyncData::TxTags variant
+    /// Otherwise, use a MemPoolSyncData::BloomFilter variant
+    /// If force_bloom_filter is true, then always make a bloom filter.  The reason for doin this


force_bloom_filter is not used in this function

pavitthrap · 2022-01-04T23:22:45Z

src/core/mempool.rs

+        let mut tags_table = HashSet::new();
+        if let MemPoolSyncData::TxTags(_, ref tags) = data {
+            for tag in tags.iter() {
+                tags_table.insert(tag.clone());
+            }
+        }


Should we precompute this set since we may call this function multiple times in a row?

The size of tags is small enough in practice that it shouldn't matter.

pavitthrap · 2022-01-05T17:50:02Z

src/core/tests/mod.rs

+                    for txid in txids.iter() {
+                        if !recent_set.contains(&txid) && bf.contains_raw(&txid.0) {
+                            fp_count += 1;
+                        }
+                        if bf.contains_raw(&txid.0) {
+                            present_count += 1;
+                        } else {
+                            absent_count += 1;
+                        }
+                    }


Why is this logic not in the branch for TxTags?

This code path is verifying that the bloom filter's false positive rate is sufficiently small. This does not apply to TxTags, since the TxTags query variant does not use a bloom filter.

pavitthrap · 2022-01-05T17:51:33Z

src/core/tests/mod.rs

+            let recent_txids = mempool.get_bloom_txids().unwrap();
+            if recent_txids.len() < (present_count + absent_count) as usize {
+                nonrecent_fp_rate = (fp_count as f64)
+                    / ((present_count + absent_count - (recent_txids.len() as u32)) as f64);


Could be easier to parse this statement if present_count + absent_count was replaced with txids.len()

No, this is deliberate -- we don't care to test the code in the if-block if recent_txids.len() < (present_count + absent_count) as usize { /* ... */ } if the query is a TxTags variant.

…ants, so we don't have to keep track of it in the PeerNetwork

…sting mempool queries

blockstack-devops · 2024-11-19T00:22:23Z

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

jcnelson added 19 commits October 15, 2021 21:25

feat: add siphasher dependency from rust. This gets used to build has…

279b6c1

…h functions in the bloom filter. It used to be in std::hash, and still is, but is deprecated. So, just require it directly.

feat: extend BlockStreamData to be able to stream transactions from t…

b9332c5

…he mempool

feat: add transaction streaming data to BlockStreamData

25cf919

refactor: expose BITVEC_LEN macro globally

c8f5575

refactor: add siphasher package

470aa8e

refactor: mempool handle is now required for calls that could stream …

03beaee

…data

feat: have ConversationP2P check whether or not a remote peer reports…

ba85c74

… supporting the new mempool query interface

feat: add codec implementations for mempool query payloads -- either …

de153d0

…a list of transaction tags (8-byte prefixes), or a bloom filter with a node-specific initial hash state

feat: expose connection settings to control how often to attempt to s…

f8c2045

…ynchronize the node's mempool, and how long it's allowed to last

feat: new nodes will report in their services bitfield that they supp…

ad1fe62

…ort mempool sync

refactor: anything that could stream http data now needs access to th…

4bf161a

…e mempool

feat: add support for parsing mempool queries and responses for /v2/m…

7b8efb5

…empool/query

feat: add mempool query and response types, and make it so that mempo…

44baad6

…ol-downloaded transactions are propagated to the relayer through the NetworkResult

feat: add a mempool sync state machine as a distinct work state for t…

8a29773

…he p2p state machine

feat: add server support for handling /v2/mempool/query

9552eda

refactor: streaming data now requires access to the mempool

048628d

refactor: add bloom module

56a2cc2

jcnelson requested review from reedrosenbluth, kantai, pavitthrap and gregorycoppola October 16, 2021 02:41

jcnelson added 7 commits October 16, 2021 15:51

fix: add siphasher dependency to clarity

7463145

fix: make it so streaming test doesn't trample on other tests

1cb5907

fix: expect new service flags in localpeer db test

e8d90fe

fix: remove tini and add siphasher

66ff218

fix: only prune transactions from the bloom counter if they weren't a…

a3800ff

…lready evicted

fix: mempool bloom counter torture test takes too long; mark as ignor…

7827374

…ed and also shorten it

fix: I can't spell "ignore"

9492324

jcnelson added 11 commits December 15, 2021 16:05

Merge branch 'develop' into feat/mempool-sync

6be3b67

refactor: last_txid --> last_randomized_txid

373dd26

refactor: last_txid --> last_randomized_txid

731b289

refactor: put bloom filter update methods into MemPoolTx; add with_bl…

16a4c65

…oom_state() helper method to safely run code on the inner bloom counter state; use better SQL queries; fix bug in tx streaming code to avoid sending more than `count` bytes; move tests to their own module

refactor: move all mempool tests to their own file

50c0290

refactor: expose tests module for mempool

4209017

refactor: use structured logging on handshake accept

e5c5d5c

refactor: use define_u8_enum! macro and use its methods for mempool s…

68419d5

…ync request codecs

docs: add rustdocs for new mempool connection options

91f0982

refactor: use structured logging in net db setup

e232d7a

refactor: use define_u8_enum! macro for mempool request message types

2a28f4e

jcnelson requested a review from kantai December 17, 2021 16:26

docs: remove needless comment on alternative bloom filter implementat…

84800d4

…ions

gregorycoppola reviewed Dec 17, 2021

View reviewed changes

kantai approved these changes Dec 20, 2021

View reviewed changes

314159265359879 mentioned this pull request Dec 21, 2021

Stacks' Congestion is Halting the Network #2980

Closed

pavitthrap approved these changes Jan 5, 2022

View reviewed changes

jcnelson added 5 commits January 5, 2022 20:41

fix: update comments to reflect code behavior

ceae2f5

refactor: keep mempool state-machine sync state within the state vari…

e3b9183

…ants, so we don't have to keep track of it in the PeerNetwork

chore: comment that transaction creation in the boilerplate is for te…

b7c77e9

…sting mempool queries

Merge branch 'develop' into feat/mempool-sync

dbf910c

fix: compile error in mempool rpc unit tests from merge

8de211e

jcnelson merged commit fd3f28d into develop Jan 6, 2022

gregorycoppola mentioned this pull request Jan 14, 2022

feat: Mempool Analysis Suite #3002

Closed

diwakergupta mentioned this pull request Feb 1, 2022

Networking: mempool anti-entropy protocol #2193

Closed

blockstack-devops added the locked label Nov 19, 2024

stacks-network locked as resolved and limited conversation to collaborators Nov 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/mempool sync #2884

Feat/mempool sync #2884

jcnelson commented Oct 16, 2021 •

edited

Loading

jcnelson commented Dec 17, 2021

gregorycoppola left a comment

gregorycoppola Dec 17, 2021

gregorycoppola Dec 17, 2021

gregorycoppola Dec 17, 2021

kantai left a comment

pavitthrap left a comment

pavitthrap Dec 23, 2021

jcnelson Jan 5, 2022

pavitthrap Jan 4, 2022

jcnelson Jan 5, 2022

pavitthrap Jan 4, 2022

jcnelson Jan 5, 2022

pavitthrap Jan 4, 2022

jcnelson Jan 5, 2022

pavitthrap Jan 4, 2022

jcnelson Jan 5, 2022

pavitthrap Jan 5, 2022

jcnelson Jan 5, 2022

pavitthrap Jan 5, 2022

jcnelson Jan 6, 2022

blockstack-devops commented Nov 19, 2024

		@@ -0,0 +1,1016 @@
		// Copyright (C) 2013-2020 Blockstack PBC, a public benefit corporation

Feat/mempool sync #2884

Feat/mempool sync #2884

Conversation

jcnelson commented Oct 16, 2021 • edited Loading

jcnelson commented Dec 17, 2021

gregorycoppola left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kantai left a comment

Choose a reason for hiding this comment

pavitthrap left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blockstack-devops commented Nov 19, 2024

jcnelson commented Oct 16, 2021 •

edited

Loading