Add LSQ leashing 0.30.0.1 by tweag-ev-ak · Pull Request #18 · tweag/ouroboros-consensus

tweag-ev-ak · 2026-02-26T13:59:22Z

No description provided.

* Update ouroboros-network ref * Add support for LeashID * Support explicit unleashing

nfrisby

This review is probably more than you had intended to ask for! You offered to have a call to discuss this code with me and I said "looks small enough for me to look over myself first", and suddenly here I am saying things like "this code needs comments", which is a bit unfair since you just wanted to discuss the ideas with me.

But I figured a more thorough/serious review than requested will likely be more useful, so quit trying to prevent myself from settling into "review mode" and let it rip. I hope the result is helpful in the way you were hoping 🤞And we can still have that call if there's particular things you'd like to discuss synchronously.

nfrisby · 2026-02-26T19:50:59Z

+     -- ^ The LoE fragment.
+  -> StrictTVar m (ChainDB.GetLoEFragment m blk)
+  -> m ()
+setGetLoEFragment readLeashingState readGsmState readGenesisLoEFragment readLoEFragment varGetLoEFragment =


What's the difference between readLoEFragment and readGenesisLoEFragment?

My suspicion is that readLoEFragment should have a more distinct name now. TODO return here when I have an opinion about what that name should be

Please see #18 (comment)

nfrisby · 2026-02-26T21:04:26Z

  , getDiffusionPipeliningSupport ::
      DiffusionPipeliningSupport
  , getBlockchainTime :: BlockchainTime m
+  , getLeashingStateVar :: StrictTVar m (ChainDB.LeashingState blk)


My instincts are that LeashingState (and so getLeashingStateVar etc) should be explicitly mentioning LSQ.

Recap of where the jargon comes from:

The Limit on Eagerness (LoE) prevents a node syncing via Ouroboros Genesis from committing to a chain that would force it to disconnect from any peer while it's syncing. The gddWatcher is already inspecting those peers' candidates, and so it is a convenient place to compute the least restrictive LoE anchor fragment that protects the current peers.

"Leashing" is a way an adversarial peer could abuse the LoE to prevent the syncing node from making progress. The Limit on Patience (LoP) and the Genesis Density Disconnector (GDD) cooperate to mitigate that attack vector.

