Fix cloneBlockchainTime #1506

nfrisby · 2020-01-27T05:31:16Z

Fixes #1489. Fixes #1524.

Fixing Issue #1489 (let nodes lead when they join) and letting k vary in the
range [2 .. 10] since the dual-ledger tests do that now revealed several
Issues.

Issues in the library, not just the test infrastructure:

Issue Debug reapplyTxSameState: unexpected error: MockInvalidInputs #1505 -- removeTxs cannot use the fast path when validating after
removing or else we might have dangling tx inputs references. Was fixed
by Mempool.removeTxs should not use the fast path #1565.
Issue Test with k = 1 in test-consensus ouroboros-consensus#726, bullet 1 (closed) -- The Empty cases in
prevPointAndBlockNo were wrong. Recent PRs have addressed this: consensus: do not assume the anchor point is genesis in prevPointAndBlockNo #1544 and
Add BlockNo to AnchoredFragment #1589.
Issue ImmutableDB is leaking file handles #1543 (closed) -- A bracket in registeredStream was spoiled by an
interruptible operation. Thomas' PR re-designed the vulnerability away. I
think this is unrelated to the other changes; it was lurking and happened
to pop up here just because I've been running hundreds of thousands of
tests.

Issues only in the test infrastructure:

Issue Fix cloning of TestBlockChaintime #1489: let nodes lead when they join. This bug slipped in recently,
when I added cloning of BlockchainTimes as part of the
restarting/rekeying loop in the test infrastructure.
PBFT reference simulator. (Was masked by 1489.) Model competing 1-block
chains in Ref.PBFT and use its results where applicable instead of the
.Expectations module. Check that the PBFT threadnet and Ref.PBFT.simulate
results agree on Ref.Nominal slots.

The Ref.PBFT module had been making assumptions that were accurate given
the guards in RealPBFT generators, given the k >= n regime. But outside
of that regime, when the security parameter exceeds the node count, it
wasn't enough. Also, it couldn't be compared against the PBFT threadnet
because of those assumptions.
PBFT reference simulator. (Cascade of above.) Add
definitelyEnoughBlocks to confirm the "at least k blocks in 2k slots"
invariant in genRealPBFTNodeJoinPlan. The existing guards in the RealPBFT
generators are intentionally insufficient by themselves; this way we can
optimize them to avoid O(n^2) complexity without risking divergence from
the suchThats.
Origin corner-case. (Was masked by 1489.) Discard DualPBFT tests that
forge in slot 0. The current cardano-ledger-specs doesn't allow for that.
My hypothesis is that cvsLastSlot would need to be able to represent
origin.
Dlg cert tx. Adjust genNodeRekeys to respect "If node X rekeys in slot
S and Y leads slot S+1, then either the topology must connect X and Y
directly, or Y must join before slot S."
Dlg cert tx. (Was (statistically?) masked by relatively large k.) Add
rekeyOracle and use it to determine which epoch number to record in the
dlg cert. The correct value depends on which block the dlg cert tx will end
up in, not when we first add it to our mempool.
Dlg cert tx. (Was (statistically?) masked by relatively large k.) Add the
dlg cert tx to the mempool each time the ledger changes.

ouroboros-consensus/test-consensus/Test/ThreadNet/BFT.hs

ouroboros-consensus/test-consensus/Test/ThreadNet/DualPBFT.hs

ouroboros-consensus/test-consensus/Test/ThreadNet/Network.hs

ouroboros-consensus/test-consensus/Test/ThreadNet/PBFT.hs

ouroboros-consensus/test-consensus/Test/ThreadNet/RealPBFT.hs

nfrisby · 2020-01-27T05:40:40Z

ouroboros-consensus/test-consensus/Test/ThreadNet/Ref/PBFT.hs

@@ -0,0 +1,620 @@
+{-# LANGUAGE LambdaCase          #-}


See the commits for a delta without the renaming (can't pass the git diff -M option to GitHub's PR renderer?).

nfrisby · 2020-01-31T08:16:03Z

I've pushed new commits and updated the PR description, but the commit history is still just a trail of breadcrumbs along my bug hunt. When I'm back online in about 6 hours, my immediate goal is to clean up those commits. The tip of this branch has passed 50000 RealPBFT k=2 tests and 50000 RealPBFT k=3 tests and tens of thousands of more varying tests.

nfrisby · 2020-02-03T23:14:18Z

That rebase cleaned up the git history, removing a lot of the code that arose during debugging.

test-consensus: let k vary in PBFT and RealPBFT tests b8d8ab5 is still a pretty big commit, but I'm not sure that splitting it up (into up to 3 commits, I'd guess) is worth the effort.

nfrisby · 2020-02-03T23:17:40Z

ouroboros-consensus/test-consensus/Test/ThreadNet/RealPBFT.hs

+            -- not reach it soon enough)
+          , let nextLeader = Ref.mkLeaderOf params $ succ jslot
+          , jslot /= coreNodeIdJoinSlot nodeJoinPlan nextLeader ||
+            cid `elem` coreNodeIdNeighbors nodeTopology nextLeader
          -> pure $ NodeRestarts $


