Add vertex store size limit; cleanup vertex store events #854

LukasGasior1 · 2024-02-27T17:19:32Z

This PR includes:

The primary feature

added a size limit to the vertex store. The limit applies to the size of a dson-serialized vertex store state. The limit is "soft", in a sense that in some circumstances it might be slightly exceeded (e.g. inserting a newer QC/TC that has more signatures than the previous one) - for practicality reasons. The main goal is to protect against SBOR overflow (e.g. when encoding a commit request containing the vertex store) and in practice there should always be a lot of room between this limit and the critical SBOR limit - enough room not to worry about such small values. The limits defaults to 150 Mb and can be configured with bft.vertex_store.max_serialized_size_bytes (min value is 10 Mb).

Additional related changes

vertex store serialization has been moved from state computer to the vertex store itself
updating vertex store metrics has been moved to the vertex store itself
vertex store events now include the serialized state to be persisted

Drive-by

small refactor to vertex store events: they're now records and there's no longer a separate BFTCommittedUpdate event. Since a (consensus) commit can only happen as a result of QC insertion, this has been captured in BFTHighQCUpdate, which allowed us to get rid of the weird logic in dispatchPostQcInsertionEvents in VertexStoreAdapter
added vertex store persist after inserting a TC (which was missing)

github-actions · 2024-02-27T18:38:20Z

Docker tags
docker.io/radixdlt/private-babylon-node:pr-854
docker.io/radixdlt/private-babylon-node:f659ccb3b1
docker.io/radixdlt/private-babylon-node:sha-f659ccb

jakrawcz-rdx

The vertex store size limit feature (i.e. break vertices_loop;) looks very much ok 👍 (I mean, I was able to understand it, and it seems to work [outside of corner-cases where "really small stuff is added to vertex store without checking size"]).

I can't say I understood 100% of the rest (i.e. the refactor) :(
I left some minor "technical Java" comments.
It does look more structured than before, but I definitely lack the Consensus knowledge to spot any new subtle bugs.

If you have faith in our Consensus regression tests - feel free to merge with my 🟢 .
Otherwise - I recommend waiting for more reviewers.

jakrawcz-rdx · 2024-03-05T17:16:53Z

common/src/main/java/com/radixdlt/monitoring/Metrics.java

@@ -233,7 +233,12 @@ public record Sync(
        Counter invalidEpochInitialQcSyncStates) {}

    public record VertexStore(
-        Gauge size, Counter forks, Counter rebuilds, Counter indirectParents) {}
+        Gauge size,


vertexCount?

sounds good 👍
note to self: update this on the dashboard later (if it's used)

jakrawcz-rdx · 2024-03-05T18:46:58Z

core/src/main/java/com/radixdlt/RadixNodeModule.java

+        vertexStoreConfig.maxSerializedSizeBytes()
+            >= VertexStoreConfig.MIN_MAX_SERIALIZED_SIZE_BYTES,
+        "Invalid configuration: bft.vertex_store.max_serialized_size_byte must be at least "
+            + VertexStoreConfig.MIN_MAX_SERIALIZED_SIZE_BYTES);


(nitpick) Preconditions has a built-int ("formatting %s", ...args) syntax

jakrawcz-rdx · 2024-03-05T21:24:26Z

core/src/main/java/com/radixdlt/consensus/vertexstore/VertexStoreJavaImpl.java