The PR that this feature adds is to intentionally let LocalStateQuery (LSQ) peers reuse the existing LoE in a way that intentionally leashes the node (regardless of whether it's syncing).

So I think "leashing state" is too vague. But "LsqLoE" or "LsqLeashing" etc would be sufficiently indicative.

And the PR should be "Add LSQ leashing". Etc.

For example, that would make it easier to notice/remember that getLeashingStateVar only exists so that the LSQ servers can keep that state up-to-date.

I agree, that's a great idea 👍

nfrisby · 2026-02-26T21:13:08Z

-          (genesisArgs, setLoEinChainDbArgs) <-
-            mkGenesisNodeKernelArgs llrnGenesisConfig
+          genesisArgs <- mkGenesisNodeKernelArgs llrnGenesisConfig
+          varGetLoEFragment <- newTVarIO $ pure ChainDB.LoEDisabled


TODO I didn't scrutinize this very much. Seems plausible as just a copy-paste from what used to be in mkGenesisNodeKernelArgs.

nfrisby · 2026-02-26T21:14:23Z

+      else 
+        readGenesisLoEFragment >>= \case
+          Just glf -> do
+            -- leashing is disabled, run old behavior


See my other big comment about "names should mention LSQ".

I think "leashing is disabled" should say "LSQ leashing is disabled", for example.

Separate concern: I don't think we need mutual exclusion "either the LSQ LoE or the GDD LoE". Because we can intersect them to correctly combine them, can't we?

Separate concern: I don't think we need mutual exclusion "either the LSQ LoE or the GDD LoE". Because we can intersect them to correctly combine them, can't we?

Yes, that's what we do right now in the leashing watcher:

wNotify :: (LsqLeashingWatcherState blk) -> m () wNotify LsqLeashingWatcherState{..} = do let lsqLeashingCandidates = Map.toList lsqLeashingState prefix = maybe curChain id genesisLoEFrag loeFrag = fst $ sharedCandidatePrefix prefix lsqLeashingCandidates

if there is a genesis LoE fragment, we use it as a base for sharedCandidatePrefix, otherwise it's current chain. Is it a correct approach?

Only later, in setGetLoEFragment, we decide which the LoE fragment to return, basing on both fragment variables, the lsqLeashingState and the gsm state.

readLoEFragment in setGetLoEFragment returns the resulting LoE fragment calculated by lsqLeashingWatcher.

readGenesisLoEFragment returns the genesis loe fragment calculated by gddWatcher.

Sorry that I confused you because of all these variables mess. I wanted to keep the original implementation as much as possible and extend it rather than changing the existent one. I thought it would be easier to understand the additions. Turned out it only messed up the understanding.

We can do that final calculation in the lsq leashing watcher instead of setGetLoEFragment. I just couldn't decide what's the correct approach. Either we do all the logic in the watchers and remove the getLoEFragment from setGetLoEFragment (it will become just plain readLoEFragment) or combine the whole calculation in setGetLoEFragment (what I ended up with to keep the old behaviour and make our additions more clear).

nfrisby · 2026-03-01T21:21:51Z

Uh oh.... I'm anticipating a burdensome amount of conflicts with IntersectMBO#1891, since it's changing how Forkers are acquired. Maybe it won't be too bad 🤞

axman6

Just a few comments while reading through

axman6 · 2026-03-12T06:44:16Z

+            let
+              leashingFragment = case mpt of
+                  ImmutableTip -> AF.Empty $ AF.anchor currentChain
+                  SpecificPoint p -> AF.takeWhileOldest (\(HeaderWithTime h _) -> headerPoint h <= p) currentChain


There are several functions for splitting an AnchoredFragment efficiently which might be better than takeWhileOldest https://ouroboros-network.cardano.intersectmbo.org/ouroboros-network/api/Ouroboros-Network-AnchoredFragment.html#g:8

axman6 · 2026-03-12T06:46:24Z

+                  ImmutableTip -> AF.Empty $ AF.anchor currentChain
+                  SpecificPoint p -> AF.takeWhileOldest (\(HeaderWithTime h _) -> headerPoint h <= p) currentChain
+                  VolatileTip -> currentChain
+            let newState = Map.insert leashId leashingFragment lsqLeashingState 


Do we need to check if this leashId exists already? I think it's fine, I'm pretty sure we always want to update to this fragment, but just want to check it isn't a situation we haven't considered.

axman6 · 2026-03-12T06:50:50Z

+      Right forker
+        | Just leashId <- mLeashId -> do
+          traceM $ "My leash id " <> show leashId 
+          atomically $ do


One thing that's worth adding to this discussion is that the main way that clients use LSQ is via short lived connections, so things like the Conn type will change for every query.

The querying functions explicitly always do Acquire -> Query -> Release -> Done (which also means a new thread is spun up for every query).

I've played around with making a slightly longer lived querying client but wasn't happy with it (hopefully the overhead of the extra round-trips isn't too large compared to the query itself). We might want to look at that again if the resource overhead on the node is a problem.

- Adds leashId to MsgDone message - Removes leashId from MsgRelease message

axman6

Looking really good! I think most of my comments are about comments, otherwise the changes look good.

axman6 · 2026-03-30T06:00:04Z

-          (genesisArgs, setLoEinChainDbArgs) <-
-            mkGenesisNodeKernelArgs llrnGenesisConfig
+          let genesisArgs = mkGenesisNodeKernelArgs llrnGenesisConfig
+          varGetLoEFragment <- newTVarIO $ pure ChainDB.LoEDisabled


Do we always want this to start as LoEDisabled? What if the CrucialLsqClients is non-empty?

axman6 · 2026-03-30T06:04:22Z

+    varGenesisLoEFragment <- newTVarIO LoEDisabled 
+    varLoEFragment <- newTVarIO LoEDisabled


Same question as above, but I'm also not sure what value could go in here when not disabled.

axman6 · 2026-03-30T06:12:13Z

@@ -1,5 +1,6 @@
 {-# LANGUAGE DisambiguateRecordFields #-}
 {-# LANGUAGE NamedFieldPuns #-}
+{-# LANGUAGE LambdaCase #-}


I'm disappointed this and OverloadedRecordDot aren't enabled by default in this repo, there's so many places they'd both be useful.

axman6 · 2026-03-30T06:16:18Z

 -- | A 'Watcher' that evaluates the GDD rule whenever necessary, writing the LoE
-- fragment to @varLoEFrag@, and then triggering ChainSel to reprocess all
-- blocks that had previously been postponed by the LoE.
+-- fragment to @varGenesisLoEFrag@, which then is picked up by the leashingWatcher


Suggested change

-- fragment to @varGenesisLoEFrag@, which then is picked up by the leashingWatcher

-- fragment to @varGenesisLoEFrag@, which is then picked up by the leashingWatcher

axman6 · 2026-03-30T06:17:05Z

  -- densities.
  STM m (Map peer (ChainSyncClientHandle m blk)) ->
-  -- | The LoE fragment. It starts at a (recent) immutable tip and ends at
+  -- | The Genesis LoE fragment. It's optional because the Genesis can be disabled.


Suggested change

-- | The Genesis LoE fragment. It's optional because the Genesis can be disabled.

-- | The Genesis LoE fragment. It's optional because Genesis can be disabled.

axman6 · 2026-03-30T06:33:03Z

+import           Ouroboros.Network.AnchoredFragment (AnchoredFragment)
+import qualified Ouroboros.Network.AnchoredFragment as AF
+import Ouroboros.Network.Protocol.LocalStateQuery.Type (LeashId)
+


I feel like we should add a comment in here which explains what leashing actually is, and how it's implemented.

axman6 · 2026-03-30T06:38:28Z

+              ChainDB.LoEEnabled frag -> frag
+              ChainDB.LoEDisabled -> curChain
+          lsqLeashingFrag = fst $ sharedCandidatePrefix prefix lsqLeashingCandidates
+          crucialLsqClientsArePresent = crucialLsqClients == (Set.fromList $ map fst lsqLeashingCandidates)


Shouldn't this be:

Suggested change

crucialLsqClientsArePresent = crucialLsqClients == (Set.fromList $ map fst lsqLeashingCandidates)

crucialLsqClientsArePresent = crucialLsqClients `Set.isSubsetOf` Map.keysSet lsqLeashingState

axman6 · 2026-03-30T06:40:04Z

+          newLoE =
+            if Set.null crucialLsqClients
+            then
+              -- if there are no crucial lsq clients and leashing state is empty, return genesis LoE
+              if Map.null lsqLeashingState
+              then genesisLoE
+              else ChainDB.LoEEnabled lsqLeashingFrag
+            else
+              -- there are crucial lsq clients,
+              -- if they are present, we leash in usual way 
+              -- otherwise return the anchor of the current chain to leash the node at immutable tip
+              if crucialLsqClientsArePresent then ChainDB.LoEEnabled lsqLeashingFrag
+              else ChainDB.LoEEnabled $ AF.Empty $ AF.castAnchor $ AF.anchor curChain


This might be a bit clearer with MultiWayIf.

axman6 · 2026-03-30T06:42:58Z

+      m (Either GetForkerError (ReadOnlyForker' m blk, r))
  -- ^ Acquire a read-only forker at a specific point if that point exists
-  -- on the db.
+  -- on the db, executing an extra STM action.


What for? I think this comments needs more explanation.

Suggested change

-- on the db, executing an extra STM action.

-- on the db, executing the given STM action which is run in the same transaction that creates the forker.

axman6 · 2026-03-30T06:44:15Z

 getTipBlockNo = fmap Network.getTipBlockNo . getCurrentTip

+getReadOnlyForkerAtPoint ::
+  (Monad (STM m), Functor m) =>


Is the Functor m constraint needed here? Or is Monad (STM m) not enough to imply it? I'm guessing probably not (but maybe a MonadSTM m constraint could be enough for both?)

tweag-ev-ak and others added 6 commits February 2, 2026 16:24

Add leashing

c65cc4b

Add leashing

f33caa4

Add leashing 0.27.0.0 leash (#13)

5ac599d

* Update ouroboros-network ref * Add support for LeashID * Support explicit unleashing

wip: fixing tests

9020145

Add leashing

ef8c646

Add leashing

4179875

tweag-ev-ak requested a review from nfrisby as a code owner February 26, 2026 13:59

nfrisby requested changes Feb 26, 2026

View reviewed changes

nfrisby marked this pull request as draft February 26, 2026 21:22

tweag-ev-ak force-pushed the add-leashing-0.30.0.1 branch from 0ecf3e1 to ce1fc45 Compare March 2, 2026 11:12

tweag-ev-ak changed the title ~~Add leashing 0.30.0.1~~ Add LSQ leashing 0.30.0.1 Mar 2, 2026

tweag-ev-ak force-pushed the add-leashing-0.30.0.1 branch from ce1fc45 to 9274b49 Compare March 2, 2026 11:38

Fix review comments

4210cb3

tweag-ev-ak force-pushed the add-leashing-0.30.0.1 branch from 9274b49 to 4210cb3 Compare March 2, 2026 11:54

tweag-ev-ak requested a review from nfrisby March 2, 2026 12:25

tweag-ev-ak added 4 commits March 9, 2026 12:29

Stop throwing AcquireFailurePointStateIsBusy error

f113d1e

Add crucialLsqClients config

be4260c

Cleanup LoE variables

a3161f4

Fix tests

278cd8f

axman6 reviewed Mar 12, 2026

View reviewed changes

tweag-ev-ak added 2 commits March 13, 2026 12:43

Update the ouroboros-network pin.

1be3526

- Adds leashId to MsgDone message - Removes leashId from MsgRelease message

Update the ouroboros-network pin 2

fa25012

tweag-ev-ak force-pushed the add-leashing-0.30.0.1 branch from 3a8f660 to c155bf7 Compare March 30, 2026 04:49

Provide an extra STM action for the forker's acquirement

9f9c384

tweag-ev-ak force-pushed the add-leashing-0.30.0.1 branch from c155bf7 to 9f9c384 Compare March 30, 2026 05:10

axman6 reviewed Mar 30, 2026

View reviewed changes

		varGenesisLoEFragment <- newTVarIO LoEDisabled
		varLoEFragment <- newTVarIO LoEDisabled

	-- fragment to @varGenesisLoEFrag@, which then is picked up by the leashingWatcher
	-- fragment to @varGenesisLoEFrag@, which is then picked up by the leashingWatcher

	-- \| The Genesis LoE fragment. It's optional because the Genesis can be disabled.
	-- \| The Genesis LoE fragment. It's optional because Genesis can be disabled.

	crucialLsqClientsArePresent = crucialLsqClients == (Set.fromList $ map fst lsqLeashingCandidates)
	crucialLsqClientsArePresent = crucialLsqClients `Set.isSubsetOf` Map.keysSet lsqLeashingState

	-- on the db, executing an extra STM action.
	-- on the db, executing the given STM action which is run in the same transaction that creates the forker.

Uh oh!

Conversation

tweag-ev-ak commented Feb 26, 2026

Uh oh!

nfrisby left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tweag-ev-ak Mar 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nfrisby commented Mar 1, 2026

Uh oh!

axman6 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

axman6 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nfrisby left a comment •

edited

Loading

tweag-ev-ak Mar 2, 2026 •

edited

Loading