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

Store notification merkleRoot in TransactionState #2940

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Store notification merkleRoot in TransactionState #2940

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
a250bc2
Store NotificationMerkleRoot in TransactionState
shargon Oct 27, 2023
9cd97d9
Merge branch 'master' into tx-notifications-state-root
shargon Oct 27, 2023
0ab910a
Fix UT
shargon Oct 27, 2023
4bc3616
Fix UT
shargon Oct 27, 2023
2475222
Merge branch 'master' into tx-notifications-state-root
shargon Nov 3, 2023
21681a2
Store the hash also for FAULT
shargon Nov 3, 2023
c491594
Merge branch 'tx-notifications-state-root' of https://github.com/neo-…
shargon Nov 3, 2023
933ec3a
Merge branch 'master' into tx-notifications-state-root
shargon Nov 5, 2023
7e53178
Merge branch 'master' into tx-notifications-state-root
Jim8y Mar 22, 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
5 changes: 4 additions & 1 deletion src/Neo/Ledger/Blockchain.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
using Akka.Actor;
using Akka.Configuration;
using Akka.IO;
using Neo.Cryptography;
using Neo.IO.Actors;
using Neo.Network.P2P;
using Neo.Network.P2P.Payloads;
Expand Down Expand Up @@ -443,6 +444,9 @@ private void Persist(Block block)
using ApplicationEngine engine = ApplicationEngine.Create(TriggerType.Application, tx, clonedSnapshot, block, system.Settings, tx.SystemFee);
engine.LoadScript(tx.Script);
transactionState.State = engine.Execute();
ApplicationExecuted application_executed = new(engine);
transactionState.NotificationMerkleRoot =
MerkleTree.ComputeRoot(application_executed.Notifications.Select(p => p.GetNotificationHash()).ToArray());
if (transactionState.State == VMState.HALT)
{
clonedSnapshot.Commit();
Expand All @@ -451,7 +455,6 @@ private void Persist(Block block)
{
clonedSnapshot = snapshot.CreateSnapshot();
}
ApplicationExecuted application_executed = new(engine);
Context.System.EventStream.Publish(application_executed);
all_application_executed.Add(application_executed);
}
Expand Down
3 changes: 2 additions & 1 deletion src/Neo/SmartContract/Native/LedgerContract.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,8 @@ internal override ContractTask OnPersist(ApplicationEngine engine)
{
BlockIndex = engine.PersistingBlock.Index,
Transaction = p,
State = VMState.NONE
State = VMState.NONE,
NotificationMerkleRoot = UInt256.Zero
}).ToArray();
engine.Snapshot.Add(CreateStorageKey(Prefix_BlockHash).AddBigEndian(engine.PersistingBlock.Index), new StorageItem(engine.PersistingBlock.Hash.ToArray()));
engine.Snapshot.Add(CreateStorageKey(Prefix_Block).Add(engine.PersistingBlock.Hash), new StorageItem(Trim(engine.PersistingBlock).ToArray()));
Expand Down
10 changes: 9 additions & 1 deletion src/Neo/SmartContract/Native/TransactionState.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,11 @@ public class TransactionState : IInteroperable
/// </summary>
public VMState State;

/// <summary>
/// The merkle root of the notifications.
/// </summary>
public UInt256 NotificationMerkleRoot;
Copy link
Member

