Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ChainSync client test: model pipelining behavior #3742

Merged
merged 4 commits into from May 19, 2022
Merged

ChainSync client test: model pipelining behavior #3742

merged 4 commits into from May 19, 2022

Conversation

amesgen
Copy link
Member

@amesgen amesgen commented May 11, 2022

Closes CAD-4192

nfrisby
nfrisby requested changes May 17, 2022
View reviewed changes
Copy link
Contributor

@nfrisby nfrisby left a comment

Choose a reason for hiding this comment

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

This looks very nice. Thanks for the very well-organized commits!

Requesting Changes for a few more Haddock comments, mainly. I also ask one relatively-open-ended question about the design.

ouroboros-consensus-test/test-consensus/Test/Consensus/MiniProtocol/ChainSync/Client.hs Outdated Show resolved Hide resolved
ouroboros-consensus-test/test-consensus/Test/Consensus/MiniProtocol/ChainSync/Client.hs Outdated Show resolved Hide resolved
ouroboros-consensus-test/test-consensus/Test/Consensus/MiniProtocol/ChainSync/Client.hs Outdated Show resolved Hide resolved
ouroboros-consensus-test/test-consensus/Test/Consensus/MiniProtocol/ChainSync/Client.hs Outdated Show resolved Hide resolved
ouroboros-consensus-test/test-consensus/Test/Consensus/MiniProtocol/ChainSync/Client.hs Outdated Show resolved Hide resolved
ouroboros-consensus-test/test-consensus/Test/Consensus/MiniProtocol/ChainSync/Client.hs Show resolved Hide resolved
ouroboros-consensus-test/test-consensus/Test/Consensus/MiniProtocol/ChainSync/Client.hs Outdated Show resolved Hide resolved
ouroboros-consensus-test/test-consensus/Test/Consensus/MiniProtocol/ChainSync/Client.hs Show resolved Hide resolved
@amesgen amesgen added consensus issues related to ouroboros-consensus testing labels May 17, 2022
nfrisby
nfrisby approved these changes May 18, 2022
View reviewed changes
Copy link
Contributor

@nfrisby nfrisby left a comment

Choose a reason for hiding this comment

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

All set. Approved! Thanks.

ouroboros-consensus-test/test-consensus/Test/Consensus/MiniProtocol/ChainSync/Client.hs Outdated Show resolved Hide resolved
ouroboros-consensus-test/test-consensus/Test/Consensus/MiniProtocol/ChainSync/Client.hs Show resolved Hide resolved
@amesgen amesgen force-pushed the amesgen/CAD-4192-pipelining-test-ChainSync-client branch from 92d86eb to 5d06588 Compare May 18, 2022 15:11
@amesgen amesgen requested review from a team, coot and bolt12 as code owners May 18, 2022 15:11
@amesgen
TestBlock: export validity, with an explicit type
03b08b1
@amesgen
ChainSync client test: generalize Schedules to arbitrary elements
efcdf54
@amesgen
ChainSync client test: also catch linked exceptions
afd99d8
@amesgen
ChainSync client test: Model honest behavior involving invalid blocks
50d8047 
In the context of diffusion pipelining, it is possible honest behavior
for an upstream peer to send us a block we already know is invalid. In
this case, we should not promptly disconnect them in ChainSync, as was
the case pre-pipelining.

This commit models the "learning process" of our local node that
certain blocks are invalid by scheduling them in random order in the
timeframe of scheduled ticks.
@amesgen amesgen force-pushed the amesgen/CAD-4192-pipelining-test-ChainSync-client branch from 5d06588 to 50d8047 Compare May 18, 2022 15:13
@amesgen amesgen removed request for a team, coot and bolt12 May 18, 2022 15:13
@amesgen
Copy link
Member Author

amesgen commented May 18, 2022

bors merge

iohk-bors bot added a commit that referenced this pull request May 18, 2022
@iohk-bors @amesgen
Merge #3742
370d90b 
3742: ChainSync client test: model pipelining behavior r=amesgen a=amesgen

Closes [CAD-4192](https://input-output.atlassian.net/browse/CAD-4192)

Co-authored-by: Alexander Esgen <alexander.esgen@iohk.io>
@iohk-bors
Copy link
Contributor

iohk-bors bot commented May 18, 2022

Timed out.

@amesgen
Copy link
Member Author

amesgen commented May 18, 2022

bors retry

iohk-bors bot added a commit that referenced this pull request May 18, 2022
@iohk-bors @amesgen
Merge #3734 #3742
f83e0c4 
3734: ChainSync server: prevent duplicate `RollForward` messages r=amesgen a=amesgen

Closes [CAD-4228](https://input-output.atlassian.net/browse/CAD-4228).

#### Problem description

Consider the following very common scenario prior to this PR:

 1. Assume that the node is idle, having selected a chain with tip A

 2. It receives a new block B that extends A

 3. We set the tentative header to B

 3. This wakes up the ChainSync server thread (in an `STM` transaction
    in `instructionHelper`)

     - The roll state is updated from `RollForwardFrom A` to `RollForwardFrom B`.

     - The chain update `AddBlock B` is emitted.

 4. After successful validation, the node selects B which involves a
    call to `fhSwitchFork` with intersection point A.

     - Because the intersection point is now behind the roll state
       point, the roll state is set to `RollBackTo A`.

     - Hence, the follower emits/the ChainSync server sends a
       `RollBack A`, immediately followed by another `AddBlock B`.

These extra messages are redundant, and will be no longer sent with
this PR.

#### Diagnosis

The arguments to `fhSwitchFork` are unaware of the tentative header,
so for a *tentative* follower it looks like a rollback is necessary.

Before pipelining, the call to `fhSwitchFork` was a no-op when rolling
forward by a block because the passed intersection point did not
precede (in fact, was equal to) the roll state point.

#### Possible solutions

(note that these are not mutually exclusive)

 1. Make the call to `fhSwitchFork` with arguments aware of the
    tentative header.

 2. Don't call `fhSwitchFork` when selecting our tentative header.

 3. Don't call `fhSwitchFork` when extending our chain.

We went with the third, as it stresses that `fhSwitchFork` is only
necessary when actually switching to a fork, and it is most
straightforward to implement.


3742: ChainSync client test: model pipelining behavior r=amesgen a=amesgen

Closes [CAD-4192](https://input-output.atlassian.net/browse/CAD-4192)

Co-authored-by: Alexander Esgen <alexander.esgen@iohk.io>
@iohk-bors
Copy link
Contributor

iohk-bors bot commented May 18, 2022

Build failed (retrying...):

@iohk-bors
Copy link
Contributor

iohk-bors bot commented May 19, 2022

Build succeeded:

@iohk-bors iohk-bors bot merged commit 847d64b into master May 19, 2022
@iohk-bors iohk-bors bot deleted the amesgen/CAD-4192-pipelining-test-ChainSync-client branch May 19, 2022 00:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nfrisby nfrisby nfrisby approved these changes

@dnadales dnadales Awaiting requested review from dnadales

Assignees
No one assigned
Labels
consensus issues related to ouroboros-consensus testing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@amesgen @nfrisby