Fix Parallel verification issues (v1) by shargon · Pull Request #1417 · neo-project/neo

shargon · 2020-01-15T14:04:02Z

Note: The issue doesn't happen without parallel verification.

src/neo/Ledger/SendersFeeMonitor.cs

shargon · 2020-01-15T14:15:39Z

I will fix the UT when we decide if this is the right fix. Maybe we should move this check to Blockchain.OnParallelVerified

Tommo-L · 2020-01-16T01:39:03Z

src/neo/Ledger/MemoryPool.cs

+                {
+                    _unsortedTransactions.Remove(hash);
+                    return false;
+                }


I think this isn't the best way. And it's not enough when balance < SendersFeeMonitor.AddSenderFee(tx), more txs will be removed from memory pool.

Since we already have UpdatePoolForBlockPersisted, and this TryAdd is called from a single thread (the Blockchain actor). We need to find the cause of this phenomenon.

neo/src/neo/Ledger/MemoryPool.cs

Lines 351 to 361 in fc1a64b

internal void UpdatePoolForBlockPersisted(Block block, StoreView snapshot)

{

bool policyChanged = LoadPolicy(snapshot);

_txRwLock.EnterWriteLock();

try

{

// First remove the transactions verified in the block.

foreach (Transaction tx in block.Transactions)

{

if (TryRemoveVerified(tx.Hash, out _)) continue;

This happend because it's not a single thread, Blockchain actor, but was send by RPC

https://github.com/neo-project/neo-modules/blob/ef91c522f13ef30bc6308a2e343a4c3466bb3885/src/RpcServer/RpcServer.Wallet.cs#L288

So we can add a lock in OnNewTransaction and check if it's solved

So we can add a lock in OnNewTransaction and check if it's solved

It doesn't work in my tests..

cloud8little

I can still get the error with one single CN, and the sender wallet with only one address.

[15:19:21.903] relay block: height=23 hash=0x35e977d9d7eeabc3e0e568cb9cc6590ebc378929d34fd283c32f19abb20e45c2 tx=0
[15:19:21.913] persist block: height=23 hash=0x35e977d9d7eeabc3e0e568cb9cc6590ebc378929d34fd283c32f19abb20e45c2 tx=0
[15:19:21.923] initialize: height=24 view=0 index=0 role=Primary
[15:19:36.922] timeout: height=24 view=0
[15:19:36.930] send prepare request: height=24 view=0
[15:19:36.954] send commit
[15:19:36.965] relay block: height=24 hash=0x85d722eecef7f7b1f8d0ac761e13f999ed27584f0abb1fc7336f9bc404cdd2ef tx=512
[ERROR][2020/1/16 7:19:37][Thread 0021][akka://NeoSystem/user/$a] Operation is not valid due to the current state of the object.
Cause: System.InvalidOperationException: Operation is not valid due to the current state of the object.
   at Neo.Ledger.Blockchain.Persist(Block block) in D:\NEO\Github\neo\src\neo\Ledger\Blockchain.cs:line 501
   at Neo.Ledger.Blockchain.OnNewBlock(Block block) in D:\NEO\Github\neo\src\neo\Ledger\Blockchain.cs:line 328
   at Neo.Ledger.Blockchain.OnReceive(Object message) in D:\NEO\Github\neo\src\neo\Ledger\Blockchain.cs:line 465
   at Akka.Actor.UntypedActor.Receive(Object message)
   at Akka.Actor.ActorBase.AroundReceive(Receive receive, Object message)
   at Akka.Actor.ActorCell.ReceiveMessage(Object message)
   at Akka.Actor.ActorCell.Invoke(Envelope envelope)
[ERROR][2020/1/16 7:19:37][Thread 0021][akka://NeoSystem/user/$a] Error while creating actor instance of type Neo.Ledger.Blockchain with 2 args: (Neo.NeoSystem,Neo.Plugins.Storage.Store)
Cause: [akka://NeoSystem/user/$a#1090028194]: Akka.Actor.PostRestartException: Exception post restart (System.InvalidOperationException)
 ---> System.TypeLoadException: Error while creating actor instance of type Neo.Ledger.Blockchain with 2 args: (Neo.NeoSystem,Neo.Plugins.Storage.Store)
 ---> System.Reflection.TargetInvocationException: Exception has been thrown by the target of an invocation.
 ---> System.InvalidOperationException: Operation is not valid due to the current state of the object.
   at Neo.Ledger.Blockchain..ctor(NeoSystem system, IStore store) in D:\NEO\Github\neo\src\neo\Ledger\Blockchain.cs:line 111
   --- End of inner exception stack trace ---
   at System.RuntimeMethodHandle.InvokeMethod(Object target, Object[] arguments, Signature sig, Boolean constructor, Boolean wrapExceptions)
   at System.Reflection.RuntimeConstructorInfo.Invoke(BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
   at System.RuntimeType.CreateInstanceImpl(BindingFlags bindingAttr, Binder binder, Object[] args, CultureInfo culture)
   at System.Activator.CreateInstance(Type type, BindingFlags bindingAttr, Binder binder, Object[] args, CultureInfo culture, Object[] activationAttributes)
   at System.Activator.CreateInstance(Type type, Object[] args)
   at Akka.Actor.Props.ActivatorProducer.Produce()
   at Akka.Actor.Props.NewActor()
   --- End of inner exception stack trace ---
   at Akka.Actor.Props.NewActor()
   at Akka.Actor.ActorCell.CreateNewActorInstance()
   at Akka.Actor.ActorCell.<>c__DisplayClass109_0.<NewActor>b__0()
   at Akka.Actor.ActorCell.UseThreadContext(Action action)
   at Akka.Actor.ActorCell.NewActor()
   at Akka.Actor.ActorCell.FinishRecreate(Exception cause, ActorBase failedActor)
   --- End of inner exception stack trace ---

sender wallet with initial balance 100000 gas.
request:

{
  "jsonrpc": "2.0",
  "method": "sendtoaddress",
  "params": ["0x8c23f196d8a1bfd103a9dcb1f9ccf0c611377d3b", "NcnELA9BMsxDCzatGmTT5Tbq5dFTEXMarN",0.00000001],
  "id": 1
}

use jmeter to send 600 requests at one time.

Tommo-L · 2020-01-16T07:50:37Z

I'll help investigate this problem

shargon · 2020-01-16T07:55:02Z

@cloud8little thanks for your test, i spent half hour without issues and before the fix the issue was thrown in the first 4 blocks... I will review it more!

shargon · 2020-01-16T12:41:18Z

@cloud8little if you try to replicate your issue with multiple threads, you will get another error produced by parallel verification

src/neo/Ledger/MemoryPool.cs

eryeer

Good job, In this case the gas calculation error can be prevented.

vncoelho · 2020-01-17T10:17:21Z

src/neo/Ledger/MemoryPool.cs

            try
            {
+                // Check balance
+                BigInteger balance = NativeContract.GAS.BalanceOf(snapshot, tx.Sender);


In this sense, the key error was to not consider the current Balance of the sender, which is updated in parallel during OnPersist event?

I think so.

I think we should move the "balance check" to BlockChain.OnParallelVerified, which is right place to verify the transaction.

cloud8little · 2020-01-17T10:28:46Z

Test cases:

start a single CN
start a normal node connect to the CN.
withdraw the initial neo and gas.
send 100000 neo and 500 gas to a wallet a.json
send rpc requests, such as below; the goal is to send 400 requests at a time, and after 30 secs, send another 400 requests.

{
  "jsonrpc": "2.0",
  "method": "sendtoaddress",
  "params": ["0x9bde8f209c88dd0e7ca3bf0af0f476cdd8207789", "NcnELA9BMsxDCzatGmTT5Tbq5dFTEXMarN",1],
  "id": 1
}

Test result: [PASS]

CN is able to handle gas insufficient situation.
after requests are all dealt with, checked the account balance is correct.

erikzhang · 2020-01-17T15:11:38Z

I have told you that it is very dangerous to use multithreading in actors.

erikzhang · 2020-01-17T15:19:32Z

I think we can call tx.VerifyForEachBlock() in OnParallelVerified().

shargon · 2020-01-17T16:23:05Z

We need to check if after this changes, parallel will be faster

shargon · 2020-01-17T16:34:30Z

I think we can call tx.VerifyForEachBlock() in OnParallelVerified().

#1423 we can choose wich is the best one.

Fix

6779b51

shargon requested review from belane, cloud8little, erikzhang, eryeer and vncoelho January 15, 2020 14:04

Fix ut

0db1633

vncoelho reviewed Jan 15, 2020

View reviewed changes

src/neo/Ledger/SendersFeeMonitor.cs Outdated Show resolved Hide resolved

Tommo-L reviewed Jan 16, 2020

View reviewed changes

cloud8little reviewed Jan 16, 2020

View reviewed changes

shargon closed this Jan 16, 2020

shargon reopened this Jan 16, 2020

shargon mentioned this pull request Jan 16, 2020

Parallel verification issues #1420

Closed

Merge branch 'master' into fix-1410

c4de3ea

Tommo-L reviewed Jan 17, 2020

View reviewed changes

src/neo/Ledger/MemoryPool.cs Outdated Show resolved Hide resolved

shargon added 2 commits January 17, 2020 09:19

Optimize

1b76111

Fix UT

780e1b6

eryeer reviewed Jan 17, 2020

View reviewed changes

vncoelho reviewed Jan 17, 2020

View reviewed changes

cloud8little approved these changes Jan 17, 2020

View reviewed changes

shargon mentioned this pull request Jan 17, 2020

Fix Parallel verification issues (v2) #1423

Closed

shargon mentioned this pull request Jan 21, 2020

Remove parallel verification #1429

Merged

Merge branch 'master' into fix-1410

c898379

shargon changed the title ~~Fix Parallel verification issues~~ Fix Parallel verification issues (v1) Jan 21, 2020

erikzhang closed this Mar 3, 2020

shargon deleted the fix-1410 branch March 3, 2020 07:14

	internal void UpdatePoolForBlockPersisted(Block block, StoreView snapshot)
	{
	bool policyChanged = LoadPolicy(snapshot);

	_txRwLock.EnterWriteLock();
	try
	{
	// First remove the transactions verified in the block.
	foreach (Transaction tx in block.Transactions)
	{
	if (TryRemoveVerified(tx.Hash, out _)) continue;

Conversation

shargon commented Jan 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

shargon commented Jan 15, 2020

Uh oh!

Tommo-L Jan 16, 2020

Choose a reason for hiding this comment

Uh oh!

Tommo-L Jan 16, 2020

Choose a reason for hiding this comment

Uh oh!

shargon Jan 16, 2020

Choose a reason for hiding this comment

Uh oh!

shargon Jan 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cloud8little left a comment

Choose a reason for hiding this comment

Uh oh!

Tommo-L commented Jan 16, 2020

Uh oh!

shargon commented Jan 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shargon commented Jan 16, 2020

Uh oh!

Uh oh!

eryeer left a comment

Choose a reason for hiding this comment

Uh oh!

vncoelho Jan 17, 2020

Choose a reason for hiding this comment

Uh oh!

erikzhang Jan 17, 2020

Choose a reason for hiding this comment

Uh oh!

Tommo-L Jan 17, 2020

Choose a reason for hiding this comment

Uh oh!

cloud8little commented Jan 17, 2020

Uh oh!

erikzhang commented Jan 17, 2020

Uh oh!

erikzhang commented Jan 17, 2020

Uh oh!

shargon commented Jan 17, 2020

Uh oh!

shargon commented Jan 17, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

shargon commented Jan 15, 2020 •

edited

Loading

shargon Jan 16, 2020 •

edited

Loading

shargon commented Jan 16, 2020 •

edited

Loading