@AnnaShaleva AnnaShaleva Nov 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I see one potential problem with keeping the notifications or their Merkle root in the native Ledger contract (correct me, if I'm wrong):

Problem description

One of the goals that #2034 is trying to solve is aligning all side-effects of transaction execution across the network nodes, which means ensuring that after block acceptance we need to ensure that a) contracts storage has the same set of changes and b) transaction notifications are the same within all the nodes (that's pretty enough for aligning). The problem is that currently, even with StateService plugin acting as a replacement to the StateRootInHeader option, we exclude native Ledger's state from the MPT: https://github.com/neo-project/neo-modules/blob/4f55458bda29a0e80258add2a3768dcc014ab24d/src/StateService/StatePlugin.cs#L97.

This change was firstly implemented in neo-project/neo-modules#470. There are several reasons why we do that, but the most significant are described in neo-project/neo-modules#520 (although this issue is related to the StatesDumper plugin, but the idea is similar).

So here with notifications or their Merkle root, our desire is to include them into comparable (and verifiable) stateroot, but it's not enough to place them into TransactionState structure.

Possible solutions
One of the solution I was thinking about is store notifications or their root under a special prefix (not inside the TransactionState structure) and slightly modify StateService plugin to exclude only a part of native Ledger's storage items from the Merkle trie (exclude transactions and blocks, and keep notifications). But I'd like to pay @roman-khimov's attention to this problem, because there might be some problems with porting this solution to the NeoGo, because we have a different storage scheme and do not store anything in our native Ledger contract. Although it might not be a problem if we're about to store only notifications root, it takes only 32 bytes, so it likely won't exploit NeoGo's storage.

Another solution that came to my mind is to store notifications in some other contract.

Side note
The described problem will remain relevant even after (or if) we'll move from the StateService plugin to StateRootInHeader option. But maybe I'm missing something and we don't have this problem at all.

Copy link
Member Author

Choose a reason for hiding this comment

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

If we use just the hash of the storage changes in the header, we can ensure that the state will be the same, also with the notifications, the problem will be with statePlugin, it won't include the ledger data, included the notifications, for me is solved with the state root changes in the header.

Copy link
Member

@AnnaShaleva AnnaShaleva Nov 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The problem is that StateRootInHeader doesn’t include the whole set of changes from the native Ledger’s storage; and at least it has to exclude blocks and transactions states. And thus, the problem remains the same for StateRootInHeader.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, if we use a simple hash we will include all of them, included the Ledger changes, you can remove blocks later, but it will take the previous state hash + the current changes hash

Copy link
Member

Choose a reason for hiding this comment

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

if we use a simple hash we will include all of them

Then we need to discuss it, because I don't think we need to include blocks and transactions into state root at all, even if we store them in native Ledger without witnesses. Why do we need them to be included? We have all accepted blocks and transactions relayed onto P2P level, this mechanism allows nodes to sync the chain without going deep into contract's storage level. I consider we don't need exactly the same blocks/transactions data to be included into native contract storage. E.g. in NeoGo we don't store them in native Ledger, we store them in the DB which allows us to implement some optimizations. And NeoGo's StateRootInHeader implementation (nspcc-dev/neo-go#1500, nspcc-dev/neo-go#1701) also doesn't take into account blocks and transactions states.

I know that NeoGo is not a reference implementation, and if we decide to include blocks/transactions into state, then NeoGo will follow, technically it's possible. But do we really need them to be presented into states? I'd suggest to wait for Roman's opinion on this topic.

Copy link
Contributor

Choose a reason for hiding this comment

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

  1. We need a complete state root in the header (How to sign the stateroot？ #1526, yay). Exchanging just the changes state root limits our abilities. Complete state root is what makes P2P state exchange #2373 possible as well as proofs.
  2. This state root should capture all side-effects (Native contract to store application logs #2034).
  3. Ledger state can't be included in full state root calculation.
  4. So storing in the Ledger doesn't seem to be appropriate. At least unless we want to have more magic in https://github.com/neo-project/neo-modules/blob/4f55458bda29a0e80258add2a3768dcc014ab24d/src/StateService/StatePlugin.cs#L97
  5. So we either move it out to a separate contract (a bit cleaner to me), or we do magic.

Copy link
Contributor

Choose a reason for hiding this comment

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

Either way, this can be discussed after #2914 release.


private ReadOnlyMemory<byte> _rawTransaction;

IInteroperable IInteroperable.Clone()
Expand All @@ -47,6 +52,7 @@ IInteroperable IInteroperable.Clone()
BlockIndex = BlockIndex,
Transaction = Transaction,
State = State,
NotificationMerkleRoot = NotificationMerkleRoot,
_rawTransaction = _rawTransaction
};
}
Expand All @@ -57,6 +63,7 @@ void IInteroperable.FromReplica(IInteroperable replica)
BlockIndex = from.BlockIndex;
Transaction = from.Transaction;
State = from.State;
NotificationMerkleRoot = from.NotificationMerkleRoot;
if (_rawTransaction.IsEmpty)
_rawTransaction = from._rawTransaction;
}
Expand All @@ -73,6 +80,7 @@ void IInteroperable.FromStackItem(StackItem stackItem)
_rawTransaction = ((ByteString)@struct[1]).Memory;
Transaction = _rawTransaction.AsSerializable<Transaction>();
State = (VMState)(byte)@struct[2].GetInteger();
NotificationMerkleRoot = new UInt256(@struct[3].GetSpan());
}