@@ -350,43 +431,15 @@ private void removeVertexAndPruneInternal(HashCode vertexId, HashCode skip) {
    var children = vertexChildren.remove(vertexId);
    if (children != null) {
      for (HashCode child : children) {
-        if (!child.equals(skip)) {
-          removeVertexAndPruneInternal(child, null);
+        if (!skip.map(child::equals).orElse(false)) {


Suggested change

if (!skip.map(child::equals).orElse(false)) {

if (!Optional.of(child).equals(skip)) {

jakrawcz-rdx · 2024-03-05T21:27:40Z

core/src/main/java/com/radixdlt/consensus/vertexstore/VertexStoreJavaImpl.java

+        .flatMap(
+            removedVertex ->
+                Optional.ofNullable(vertexChildren.get(removedVertex.vertex().getParentVertexId())))
+        .ifPresent(siblings -> siblings.remove(vertexId));


some lines could be a lot less awkward if this.vertexChildren was really a Multimap<HashCode, HashCode> (e.g. = HashMultimap.create())

Splendid idea! Done.

sonarcloud · 2024-03-06T17:21:42Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
90.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

dhedey

Generally really nice changes. I like the event fixing, the metrics tweaks, and the use of sealed interfaces for better error propagation.

The code in VertexStoreImpl isn't the prettiest, but then again, the whole class isn't great to start with... I wonder if we should change the size check to a simple check on current (cached) serialized size before we try inserting each vertex; rather than a check of the state-after-inserting-the-vertex? This would reduce the work done on the happy path, and mean we can be happier in not checking the size on the other code paths which just update the highQcs. Anyway, just an idea.

I don't think any of these notes are blockers, but ideally some of the more important ones could do with a small fix before we merge.

dhedey · 2024-04-16T12:32:59Z

core/src/main/java/com/radixdlt/consensus/bft/BFTHighQCUpdate.java

+ * An event emitted when vertex store updates its highQC, which possibly results in some vertices
+ * being committed.
+ */
+public record BFTHighQCUpdate(


We've lost the nice toString here - perhaps we should override it?

Otherwise we could end up with a massive log of the serialized vertex state in hex.

dhedey · 2024-04-16T12:33:45Z

core/src/main/java/com/radixdlt/consensus/bft/BFTInsertUpdate.java

-  }
-}
+/** An event emitted after a vertex has been inserted into the vertex store. */
+public record BFTInsertUpdate(


We've lost the nice toString here - perhaps we should override it?

Otherwise we could end up with a massive log of the serialized vertex state in hex.

dhedey · 2024-04-16T12:34:12Z

core/src/main/java/com/radixdlt/consensus/bft/BFTRebuildUpdate.java

-  }
-}
+/** An even emitted when the vertex store has been rebuilt. */
+public record BFTRebuildUpdate(


We've lost the nice toString here - perhaps we should override it?

Otherwise we could end up with a massive log of the serialized vertex state in hex.

dhedey · 2024-04-16T12:40:47Z

core/src/main/java/com/radixdlt/consensus/vertexstore/VertexStoreAdapter.java

  }

  public boolean insertTimeoutCertificate(TimeoutCertificate timeoutCertificate) {
-    return vertexStore.insertTimeoutCertificate(timeoutCertificate);
+    final var result = vertexStore.insertTimeoutCertificate(timeoutCertificate);


For code beauty, can we align the insertQc below with this?

e.g. the code below should be insertQuorumCertificate, it should match on result, and we can inline dispatchPostQcInsertionEvents

dhedey · 2024-04-16T12:54:31Z

core/src/main/java/com/radixdlt/consensus/vertexstore/VertexStoreJavaImpl.java

    if (hasAnyChildren) {
      // TODO: Check to see if qc's match in case there's a fault
      return new VertexStore.InsertQcResult.Ignored();
    }

-    // proposed vertex doesn't have any children
+    // Proposed vertex doesn't have any children


<sidenote>Still really want to rename HighQC => HighCertificates to make this less confusing :D</sidenote>

dhedey · 2024-04-16T13:53:00Z

core/src/main/java/com/radixdlt/ledger/StateComputerLedger.java

+      highQcUpdate
+          .committedVertices()
+          .ifPresent(
+              committedVertices -> {


Just as a minor style thing - this is nested quite deep. Perhaps we should consider either pulling out some methods, or returning early in the not-present case to de-nest things.

dhedey · 2024-04-16T13:55:14Z

core/src/main/java/com/radixdlt/modules/DispatcherModule.java

-      @ProcessOnDispatch Set<EventProcessor<BFTInsertUpdate>> processors,
-      Environment environment,
-      Metrics metrics) {
+      @ProcessOnDispatch Set<EventProcessor<BFTInsertUpdate>> processors, Environment environment) {


Nice moving of metrics into the vertex store. I think this makes this simpler to comprehend.

dhedey · 2024-04-16T14:03:45Z

...src/test-core/java/com/radixdlt/harness/simulation/monitors/consensus/LivenessInvariant.java

                  },
-                  BFTCommittedUpdate.class);
+                  BFTHighQCUpdate.class);


I think this code is now a duplicate so should be deleted?

yup, good catch

dhedey · 2024-04-16T14:06:57Z

core/src/main/java/com/radixdlt/ledger/StateComputerLedger.java

@@ -162,7 +161,8 @@ StateComputerPrepareResult prepare(
        List<RawNotarizedTransaction> proposedTransactions,
        RoundDetails roundDetails);

-    LedgerProofBundle commit(LedgerExtension ledgerExtension, VertexStoreState vertexStore);
+    LedgerProofBundle commit(
+        LedgerExtension ledgerExtension, Option<byte[]> serializedVertexStoreState);


Out of interest, why is this Option<byte[]> and not Option<WrappedByteArray>?

no reason at all, I'll change to Option<WrappedByteArray>

dhedey · 2024-04-16T14:08:45Z

...lt/integration/steady_state/deterministic/consensus/DivergentExecutionLivenessBreakTest.java

+*            and the forced divergent vertex execution in `prepare` is reverted.
+ */
+// spotless:on
+public final class DivergentExecutionLivenessBreakTest {


Nice test 👍

…re-overflow-mitigations

codecov-commenter · 2024-06-12T15:02:07Z

Codecov Report

Attention: Patch coverage is 83.03571% with 38 lines in your changes missing coverage. Please review.

Project coverage is 42.8%. Comparing base (71327ad) to head (e791b13).

Additional details and impacted files

@@             Coverage Diff             @@
##             develop    #854     +/-   ##
===========================================
- Coverage       42.8%   42.8%   -0.1%     
+ Complexity      4299    4295      -4     
===========================================
  Files           1692    1692             
  Lines          51999   52035     +36     
  Branches        1494    1496      +2     
===========================================
+ Hits           22298   22312     +14     
- Misses         29231   29250     +19     
- Partials         470     473      +3

Flag	Coverage Δ
rust	`42.8% <83.0%> (-0.1%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
...src/main/java/com/radixdlt/monitoring/Metrics.java	`0.0% <ø> (ø)`
...adixdlt/consensus/epoch/EpochsConsensusModule.java	`84.2% <ø> (-0.3%)`	⬇️
...ava/com/radixdlt/consensus/liveness/Pacemaker.java	`92.3% <100.0%> (ø)`
...ixdlt/consensus/vertexstore/VertexStoreConfig.java	`100.0% <100.0%> (ø)`
...dixdlt/consensus/vertexstore/VertexStoreState.java	`79.7% <100.0%> (-1.1%)`	⬇️
...in/java/com/radixdlt/modules/DispatcherModule.java	`92.9% <100.0%> (+1.0%)`	⬆️
...c/main/java/com/radixdlt/modules/LedgerModule.java	`100.0% <100.0%> (ø)`
...in/java/com/radixdlt/modules/SystemInfoModule.java	`69.2% <ø> (-2.2%)`	⬇️
...main/java/com/radixdlt/rev2/REv2StateComputer.java	`97.1% <100.0%> (-0.1%)`	⬇️
...radixdlt/rev2/modules/MockedVertexStoreModule.java	`0.0% <ø> (ø)`
... and 11 more

... and 1 file with indirect coverage changes

sonarcloud · 2024-06-12T15:04:31Z

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
85.5% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

dhedey · 2024-06-28T14:28:35Z

core/src/main/java/com/radixdlt/rev2/REv2StateComputer.java

    final var proof = ledgerExtension.proof();
    final var header = proof.ledgerHeader();

    var commitRequest =
        new CommitRequest(
            ledgerExtension.transactions(),
            proof,
-            serializedVertexStoreState,
+            serializedVertexStoreState.map(WrappedByteArray::value),


We should just always handle it as a WrappedByteArray I think? It's just safer than passing around a big byte[].

If we need to, let's add an SborCodec for WrappedByteArray:

public static void registerCodec(CodecMap codecMap) { codecMap.register( WrappedByteArray.class, codecs -> new CustomByteArrayCodec<>( WrappedByteArray::value, WrappedByteArray::new)); }

LukasGasior1 force-pushed the feature/vertex-store-overflow-mitigations branch 3 times, most recently from 3ac1d06 to b8f0707 Compare February 27, 2024 17:50

Add vertex store size limit; cleanup vertex store events

501a3bf

LukasGasior1 force-pushed the feature/vertex-store-overflow-mitigations branch from b8f0707 to 501a3bf Compare February 27, 2024 18:05

jakrawcz-rdx approved these changes Mar 5, 2024

View reviewed changes

LukasGasior1 added 2 commits March 6, 2024 17:39

Rename vertex store size metric to vertexCount

783d36f

Use Multimap for VertexStore's vertexChildren

a79c14b

dhedey self-assigned this Mar 22, 2024

dhedey reviewed Apr 16, 2024

View reviewed changes

Merge remote-tracking branch 'origin/develop' into feature/vertex-sto…

b707db8

…re-overflow-mitigations

Address code review comments

1ad56a4

LukasGasior1 force-pushed the feature/vertex-store-overflow-mitigations branch from e791b13 to 1ad56a4 Compare June 12, 2024 16:16

dhedey reviewed Jun 28, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add vertex store size limit; cleanup vertex store events #854

Add vertex store size limit; cleanup vertex store events #854

LukasGasior1 commented Feb 27, 2024

github-actions bot commented Feb 27, 2024 •

edited

Loading

jakrawcz-rdx left a comment

jakrawcz-rdx Mar 5, 2024

LukasGasior1 Mar 6, 2024

jakrawcz-rdx Mar 5, 2024

jakrawcz-rdx Mar 5, 2024

jakrawcz-rdx Mar 5, 2024

LukasGasior1 Mar 6, 2024

sonarcloud bot commented Mar 6, 2024

dhedey left a comment •

edited

Loading

dhedey Apr 16, 2024

dhedey Apr 16, 2024

dhedey Apr 16, 2024

dhedey Apr 16, 2024

dhedey Apr 16, 2024

dhedey Apr 16, 2024

dhedey Apr 16, 2024

dhedey Apr 16, 2024

dhedey Apr 16, 2024

dhedey Apr 16, 2024

dhedey Apr 16, 2024

LukasGasior1 Jun 11, 2024

dhedey Apr 16, 2024

LukasGasior1 Jun 11, 2024

dhedey Apr 16, 2024

codecov-commenter commented Jun 12, 2024

sonarcloud bot commented Jun 12, 2024

dhedey Jun 28, 2024

	if (!skip.map(child::equals).orElse(false)) {
	if (!Optional.of(child).equals(skip)) {

Add vertex store size limit; cleanup vertex store events #854

Are you sure you want to change the base?

Add vertex store size limit; cleanup vertex store events #854

Conversation

LukasGasior1 commented Feb 27, 2024

github-actions bot commented Feb 27, 2024 • edited Loading

jakrawcz-rdx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Mar 6, 2024

Quality Gate passed

dhedey left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Jun 12, 2024

Codecov Report

sonarcloud bot commented Jun 12, 2024

Quality Gate passed

Choose a reason for hiding this comment

github-actions bot commented Feb 27, 2024 •

edited

Loading

dhedey left a comment •

edited

Loading