Rewrite to:

* make invariants more clear and enforced in only one place
* not track the previous entire RIB unless necessary

The main thing missing was to defer processing of external
advertisements to the first round of BGP instead of doing it
in the neutral zone between IGP and EGP computations.

projects/batfish/src/main/java/org/batfish/dataplane/ibdp/BgpRoutingProcess.java

@@ -126,6 +126,13 @@ final class BgpRoutingProcess implements RoutingProcess<BgpTopology, BgpRoute<?,
   @Nonnull @VisibleForTesting
   SortedMap<EdgeId, Queue<RouteAdvertisement<EvpnType3Route>>> _evpnType3IncomingRoutes;
 
+  /**
+   * External BGP announcements to be processed upon the first iteration of BGP on this node.
+   *
+   * <p>Always null after that first iteration.
+   */
+  @Nullable Set<BgpAdvertisement> _externalAdvertisements;
+
   // RIBs and RIB delta builders
   /** Helper RIB containing all paths obtained with external BGP, for IPv4 unicast */
   @Nonnull final Bgpv4Rib _ebgpv4Rib;
@@ -140,10 +147,11 @@ final class BgpRoutingProcess implements RoutingProcess<BgpTopology, BgpRoute<?,
   // currently used for VRF-leaking only.
   @Nonnull private RibDelta<Bgpv4Route> _localDeltaPrev = RibDelta.empty();
 
-  // copy of RIBs from prev round, for new links in the current round
-  @Nonnull private Set<Bgpv4Route> _ebgpv4Prev = ImmutableSet.of();
-  @Nonnull private Set<Bgpv4Route> _bgpv4Prev = ImmutableSet.of();
-  @Nonnull private Set<AnnotatedRoute<AbstractRoute>> _mainRibPrev = ImmutableSet.of();
+  // copy of RIBs from prev round, for new links in the current round.
+  // Nullable so they crash on improper use.
+  private @Nullable Set<Bgpv4Route> _ebgpv4Prev;
+  private @Nullable Set<Bgpv4Route> _bgpv4Prev;
+  private @Nullable Set<AnnotatedRoute<AbstractRoute>> _mainRibPrev;
 
   /** Combined BGP (both iBGP and eBGP) RIB, for IPv4 unicast */
   @Nonnull Bgpv4Rib _bgpv4Rib;
@@ -394,8 +402,7 @@ Stream<EdgeId> getEdgeIdStream(
   }
 
   @Override