TODO We also need to prevent the rekeying node from restarting too soon, since it might need to propagate its dlg cert tx more than once.

nfrisby · 2020-02-06T15:23:31Z

That force push is passing tests. Remaining work:

resolve a few TODOs in the code and from this PR's comments
rebase to origin/master (blocked at least by Update non-integral.tex cardano-ledger#715)

ouroboros-consensus/test-consensus/Test/ThreadNet/General.hs

nfrisby · 2020-02-06T17:12:59Z

I rebased onto cee384a, because that's the last commit on master that still has the Byron ledger wrapper in this repo. My patch for cardano-ledger Issue 715 cannot exist in this repo beyond that commit.

Unfortunately, the fixes for 1544 (prevPointAndBlockNo) and 1565 (removeTxs) come later.

I'm going to open a PR in cardano-ledger if only to get the ball rolling.

nfrisby · 2020-02-06T19:00:55Z

I opened IntersectMBO/cardano-ledger#716, which I think is the necessary fix (it is the same as the patch I have locally on this branch). Once that's merged, then my PR can update the deps in the repo accordingly and therefore rebase all the way to our origin/master.

nfrisby · 2020-02-07T00:58:50Z

After rebasing onto master (now that a cardano-ledger fix commit is available for gitfetch), I'm seeing errors that I believe are due to Issue #1147. I see the characteristic FetchDeclineConcurrencyLimit BF decision between a CompletedBlockFetch and CompletedFetchBatch, and if I cherry-pick my patch for that from PR 1131 (i.e. commit 0d2fa18), then the failing repro passes.

I'm guessing the recent changes to ChainSync client (in light of the recent refinement of tips to possibly include "origin" instead of an off-by-one blockNumber = 0") has somehow enabled this family of mini protocol message/event interleavings, which PR 1131 instead has been exploring by adding network latencies.

cc: @dcoutts @edsko @mrBliss

1691: consensus: handle EBBs in rewindHeaderState r=nfrisby a=nfrisby Fixes #1690. I believe this bug was masked by Issue #1489; I discovered it and confirmed that this patch fixes it on my related WIP branch. This PR does not include a repro for this bug, but that branch does, and PR #1506 will as soon as I'm able to update it with that WIP branch -- that's my current focus. Co-authored-by: Nicolas Frisby <nick.frisby@iohk.io>

nfrisby · 2020-02-27T22:53:32Z

I eventually realized that the Just-In-Time EBBs (enabled by a recent PR on master) avoid the error case that otherwise required a patch for Issue 1631 (Chain Density check in NodeKernel) and a rewrite of the reference simulator. This meant I could drop the NodeKernel change, which simplified the PR quite a bit. It no longer addresses 1631, leaving that for future work.

edsko

Some minor comments.

edsko · 2020-03-02T15:02:48Z

ouroboros-consensus/ouroboros-consensus-test-infra/src/Test/Util/Pam.hs

+--
+-- INVARIANT the 'NonEmpty's are all ascending
+--
+newtype Pam v k = UnsafePam {getPam :: Map v (NonEmpty k)}


Let's rename this to InvertedMap.

Done in commit 3de1fc8

edsko · 2020-03-02T15:04:56Z

ouroboros-consensus/src/Ouroboros/Consensus/BlockchainTime/Mock.hs

@@ -74,8 +73,10 @@ data TestClock =
  deriving (Eq, Generic, NoUnexpectedThunks)

 data TestBlockchainTime m = TestBlockchainTime


We really ought to move this to test-infra.

ouroboros-consensus-byron/test/Test/ThreadNet/RealPBFT.hs

edsko · 2020-03-02T15:13:38Z

ouroboros-consensus/ouroboros-consensus-test-infra/src/Test/ThreadNet/Network.hs

+         -- EBB predecessor's hash
+      -> blk
+  , ebbSlotBefore :: SlotNo -> SlotNo
+    -- ^ the slot of the most recent expected that precedes the given slot


I don't understand this name or this comment :)

Elaborated in commit 9b2aa86

edsko · 2020-03-02T15:15:06Z

ouroboros-consensus/ouroboros-consensus-test-infra/src/Test/ThreadNet/Network.hs

+    -- If we add the transaction and then the mempools discards it for some
+    -- reason, this thread will add it again.
+    --
+    forkTxs0


This could do with a one or two line explanation of what the purpose of it is.

Elaborated in commit 183da45

edsko · 2020-03-02T15:17:04Z

ouroboros-consensus-byron/test/Test/ThreadNet/DualPBFT.hs

+
+    -- The test infrastructure allows nodes to forge in slot 0; however, the
+    -- cardano-ledger-specs code causes @PBFTFailure (SlotNotAfterLastBlock
+    -- (Slot 0) (Slot 0))@ in that case. So we discard such tests.


Could do with a comment that the spec here is therefore more strict than the real implementation.

Elaborated in commit d0134ca

nfrisby · 2020-03-02T17:42:57Z

OK. I addressed each comment with a commit, and now I'm going to squash those down and then bors it.

nfrisby · 2020-03-02T18:40:09Z

bors r+

iohk-bors · 2020-03-02T19:16:18Z

Build succeeded