feat: bodies stage #190

onbjerg · 2022-11-10T21:25:47Z

Almost complete bodies stage, a few hanging todos + tests left. Also includes some minor touchups to documentation when I stumbled upon it.

The stage uses a Downloader like the headers stage, but this downloader is implemented using a stream. An iterator of block hashes for blocks we want to download is handed over to the downloader and the downloader will fetch the blocks. The blocks are fetched somewhat concurrently but emitted by the stream in order - so, if we are fetching 10 blocks (1-10) and block 3 finishes downloading first, then block 2 and then block 1, the order of emitted values from the stream will still be 1, 2, 3. This simplifies some of the stage logic

There are a few types we should probably consider consolidating wrt block bodies, I've marked them with todos for now

Edit: I've also moved the proofs module from the consensus crate to the primitives crate since the proofs are used by multiple modules. Some of the verification functions in the consensus crate that only operate on blocks/transactions/headers should probably also just live directly on those types in the primitives module, but I'll leave that for later

codecov · 2022-11-10T21:47:41Z

Codecov Report

Merging #190 (e38e193) into main (9575eb8) will increase coverage by 0.29%.
The diff coverage is 75.07%.

@@            Coverage Diff             @@
##             main     #190      +/-   ##
==========================================
+ Coverage   69.80%   70.10%   +0.29%     
==========================================
  Files         200      209       +9     
  Lines       16538    17375     +837     
==========================================
+ Hits        11544    12180     +636     
- Misses       4994     5195     +201

Impacted Files	Coverage Δ
crates/consensus/src/consensus.rs	`0.00% <0.00%> (ø)`
crates/consensus/src/lib.rs	`100.00% <ø> (ø)`
crates/db/src/kv/mod.rs	`94.83% <ø> (ø)`
crates/interfaces/src/consensus.rs	`100.00% <ø> (ø)`
crates/interfaces/src/db/codecs/scale.rs	`100.00% <ø> (ø)`
crates/interfaces/src/db/mod.rs	`94.73% <ø> (ø)`
crates/interfaces/src/db/tables.rs	`0.00% <ø> (ø)`
crates/interfaces/src/p2p/bodies/client.rs	`0.00% <0.00%> (ø)`
crates/interfaces/src/p2p/error.rs	`0.00% <0.00%> (ø)`
crates/interfaces/src/test_utils/bodies.rs	`0.00% <0.00%> (ø)`
... and 37 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

crates/stages/src/stages/bodies.rs

crates/net/bodies-downloaders/src/concurrent.rs

rkrasiuk · 2022-11-11T08:41:18Z

crates/stages/src/stages/bodies.rs

+            for num in 0..body.tx_amount {
+                let tx_id = body.base_tx_id + num;
+                if transaction_cursor.seek_exact(tx_id)?.is_some() {
+                    transaction_cursor.delete_current()?;
+                }
+            }


you can use unwind_table_by_num here

I can't since tx is already immutably borrowed by block_body_cursor

crates/stages/src/stages/bodies.rs

crates/consensus/src/verification.rs

crates/interfaces/src/db/models/blocks.rs

crates/interfaces/src/p2p/bodies/client.rs

crates/net/bodies-downloaders/src/concurrent.rs

mattsse · 2022-11-11T16:35:35Z

crates/net/bodies-downloaders/src/concurrent.rs

