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

Create only one snapshot for persist #1602

Merged
Merged
Changes from 4 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
9 changes: 7 additions & 2 deletions src/neo/Ledger/Blockchain.cs
Original file line number Diff line number Diff line change
Expand Up @@ -488,6 +488,7 @@ private void Persist(Block block)
}
}
snapshot.Blocks.Add(block.Hash, block.Trim());
StoreView clonedSnapshot = snapshot.Clone();
foreach (Transaction tx in block.Transactions)
{
var state = new TransactionState
Expand All @@ -498,13 +499,17 @@ private void Persist(Block block)

snapshot.Transactions.Add(tx.Hash, state);
Copy link
Member

Choose a reason for hiding this comment

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

You added a new record to the snapshot after Clone(). It may be safe now, but I am not sure if it will cause problems in the future. For example, cache non-existent records.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's right. Maybe adding a todo comment to warn developer for future modification can help.

Copy link
Member

Choose a reason for hiding this comment

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

I think it conflicts with the cache to the non-existent keys. Maybe we should test which one is better first?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have updated the writing logic here & should be able to avoid such occasions:)


using (ApplicationEngine engine = new ApplicationEngine(TriggerType.Application, tx, snapshot.Clone(), tx.SystemFee))
using (ApplicationEngine engine = new ApplicationEngine(TriggerType.Application, tx, clonedSnapshot, tx.SystemFee))
{
engine.LoadScript(tx.Script);
state.VMState = engine.Execute();
shargon marked this conversation as resolved.
Show resolved Hide resolved
if (state.VMState == VMState.HALT)
{
engine.Snapshot.Commit();
clonedSnapshot.Commit();
shargon marked this conversation as resolved.
Show resolved Hide resolved
}
else
{
clonedSnapshot = snapshot.Clone();
}
ApplicationExecuted application_executed = new ApplicationExecuted(engine);
Context.System.EventStream.Publish(application_executed);
Expand Down