Remove the lock from SendersFeeMonitor and rename it to TransactionVerificationContext

[erikzhang]

No description provided.

[erikzhang]

The UTs need to be fixed.

[Tommo-L]

The UTs need to be fixed.

We'll fix the UT.

[shargon]

If we remove the lock we should close this (#1507) and add a comment that it's not thread safe

[Tommo-L]

See #1759

[erikzhang]

@neo-project/ngd-shanghai It needs to be tested.

[cloud8little]

@neo-project/ngd-shanghai It needs to be tested.

I will have a test.
Test scenarios:

1.  Send single transaction neo/gas.
2.  Parallel send transaction(neo) when gas is sufficient & insufficient.
3.  Parallel send transaction(gas) when gas is sufficient & insufficient. 
4.  Parallel send transaction(neo) when neo is insufficient.

[cloud8little]

There is one issue needed to be investigated. When sending parallel tx, and somehow there are txs can not be relayed and stayed in the memory pool, then send valid tx will not either added in the mempool, or relay to the consensus node.

1. Send single transaction neo/gas. PASSED
2. Parallel send transaction(neo) when gas is sufficient & insufficient. PASSED
3. Parallel send transaction(gas) when gas is sufficient & insufficient. PASSED
4. Parallel send transaction(neo) when neo is insufficient. PASSED
5. When mempool has tx, send valid tx. FAIL

[erikzhang]

When mempool has tx, send valid tx. FAIL

How does this happen?

[cloud8little]

When mempool has tx, send valid tx. FAIL

How does this happen?

Not sure how this happen, I am trying to reproduce it. Currently thought it maybe caused by two addresses in the account, and one with insufficient gas and one with enough gas, send massive gas at the same time(use sendtoaddress request).

[Tommo-L]

I'll help investigate the problem tomorrow.

[cloud8little]

When mempool has tx, send valid tx. FAIL

How does this happen?

1. Prepare a wallet, here I use wallet_1.json, actually it is my CN wallet. with one standard address, one multisig address(m=1); initial asset is

neo> list asset
Nc56Zrq72VN9Xr6wo3HyFn3oFZUvoYQJtj
NEO: 49990000
GAS: 47.3869169

Ncu47NyTJLJPEFi859gmuqQ8fPPt1AVwHX
NEO: 50000000
GAS: 29930613.53667778

----------------------------------------------------
Total:   NEO: 99990000    GAS: 29930660.92359468

NEO hash: 0xde5f57d430d3dece511cf975a8d37848cb9e0525
GAS hash: 0x668e0c1f9d7b70a99dd9e06eadd4c784d641afbc

2. use Jmeter to run parallel sendtoaddress requests;

{
  "jsonrpc": "2.0",
  "method": "sendtoaddress",
  "params": ["0x668e0c1f9d7b70a99dd9e06eadd4c784d641afbc", "NSJZ186oShqryg6D8xfcv5Te5GhQ8GFno9", 10.23456789],
  "id": 1
}

3. when the requests are not finished yet, stop the task.
4. then I getrawmempool from the node, there are 9 txs in the pool for my case.

List asset:

5. Then send a single sendtoaddress request to the node. From the debug process, MemPool.TryAdd always return Insufficient when deal with the sendtoaddress tx.
   

[cloud8little]

For comparison, use Neo without this PR, there is not such issue, when stop sending massive tx, tx in mempool can be sent successfully.

[Qiao-Jin]

If we remove the lock we should close this (#1507) and add a comment that it's not thread safe

We don't need to close #1507, as balance checking is not done in parallel in this PR. #1507 only checks witness in parallel.

[shargon]

We don't need to close #1507, as balance checking is not done in parallel in this PR. #1507 only checks witness in parallel.

I see that it check that it's standard account, very nice!

[erikzhang]

@cloud8little Please test it again.

[cloud8little]

@cloud8little Please test it again.

Got it.

[Tommo-L]

Then send a single sendtoaddress request to the node. From the debug process, MemPool.TryAdd always return Insufficient when deal with the sendtoaddress tx.

This is correct, it's insufficient, as there are 9 txs in mempool.

then I getrawmempool from the node, there are 9 txs in the pool for my case.

The 9 txs need to be rebroadcastet, but it takes a lot of time. This is still under confirmation.

[shargon]

The code looks well, waiting for @cloud8little

[cloud8little]

The code looks well, waiting for @cloud8little

There is still the issue, and initial balance is

Nc56Zrq72VN9Xr6wo3HyFn3oFZUvoYQJtj
NEO: 50000000
GAS: 60.3456922

Ncu47NyTJLJPEFi859gmuqQ8fPPt1AVwHX
NEO: 50000000
GAS: 29999870.8971756

after running massive sendtoaddress requests:
there are two txs stay in the mempool and can not added to the blockchain. call self-defined method to print the sender fee as below.
[Highlight part]
(TransactionVerificationContext.cs)

public void Print()
        {
            foreach(var item in senderFee)
            {
                Console.WriteLine("sender: " + item.Key + " fee: " + item.Value);
            }
        }

Debug info:

It can be rebroadcast,but can not added to the blockchain.

[erikzhang]

Why they can't be added to blockchain?

[shargon]

@cloud8little could you try without this PR the same tests?

[cloud8little]

@cloud8little could you try without this PR the same tests?

Tried without this PR, with random sending massive tx and with the similar wallet asset configuration, out of 1/10+ times, there will be the same issue: one tx stay in the mempool and can not be broadcast.

Compare with this PR（about show up 1/4 times）, the issue show up less, it is easier to show up in this PR.

Why they can't be added to blockchain?

Not very clear with this, only found the following lines can be touched. but there is no tx in CN node's mempool.

neo/src/neo/Ledger/MemoryPool.cs

Lines 446 to 450 in 9d996e0

	if (item.LastBroadcastTimestamp < rebroadcastCutOffTime)
	{
	_system.LocalNode.Tell(new LocalNode.RelayDirectly { Inventory = item.Tx }, _system.Blockchain);
	item.LastBroadcastTimestamp = DateTime.UtcNow;
	}

[shargon]

Then I think that it's not related to this PR, we can merge it, and open a new issue

[shargon]

It's possible that this node have this tx in knownHashes but it removed this tx from his memory pool?

[erikzhang]

It's possible that this node have this tx in knownHashes but it removed this tx from his memory pool?

That's it! The transactions are removed by MemoryPool.RemoveOverCapacity(), but they are still in knownHashes.

neo/src/neo/Ledger/MemoryPool.cs

Lines 296 to 309 in 9d996e0

	private List<Transaction> RemoveOverCapacity()
	{
	List<Transaction> removedTransactions = new List<Transaction>();
	do
	{
	PoolItem minItem = GetLowestFeeTransaction(out var unsortedPool, out var sortedPool);
	unsortedPool.Remove(minItem.Tx.Hash);
	sortedPool.Remove(minItem);
	removedTransactions.Add(minItem.Tx);
	} while (Count > Capacity);
	return removedTransactions;
	}

[erikzhang]

I will merge this PR first.