Revert "Revise SWIP-39 for clarity and detail enhancement" by zelig · Pull Request #89 · ethersphere/SWIPs

zelig · 2026-03-21T07:50:37Z

Reverts #87

This reverts commit 54c1fd2.

Copilot

Pull request overview

This PR reverts prior edits to SWIP-39 (“Balanced Neighbourhood Registry / Smart Neighbourhood Management”), restoring an earlier draft structure and content.

Changes:

Replaces the revised abstract/model/invariant-focused text with an earlier, more implementation-oriented architecture/specification narrative.
Reintroduces detailed assignment/expiry/initialisation sections and adds an illustrative Solidity contract example.
Removes the previously added formalism (e.g., structural invariant / trie-based mechanism descriptions).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-21T07:55:48Z

SWIPs/swip-39.md

+    function _removeCommitter(uint index) internal {
+        if (index >= committers.length) return;
+
+        for (uint i = index; i < committers.length - 1; i++) {
+            committers[i] = committers[i + 1];
+        }
+        committers.pop();
+    }


Gas/DoS risk in the example contract: _removeCommitter() shifts the whole committers array on every removal, and _expire() can call it repeatedly from the front. For large committer queues this can become O(n^2) and make assign()/expiry processing run out of gas. Consider a queue with a head index (no shifting) or a mapping-based linked list/index map.

Copilot · 2026-03-21T07:55:48Z

SWIPs/swip-39.md

+The assign call is the second transactional endpoint called by the staking contract. It takes the provider's ether address and as well as the mined overlay as arguments.
+After calling expiry, the validity of the registration is checked by finding the entry for the ether address in the committers' list.


Wording/grammar: "It takes the provider's ether address and as well as the mined overlay" is ungrammatical and hard to parse. Rephrase to clearly state the parameters (e.g., "takes the provider's ether address as well as the mined overlay").

Suggested change

The assign call is the second transactional endpoint called by the staking contract. It takes the provider's ether address and as well as the mined overlay as arguments.

After calling expiry, the validity of the registration is checked by finding the entry for the ether address in the committers' list.

The assign call is the second transactional endpoint called by the staking contract. It takes the provider's ether address and the mined overlay as arguments.

After calling expiry, it checks the validity of the registration by finding the entry corresponding to the ether address in the committers' list.

Copilot · 2026-03-21T07:55:49Z

SWIPs/swip-39.md

+    function getNeighbourhood(address _a) public view returns (uint256) {
+        uint256 r = _randomSeed(_a);
+        require(R.length > 0, "No available neighbourhoods");
+        return R[r % R.length];
+    }


getNeighbourhood() requires R.length > 0, but the example contract never initialises/populates R (no constructor/bootstrap), so this will revert for all callers. Initialise R for the starting depth (e.g., fill with 0..2^d-1) or call an initialisation routine before exposing this endpoint.

Copilot · 2026-03-21T07:55:49Z

SWIPs/swip-39.md

+    function unregister(address _a, bytes32 _overlay) external {
+        uint nh = uint256(_overlay) >> (256 - d);
+        A[nh] = bytes32(0);
+        R.push(nh);
+        N--;
+        // return funds to the committer
+        payable(_a).transfer(STAKE);
+        emit Unassigned(_a, nh);
+    }   


unregister() is external and has no access control, so any account can unassign arbitrary overlays/neighbourhoods and trigger a stake refund to _a, effectively griefing the registry state. If this endpoint is intended to be called only by the Staking contract (as described earlier), enforce that (e.g., onlyStaking) and validate that _overlay matches the currently assigned overlay for _a before clearing state.

Copilot · 2026-03-21T07:55:49Z

SWIPs/swip-39.md

+            R[i] = j;
+        }
+        d++;
+        require(d <= 32, "Maximum depth exceeded");


require(d <= 32, ...) appears inconsistent with using bytes32 overlays and extracting prefixes via >> (256 - d), where d is a bit-depth (max 256). If d is intended to be measured in bits, this limit should likely be 256; if it's meant to be bytes, the shift logic should be adjusted accordingly.

Suggested change

require(d <= 32, "Maximum depth exceeded");

require(d <= 256, "Maximum depth exceeded");

Copilot · 2026-03-21T07:55:51Z

SWIPs/swip-39.md

+        uint256 nh = getNeighbourhood(_a);
+        uint256 overlayNh = uint256(_overlay) >> (256 - d);
+
+        require(overlayNh == nh, "Overlay doesn't match neighbourhood");


assign() writes A[nh] = _overlay, but A is never sized/initialised before first use in this example. This will revert with an out-of-bounds access unless A is preallocated for 2^d neighbourhoods (or you switch to a mapping).

Suggested change

require(overlayNh == nh, "Overlay doesn't match neighbourhood");

require(overlayNh == nh, "Overlay doesn't match neighbourhood");

require(nh < A.length, "Neighbourhood index out of bounds");

Copilot · 2026-03-21T07:55:51Z

SWIPs/swip-39.md

