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.

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

chore: Cherry Pick: State validation fails for round 191161423 on LSE #16796

Open
wants to merge 1 commit into
base: release/0.57
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,12 @@ public void saveRecords(
}

delegate.saveRecords(
firstLeafPath, lastLeafPath, pathHashRecordsToUpdate, leaves.stream(), leafRecordsToDelete);
firstLeafPath,
lastLeafPath,
pathHashRecordsToUpdate,
leaves.stream(),
leafRecordsToDelete,
isReconnectContext);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -287,6 +287,18 @@
}
}

/**
* If a dirty leaves stream is empty, returns {@code null}. If leaf path is empty, that
* is when {@code firstLeafPath} and/or {@code lastLeafPath} are zero or less, and
* dirty leaves stream is not empty, throws an {@link IllegalArgumentException}.
*
* @param hashReader A function to read hashes for clean paths
* @param sortedDirtyLeaves A stream of leaf records, sorted by path
* @param firstLeafPath First leaf path
* @param lastLeafPath Last leaf path
* @param listener Hash listener. May be null
* @param virtualMapConfig VirtualMap config
*/
public Hash hash(
final LongFunction<Hash> hashReader,
final Iterator<VirtualLeafRecord<K, V>> sortedDirtyLeaves,
Expand All @@ -296,15 +308,6 @@
final @NonNull VirtualMapConfig virtualMapConfig) {
requireNonNull(virtualMapConfig);

// If the first or last leaf path are invalid, then there is nothing to hash.
if (firstLeafPath < 1 || lastLeafPath < 1) {
return null;
}

if (!sortedDirtyLeaves.hasNext()) {
return null;
}

// We don't want to include null checks everywhere, so let the listener be NoopListener if null
if (listener == null) {
listener =
Expand All @@ -313,6 +316,19 @@
};
}

// Let the listener know we have started hashing.
listener.onHashingStarted();

if (!sortedDirtyLeaves.hasNext()) {
// Nothing to hash.
listener.onHashingCompleted();
return null;
} else {
if ((firstLeafPath < 1) || (lastLeafPath < 1)) {
throw new IllegalArgumentException("Dirty leaves stream is not empty, but leaf path range is empty");

Check warning on line 328 in platform-sdk/swirlds-virtualmap/src/main/java/com/swirlds/virtualmap/internal/hash/VirtualHasher.java

View check run for this annotation

Codecov / codecov/patch

platform-sdk/swirlds-virtualmap/src/main/java/com/swirlds/virtualmap/internal/hash/VirtualHasher.java#L328

Added line #L328 was not covered by tests
}
}

this.hashReader = hashReader;
this.listener = listener;
this.cryptography = CryptographyHolder.get();
Expand Down Expand Up @@ -346,9 +362,6 @@
int firstLeafRank = Path.getRank(firstLeafPath);
int lastLeafRank = Path.getRank(lastLeafPath);

// Let the listener know we have started hashing.
listener.onHashingStarted();

// This map contains all tasks created, but not scheduled for execution yet
final HashMap<Long, ChunkHashTask> map = new HashMap<>();
// The result task is never executed but used as an output dependency for
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -182,11 +182,11 @@ public void onHashingCompleted() {
finalLeavesToFlush = leaves;
leaves = null;
}
if (!finalNodesToFlush.isEmpty() || !finalLeavesToFlush.isEmpty()) {
assert !flushInProgress.get() : "Flush must not be in progress when hashing is complete";
flushInProgress.set(true);
flush(finalNodesToFlush, finalLeavesToFlush);
}
assert !flushInProgress.get() : "Flush must not be in progress when hashing is complete";
flushInProgress.set(true);
// Nodes / leaves lists may be empty, but a flush is still needed to make sure
// all stale leaves are removed from the data source
flush(finalNodesToFlush, finalLeavesToFlush);
}

// Since flushes may take quite some time, this method is called outside synchronized blocks,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import com.swirlds.virtualmap.VirtualKey;
import com.swirlds.virtualmap.VirtualValue;
import com.swirlds.virtualmap.datasource.VirtualLeafRecord;
import com.swirlds.virtualmap.internal.Path;
import com.swirlds.virtualmap.internal.RecordAccessor;
import java.util.HashSet;
import java.util.Set;
Expand Down Expand Up @@ -146,15 +147,12 @@ public synchronized void newLeafNode(final long path, final K newKey) {
}

