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

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

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
501a3bf
Add vertex store size limit; cleanup vertex store events
LukasGasior1 Feb 27, 2024
783d36f
Rename vertex store size metric to vertexCount
LukasGasior1 Mar 6, 2024
a79c14b
Use Multimap for VertexStore's vertexChildren
LukasGasior1 Mar 6, 2024
b707db8
Merge remote-tracking branch 'origin/develop' into feature/vertex-sto…
LukasGasior1 Jun 11, 2024
1ad56a4
Address code review comments
LukasGasior1 Jun 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 6 additions & 12 deletions core-rust-bridge/src/main/java/com/radixdlt/api/EngineStateApiServerModule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,10 +68,10 @@
import com.google.inject.Provides;
import com.google.inject.Singleton;
import com.google.inject.multibindings.ProvidesIntoSet;
import com.radixdlt.consensus.bft.BFTHighQCUpdate;
import com.radixdlt.harness.invariants.SafetyChecker;
import com.radixdlt.harness.simulation.TestInvariant;
import java.util.Optional;
import com.radixdlt.environment.EngineStateApiServerConfig;
import com.radixdlt.environment.NodeAutoCloseable;
import com.radixdlt.environment.NodeRustEnvironment;
import com.radixdlt.utils.UInt32;

public final class EngineStateApiServerModule extends AbstractModule {

Expand All @@ -89,13 +89,7 @@ private EngineStateApiServer engineStateApiServer(NodeRustEnvironment nodeRustEn
}

@ProvidesIntoSet
public NodeEvents.NodeEventProcessor<?> safetyCheckProcessor(SafetyChecker safetyChecker) {
return new NodeEvents.NodeEventProcessor<>(
BFTHighQCUpdate.class,
(node, update) -> {
Optional<TestInvariant.TestInvariantError> maybeError =
safetyChecker.process(node, update);
assertThat(maybeError).isEmpty();
});
NodeAutoCloseable closeable(EngineStateApiServer engineStateApiServer) {
return engineStateApiServer::stop;
}
}
4 changes: 2 additions & 2 deletions core-rust/state-manager/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,6 @@ pub use crate::types::*;
pub mod engine_prelude {
pub use radix_common::prelude::*;

pub use radix_engine::*;
pub use radix_engine::errors::*;
pub use radix_engine::system::bootstrap::*;
#[cfg(feature = "db_checker")]
Expand All @@ -109,9 +108,10 @@ pub mod engine_prelude {
pub use radix_engine::transaction::*;
pub use radix_engine::updates::*;
pub use radix_engine::vm::*;
pub use radix_engine::*;

pub use radix_engine_interface::blueprints::transaction_processor::*;
pub use radix_engine_interface::blueprints::account::*;
pub use radix_engine_interface::blueprints::transaction_processor::*;
pub use radix_engine_interface::prelude::*;

pub use radix_substate_store_impls::state_tree::tree_store::*;
Expand Down
23 changes: 13 additions & 10 deletions ...integration/steady_state/deterministic/consensus/DivergentExecutionLivenessBreakTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,13 +90,13 @@
import com.radixdlt.modules.FunctionalRadixNodeModule.NodeStorageConfig;
import com.radixdlt.modules.StateComputerConfig;
import com.radixdlt.monitoring.Metrics;
import com.radixdlt.networks.Network;
import com.radixdlt.p2p.NodeId;
import com.radixdlt.rev2.Decimal;
import com.radixdlt.rev2.REV2TransactionGenerator;
import com.radixdlt.rev2.REv2StateComputer;
import com.radixdlt.rev2.REv2TransactionsAndProofReader;
import com.radixdlt.transactions.RawNotarizedTransaction;
import com.radixdlt.utils.WrappedByteArray;
import java.util.List;
import java.util.Random;
import java.util.function.Consumer;
Expand Down Expand Up @@ -196,7 +196,8 @@ public StateComputerLedger.StateComputerPrepareResult prepare(

@Override
public LedgerProofBundle commit(
LedgerExtension ledgerExtension, Option<byte[]> serializedVertexStoreState) {
LedgerExtension ledgerExtension,
Option<WrappedByteArray> serializedVertexStoreState) {
return underlyingStateComputer.commit(ledgerExtension, serializedVertexStoreState);
}
};
Expand Down Expand Up @@ -235,14 +236,16 @@ private DeterministicTest createTest() {
FunctionalRadixNodeModule.SafetyRecoveryConfig.BERKELEY_DB,
INITIAL_CONSENSUS_CONFIG,
FunctionalRadixNodeModule.LedgerConfig.stateComputerNoSync(
StateComputerConfig.rev2(
Network.INTEGRATIONTESTNET.getId(),
GenesisBuilder.createTestGenesisWithNumValidators(
NUM_VALIDATORS,
Decimal.ONE,
GenesisConsensusManagerConfig.Builder.testWithRoundsPerEpoch(100000)),
StateComputerConfig.REV2ProposerConfig.transactionGenerator(
new REV2TransactionGenerator(), 1)))));
StateComputerConfig.rev2()
.withGenesis(
GenesisBuilder.createTestGenesisWithNumValidators(
NUM_VALIDATORS,
Decimal.ONE,
GenesisConsensusManagerConfig.Builder.testWithRoundsPerEpoch(
100000)))
.withProposerConfig(
StateComputerConfig.REV2ProposerConfig.transactionGenerator(
new REV2TransactionGenerator(), 1)))));
}

@Test
Expand Down
2 changes: 1 addition & 1 deletion ...rc/integration/java/com/radixdlt/integration/targeted/consensus/EpochTimeoutCertTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ public void no_byzantine_event_occurs_on_epoch_tc_event() {
test.runUntilMessage(
proposalAtRound(ROUNDS_PER_EPOCH + 2),
true,
10 * NUM_NODES * NUM_NODES * ((int) ROUNDS_PER_EPOCH));
10 * NUM_NODES * NUM_NODES * ROUNDS_PER_EPOCH);
// Run for a while more and verify that no byzantine issues occur
test.runForCount(40000);
}
Expand Down
16 changes: 15 additions & 1 deletion core/src/main/java/com/radixdlt/consensus/bft/BFTHighQCUpdate.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,9 +66,11 @@

import com.google.common.collect.ImmutableList;
import com.radixdlt.consensus.HighQC;
import com.radixdlt.consensus.event.LocalEvent;
import com.radixdlt.consensus.vertexstore.ExecutedVertex;
import com.radixdlt.lang.Option;
import com.radixdlt.utils.WrappedByteArray;
import java.util.AbstractCollection;

/**
* An event emitted when vertex store updates its highQC, which possibly results in some vertices
Expand All @@ -77,4 +79,16 @@
public record BFTHighQCUpdate(
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

HighQC newHighQc,
Option<ImmutableList<ExecutedVertex>> committedVertices,
WrappedByteArray serializedVertexStoreState) {}
WrappedByteArray serializedVertexStoreState)
implements LocalEvent {

@Override
public String toString() {
return String.format(
"%s[newHighQc=%s numCommittedVertices=%s serializedVertexStoreStateSize=%s]",
getClass().getSimpleName(),
newHighQc,
committedVertices.map(AbstractCollection::size).orElse(0),
serializedVertexStoreState.size());
}
}
11 changes: 10 additions & 1 deletion core/src/main/java/com/radixdlt/consensus/bft/BFTInsertUpdate.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,9 +64,18 @@

package com.radixdlt.consensus.bft;

import com.radixdlt.consensus.event.LocalEvent;
import com.radixdlt.consensus.vertexstore.ExecutedVertex;
import com.radixdlt.utils.WrappedByteArray;

/** An event emitted after a vertex has been inserted into the vertex store. */
public record BFTInsertUpdate(
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

ExecutedVertex insertedVertex, WrappedByteArray serializedVertexStoreState) {}
ExecutedVertex insertedVertex, WrappedByteArray serializedVertexStoreState)
implements LocalEvent {
@Override
public String toString() {
return String.format(
"%s[insertedVertex=%s serializedVertexStoreStateSize=%s]",
getClass().getSimpleName(), insertedVertex(), serializedVertexStoreState.size());
}
}
11 changes: 10 additions & 1 deletion core/src/main/java/com/radixdlt/consensus/bft/BFTRebuildUpdate.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,4 +70,13 @@

/** An even emitted when the vertex store has been rebuilt. */
public record BFTRebuildUpdate(
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

VertexStoreState vertexStoreState, WrappedByteArray serializedVertexStoreState) {}
VertexStoreState vertexStoreState, WrappedByteArray serializedVertexStoreState)
implements LocalEvent {

@Override
public String toString() {
return String.format(
"%s[serializedVertexStoreStateSize=%s]",
getClass().getSimpleName(), serializedVertexStoreState.size());
}
}
7 changes: 4 additions & 3 deletions core/src/main/java/com/radixdlt/consensus/sync/BFTSync.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@
import static java.util.function.Predicate.not;