StackItem IInteroperable.ToStackItem(ReferenceCounter referenceCounter)
Expand All @@ -81,7 +89,7 @@ StackItem IInteroperable.ToStackItem(ReferenceCounter referenceCounter)
return new Struct(referenceCounter) { BlockIndex };
if (_rawTransaction.IsEmpty)
_rawTransaction = Transaction.ToArray();
return new Struct(referenceCounter) { BlockIndex, _rawTransaction, (byte)State };
return new Struct(referenceCounter) { BlockIndex, _rawTransaction, (byte)State, NotificationMerkleRoot.ToArray() };
}
}
}
27 changes: 27 additions & 0 deletions src/Neo/SmartContract/NotifyEventArgs.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,13 @@
// Redistribution and use in source and binary forms with or without
// modifications are permitted.

using Neo.Cryptography;
using Neo.IO;
using Neo.Network.P2P.Payloads;
using Neo.VM;
using Neo.VM.Types;
using System;
using System.IO;
using Array = Neo.VM.Types.Array;

namespace Neo.SmartContract
Expand Down Expand Up @@ -58,6 +60,31 @@
this.State = state;
}

/// <summary>
/// Get notification Hash, or UInt256.Zero if something is wrong
/// </summary>
public UInt256 GetNotificationHash()
{
using MemoryStream ms = new();
using BinaryWriter writer = new(ms, Utility.StrictUTF8, true);
writer.Write(ScriptHash);

try
{
writer.Write(EventName);
writer.Write(BinarySerializer.Serialize(State, 32));

Check failure on line 75 in src/Neo/SmartContract/NotifyEventArgs.cs

View workflow job for this annotation

GitHub Actions / Test (macos-latest) 

Argument 2: cannot convert from 'int' to 'Neo.VM.ExecutionEngineLimits'

Check failure on line 75 in src/Neo/SmartContract/NotifyEventArgs.cs

View workflow job for this annotation

GitHub Actions / Test (macos-latest) 

Argument 2: cannot convert from 'int' to 'Neo.VM.ExecutionEngineLimits'
}
catch
{
// It might have more state entries than expected or an unsupported event name encoding.

return UInt256.Zero;
}

writer.Flush();
return new UInt256(ms.ToArray().Sha256());
}

public void FromStackItem(StackItem stackItem)
{
throw new NotSupportedException();
Expand Down
19 changes: 16 additions & 3 deletions tests/Neo.UnitTests/Ledger/UT_TransactionState.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,10 +42,17 @@ public void Initialize()
InvocationScript=Array.Empty<byte>(),
VerificationScript=Array.Empty<byte>()
} }
}
},
NotificationMerkleRoot = UInt256.Zero
};
originTrimmed = new TransactionState
{
NotificationMerkleRoot = UInt256.Zero,
ConflictingSigners = new UInt160[]
{
new UInt160(Crypto.Hash160(new byte[] { 1, 2, 3 })),
new UInt160(Crypto.Hash160(new byte[] { 4, 5, 6 }))
}
BlockIndex = 1,
};
}
Expand All @@ -56,7 +63,10 @@ public void TestDeserialize()
var data = BinarySerializer.Serialize(((IInteroperable)origin).ToStackItem(null), ExecutionEngineLimits.Default);
var reader = new MemoryReader(data);

TransactionState dest = new();
TransactionState dest = new()
{
NotificationMerkleRoot = UInt256.Zero
};
((IInteroperable)dest).FromStackItem(BinarySerializer.Deserialize(ref reader, ExecutionEngineLimits.Default, null));

dest.BlockIndex.Should().Be(origin.BlockIndex);
Expand All @@ -70,7 +80,10 @@ public void TestDeserializeTrimmed()
var data = BinarySerializer.Serialize(((IInteroperable)originTrimmed).ToStackItem(null), ExecutionEngineLimits.Default);
var reader = new MemoryReader(data);

TransactionState dest = new();
TransactionState dest = new()
{
NotificationMerkleRoot = UInt256.Zero
};
((IInteroperable)dest).FromStackItem(BinarySerializer.Deserialize(ref reader, ExecutionEngineLimits.Default, null));

dest.BlockIndex.Should().Be(originTrimmed.BlockIndex);
Expand Down
Loading