+            A.push(bytes32(0));
+        }
+        for (uint i = currentPower - 1; i < currentPower; i--) {
+            uint b = uint256(A[i]) >> (256 - d) % 2;


Operator precedence bug: uint256(A[i]) >> (256 - d) % 2 is parsed as shifting by ((256 - d) % 2). If the intent is to take the next bit after shifting, it should be (uint256(A[i]) >> (256 - d)) % 2 (or equivalent).

Suggested change

uint b = uint256(A[i]) >> (256 - d) % 2;

uint b = (uint256(A[i]) >> (256 - d)) % 2;

Copilot · 2026-03-21T07:55:52Z

SWIPs/swip-39.md

+#### Random seed

-## Deregistration and Rebalancing
+This internal read-only call takes as its single argument an ether address ($a^\Xi$) and returns a (?) random $\rho \in \mathbb{Z}_\text{uint256}$. 


The spec currently says "returns a (?) random" for the random seed. Please remove the placeholder "(?)" and precisely define the type/range and randomness source, since this affects implementability and reviewability of the SWIP.

Suggested change

This internal read-only call takes as its single argument an ether address ($a^\Xi$) and returns a (?) random $\rho \in \mathbb{Z}_\text{uint256}$.

This internal read-only call takes as its single argument an ether address ($a^\Xi$) and returns a pseudo-random value $\rho \in \mathbb{Z}_\text{uint256}$, deterministically derived from the block randomness (`difficulty` / randao) of the block immediately following the registration block for $a^\Xi$ and the address $a^\Xi$ itself, as defined below.

Copilot · 2026-03-21T07:55:52Z

SWIPs/swip-39.md

+The balanced node assignments are orchestrated by a smart contract which will maintain the active node list, their corresponding neighbourhoods, the overlay addresses they have determined, and manage the ingress/egress processes as nodes joini and exit the set of active network service providers.

-The system maintains a depth parameter $d \in \mathbb{N}$ such that
+This contract is deployed together with a staking contract similar to the [swarm storage incentive staking contract](https://github.com/ethersphere/storage-incentives/blob/master/src/Staking.sol). This contract will retain the total stake treasury, as well as enabling a node operator to deposit, withdrawal and maintain their stake. Concerns should be strictly separated to improve security of locked funds and upgradability of both contracts.


Typo/wording: "nodes joini and exit" should be "join and exit" (and consider using "withdraw" rather than "withdrawal" as a verb in the same paragraph).

Copilot · 2026-03-21T07:55:52Z

SWIPs/swip-39.md

+            // Ensure A has enough space for the new neighbourhoods);   
+            A.push(bytes32(0));
+        }
+        for (uint i = currentPower - 1; i < currentPower; i--) {


The reverse loop for (uint i = currentPower - 1; i < currentPower; i--) will underflow and run forever in Solidity (uint wraps), causing out-of-gas. Use a safe decrement pattern (e.g., for (uint i = currentPower; i-- > 0;)) or a signed index.

Suggested change

for (uint i = currentPower - 1; i < currentPower; i--) {

for (uint i = currentPower; i-- > 0;) {

Revert "Revise SWIP-39 for clarity and detail enhancement (#87)"

68e39d6

This reverts commit 54c1fd2.

Copilot AI review requested due to automatic review settings March 21, 2026 07:50

Copilot started reviewing on behalf of zelig March 21, 2026 07:51 View session

zelig merged commit ce60560 into swip-39 Mar 21, 2026
2 checks passed

Copilot AI reviewed Mar 21, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revert "Revise SWIP-39 for clarity and detail enhancement"#89

Revert "Revise SWIP-39 for clarity and detail enhancement"#89
zelig merged 1 commit intoswip-39from
revert-87-zelig-patch-1

zelig commented Mar 21, 2026

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 21, 2026

Uh oh!

Copilot AI Mar 21, 2026

Uh oh!

Copilot AI Mar 21, 2026

Uh oh!

Copilot AI Mar 21, 2026

Uh oh!

Copilot AI Mar 21, 2026

Uh oh!

Copilot AI Mar 21, 2026

Uh oh!

Copilot AI Mar 21, 2026

Uh oh!

Copilot AI Mar 21, 2026

Uh oh!

Copilot AI Mar 21, 2026

Uh oh!

Copilot AI Mar 21, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		The assign call is the second transactional endpoint called by the staking contract. It takes the provider's ether address and as well as the mined overlay as arguments.
		After calling expiry, the validity of the registration is checked by finding the entry for the ether address in the committers' list.

	require(d <= 32, "Maximum depth exceeded");
	require(d <= 256, "Maximum depth exceeded");

	require(overlayNh == nh, "Overlay doesn't match neighbourhood");
	require(overlayNh == nh, "Overlay doesn't match neighbourhood");
	require(nh < A.length, "Neighbourhood index out of bounds");

	uint b = uint256(A[i]) >> (256 - d) % 2;
	uint b = (uint256(A[i]) >> (256 - d)) % 2;

	This internal read-only call takes as its single argument an ether address ($a^\Xi$) and returns a (?) random $\rho \in \mathbb{Z}_\text{uint256}$.
	This internal read-only call takes as its single argument an ether address ($a^\Xi$) and returns a pseudo-random value $\rho \in \mathbb{Z}_\text{uint256}$, deterministically derived from the block randomness (`difficulty` / randao) of the block immediately following the registration block for $a^\Xi$ and the address $a^\Xi$ itself, as defined below.

	for (uint i = currentPower - 1; i < currentPower; i--) {
	for (uint i = currentPower; i-- > 0;) {

Conversation

zelig commented Mar 21, 2026

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 21, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants