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

Change DataCache.Add() to never allow overwriting existing values #1177

Closed
wants to merge 8 commits into from
Closed

Change DataCache.Add() to never allow overwriting existing values #1177

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
6ee22a8
Change Add() to never allow overwriting existing values
ixje Oct 22, 2019
11bd6a7
fix lint error
ixje Oct 22, 2019
be31a2a
take Deleted -> Added scenario into account
ixje Oct 22, 2019
20c6e40
fixes for unittests
ixje Oct 22, 2019
29cebaf
Fixing mempool UT to GetAndChange
vncoelho Dec 9, 2019
115372a
Removing wrong test on TestCheckPolicy of Blocked Accounts
vncoelho Dec 9, 2019
34a8ae0
Merge branch 'master' into fix1173
vncoelho Dec 10, 2019
105f7e9
Merge branch 'master' into fix1173
shargon Dec 10, 2019
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
13 changes: 11 additions & 2 deletions src/neo/IO/Caching/DataCache.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,17 @@ public void Add(TKey key, TValue value)
{
lock (dictionary)
{
if (dictionary.TryGetValue(key, out Trackable trackable) && trackable.State != TrackState.Deleted)
throw new ArgumentException();
if (dictionary.TryGetValue(key, out Trackable trackable))
{
if (trackable.State != TrackState.Deleted)
throw new ArgumentException();
}
else
{
if (TryGetInternal(key) != null)
throw new ArgumentException();
}
Comment on lines +50 to +59
Copy link
Member

Choose a reason for hiding this comment

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

When we are using Add(), we assume that the data does not exist. If we can't assume that, we should use GetAndChange(). So, since we are using Add(), there is no need to add another data read operation.

Copy link
Contributor Author

@ixje ixje Dec 10, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

The idea behind this PR (and the above code change) is to create consistent behaviour of the cache by not just validating that the value exists in cache, but to also validate that it does not exist in the real backend. You previously choose this exact solution (#1173 (comment)) instead of always overriding the data (described in the linked comment as option 1).

If we don't choose either option 1 or 2 we will have inconsistent behaviour as described in #1173

I don't mind which of the 2 we choose, but right now the feedback looks like you don't want to change anything?! 😕 I think it is wrong to assume that data does not exist and I think you agreed with that when you wrote the code initially, otherwise this check would not exists

    if (trackable.State != TrackState.Deleted)
                        throw new ArgumentException();

Copy link
Member

Choose a reason for hiding this comment

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

The overhead of this check is very small, so just do it by the way.

Copy link
Member

Choose a reason for hiding this comment

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

I did not understand the point as well, @erikzhang.

The implementation here seems to be option 2 of that aforementioned issue.
Do you want to leave the code as in the original implementation?

Copy link
Member

Choose a reason for hiding this comment

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

I don't want to change it. The purpose of Add() is to add records when they do not exist, and save the cost of a read operation. If you are not sure that the record exists or not, you should use GetAndChange().

Copy link
Member

Choose a reason for hiding this comment

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

I see, @erikzhang.
It looks like to be more efficient in the original code, yes? In the sense of avoiding a second check with if (TryGetInternal(key) != null).

@ixje, if that is the case, perhaps it is better to close PR and issue and open another one for commenting the code properly in order to avoid future misinterpretations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So everybody basically wasted their time in creating the PR, reviewing it and fixing unit tests (still thanks for that @vncoelho) because 1.5 months after opening the PR and the initial review we're suddenly trying to save performance on a db operation that takes < 10 microseconds (according to leveldb benchmarks for random reads) while elsewhere in the code base millisecond delays are accepted.

Don't get me wrong here. I'm all cool with keeping performance in mind for reaching the highest TPS, but how are we supposed to make a sound decision what makes sense to spend time on when performance criteria seems randomly applied?

Copy link
Member

Choose a reason for hiding this comment

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

@ixje, I really apologize for that.

I really did not have enough knowledge for checking it before.
In fact, I would like to thank you for your dedication. In addition, your insights helped me to investigate further.

Perhaps, there are methods that are not being used from the DataCache, such as GetOrAdd.
We need to solve these questions and improve what needs to be improved.


dictionary[key] = new Trackable
{
Key = key,
Expand Down
8 changes: 4 additions & 4 deletions src/neo/SmartContract/Native/PolicyContract.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,19 +47,19 @@ private bool CheckValidators(ApplicationEngine engine)
internal override bool Initialize(ApplicationEngine engine)
{
if (!base.Initialize(engine)) return false;
engine.Snapshot.Storages.Add(CreateStorageKey(Prefix_MaxBlockSize), new StorageItem
engine.Snapshot.Storages.GetAndChange(CreateStorageKey(Prefix_MaxBlockSize), () => new StorageItem
Copy link
Member

Choose a reason for hiding this comment

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

This should be the same, if we already know that the item doesn't exists, why we need to call GetAndChange?

{
Value = BitConverter.GetBytes(1024u * 256u)
});
engine.Snapshot.Storages.Add(CreateStorageKey(Prefix_MaxTransactionsPerBlock), new StorageItem
engine.Snapshot.Storages.GetAndChange(CreateStorageKey(Prefix_MaxTransactionsPerBlock), () => new StorageItem
{
Value = BitConverter.GetBytes(512u)
});
engine.Snapshot.Storages.Add(CreateStorageKey(Prefix_FeePerByte), new StorageItem
engine.Snapshot.Storages.GetAndChange(CreateStorageKey(Prefix_FeePerByte), () => new StorageItem
{
Value = BitConverter.GetBytes(1000L)
});
engine.Snapshot.Storages.Add(CreateStorageKey(Prefix_BlockedAccounts), new StorageItem
engine.Snapshot.Storages.GetAndChange(CreateStorageKey(Prefix_BlockedAccounts), () => new StorageItem
{
Value = new UInt160[0].ToByteArray()
});
Expand Down
10 changes: 8 additions & 2 deletions tests/neo.UnitTests/Ledger/UT_MemoryPool.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -487,8 +487,14 @@ public void TestUpdatePoolForBlockPersisted()
var key2 = CreateStorageKey(Prefix_FeePerByte);
key1.ScriptHash = NativeContract.Policy.Hash;
key2.ScriptHash = NativeContract.Policy.Hash;
snapshot.Storages.Add(key1, item1);
snapshot.Storages.Add(key2, item2);
snapshot.Storages.GetAndChange(key1, () => new StorageItem
Copy link
Member

Choose a reason for hiding this comment

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

@ixje, this change is expected, right? Since the value was previously existing.

Copy link
Contributor Author

@ixje ixje Dec 10, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@vncoelho I don't remember, but it is most likely caused by the TestBlockchain isolation problem. I think the best way forward is to have that fixed first, reset all changes to the unit tests and try again. Some of the changes (like here) have likely been applied because of the isolation problem and might not be needed anymore once that is fixed

Copy link
Member

Choose a reason for hiding this comment

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

AHeauehuaea, @ixje, this change was done by me, please check the last commits.

The question was to be sure this was consistent.

All UTs were fixed, the PR is kind of ready to merge.

{
Value = item1.Value
});
snapshot.Storages.GetAndChange(key2, () => new StorageItem
{
Value = item2.Value
});

var tx1 = CreateTransaction();
var tx2 = CreateTransaction();
Expand Down
19 changes: 4 additions & 15 deletions tests/neo.UnitTests/SmartContract/Native/UT_PolicyContract.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,10 @@
using Neo.VM;
using System.Linq;


using Neo.Cryptography;
using System;

namespace Neo.UnitTests.SmartContract.Native
{
[TestClass]
Expand Down Expand Up @@ -231,21 +235,6 @@ public void TestCheckPolicy()
{
Transaction tx = Blockchain.GenesisBlock.Transactions[0];
var snapshot = Blockchain.Singleton.GetSnapshot();

Copy link
Member

Choose a reason for hiding this comment

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

@erikzhang @eryeer @shargon, double check this because perhaps this test was wrong.

Copy link
Contributor

Choose a reason for hiding this comment

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

How about change it like this.

        [TestMethod]
        public void TestCheckPolicy()
        {
            Transaction tx = Blockchain.GenesisBlock.Transactions[0];
            var snapshot = Blockchain.Singleton.GetSnapshot();
            StorageKey storageKey = new StorageKey
            {
                ScriptHash = NativeContract.Policy.Hash,
                Key = new byte[sizeof(byte)]
            };
            storageKey.Key[0] = 15;
        
            var item = snapshot.Storages.GetAndChange(storageKey);
            item.Value = new UInt160[] { tx.Sender }.ToByteArray();
            NativeContract.Policy.CheckPolicy(tx, snapshot).Should().BeFalse();

            snapshot = Blockchain.Singleton.GetSnapshot();
            NativeContract.Policy.CheckPolicy(tx, snapshot).Should().BeTrue();
        }

Copy link
Member

Choose a reason for hiding this comment

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

I tried, it says True instead of False, that is why I believe the test was wrong. Why was it False in any situation?

Copy link
Contributor

@eryeer eryeer Dec 10, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Using this code, the test will pass. We need to change the return value of snapshot.Storages.GetAndChange(storageKey) instead of passing a StorageItem creating function into it. Because the initalization will add a storageItem into storage, trackable.Item won't be null.

Copy link
Member

Choose a reason for hiding this comment

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

Anhhh, I see, but it would be better to DeleteKey, no?

Copy link
Contributor

Choose a reason for hiding this comment

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

storageKey.Key[0] = 15; this key is the blockedAccount key

neo/src/neo/SmartContract/Native/PolicyContract.cs

Line 24 in 0f75741

private const byte Prefix_BlockedAccounts = 15;

Because it is private, we can only use the 15 instead of Prefix_BlockedAccounts

Copy link
Member

Choose a reason for hiding this comment

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

I partially understood! Thanks for the explanation, @eryeer.

However, I still need to figure out this part:

We need to change the return value of snapshot.Storages.GetAndChange(storageKey) instead of passing a StorageItem creating function into it. Because the initalization will add a storageItem into storage, trackable.Item won't be null.

Copy link
Contributor

@eryeer eryeer Dec 10, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

😄 Here is the initalization.

neo/src/neo/SmartContract/Native/PolicyContract.cs

Lines 62 to 65 in 0f75741

engine.Snapshot.Storages.Add(CreateStorageKey(Prefix_BlockedAccounts), new StorageItem
{
Value = new UInt160[0].ToByteArray()
});

So whenever we get the BlockedAccounts from Internal storage, the value will be new UInt160[0].ToByteArray() by default. We need to change the defult value instead of create a new one to add to storage.

Copy link
Member

@vncoelho vncoelho Dec 10, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I think that I understood.

This would also work:

        [TestMethod]
        public void TestCheckPolicy()
        {
            Transaction tx = Blockchain.GenesisBlock.Transactions[0];
            var snapshot = Blockchain.Singleton.GetSnapshot();
            StorageKey storageKey = new StorageKey
            {
                ScriptHash = NativeContract.Policy.Hash,
                Key = new byte[sizeof(byte)]
            };
            storageKey.Key[0] = 15;
        
            //var item = snapshot.Storages.GetAndChange(storageKey);
            //item.Value = new UInt160[] { tx.Sender }.ToByteArray();
            snapshot.Storages.Delete(storageKey);
            snapshot.Storages.GetAndChange(storageKey, () => new StorageItem
            {
                Value = new UInt160[] { tx.Sender }.ToByteArray()
            });
            snapshot.Commit();
            NativeContract.Policy.CheckPolicy(tx, snapshot).Should().BeFalse();
            snapshot = Blockchain.Singleton.GetSnapshot();

            snapshot.Storages.Delete(storageKey);
            snapshot.Storages.GetAndChange(storageKey, () => new StorageItem
            {
                Value = new UInt160[0].ToByteArray()
            });
            snapshot.Commit();

            NativeContract.Policy.CheckPolicy(tx, snapshot).Should().BeTrue();
        }

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh yes, congratulations!

StorageKey storageKey = new StorageKey
{
ScriptHash = NativeContract.Policy.Hash,
Key = new byte[sizeof(byte)]
};
storageKey.Key[0] = 15;
snapshot.Storages.Add(storageKey, new StorageItem
{
Value = new UInt160[] { tx.Sender }.ToByteArray(),
});

NativeContract.Policy.CheckPolicy(tx, snapshot).Should().BeFalse();

snapshot = Blockchain.Singleton.GetSnapshot();
NativeContract.Policy.CheckPolicy(tx, snapshot).Should().BeTrue();
}
}
Expand Down