Block diffusion pipelining #3688

nfrisby · 2022-04-04T13:38:35Z

This PR adds block diffusion pipelining, primarily by finally introducing the long-existing concept of the tentative chain into the code.

Main changes:

add the tentative chain header to the ChainDB state.
have the Followers for the Node-To-Node ChainSync server follow the tentative chain instead of only the selected chain, thus sending the tentative header before the underlying block has been validated
this now means some honest nodes will send us an invalid block, when the block is tentative, so we adjust ChainSync and BlockFetch clients to allow that in limited number of scenarios necessary for the common pipelining scenarios

nfrisby · 2022-04-04T15:38:43Z

Esgen thinks the CI failure on 4cc618f is spurious. He's going to write an explanation here soon.

I'm going to rerun Hydra right now -- this comment is just a placemarker since the CI failure he will reference is likely to no longer be apparent.

amesgen · 2022-04-04T15:44:37Z

Esgen thinks the CI failure on 4cc618f is spurious. He's going to write an explanation here soon.

See #3689 (which reproduces the issue on current master).

amesgen

~~The GH Windows failure is unrelated (see #3678),~~ (EDIT: now fixed) so LGTM from my side 🎉

nfrisby · 2022-04-11T19:50:38Z

Status update: the first benchmarking run showed that we're not actually pipelining. We believe we've fixed it and are now awaiting a second benchmarking run. We'll be pushing those commits here soon.

nfrisby · 2022-04-12T17:26:01Z

An (optional) idea: split 4fa8cda into a commit that just undoes/reverts changes and a second commit that adds the new behavior.

nfrisby · 2022-04-12T17:26:36Z

Minor: 4fa8cda has some extra layer of do in the clearTentativeHeader definition. I'm guessing the tracer commit puts that to use, but it looks a little odd isolated in the previous commit

amesgen · 2022-04-12T17:30:09Z

An (optional) idea: split 4fa8cda into a commit that just undoes/reverts changes and a second commit that adds the new behavior.

I can do that if it is helpful for reviewing 👍. In the end, we want to squash 4fa8cda into aa43e5d, so it won't matter there.

Minor: 4fa8cda has some extra layer of do in the clearTentativeHeader definition. I'm guessing the tracer commit puts that to use, but it looks a little odd isolated in the previous commit

Ah yes, that happend while I swapped the two commits (such that the squashing becomes trivial), will change that (EDIT: done).

nfrisby

I rebased onto origin/master and did one last pass. I found a tvar with a Maybe and a stale comment.

@amesgen would you fix that (preferably squash the fix) and then merge?

ouroboros-consensus/src/Ouroboros/Consensus/Storage/ChainDB/Impl/Types.hs

…l duplex connections Also, bump the default max version to V_8, to include pipelining.

We want to store the `SelectView` of the last invalid tentative header in an `StrictTVar`.

Disconnect if either the block itself is invalid or one of the blocks on its prefix is.

The idea is to change the instruction-emitting parts of the `Follower` abstraction to be aware of the tentative chain. Concretely, we now use `getCurrentChainByType` instead of `readTVar cdbChain` in two occasions: 1. When we are rolling forward from an in-memory point, which is the most relevant case for pipelining (as pipelining is only relevant when the follower is at the tip of our chain). This change is crucial: If we erroneously would use the selected chain in a tentative follower, rolling forward from the tentative point would result in us jumping back to the immutable DB as the tentative header is never part of the selected chain, which would then trigger an error as the tentative header is never in the immutable DB. 2. When rolling forward from a point in the immutable DB, when we are about to switch to the in-memory fragment. In most cases, the choice will be irrelevant here, except for this rare scenario (data loss in the VolDB): `cdbChain` could be empty, and the tentative header could be set, in which case we can roll forward tentative followers which are at the tip of the immutable DB. We leave follower forwarding (triggered via `MsgFindInteresct`) untouched as pipelining is only concerned with up-to-date peers.

nfrisby · 2022-04-20T20:51:33Z

bors merge

nfrisby · 2022-04-20T20:52:30Z

Thank @amesgen for preparing this one with me! The benchmarking results show improvement, so we're good to merge. We still have some room for improvement in the benchmarks and new tests, but this PR is a solid first step.

iohk-bors · 2022-04-20T22:42:06Z

Build succeeded:

3688: Block diffusion pipelining r=nfrisby a=nfrisby This PR adds block diffusion pipelining, primarily by finally introducing the long-existing concept of _the tentative chain_ into the code. Main changes: - add the tentative chain header to the ChainDB state. - have the `Follower`s for the Node-To-Node ChainSync server follow the tentative chain instead of only the selected chain, thus sending the tentative header before the underlying block has been validated - this now means some honest nodes will send us an invalid block, when the block is tentative, so we adjust ChainSync and BlockFetch clients to allow that in limited number of scenarios necessary for the common pipelining scenarios Co-authored-by: Nicolas Frisby <nick.frisby@iohk.io> Co-authored-by: Alexander Esgen <alexander.esgen@iohk.io>

nfrisby added the consensus issues related to ouroboros-consensus label Apr 4, 2022

nfrisby requested review from dcoutts and amesgen April 4, 2022 13:38

nfrisby requested review from dnadales, coot, bolt12 and EncodePanda as code owners April 4, 2022 13:38

nfrisby force-pushed the feature/CAD-4011-pipelining-via-tentative branch from 63b8012 to 4cc618f Compare April 4, 2022 13:48

amesgen approved these changes Apr 4, 2022

View reviewed changes

nfrisby force-pushed the feature/CAD-4011-pipelining-via-tentative branch from 4cc618f to bb78cfc Compare April 5, 2022 18:50

amesgen force-pushed the feature/CAD-4011-pipelining-via-tentative branch from 9da1886 to 34086e3 Compare April 12, 2022 17:16

amesgen force-pushed the feature/CAD-4011-pipelining-via-tentative branch 3 times, most recently from 8dae7a9 to a50c3f5 Compare April 19, 2022 08:34

nfrisby force-pushed the feature/CAD-4011-pipelining-via-tentative branch from d5af480 to 60f0771 Compare April 19, 2022 19:18

nfrisby commented Apr 19, 2022

View reviewed changes

ouroboros-consensus/src/Ouroboros/Consensus/Storage/ChainDB/Impl/Types.hs Outdated Show resolved Hide resolved

nfrisby force-pushed the feature/CAD-4011-pipelining-via-tentative branch from 60f0771 to 20df233 Compare April 19, 2022 19:45

amesgen force-pushed the feature/CAD-4011-pipelining-via-tentative branch from 20df233 to fef1c47 Compare April 20, 2022 08:32

nfrisby and others added 6 commits April 20, 2022 13:49

consensus: add Strict.newTVar

a4bcf23

Shelley: remove isSelfIssued tiebreaker

6bfeaf6

network: repurpose NodeToNodeV_8 for pipelining, introduce _9 for ful…

16669d3

…l duplex connections Also, bump the default max version to V_8, to include pipelining.

Require SelectView to be NoThunks

44d34af

We want to store the `SelectView` of the last invalid tentative header in an `StrictTVar`.

Add TentativeState type

5de757e

consensus: disconnect from peer that sent block on invalid chain

d5252a1

Disconnect if either the block itself is invalid or one of the blocks on its prefix is.

nfrisby and others added 7 commits April 20, 2022 13:49

consensus: forgive ascending invalid tips in BlockFetch disconnect

0e06d69

consensus: do not check ChainSync tip for block invalidity

32f9984

Track the tentative header right before/after body validation

17d1934

Sanity check for tentative followers in the ChainDB q-s-m test

995a17c

Add tracing events for pipelining

d899e53

Add a basic ThreadNet test for pipelining of forged blocks

e140bb4

nfrisby force-pushed the feature/CAD-4011-pipelining-via-tentative branch from fef1c47 to e140bb4 Compare April 20, 2022 20:49

iohk-bors bot merged commit 1f6433d into master Apr 20, 2022

iohk-bors bot deleted the feature/CAD-4011-pipelining-via-tentative branch April 20, 2022 22:42

amesgen mentioned this pull request Aug 31, 2022

Johnalanwoods/block diffusion pipelining #3579

Closed

amesgen mentioned this pull request Nov 30, 2023

Finalize the documentation of block diffusion pipelining via delayed validation IntersectMBO/ouroboros-consensus#559

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Block diffusion pipelining #3688

Block diffusion pipelining #3688

nfrisby commented Apr 4, 2022 •

edited by amesgen

Loading

nfrisby commented Apr 4, 2022

amesgen commented Apr 4, 2022

amesgen left a comment •

edited

Loading

nfrisby commented Apr 11, 2022

nfrisby commented Apr 12, 2022

nfrisby commented Apr 12, 2022

amesgen commented Apr 12, 2022 •

edited

Loading

nfrisby left a comment

nfrisby commented Apr 20, 2022

nfrisby commented Apr 20, 2022

iohk-bors bot commented Apr 20, 2022

Block diffusion pipelining #3688

Block diffusion pipelining #3688

Conversation

nfrisby commented Apr 4, 2022 • edited by amesgen Loading

nfrisby commented Apr 4, 2022

amesgen commented Apr 4, 2022

amesgen left a comment • edited Loading

Choose a reason for hiding this comment

nfrisby commented Apr 11, 2022

nfrisby commented Apr 12, 2022

nfrisby commented Apr 12, 2022

amesgen commented Apr 12, 2022 • edited Loading

nfrisby left a comment

Choose a reason for hiding this comment

nfrisby commented Apr 20, 2022

nfrisby commented Apr 20, 2022

iohk-bors bot commented Apr 20, 2022

nfrisby commented Apr 4, 2022 •

edited by amesgen

Loading

amesgen left a comment •

edited

Loading

amesgen commented Apr 12, 2022 •

edited

Loading