New staking - part 1 and 2 - SWIP-19 and 20 combined

[0xCardinalError]

This implements the feature allowing staked nodes to change their overlay without restaking (called neighbourhood hopping, see SWIP-19)

Implemented by changing stake registry to be keyed with node address instead of node overlay.

[zelig]

fundamental issues with how stake is queried, in its current form allows for a critical exploit: it is possible to play with several overlays derived from the same address simultaneously while using only one stake. This is easily fixable by passing the overlay as well as an argument (to functions called from redis contract to validate stake of a node) and checking the overlay against the one recorded to the address currently.

The chosen implementation (see discussion on research internal) is probably the best option but it requires extensive changes in the interfaces therefore the most work for the bee team. Also it needs a thorough change to the SWIP-19

[zelig]

i took back the attack story, the overlay is checked here

storage-incentives/src/Redistribution.sol

Line 303 in 25e7b18

if (Stakes.overlayOfAddress(msg.sender) != _overlay) {

but still issues left

Solved

[nugaon]

Nothing critical but it would make difficulties with maintainability in a long run and confusion around keeping track overlay changes.

Also check the linter errors, the code has many because of the max-line-length rule which is set to 120.

[nugaon]

overlay can be changed in depositStake function as well, but this event is not omitted. I would remove this function and if overlay changes in depositStake this event should be emitted. having two function to do the same and only one keeping track with one specific change leads to confusion.

[nugaon]

same as above

[zelig]

I second @nugaon 's comments
and wonder why you removed the stake struct lookup to prevent repeated map lookups

[0xCardinalError]

I second @nugaon 's comments and wonder why you removed the stake struct lookup to prevent repeated map lookups

I made response on this earlier but seems it been missed so I will copy paste it here also at @nugaon @zelig

"Did some measurements...
Thing is that "getStake" is wasteful in cases where there is lookup for just one of the struct values.
Getter for each individual struct value by itself uses up to 2900 gas while getStake uses 9861 gas, as it fetches all values from storage, which means that getStake is only useful if we are operating with all values from struct, if not, then its less desirable option and we should use single getters for each value, which means all this single value getters are staying and we do use them in most cases."

[ldeffenb]

First, is there going to be a migration of the existing stakes from overlay-based to owner-based?

And if so, has anyone given thought (or better yet, a test) to what happens with multiple abandoned overlay-based stakes that are all owned by a single account? This happens when people keep their keys, but delete the rest of their data-dir causing a new overlay to be generated from the original account.

See this (short) conversation in Discord. Start at the link and follow the replies backwards in time.
https://discord.com/channels/799027393297514537/811553590170353685/1251173722245693460

[0xCardinalError]

First, is there going to be a migration of the existing stakes from overlay-based to owner-based?

And if so, has anyone given thought (or better yet, a test) to what happens with multiple abandoned overlay-based stakes that are all owned by a single account? This happens when people keep their keys, but delete the rest of their data-dir causing a new overlay to be generated from the original account.

See this (short) conversation in Discord. Start at the link and follow the replies backwards in time. https://discord.com/channels/799027393297514537/811553590170353685/1251173722245693460

yes, there will be migration but basically it will be from users/operators side and we will need to write instructions on that and communicate them. Basically old contract would be paused, nodes would be taking funds out and could put them in new contract for staking.

About abandoned overlays, if the user has private key of the address used for that node he should be able to take funds out, if he knows the overlay, he can just input it and take funds out.
If ppl are forgetting what they own, we could take data from blockchain and make a document with which address has which overlay and what amount so they could search for it there and take the funds out for themselves.