public synchronized void allNodesReceived() {
if (newLastLeafPath < 0) {
// Empty teacher
return;
}
// no-op if newLastLeafPath is greater or equal to oldLastLeafPath
logger.info(
RECONNECT.getMarker(),
"allNodesReceived(): newLastLeafPath = " + newLastLeafPath + ", oldLastLeafPath = " + oldLastLeafPath);
for (long p = newLastLeafPath + 1; p <= oldLastLeafPath; p++) {
final long firstOldStalePath = (newLastLeafPath == Path.INVALID_PATH) ? 1 : newLastLeafPath + 1;
// No-op if newLastLeafPath is greater or equal to oldLastLeafPath
for (long p = firstOldStalePath; p <= oldLastLeafPath; p++) {
final VirtualLeafRecord<K, ?> oldExtraLeafRecord = oldRecords.findLeafRecord(p, false);
assert oldExtraLeafRecord != null || p < oldFirstLeafPath;
if (oldExtraLeafRecord != null) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -91,24 +91,48 @@ void emptyStreamProducesNull() {
*/
@Test
@Tag(TestComponentTags.VMAP)
@DisplayName("Invalid leaf paths returns null for hash")
void invalidFirstOrLastLeafPathProducesNull() {
@DisplayName("Invalid leaf paths")
void invalidLeafPaths() {
final TestDataSource ds = new TestDataSource(Path.INVALID_PATH, Path.INVALID_PATH);
final VirtualHasher<TestKey, TestValue> hasher = new VirtualHasher<>();
final List<VirtualLeafRecord<TestKey, TestValue>> leaves = new ArrayList<>();
leaves.add(appleLeaf(VirtualTestBase.A_PATH));
final List<VirtualLeafRecord<TestKey, TestValue>> emptyLeaves = new ArrayList<>();
// Empty dirty leaves stream -> null hash
assertNull(
hasher.hash(
ds::loadHash, emptyLeaves.iterator(), Path.INVALID_PATH, Path.INVALID_PATH, VIRTUAL_MAP_CONFIG),
"Call should have produced null");
assertNull(
hasher.hash(ds::loadHash, leaves.iterator(), Path.INVALID_PATH, 2, VIRTUAL_MAP_CONFIG),
hasher.hash(ds::loadHash, emptyLeaves.iterator(), Path.INVALID_PATH, 2, VIRTUAL_MAP_CONFIG),
"Call should have produced null");
assertNull(
hasher.hash(ds::loadHash, leaves.iterator(), 1, Path.INVALID_PATH, VIRTUAL_MAP_CONFIG),
hasher.hash(ds::loadHash, emptyLeaves.iterator(), 1, Path.INVALID_PATH, VIRTUAL_MAP_CONFIG),
"Call should have produced null");
assertNull(
hasher.hash(ds::loadHash, leaves.iterator(), 0, 2, VIRTUAL_MAP_CONFIG),
hasher.hash(ds::loadHash, emptyLeaves.iterator(), 0, 2, VIRTUAL_MAP_CONFIG),
"Call should have produced null");
assertNull(
hasher.hash(ds::loadHash, leaves.iterator(), 1, 0, VIRTUAL_MAP_CONFIG),
hasher.hash(ds::loadHash, emptyLeaves.iterator(), 1, 0, VIRTUAL_MAP_CONFIG),
"Call should have produced null");
// Non-empty dirty leaves stream + empty leaf path range -> IllegalStateException
final List<VirtualLeafRecord<TestKey, TestValue>> nonEmptyLeaves = new ArrayList<>();
nonEmptyLeaves.add(appleLeaf(VirtualTestBase.A_PATH));
assertThrows(
IllegalArgumentException.class,
() -> hasher.hash(
ds::loadHash,
nonEmptyLeaves.iterator(),
Path.INVALID_PATH,
Path.INVALID_PATH,
VIRTUAL_MAP_CONFIG),
"Non-null leaves iterator + invalid paths should throw an exception");
assertThrows(
IllegalArgumentException.class,
() -> hasher.hash(ds::loadHash, nonEmptyLeaves.iterator(), 0, 2, VIRTUAL_MAP_CONFIG),
"Non-null leaves iterator + invalid paths should throw an exception");
assertThrows(
IllegalArgumentException.class,
() -> hasher.hash(ds::loadHash, nonEmptyLeaves.iterator(), 1, 0, VIRTUAL_MAP_CONFIG),
"Non-null leaves iterator + invalid paths should throw an exception");
}

/**
Expand Down
Loading