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

[Fix]: keyPrefix in DataCache.Find can be all 0xff. #3681

Closed
wants to merge 3 commits into from
Closed

[Fix]: keyPrefix in DataCache.Find can be all 0xff. #3681

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d808195
Fix: allow keyPrefix in DataCache.Find to be null, empty, and all 0xff
nan01ab Jan 20, 2025
06cd75c
Fix: code format
nan01ab Jan 20, 2025
2b89656
fix: do not change behavior when key is null or empty and SeekDirecti…
nan01ab Jan 20, 2025
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
69 changes: 36 additions & 33 deletions src/Neo/Persistence/DataCache.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,50 +202,58 @@ public void Delete(StorageKey key)
/// <summary>
/// Finds the entries starting with the specified prefix.
/// </summary>
/// <param name="key_prefix">The prefix of the key.</param>
/// <param name="keyPrefix">The prefix of the key.</param>
/// <param name="direction">The search direction.</param>
/// <returns>The entries found with the desired prefix.</returns>
public IEnumerable<(StorageKey Key, StorageItem Value)> Find(byte[]? key_prefix = null, SeekDirection direction = SeekDirection.Forward)
public IEnumerable<(StorageKey Key, StorageItem Value)> Find(byte[]? keyPrefix = null, SeekDirection direction = SeekDirection.Forward)
{
var seek_prefix = key_prefix;
var seekPrefix = keyPrefix;
if (direction == SeekDirection.Backward)
{
if (key_prefix == null)
if (keyPrefix == null)
{
// Backwards seek for null prefix is not supported for now.
throw new ArgumentNullException(nameof(key_prefix));
throw new ArgumentNullException(nameof(keyPrefix));
}
if (key_prefix.Length == 0)
if (keyPrefix.Length == 0)
{
// Backwards seek for zero prefix is not supported for now.
throw new ArgumentOutOfRangeException(nameof(key_prefix));
throw new ArgumentOutOfRangeException(nameof(keyPrefix));
}
seek_prefix = null;
for (var i = key_prefix.Length - 1; i >= 0; i--)

seekPrefix = null;
for (var i = keyPrefix.Length - 1; i >= 0; i--)
{
if (key_prefix[i] < 0xff)
if (keyPrefix[i] < 0xff)
{
seek_prefix = key_prefix.Take(i + 1).ToArray();
// The next key after the key_prefix.
seek_prefix[i]++;
seekPrefix = keyPrefix.Take(i + 1).ToArray();
seekPrefix[i]++; // The next key after the key_prefix.
break;
}
}
if (seek_prefix == null)

if (seekPrefix == null)
{
throw new ArgumentException($"{nameof(key_prefix)} with all bytes being 0xff is not supported now");
// This case is rare
seekPrefix = new byte[ApplicationEngine.MaxStorageKeySize + 1];
Copy link
Contributor Author

@nan01ab nan01ab Jan 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

MaxStorageKeySize.
Maybe not long enough.

Array.Fill(seekPrefix, (byte)0xff);
Copy link
Contributor

Choose a reason for hiding this comment

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

is this a logic change @shargon?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it is

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it doesn't matter.

}

}
return FindInternal(key_prefix, seek_prefix, direction);

return FindInternal(keyPrefix, seekPrefix, direction);
}

private IEnumerable<(StorageKey Key, StorageItem Value)> FindInternal(byte[]? key_prefix, byte[]? seek_prefix, SeekDirection direction)
private IEnumerable<(StorageKey Key, StorageItem Value)> FindInternal(byte[]? keyPrefix, byte[]? seekPrefix, SeekDirection direction)
{
foreach (var (key, value) in Seek(seek_prefix, direction))
if (key_prefix == null || key.ToArray().AsSpan().StartsWith(key_prefix))
var prefixIsEmpty = keyPrefix == null || keyPrefix.Length == 0;
foreach (var (key, value) in Seek(seekPrefix, direction))
{
if (prefixIsEmpty || key.ToArray().AsSpan().StartsWith(keyPrefix))
yield return (key, value);
else if (direction == SeekDirection.Forward || (seek_prefix == null || !key.ToArray().SequenceEqual(seek_prefix)))
else if (direction == SeekDirection.Forward || (seekPrefix == null || !key.ToArray().SequenceEqual(seekPrefix)))
yield break;
}
}

/// <summary>
Expand All @@ -261,10 +269,12 @@ public void Delete(StorageKey key)
? ByteArrayComparer.Default
: ByteArrayComparer.Reverse;
foreach (var (key, value) in Seek(start, direction))
{
if (comparer.Compare(key.ToArray(), end) < 0)
yield return (key, value);
else
yield break;
}
}