+    fn stream_bodies<'a, 'b, I>(&'a self, headers: I) -> BodiesStream<'a>
+    where
+        I: IntoIterator<Item = H256>,
+        <I as IntoIterator>::IntoIter: Send + 'b,
+        'b: 'a,
+    {


the lifetime won't be necessary if you clone and move the client, but only useful depending on how it's called, so perhaps borrowing is sufficient here.

I tried cloning and moving the client but it didn't work :S

Edit: See the error here, maybe I am missing something

error: captured variable cannot escape `FnMut` closure body --> crates/net/bodies-downloaders/src/concurrent.rs:48:21 | 44 | let client = Arc::clone(&self.client); | ------ variable defined here 45 | Box::pin( 46 | stream::iter(headers.into_iter().map(move |header_hash| { | - inferred to be a `FnMut` closure 47 | { 48 | client | ^----- | | | _____________________variable captured here | | 49 | | .get_block_body(header_hash) 50 | | .map_ok(move |body| (header_hash, body)) 51 | | .map_err(|err| match err { ... | 55 | | err => DownloadError::Client { source: err }, 56 | | }) | |__________________________^ returns a reference to a captured variable which escapes the closure body | = note: `FnMut` closures only have access to their captured variables while they are executing... = note: ...therefore, they cannot allow references to captured variables to escape

crates/interfaces/src/p2p/bodies/downloader.rs

crates/interfaces/src/p2p/headers/downloader.rs

mattsse · 2022-11-11T16:44:06Z

crates/net/bodies-downloaders/src/concurrent.rs

+                        })
+                }
+            }))
+            .buffered(self.batch_size),


this is not the batch size for GetBodies, instead, this is the number of concurrently active requests.

There should be different settings for both.

the batch_size is size for the headers chunks to divide via Vec::chunks(batch_size), and for buffured we need a separate value that limits the number of request we send.

It's unclear to me what the actual difference is, my assumption is that if we request multiple bodies in one GetBodies call they are downloaded concurrently but there is no order guarantee. The reason I only request one per call is because the order needs to be the same as the request order, otherwise we may skip bodies in the database

there's no guarantee, only blocks that were found are returned.

So perhaps, if the BodiesStream needs to return them in order it probably needs to keep track of the next header to return and buffer the rest

That's what this is doing, just without requesting multiple bodies per call. Is there any tangible benefit to requesting multiple per call?

this we should test, can then tune

mattsse · 2022-11-11T16:45:26Z

crates/interfaces/src/p2p/bodies/client.rs

+#[auto_impl::auto_impl(&, Arc, Box)]
+pub trait BodiesClient: Send + Sync + Debug {
+    /// Fetches the block body for the requested block.
+    async fn get_block_body(&self, hash: H256) -> Result<BlockBody, BodiesClientError>;


while it is possible to send a request for single block.

the GetBlockBodies request takes a Vec<H256> to fetch multiple bodies with one request

Is there an order guarantee? If not, see my above comment

mattsse · 2022-11-11T16:45:50Z

crates/interfaces/src/p2p/bodies/downloader.rs

+    ///
+    /// It is *not* guaranteed that all the requested bodies are fetched: the downloader may close
+    /// the stream before the entire range has been fetched for any reason
+    fn stream_bodies<'a, 'b, I>(&'a self, headers: I) -> BodiesStream<'a>


This is okay for now I think but I think we need to roll a custom impl of a buffered Stream that handles retries

Any particular reason to roll our own? Adding retries can be done in this loop w/o issue I think, we just need a generalized retry abstraction for futures

https://docs.rs/futures-retry/latest/futures_retry/ have used this succesfully in the past

https://github.com/interledger-rs/interledger-rs/blob/e15ae42d5ce80e45ace8c7bebd5875372b89dcfb/crates/interledger-settlement/src/core/settlement_client.rs#L37-L40

https://github.com/interledger-rs/interledger-rs/blob/e15ae42d5ce80e45ace8c7bebd5875372b89dcfb/crates/interledger-settlement/src/core/settlement_client.rs#L157-L196

crates/interfaces/src/p2p/bodies/downloader.rs

crates/interfaces/src/p2p/bodies/client.rs

onbjerg · 2022-11-14T16:35:29Z

Pulling it out of draft so we can get some of the other tweaks I have stacked in this PR merged. Will continue to iterate on it (adding tests for the downloader, addressing some of the todos I left in the code etc) if it gets merged

mattsse

lgtm

mattsse · 2022-11-15T07:15:04Z

crates/net/bodies-downloaders/src/concurrent.rs

+                        })
+                }
+            }))
+            .buffered(self.batch_size),


this we should test, can then tune

onbjerg · 2022-11-15T07:49:06Z

Merging and building on top

onbjerg added C-enhancement New feature or request A-staged-sync Related to staged sync (pipelines and stages) labels Nov 10, 2022

onbjerg force-pushed the onbjerg/bodies-stage branch from 6817d5b to 83c5c0a Compare November 10, 2022 21:40

rkrasiuk reviewed Nov 11, 2022

View reviewed changes

rkrasiuk mentioned this pull request Nov 11, 2022

feat(sync): sender recovery stage #181

Merged

rakita reviewed Nov 11, 2022

View reviewed changes

crates/consensus/src/verification.rs Show resolved Hide resolved

rakita reviewed Nov 11, 2022

View reviewed changes

crates/interfaces/src/db/models/blocks.rs Show resolved Hide resolved

rakita reviewed Nov 11, 2022

View reviewed changes

crates/interfaces/src/p2p/bodies/client.rs Outdated Show resolved Hide resolved

mattsse requested changes Nov 11, 2022

View reviewed changes

