Fix taskManager

[Tommo-L]

No description provided.

[erikzhang]

What is this?

[Tommo-L]

What is this?

The last line will block nodes to relay transactions, even if the node is only one block behind.

neo/src/neo/Network/P2P/TaskManager.cs

Lines 93 to 99 in 095d7f7

	private void OnNewTasks(InvPayload payload)
	{
	if (!sessions.TryGetValue(Sender, out TaskSession session))
	return;
	// Do not accept payload of type InventoryType.TX if not synced on best known HeaderHeight
	if (payload.Type == InventoryType.TX && Blockchain.Singleton.Height < sessions.Values.Max(p => p.LastBlockIndex))
	return;

[erikzhang]

Suggested change

	if (payload.Type == InventoryType.TX && sessions.Values.Where(p => p.LastBlockIndex > Blockchain.Singleton.Height + 12).Count() > sessions.Count / 2)
	if (payload.Type == InventoryType.TX && sessions.Values.Any(p => p.LastBlockIndex > Blockchain.Singleton.Height + 12))

What do you think?

[Tommo-L]

No, it can be easily attacked.

[erikzhang]

I think sessions.Count / 2 is too large.

[Tommo-L]

Adjusted to sessions.Count/3.

[vncoelho]

So as it is now with +12 it will accept more transactions? Also based on the number of peers it has?

[Tommo-L]

Before this, some seed nodes can't relay transactions to consensus nodes in preview4 testnet.

[roman-khimov]

FYI, related: https://github.com/nspcc-dev/neo-go/blob/df6cb1101780c1acded30fc2befdd9d6d2048611/pkg/network/server.go#L414-L445

[shargon]

What happens if I connect to you some nodes that never sync?

[superboyiii]

Current testnet has this issue that Preview3 nodes are influencing Preview4 nodes because they're higher in peers list which makes tx can't be relayed.

[shargon]

I think that it's better to remove this lines, the mempool will work as expected, if it's filled, will be trimmed, so why we need to avoid this messages during sync based on untrusted information? Please check #2180

[roman-khimov]

That's the way neo-go currently works (IsInSync logic mentioned earlier is only used to determine when we should start dbft). The only problem with this is that the node can try relaying invalid transactions which can be interpreted as suspicious behaviour by its peers. But these transactions should at the same time fall within MaxValidUntilBlockIncrement range from current node's height which is not really likely to happen.

[superboyiii]

Let's move on?

[superboyiii]

Already updated testnet with this fix, works well now. @erikzhang

[superboyiii]

@erikzhang Merge?

[superboyiii]

@erikzhang @shargon Merge?

[erikzhang]

@Tommo-L I think maybe this is not the best solution. In NEO2, we sync the headers first, we can easily detect whether we are on the best known height. Now we sync the blocks directly, we can't do that.

[Tommo-L]

Then we'd better merge this #2180 first, and we can add #1811 later.

[erikzhang]

#1811 doesn't solve the problem.

[superboyiii]

Can we make out a good solution before Preview5 release? If not, maybe we could merge this first, although not the best way but at least solve the issue. Otherwise we'll meet the same issue on Preview5 testnet.

[Tommo-L]

@Tommo-L I think maybe this is not the best solution. In NEO2, we sync the headers first, we can easily detect whether we are on the best known height. Now we sync the blocks directly, we can't do that.

If a node is too far behind, every time a block is synchronized, the transaction in the memory pool will be re-verified. In fact, this part is not very time-consuming, because most of the verification time is spent on verifying the signature.

In addition, we can optimize in the future. If multiple blocks are being synchronized, we can only use the last block(the highest height block) to verify transactions in the memory pool.

[erikzhang]

In fact, this part is not very time-consuming, because most of the verification time is spent on verifying the signature.

This is uncertain, and we cannot rely on this assumption. Otherwise this will become a weakness.

[Tommo-L]

Maybe we can derive the "best" header from UnverifiedBlock, then we can avoid excessive re-verify.

neo/src/neo/Ledger/Blockchain.cs

Lines 301 to 307 in 8c72d54

	private VerifyResult OnNewBlock(Block block)
	{
	if (block.Index <= Height)
	return VerifyResult.AlreadyExists;
	if (block.Index - 1 > Height)
	{
	AddUnverifiedBlockToCache(block);

[Tommo-L]

We'll add header back.

[Tommo-L]

if (payload.Type == InventoryType.TX && !sessions.Values.Any(p => p.LastBlockIndex > Blockchain.Singleton.Height  - 12 && p.LastBlockIndex < Blockchain.Singleton.Height + 12))

Will it be ok? I think one node must connect a correct node.