Node table limiting and cache for node filter

[VladLupashevskyi]

After removing node filter cache (#10121), the nodes were still loosing the the peers, however in this case the time when it starts to loose them was very dependent on the CPU power, so when I was testing the fix it was working well for couple of days on my i7 4th gen, but when I left for longer time peers loosing started to occur again.

I have done some research and found out that the problem was in this line:
https://github.com/paritytech/parity-ethereum/blob/06cae8a53556dede67e163325657bee3fba8feda/util/network-devp2p/src/host.rs#L598

where it was querying the contract on every state. But the interesting thing I found that the node table was not limited in size and was always growing. So after 4 hrs of running there were already 10k entries, so the state was queried 10k times each second. In this case introducing the cache is not really a solution, because once we hit the limit of it the querying starts again.

First of all I thought I found a typo in update_table fn where ; was missing after for loop:
https://github.com/paritytech/parity-ethereum/blob/06cae8a53556dede67e163325657bee3fba8feda/util/network-devp2p/src/node_table.rs#L349
But because of lack of experience in rust adding ; was not helpful.

So I decided to introduce the node table limiting which would allow to use caching for Node Filter, but also guarantee more predictable behaviour for nodes that don't use it, so the large size won't be causing any performance issues when node runs for very long time.

The limiting is done in the way that in case of adding a new node and hitting the limit, the least important node gets removed (either failure one with the most recent last_connected state, or node with unknown last_connected state, or the node with the oldest successful last_connected state).

I introduced simple cache for Node Filter based on RwLock which is reset on the new block. It's not a perfect solution, but I would say more generic, and especially useful for the chains where the filter gets dynamically updated. As further improvement I would see an opportunity to introduce a new version of node permission contract where the event NodeTableUpdated would be emitted when the list of allowed nodes is updated and clear the cache in this case, or for current contract version - checking the txes to the node permission contract on each block and clear the cache if they are present there (not sure how it would affect the performance though, but I believe it's better than just clearing the cache on each block).

I have done some checks and on current 2.3.0-beta when node filter contract is enabled, the CPU has 30% of load in the beginning and keeps growing, with this update the CPU load is constantly about 15-30% on AuRa chain with block time 4 seconds. When the contract is disabled the load is 5-10%.

Looking forward to get any comments or suggestions to these changes :)

[parity-cla-bot]

It looks like @VladLupashevskyi signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

[parity-cla-bot]

It looks like @VladLupashevskyi signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

[HCastano]

I took a quick peek through the code and it looks like there are a few .unwrap() calls throughout. As a general rule we try and avoid panics, so you're either going to need to:

a) Remove the call to .unwrap()and handle the failure case more gracefully
b) Write a little proof stating why this will not panic, and include that within an .expect() call

[ascjones]

Thanks for the PR. Just realised I had left this review in progress, but similar comments to @HCastano regarding unwraps.

[grbIzl]

If we understand the real reason for CPU overload (not limited Node table), can we just rollback #10143 and return previous LRUCache, that was used here? I frankly don't see the advantages of proposed solution in comparison with the previous one.

[VladLupashevskyi]

The idea of implementing it via RwLock was to make it possible to read the cache from different threads without blocking each other.

[grbIzl]

Ok. Than may be just change previous Mutex to RwLock? Because order field in Cache is just the implementation of Lru

[VladLupashevskyi]

Got it! I think that makes sense. Sorry I didn't notice and I was thinking that LruCache comes already with built-in Mutex 😅

[VladLupashevskyi]

Actually I cannot really use LruCache since it does not provide non-mutable get method, so it's not possible to use RwLock with it

[ngotchac]

What about the peek method: https://docs.rs/lru/0.1.13/lru/struct.LruCache.html#method.peek ?

[grbIzl]

@VladLupashevskyi In this part of code: https://github.com/paritytech/parity-ethereum/blob/d89b8d904ff28382d3692290a936706304627255/util/network-devp2p/src/host.rs#L595 we create an iterator over the node table and take some amount of nodes from it (~30 nodes). So the only scenario, when such performance degradation is possible, is when you have a lot of nodes forbidden in your permission contract and iterator goes over them, looking for allowed. I think, that it will be true for whitelisting permission contracts. For blacklisting contracts this code should return the required amount of nodes quicker.

[VladLupashevskyi]

@VladLupashevskyi In this part of code:

parity-ethereum/util/network-devp2p/src/host.rs

Line 595 in d89b8d9

for id in nodes.filter(|id|
we create an iterator over the node table and take some amount of nodes from it (~30 nodes). So the only scenario, when such performance degradation is possible, is when you have a lot of nodes forbidden in your permission contract and iterator goes over them, looking for allowed. I think, that it will be true for whitelisting permission contracts. For blacklisting contracts this code should return the required amount of nodes quicker.

Yes, you are absolutely right - I've experienced this behaviour when was using whitelisting permissioning in the contract. But shouldn't this contract have a generic design, allowing to do whitelisting and blacklisting depending on the design of the blockchain? I believe that first is very crucial in permissioned consortium chains.

[grbIzl]

But shouldn't this contract have a generic design, allowing to do whitelisting and blacklisting depending on the design of the blockchain? I believe that first is very crucial in permissioned consortium chains.

I'm not arguing, it was just an analysis to help better understand the issue. We indeed must support any type of permissioning contract.

[VladLupashevskyi]

Here I fixed some bugs with get_index_to_insert and added tests for it. I think the most appropriate place to test this function is in table_last_contact_order test, so in order to prevent its failing on macos due to lost nanoseconds precision I had to revert the fix 5a1dc3e was done by @debris, because the test become more complicated there and use sleep(Duration::from_micros(1)) instead as simple fix. I think it shouldn't affect the test execution time, however the code become slightly bulky.

[VladLupashevskyi]

I also removed get_mut method for NodeTable and introduced get instead to prevent occasional modification of the nodes HashMap.

[VladLupashevskyi]

Could somebody review this? We are looking forward to that PR being reviewed and merged if possible :)

[grbIzl]

I'm ok with these changes, but need more eyes for node_table new logic. @ngotchac can you help with one more review?