import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import com.google.common.hash.HashCode;
import com.google.common.util.concurrent.RateLimiter;
import com.radixdlt.consensus.*;
Expand Down Expand Up @@ -244,7 +245,7 @@ public SyncResult syncToQC(HighQC highQC, @Nullable NodeId author, HighQcSource
return SyncResult.INVALID;
}

return switch (vertexStore.insertQc(qc)) {
return switch (vertexStore.insertQuorumCertificate(qc)) {
case VertexStore.InsertQcResult.Inserted ignored -> {
// QC was inserted, try TC too (as it can be higher), and then process a new highQC
highQC.highestTC().map(vertexStore::insertTimeoutCertificate);
Expand Down Expand Up @@ -464,8 +465,8 @@ private void rebuildAndSyncQC(SyncState syncState) {
// TODO: check if there are any vertices which haven't been local sync processed yet
if (requiresLedgerSync(syncState)) {
syncState.fetched.sort(Comparator.comparing(v -> v.vertex().getRound()));
ImmutableList<VertexWithHash> nonRootVertices =
syncState.fetched.stream().skip(1).collect(ImmutableList.toImmutableList());
ImmutableSet<VertexWithHash> nonRootVertices =
syncState.fetched.stream().skip(1).collect(ImmutableSet.toImmutableSet());

final var syncStateHighestCommittedQc = syncState.highQC.highestCommittedQC();
final var syncStateHighestTc = syncState.highQC.highestTC();
Expand Down
32 changes: 20 additions & 12 deletions core/src/main/java/com/radixdlt/consensus/vertexstore/VertexStoreAdapter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,10 +133,15 @@ public boolean insertTimeoutCertificate(TimeoutCertificate timeoutCertificate) {
};
}

public VertexStore.InsertQcResult insertQc(QuorumCertificate qc) {
public VertexStore.InsertQcResult insertQuorumCertificate(QuorumCertificate qc) {
final var result = vertexStore.insertQc(qc);
if (result instanceof VertexStore.InsertQcResult.Inserted inserted) {
dispatchPostQcInsertionEvents(inserted);
switch (result) {
case VertexStore.InsertQcResult.Inserted inserted -> this.highQCUpdateDispatcher.dispatch(
new BFTHighQCUpdate(
inserted.newHighQc(),
inserted.committedUpdate().map(VertexStore.CommittedUpdate::committedVertices),
inserted.serializedVertexStoreState()));
default -> {} // no-op
}
return result;
}
Expand All @@ -151,18 +156,21 @@ public List<ExecutedVertex> getPathFromRoot(HashCode vertexHash) {

public void insertVertexChain(VertexChain vertexChain) {
final var result = vertexStore.insertVertexChain(vertexChain);
result.insertedQcs().forEach(this::dispatchPostQcInsertionEvents);
result
.insertedQcs()
.forEach(
inserted -> {
this.highQCUpdateDispatcher.dispatch(
new BFTHighQCUpdate(
inserted.newHighQc(),
inserted
.committedUpdate()
.map(VertexStore.CommittedUpdate::committedVertices),
inserted.serializedVertexStoreState()));
});
result.insertUpdates().forEach(bftUpdateDispatcher::dispatch);
}

private void dispatchPostQcInsertionEvents(VertexStore.InsertQcResult.Inserted inserted) {
this.highQCUpdateDispatcher.dispatch(
new BFTHighQCUpdate(
inserted.newHighQc(),
inserted.committedUpdate().map(VertexStore.CommittedUpdate::committedVertices),
inserted.serializedVertexStoreState()));
}

public Optional<ImmutableList<VertexWithHash>> getVertices(HashCode vertexId, int count) {
return vertexStore.getVertices(vertexId, count).toOptional();
}
Expand Down
10 changes: 7 additions & 3 deletions core/src/main/java/com/radixdlt/consensus/vertexstore/VertexStoreJavaImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@
import com.google.common.annotations.VisibleForTesting;
import com.google.common.collect.HashMultimap;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Multimap;
import com.google.common.hash.HashCode;
import com.radixdlt.consensus.BFTHeader;
Expand Down Expand Up @@ -425,7 +426,10 @@ private void removeVertexAndPruneInternal(HashCode vertexId, Optional<HashCode>
return;
}

final var children = vertexChildren.removeAll(vertexId);
// Note that we need a copy of the children list here (.stream().toList()) - that is because
// we're removing the items in the loop (otherwise, this could lead to
// ConcurrentModificationException)
final var children = vertexChildren.get(vertexId).stream().toList();
for (HashCode child : children) {
if (!Optional.of(child).equals(skip)) {
removeVertexAndPruneInternal(child, Optional.empty());
Expand Down Expand Up @@ -468,13 +472,13 @@ public boolean containsVertex(HashCode vertexId) {

private VertexStoreState getState() {
// TODO: store list dynamically rather than recomputing
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not that expensive, so it's not a big deal... Although I wonder why we can't just do this.vertices.values().toImmutableList() or something. I guess either the order matters, or the scoping to the root vertex matters (if the store doesn't enforce that already?)

ImmutableList.Builder<VertexWithHash> verticesBuilder = ImmutableList.builder();
ImmutableSet.Builder<VertexWithHash> verticesBuilder = ImmutableSet.builder();
getChildrenVerticesList(this.rootVertex, verticesBuilder);
return VertexStoreState.create(this.highQC(), this.rootVertex, verticesBuilder.build(), hasher);
}

private void getChildrenVerticesList(
VertexWithHash parent, ImmutableList.Builder<VertexWithHash> builder) {
VertexWithHash parent, ImmutableSet.Builder<VertexWithHash> builder) {
for (HashCode child : this.vertexChildren.get(parent.hash())) {
final var v = vertices.get(child);
builder.add(v);
Expand Down
15 changes: 8 additions & 7 deletions core/src/main/java/com/radixdlt/consensus/vertexstore/VertexStoreState.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@
import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.annotation.JsonProperty;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import com.google.common.hash.HashCode;
import com.radixdlt.consensus.*;
import com.radixdlt.consensus.bft.Round;
Expand Down Expand Up @@ -94,10 +95,10 @@ public final class VertexStoreState {

private final VertexWithHash root;
private final HighQC highQC;
private final ImmutableList<VertexWithHash> vertices;
private final ImmutableSet<VertexWithHash> vertices;

private VertexStoreState(
HighQC highQC, VertexWithHash root, ImmutableList<VertexWithHash> vertices) {
HighQC highQC, VertexWithHash root, ImmutableSet<VertexWithHash> vertices) {
this.highQC = highQC;
this.root = root;
this.vertices = vertices;
Expand All @@ -123,11 +124,11 @@ public static VertexStoreState createNewForNextEpoch(
}

public static VertexStoreState create(HighQC highQC, VertexWithHash root, Hasher hasher) {
return create(highQC, root, ImmutableList.of(), hasher);
return create(highQC, root, ImmutableSet.of(), hasher);
}

public static VertexStoreState create(
HighQC highQC, VertexWithHash root, ImmutableList<VertexWithHash> vertices, Hasher hasher) {
HighQC highQC, VertexWithHash root, ImmutableSet<VertexWithHash> vertices, Hasher hasher) {
final var processedQcCommit =
highQC
.highestCommittedQC()
Expand Down Expand Up @@ -216,7 +217,7 @@ public VertexStoreState withVertex(VertexWithHash vertex) {
return new VertexStoreState(
this.highQC,
this.root,
ImmutableList.<VertexWithHash>builder().addAll(this.vertices).add(vertex).build());
ImmutableSet.<VertexWithHash>builder().addAll(this.vertices).add(vertex).build());
}

public SerializedVertexStoreState toSerialized() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm should we call this SerializableVertexStoreState instead? Because SerializedVertexStoreState is actually the WrappedByteArray...

Copy link
Contributor

Choose a reason for hiding this comment

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

Also I wonder if we should just make this serialize() -> WrappedByteArray and have a static method to deserialize; and just use SerializableVertexStoreState in this one place, not in any public methods?

Expand All @@ -236,7 +237,7 @@ public VertexWithHash getRoot() {
return root;
}

public ImmutableList<VertexWithHash> getVertices() {
public ImmutableSet<VertexWithHash> getVertices() {
return vertices;
}

Expand Down Expand Up @@ -309,7 +310,7 @@ public VertexStoreState toVertexStoreState(Hasher hasher) {
var rootWithHash = root.withId(hasher);

var verticesWithHash =
vertices.stream().map(v -> v.withId(hasher)).collect(ImmutableList.toImmutableList());
vertices.stream().map(v -> v.withId(hasher)).collect(ImmutableSet.toImmutableSet());

return VertexStoreState.create(highQC, rootWithHash, verticesWithHash, hasher);
}
Expand Down
Loading
Loading