rakita mentioned this pull request Nov 11, 2022

feat(executor): transaction and receipts verification #195

Merged

onbjerg force-pushed the onbjerg/bodies-stage branch from ac0b98c to ebcb176 Compare November 12, 2022 06:52

mattsse mentioned this pull request Nov 12, 2022

refactor: move RequestError from network to interfaces p2p #197

Merged

rakita mentioned this pull request Nov 12, 2022

Discuss: where to store ommers #198

Closed

onbjerg added 16 commits November 14, 2022 12:10

chore: clean up .gitignore

c9fa764

fix: make RO cursors Send + Sync

4a168d3

feat(wip): bodies stage

3fa663d

driveby: improve docs

70c8c4e

chore: don't panic if we're the first stage

317a0d0

chore: use Vec for ommers

ca1695f

feat: error handling in bodies downloader

175d902

chore: remove stale comment

b1ddc36

chore: pascal-case stage id

794f2dc

refactor: remove unused new fns

7df907b

refactor: distinguish downloaders with prefix

9125914

refactor: move downloader errs to own module

9d08695

refactor: stream_bodies -> bodies_stream

16d3bb8

test: fix borked imports in header stage

447ebc3

test: clean up header tests

8214e10

test: add basic body stage tests

41cfe63

onbjerg added 5 commits November 14, 2022 12:10

test: add 2 more body stage test skeletons

a2b1ad3

test: move generator test utils to own module

e1b5724

refactor: move proof functions to primitives crate

f66105e

feat: add block generator test utils

74fd799

test: more body stage tests

32728a9

onbjerg force-pushed the onbjerg/bodies-stage branch from 143cc63 to 32728a9 Compare November 14, 2022 13:36

onbjerg added 8 commits November 14, 2022 14:36

chore: fix typo (Cannonical* -> Canonical)

7245c3e

docs: document bodies_to_download

f9184fd

test: more body stage tests

fe733fd

test: more body stage tests

e1e05eb

refactor: clean up body stage tests a bit

be52c98

test: fix broken tests

683bf96

refactor: clean up body stage tests

0e0e9cd

test: more body stage tests

e38e193

onbjerg changed the title ~~wip: bodies stage~~ bodies stage Nov 14, 2022

onbjerg changed the title ~~bodies stage~~ feat: bodies stage Nov 14, 2022

onbjerg marked this pull request as ready for review November 14, 2022 16:35

mattsse approved these changes Nov 15, 2022

View reviewed changes

crates/net/bodies-downloaders/src/concurrent.rs

})

}

}))

.buffered(self.batch_size),

Copy link

Collaborator

mattsse Nov 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this we should test, can then tune

onbjerg merged commit 92a7818 into main Nov 15, 2022

onbjerg deleted the onbjerg/bodies-stage branch November 15, 2022 07:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: bodies stage #190

feat: bodies stage #190

onbjerg commented Nov 10, 2022 •

edited

codecov bot commented Nov 10, 2022 •

edited

rkrasiuk Nov 11, 2022

onbjerg Nov 12, 2022

mattsse Nov 11, 2022 •

edited

onbjerg Nov 12, 2022 •

edited

mattsse Nov 11, 2022

onbjerg Nov 12, 2022

mattsse Nov 12, 2022

onbjerg Nov 12, 2022 •

edited

mattsse Nov 15, 2022

mattsse Nov 11, 2022

onbjerg Nov 12, 2022

mattsse Nov 11, 2022

onbjerg Nov 12, 2022

gakonst Nov 15, 2022 •

edited

onbjerg commented Nov 14, 2022

mattsse left a comment

mattsse Nov 15, 2022

onbjerg commented Nov 15, 2022

feat: bodies stage #190

feat: bodies stage #190

Conversation

onbjerg commented Nov 10, 2022 • edited

codecov bot commented Nov 10, 2022 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattsse Nov 11, 2022 • edited

Choose a reason for hiding this comment

onbjerg Nov 12, 2022 • edited

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

onbjerg Nov 12, 2022 • edited

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

gakonst Nov 15, 2022 • edited

Choose a reason for hiding this comment

onbjerg commented Nov 14, 2022

mattsse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

onbjerg commented Nov 15, 2022

onbjerg commented Nov 10, 2022 •

edited

codecov bot commented Nov 10, 2022 •

edited

mattsse Nov 11, 2022 •

edited

onbjerg Nov 12, 2022 •

edited

onbjerg Nov 12, 2022 •

edited

gakonst Nov 15, 2022 •

edited