-
Notifications
You must be signed in to change notification settings - Fork 21
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add SWIP-19: Neighbourhood hopping #48
Open
dysordys
wants to merge
13
commits into
master
Choose a base branch
from
nhood-hopping
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 5 commits
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
1fad2c3
Neighbourhood hopping SWIP fully specified
0f5fd10
SWIP-19: minor edits
mamborider 3eb0f08
SWIP-19 minor edits
mamborider 1b82350
Merge pull request #52 from mamborider/nhood-hopping
zelig 05268a7
add interface changes and used logic in whole improvment
n00b21337 4dafc79
bee changes
zelig a162fbe
Update SWIPs/swip-19.md
n00b21337 e19c839
Update SWIPs/swip-19.md
n00b21337 8b8fc6b
bee node support for hopping
zelig b877fec
Update swip-19.md
n00b21337 1c577ca
Update swip-19.md
n00b21337 2671c20
Update swip-19.md
n00b21337 12abec7
Update swip-19.md
n00b21337 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,10 +1,132 @@ | ||
--- | ||
SWIP: 19 | ||
title: Neighbourhood hopping | ||
author: György Barabás <gyorgy@ethswarm.org> (@dysordys), Viktor Trón <viktor@ethswarm.org> (@zelig) | ||
author: György Barabás <gyorgy@ethswarm.org> (@dysordys), Viktor Trón <viktor@ethswarm.org> (@zelig), Mark Bliss <marko@ethswarm.org> | ||
discussions-to: https://discord.gg/Q6BvSkCv | ||
status: Draft | ||
type: Standards Track | ||
category: Core | ||
created: 2024-05-08 | ||
--- | ||
|
||
## Abstract | ||
|
||
This SWIP describes the process to support **neighbourhood hopping**, i.e., moving a node to another neighbourhood without the need to pay the stake each time. | ||
|
||
## Objectives | ||
|
||
Enable a node to change its overlay address while preserving the stake it already possesses. | ||
|
||
## Context | ||
|
||
Neighbourhood hopping is currently supported by the Bee client. The overlay address, which specifies which neighbourhood a node falls into, is derived by hashing the node key together with an arbitrary overlay nonce chosen by the user (refer to the figure below). | ||
|
||
![](assets/swip-19/overlay-definition.png) | ||
|
||
Bee already supports placing one's node in an arbitrary neighbourhood by mining an overlay nonce that will result in the corresponding neighbourhood. | ||
|
||
Currently, the staking smart contract associates a stake with an overlay address. Therefore, neighbourhood hopping requires the node operator to re-stake their node each time they change neighbourhoods. This is considered unfair and contrary to the system's ethos, as the ability to change neighbourhoods fluently is essential for the balanced distribution of nodes across the network. | ||
|
||
## Specification | ||
|
||
To support neighbourhood hopping with transferable stakes, the staking contract should register the user's Ethereum address instead of their overlay address. | ||
|
||
## Implementation notes | ||
|
||
The implementation requires change of indices: from one keyed by the overlay address to another keyed by Ethereum address. The staking API already requires the sender to send the so-called overlay nonce to the nodes to calculate the overlay address. Therefore, there will be changes to the interface functions. | ||
|
||
### Staking contract changes | ||
|
||
#### The overlay change endpoint: | ||
|
||
1. Set the new nonce to | ||
|
||
`function changeOverlay(bytes32 _nonce)` | ||
|
||
node address is auto picked from the msg.sender and used to calculate new overlay | ||
|
||
2. Event will be trigered to show that overlay has been changed | ||
|
||
`emit OverlayChanged(msg.sender, overlay);` | ||
|
||
#### The stake deposit endpoint: | ||
|
||
From previous function we now have the one where we don't send nodes addrees it is auto picked from sender and calculated | ||
n00b21337 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
`function depositStake(bytes32 _nonce, uint256 _amount)` | ||
|
||
We also changed the event, now it emits different indexed variable which is owner of node | ||
|
||
`emit StakeUpdated(msg.sender, updatedAmount, overlay, block.number);` | ||
|
||
#### Withdraw from stake | ||
|
||
Withdraw will now also be done with address and not overlay so this should be called, address is auto picked from msg.sender | ||
|
||
`function withdrawFromStake(uint256 _amount)` | ||
|
||
#### Freez and slash deposit | ||
|
||
Both freeze and slash deposit are now done by address so we call | ||
|
||
`function freezeDeposit(address _owner, uint256 _time)` | ||
`function slashDeposit(address _owner, uint256 _amount` | ||
n00b21337 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
and emit | ||
|
||
`emit StakeFrozen(_owner, stakes[_owner].overlay, _time);` | ||
`emit StakeSlashed(_owner, stakes[_owner].overlay, _amount);` | ||
|
||
#### Additional read only function changes | ||
|
||
From `function overlayNotFrozen(bytes32 overlay)` to `function addressNotFrozen(address _owner)` | ||
|
||
From `function usableStakeOfOverlay(bytes32 overlay)` to `function usableStakeOfAddress(address _owner)` | ||
|
||
From `function lastUpdatedBlockNumberOfOverlay(bytes32 overlay)` to `function lastUpdatedBlockNumberOfAddress(address _owner)` | ||
|
||
From `function ownerOfOverlay(bytes32 overlay)` to `function overlayOfAddress(address _owner)` | ||
|
||
### Redistribution contract changes | ||
|
||
As in Staking contract where we removed usage of overlay sending as it is auto fetched from staking contract and msg.sender, the same principle is applied here | ||
|
||
#### Commits | ||
|
||
Commits are now done just with nonce and obfsHash | ||
|
||
`function commit(bytes32 _obfuscatedHash, uint64 _roundNumber)` | ||
|
||
#### Reveals | ||
|
||
Reveals are also done without overlay so we just need nodes to send these parameters | ||
|
||
`function reveal(uint8 _depth, bytes32 _hash, bytes32 _revealNonce)` | ||
|
||
#### Other | ||
|
||
We also now have read function with just depth parameters, address is fetched from msg.sender | ||
|
||
`function isParticipatingInUpcomingRound(uint8 _depth)` | ||
|
||
we also have overload function with 2 parameters | ||
|
||
`function isParticipatingInUpcomingRound(address _owner, uint8 _depth)` | ||
|
||
which works as before, but is left more for debugging purposes and its safe for nodes to use the first one. | ||
|
||
## Backward compatibility | ||
|
||
Since we are changing how information is passed (both the overlay address and the Ethereum address), this change is not backwards-compatible. | ||
|
||
## Test cases | ||
|
||
Case study, with individual items needing to be tested: | ||
|
||
1. Can one stake a node? | ||
2. Is it then possible to change neighbourhoods? | ||
3. If so, then did that happen with the stake transferred without a change? | ||
|
||
## Copyright | ||
|
||
Copyright and related rights waived via [CC0](https://creativecommons.org/publicdomain/zero/1.0/). |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Neighbourhood hopping is currently supported by the Bee client.
is a questionable statement. As mentioned in this parameter, it is possible to mine an overlay address that falls into a specific neighbourhood, but neighbourhood hopping has to be defined in the terms of the feature requirements.As the overlay address changes, syncing indexes and intervals in the database need to be adjusted, which will create a noticeable impact on the usability and performance of the node. I am not aware of an automated way to reindex syncing, either at the runtime or before the node is started. This has to be implemented on regards when neighbourhood hopping can happen, at the runtime or on start. Alternative is to remove all node storage, but is not ideal.
The frequency of overlay address changes is also important as it makes the network topology changes more frequent with a consequence of the higher syncing activity, or even p2p disconnections. The implementation may open a door for a potential disruptions of the network by very frequently changing the overlay address. The benefit is that the node needs to be resilient to that behaviour resulting a more stable node, but this aspect has to be taken into account for the implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you janos for raising these points.
Indeed how the node is supposed to do neighbourhood hopping should be discussed and written.
But I am not sure the details should part of this SWIP, rather they belong to documention.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And yes, the node must nuke their db and sync from scratch in the new neighbourhood.
To optimise, the old node's reserve and cache should be the new node's cache.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very good point about syncing and frequent hoppers.
unfortunately the frequent hopping attack can be played the same without stake, so this swip made no difference in that.
Mitigating against this attack must be part of the planned pull-sync upgrade SWIP. That proposal aspires to optimise the sync protocol by eliminating predictable redundancies that are the cause of excessive bandwidth.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zelig thanks for addressing these concerns. I agree that the documentation is the right place for details about the neighbourhood hopping procedure on the UX and internals. It is the case that current state of the implementation does not have a clear interface to perform the hopping from the UX perspective, by my opinion.
I support the effort to harden the syncing protocol in regards of the frequent hopping attack. That is indeed possible now, but I believe it would be easier to accomplish (or just be more obvious) when neighbourhood hopping is implemented and advertised.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this feature is being supported on client start specifying
target-neighborhood
. corresponding PR and discussion