-  public void updateTopology(
-      BgpTopology topology, Set<AnnotatedRoute<AbstractRoute>> mainRibPrevRound) {
+  public void updateTopology(BgpTopology topology) {
     BgpTopology oldTopology = _topology;
     _topology = topology;
     initBgpQueues(_topology);
@@ -418,7 +425,23 @@ public void updateTopology(
                 .collect(ImmutableSet.toImmutableSet()),
             getEdgeIdStream(oldTopology.getGraph(), BgpPeerConfig::getEvpnAddressFamily, Type.EVPN)
                 .collect(ImmutableSet.toImmutableSet()));
-    _mainRibPrev = mainRibPrevRound;
+    if (!_unicastEdgesWentUp.isEmpty()) {
+      // We copy these three RIBs here instead of at the end of the previous round to avoid
+      // unnecessary space/time use. However, this process is only correct if the RIBs have not
+      // changed since the end of the previous round. That means deltas must (still) be empty.
+      // Main RIB invariant is checked at the VirtualRouter caller, since this class does not
+      // have access to the main RIB delta.
+      assert _bgpv4DeltaBuilder.isEmpty();
+      assert _ebgpv4DeltaBuilder.isEmpty();
+
+      _mainRibPrev = _mainRib.getTypedRoutes();
+      _bgpv4Prev = _bgpv4Rib.getTypedRoutes();
+      _ebgpv4Prev = _ebgpv4Rib.getTypedRoutes();
+    } else {
+      assert _mainRibPrev == null;
+      assert _bgpv4Prev == null;
+      assert _ebgpv4Prev == null;
+    }
     _topology = topology;
     // TODO: compute edges that went down, remove routes we received from those neighbors
   }
@@ -689,6 +712,13 @@ void processBgpV4UnicastMessages(
     ribDeltaBuilders.put(_ebgpv4Rib, RibDelta.builder());
     ribDeltaBuilders.put(_ibgpv4Rib, RibDelta.builder());
 
+    // If there are any, process external advertisements. This will only be true once, the first
+    // time any BGP routes are pulled.
+    if (_externalAdvertisements != null) {
+      _externalAdvertisements.forEach(a -> processExternalBgpAdvertisement(a, ribDeltaBuilders));
+      _externalAdvertisements = null;
+    }
+
     // Process updates from each neighbor
     for (EdgeId edgeId : _bgpv4Edges) {
       pullV4UnicastMessages(
@@ -811,7 +841,6 @@ private void pullV4UnicastMessages(
         }
       } else {
         // Merge into staging rib, note delta
-
         ribDeltas.get(targetRib).from(targetRib.mergeRouteGetDelta(transformedIncomingRoute));
         if (useRibGroups) {
           perNeighborDeltaForRibGroups.add(annotatedTransformedRoute);
@@ -1537,62 +1566,56 @@ private void processExternalBgpAdvertisement(
   }
 
   /**
-   * Initializes BGP RIBs prior to any dataplane iterations based on the external BGP advertisements
-   * coming into the network.
-   *
-   * <p>Note: assumes the external advertisements are pre-transformation and will run import policy
-   * on them, if present.
+   * Identifies the given external advertisements for this node and saves them. They will be
+   * processed at the start of the BGP computation.
    *
    * @param externalAdverts a set of external BGP advertisements
    * @param ipVrfOwners mapping of IPs to their owners in our network
    */
-  void processExternalBgpAdvertisements(
+  void stageExternalAdvertisements(
       Set<BgpAdvertisement> externalAdverts, Map<Ip, Map<String, Set<String>>> ipVrfOwners) {
+    // Retain only advertisements that are valid, and stage them for processing once we start up.
+    _externalAdvertisements =
+        externalAdverts.stream()
+            .filter(
+                advert -> {
+                  // If it is not for us, ignore it
+                  if (!advert.getDstNode().equals(_hostname)) {
+                    return false;
+                  }
 
-    // Keep track of changes to the RIBs using delta builders, keyed by RIB type
-    Map<Bgpv4Rib, RibDelta.Builder<Bgpv4Route>> ribDeltas = new IdentityHashMap<>();
-    ribDeltas.put(_ebgpv4Rib, RibDelta.builder());
-    ribDeltas.put(_ibgpv4Rib, RibDelta.builder());
-
-    // Process each BGP advertisement
-    for (BgpAdvertisement advert : externalAdverts) {
-
-      // If it is not for us, ignore it
-      if (!advert.getDstNode().equals(_hostname)) {
-        continue;
-      }
-
-      // If we don't own the IP for this advertisement, ignore it
-      Ip dstIp = advert.getDstIp();
-      Map<String, Set<String>> dstIpOwners = ipVrfOwners.get(dstIp);
-      String hostname = _hostname;
-      if (dstIpOwners == null || !dstIpOwners.containsKey(hostname)) {
-        continue;
-      }
+                  // If we don't own the IP for this advertisement, ignore it
+                  Ip dstIp = advert.getDstIp();
+                  Map<String, Set<String>> dstIpOwners = ipVrfOwners.get(dstIp);
+                  if (dstIpOwners == null || !dstIpOwners.containsKey(_hostname)) {
+                    return false;
+                  }
 
-      Ip srcIp = advert.getSrcIp();
-      // TODO: support passive and unnumbered bgp connections
-      Prefix srcPrefix = srcIp.toPrefix();
-      BgpPeerConfig neighbor = _process.getActiveNeighbors().get(srcPrefix);
-      if (neighbor == null) {
-        continue;
-      }
+                  Ip srcIp = advert.getSrcIp();
+                  // TODO: support passive and unnumbered bgp connections
+                  Prefix srcPrefix = srcIp.toPrefix();
+                  BgpPeerConfig neighbor = _process.getActiveNeighbors().get(srcPrefix);
+                  if (neighbor == null) {
+                    return false;
+                  }
 
-      if (advert.getType().isEbgp()
-          && advert.getAsPath().containsAs(neighbor.getLocalAs())
-          && !neighbor
-              .getIpv4UnicastAddressFamily()
-              .getAddressFamilyCapabilities()
-              .getAllowLocalAsIn()) {
-        // skip routes containing this peer's AS unless the session is configured to allow loops.
-        continue;
-      }
+                  if (advert.getType().isEbgp()
+                      && advert.getAsPath().containsAs(neighbor.getLocalAs())
+                      && !neighbor
+                          .getIpv4UnicastAddressFamily()
+                          .getAddressFamilyCapabilities()
+                          .getAllowLocalAsIn()) {
+                    // skip routes containing this peer's AS unless the session is configured to
+                    // allow loops.
+                    return false;
+                  }
 
-      processExternalBgpAdvertisement(advert, ribDeltas);
+                  return true;
+                })
+            .collect(ImmutableSet.toImmutableSet());
+    if (_externalAdvertisements.isEmpty()) {
+      _externalAdvertisements = null;
     }
-
-    // Propagate received routes through all the RIBs
-    unstage(ribDeltas);
   }
 
   /**
@@ -1703,17 +1726,18 @@ Once the route is leaked to a new VRF it can become routing again (it could have
   }
 
   public void endOfRound() {
-    _bgpv4Prev = _bgpv4Rib.getTypedRoutes();
     _bgpv4DeltaPrev = _bgpv4DeltaBuilder.build();
     _bgpv4DeltaBuilder = RibDelta.builder();
-    _ebgpv4Prev = _ebgpv4Rib.getTypedRoutes();
     _ebgpv4DeltaPrev = _ebgpv4DeltaBuilder.build();
     _ebgpv4DeltaBuilder = RibDelta.builder();
     _localDeltaPrev = _localDeltaBuilder.build();
     _localDeltaBuilder = RibDelta.builder();
-    _mainRibPrev = ImmutableSet.of();
+    // Delete all the state from the start of a topology round.
     _evpnEdgesWentUp = ImmutableSet.of();
     _unicastEdgesWentUp = ImmutableSet.of();
+    _mainRibPrev = null;
+    _bgpv4Prev = null;
+    _ebgpv4Prev = null;
     // Legacy redistribution map
     _toRedistribute = new HashMap<>();
   }

projects/batfish/src/main/java/org/batfish/dataplane/ibdp/EigrpRoutingProcess.java

@@ -128,8 +128,7 @@ public void initialize(Node n) {
   }
 
   @Override
-  public void updateTopology(
-      EigrpTopology topology, Set<AnnotatedRoute<AbstractRoute>> mainRibPrevRound) {
+  public void updateTopology(EigrpTopology topology) {
     EigrpTopology oldTopology = _topology;
     _topology = topology;
     updateQueues(_topology);

projects/batfish/src/main/java/org/batfish/dataplane/ibdp/OspfRoutingProcess.java

@@ -250,8 +250,7 @@ private void updateQueues(OspfTopology topology) {
   }
 
   @Override
-  public void updateTopology(
-      OspfTopology topology, Set<AnnotatedRoute<AbstractRoute>> mainRibPrevRound) {
+  public void updateTopology(OspfTopology topology) {
     _topology = topology;
     updateQueues(topology);
     /*

projects/batfish/src/main/java/org/batfish/dataplane/ibdp/RoutingProcess.java

@@ -1,7 +1,6 @@
 package org.batfish.dataplane.ibdp;
 
 import java.util.Map;
-import java.util.Set;
 import javax.annotation.Nonnull;
 import javax.annotation.ParametersAreNonnullByDefault;
 import org.batfish.datamodel.AbstractRoute;
@@ -27,10 +26,8 @@ public interface RoutingProcess<T, R extends AbstractRouteDecorator> {
    * protocol-specific topology is available, which could result in neighbor adjacency updates.
    *
    * @param topology the updated protocol-specific topology
-   * @param mainRibPrevRound the routes in the main RIB at the end of the previous round. Will be
-   *     sent to new edges in a protocol-specific way.
    */
-  void updateTopology(T topology, Set<AnnotatedRoute<AbstractRoute>> mainRibPrevRound);
+  void updateTopology(T topology);
 
   /**
    * Execute one iteration of dataplane computation. Involves processing route updates from

projects/batfish/src/main/java/org/batfish/dataplane/ibdp/VirtualRouter.java

@@ -154,11 +154,6 @@ public final class VirtualRouter {
    * Will inform redistribution/VRF leaking in current round.
    */
   RibDelta<AnnotatedRoute<AbstractRoute>> _mainRibDeltaPrevRound;
-  /**
-   * The state of the main RIB after the previous round. This will be used for new sessions that
-   * come up.
-   */
-  Set<AnnotatedRoute<AbstractRoute>> _mainRibPrevRound;
 
   /** The VRF name for this virtual router */
   @Nonnull private final String _name;
@@ -210,7 +205,6 @@ public final class VirtualRouter {
     _mainRib = new Rib();
     _mainRibs = ImmutableMap.of(RibId.DEFAULT_RIB_NAME, _mainRib);
     _mainRibDeltaPrevRound = RibDelta.empty();
-    _mainRibPrevRound = ImmutableSet.of();
     _mainRibRouteDeltaBuilder = RibDelta.builder();
     _routesForIsisRedistribution = RibDelta.builder();
     // Init rest of the RIBs
@@ -337,15 +331,18 @@ private void initEigrp() {
    * Prepare for the EGP part of the computation. Handles updating routing processes given new
    * topology information.
    *
+   * <p>Must be called between rounds, aka, all delta builder should be empty.
+   *
    * @param topologyContext The various network topologies
    */
   void initForEgpComputationWithNewTopology(TopologyContext topologyContext) {
-    // Save the main RIB at the current state, so that it can be used in new sessions.
+    assert _mainRibRouteDeltaBuilder.isEmpty(); // or else invariant is not maintained
+
+    initQueuesAndDeltaBuilders(topologyContext);
     if (_bgpRoutingProcess != null) {
       // If the process exists, update the topology
-      _bgpRoutingProcess.updateTopology(topologyContext.getBgpTopology(), _mainRibPrevRound);
+      _bgpRoutingProcess.updateTopology(topologyContext.getBgpTopology());
     }
-    initQueuesAndDeltaBuilders(topologyContext, _mainRibPrevRound);
   }
 
   /**
@@ -358,12 +355,13 @@ void initForEgpComputationBeforeTopologyLoop(
     Merge post-IGP main rib in to a mainRibDelta.
     This effectively makes the entire IGP computation a "previous round".
     */
-    _mainRibPrevRound = _mainRib.getTypedRoutes();
     _mainRibDeltaPrevRound =
-        RibDelta.<AnnotatedRoute<AbstractRoute>>builder().add(_mainRibPrevRound).build();
+        RibDelta.<AnnotatedRoute<AbstractRoute>>builder().add(_mainRib.getTypedRoutes()).build();
+    _mainRibRouteDeltaBuilder = RibDelta.builder();
+
     if (_bgpRoutingProcess != null && !_bgpRoutingProcess.isInitialized()) {
       _bgpRoutingProcess.initialize(_node);
-      _bgpRoutingProcess.processExternalBgpAdvertisements(externalAdverts, ipVrfOwners);
+      _bgpRoutingProcess.stageExternalAdvertisements(externalAdverts, ipVrfOwners);
     }
   }
 
@@ -373,12 +371,11 @@ void initForEgpComputationBeforeTopologyLoop(
    * @param topologyContext The various network topologies
    */
   @VisibleForTesting
-  void initQueuesAndDeltaBuilders(
-      TopologyContext topologyContext, Set<AnnotatedRoute<AbstractRoute>> mainRibPrevRound) {
+  void initQueuesAndDeltaBuilders(TopologyContext topologyContext) {
     // Update topology/re-initialize message queues for EIGRP neighbors
     _eigrpProcesses
         .values()
-        .forEach(proc -> proc.updateTopology(topologyContext.getEigrpTopology(), mainRibPrevRound));
+        .forEach(proc -> proc.updateTopology(topologyContext.getEigrpTopology()));
     // Initialize message queues for each IS-IS neighbor
     initIsisQueues(topologyContext.getIsisTopology());
     // Initialize message queues for all neighboring VRFs/VirtualRouters
@@ -1472,7 +1469,6 @@ void mergeBgpRoutesToMainRib() {
   /** End of a single "EGP" routing round. */
   void endOfEgpRound() {
     _mainRibDeltaPrevRound = _mainRibRouteDeltaBuilder.build();
-    _mainRibPrevRound = _mainRib.getTypedRoutes();
     _mainRibRouteDeltaBuilder = RibDelta.builder();
     if (_bgpRoutingProcess != null) {
       _bgpRoutingProcess.endOfRound();

projects/batfish/src/test/java/org/batfish/dataplane/ibdp/BgpRoutingProcessTest.java

@@ -371,7 +371,7 @@ public void testQueueInitializationAddressFamiliesMustOverlap() {
         peer1Id,
         sessionBuilderReverse.setAddressFamilies(ImmutableSet.of(Type.IPV4_UNICAST)).build());
     topology = new BgpTopology(graph);
-    routingProcess.updateTopology(topology, ImmutableSet.of());
+    routingProcess.updateTopology(topology);
 
     assertThat(
         routingProcess
@@ -508,8 +508,8 @@ public void testResendInitializationOnTopologyUpdate() {
         peer2Id,
         peer1Id,
         sessionBuilderReverse.setAddressFamilies(ImmutableSet.of(Type.EVPN)).build());
-    routingProcNode1.updateTopology(new BgpTopology(graph), ImmutableSet.of());
-    routingProcNode2.updateTopology(new BgpTopology(graph), ImmutableSet.of());
+    routingProcNode1.updateTopology(new BgpTopology(graph));
+    routingProcNode2.updateTopology(new BgpTopology(graph));
     routingProcNode1.executeIteration(
         ImmutableSortedMap.of(
             node.getConfiguration().getHostname(),

projects/batfish/src/test/java/org/batfish/dataplane/ibdp/OspfRoutingProcessTest.java

@@ -477,7 +477,7 @@ public void testComputeDefaultInterAreaRouteToInjectRepeatedCalls() {
 
   @Test
   public void testUpdateTopology() {
-    _routingProcess.updateTopology(nonEmptyOspfTopology(), ImmutableSet.of());
+    _routingProcess.updateTopology(nonEmptyOspfTopology());
     OspfNeighborConfigId n1 =
         new OspfNeighborConfigId(HOSTNAME, VRF_NAME, "1", ACTIVE_IFACE_NAME, ACTIVE_ADDR_1);
     OspfNeighborConfigId n2 =
@@ -490,7 +490,7 @@ public void testUpdateTopology() {
 
   @Test
   public void updateTopologyIsNonDestructive() {
-    _routingProcess.updateTopology(nonEmptyOspfTopology(), ImmutableSet.of());
+    _routingProcess.updateTopology(nonEmptyOspfTopology());
     OspfNeighborConfigId n1 =
         new OspfNeighborConfigId(HOSTNAME, VRF_NAME, "1", ACTIVE_IFACE_NAME, ACTIVE_ADDR_1);
     OspfNeighborConfigId n2 =
@@ -505,7 +505,7 @@ public void updateTopologyIsNonDestructive() {
                     .setNextHopIp(Ip.parse("8.8.8.8"))
                     .build())));
     // Re-update topology
-    _routingProcess.updateTopology(nonEmptyOspfTopology(), ImmutableSet.of());
+    _routingProcess.updateTopology(nonEmptyOspfTopology());
     // Ensure still in dirty state (didn't lose the queued advertisement)
     assertTrue(_routingProcess.isDirty());
   }