/// <summary>
Expand Down Expand Up @@ -415,31 +425,24 @@ public StorageItem GetOrAdd(StorageKey key, Func<StorageItem> factory)
{
IEnumerable<(byte[], StorageKey, StorageItem)> cached;
HashSet<StorageKey> cachedKeySet;
ByteArrayComparer comparer = direction == SeekDirection.Forward ? ByteArrayComparer.Default : ByteArrayComparer.Reverse;
var comparer = direction == SeekDirection.Forward ? ByteArrayComparer.Default : ByteArrayComparer.Reverse;
lock (_dictionary)
{
cached = _dictionary
.Where(p => p.Value.State != TrackState.Deleted && p.Value.State != TrackState.NotFound && (keyOrPrefix == null || comparer.Compare(p.Key.ToArray(), keyOrPrefix) >= 0))
.Select(p =>
(
KeyBytes: p.Key.ToArray(),
p.Key,
p.Value.Item
))
.Select(p => (KeyBytes: p.Key.ToArray(), p.Key, p.Value.Item))
.OrderBy(p => p.KeyBytes, comparer)
.ToArray();
cachedKeySet = new HashSet<StorageKey>(_dictionary.Keys);
}

var uncached = SeekInternal(keyOrPrefix ?? Array.Empty<byte>(), direction)
.Where(p => !cachedKeySet.Contains(p.Key))
.Select(p =>
(
KeyBytes: p.Key.ToArray(),
p.Key,
p.Value
));
.Select(p => (KeyBytes: p.Key.ToArray(), p.Key, p.Value));

using var e1 = cached.GetEnumerator();
using var e2 = uncached.GetEnumerator();

(byte[] KeyBytes, StorageKey Key, StorageItem Item) i1, i2;
bool c1 = e1.MoveNext();
bool c2 = e2.MoveNext();
Expand Down
44 changes: 44 additions & 0 deletions tests/Neo.UnitTests/Persistence/UT_DataCache.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,7 @@ public void TestFind()
// null and empty with the backwards direction -> miserably fails.
Action action = () => myDataCache.Find(null, SeekDirection.Backward);
action.Should().Throw<ArgumentException>();

action = () => myDataCache.Find(new byte[] { }, SeekDirection.Backward);
action.Should().Throw<ArgumentException>();

Expand Down Expand Up @@ -404,5 +405,48 @@ public void TestFindInvalid()
items.Current.Key.Should().Be(key4);
items.MoveNext().Should().Be(false);
}

[TestMethod]
public void TestFindEmptyPrefix()
{
using var store = new MemoryStore();
using var dataCache = new SnapshotCache(store);

var k1 = StorageKey.CreateSearchPrefix(-1, []);
var k2 = StorageKey.CreateSearchPrefix(-1, [0x01]);
var k3 = StorageKey.CreateSearchPrefix(-1, [0xff, 0x02]);

dataCache.Add(k1, value1);
dataCache.Add(k2, value2);
dataCache.Add(k3, value3);

var items = dataCache.Find().ToArray();
items.Length.Should().Be(3);
items[0].Key.ToArray().Should().BeEquivalentTo(k1.ToArray());
items[1].Key.ToArray().Should().BeEquivalentTo(k2.ToArray());
items[2].Key.ToArray().Should().BeEquivalentTo(k3.ToArray());

items = dataCache.Find([0xff, 0xff, 0xff, 0xff, 0xff]).ToArray();
items.Length.Should().Be(1);
items[0].Key.ToArray().Should().BeEquivalentTo(k3.ToArray());

// null and empty are not supported for backwards direction now.
Action action = () => myDataCache.Find(null, SeekDirection.Backward);
action.Should().Throw<ArgumentException>();

action = () => myDataCache.Find(new byte[] { }, SeekDirection.Backward);
action.Should().Throw<ArgumentException>();

items = dataCache.Find([0xff, 0xff, 0xff, 0xff, 0xff], SeekDirection.Backward).ToArray();
items.Length.Should().Be(1);
items[0].Key.ToArray().Should().BeEquivalentTo(k3.ToArray());

items = dataCache.Find([0xff], SeekDirection.Backward).ToArray();
items.Length.Should().Be(3);
items[0].Key.ToArray().Should().BeEquivalentTo(k3.ToArray());
items[1].Key.ToArray().Should().BeEquivalentTo(k2.ToArray());
items[2].Key.ToArray().Should().BeEquivalentTo(k1.ToArray());
}
}
}

Loading