Potential Bug: `NetMessageV1::Unsubscribed` handler calls `subscribe` instead of removing the unsubscribing peer

[sanity]

Title: Potential Bug: NetMessageV1::Unsubscribed handler calls subscribe instead of removing the unsubscribing peer

Labels: bug, network, subscriptions

Body:

Description:

During a code review focused on update propagation and subscription management, identified that the handler for NetMessageV1::Unsubscribed messages appears to have unexpected behavior.

The handler is located in crates/core/src/node/mod.rs within the process_message_v1 async function:

// In crates/core/src/node/mod.rs, inside process_message_v1

            // ... other message types ...

            NetMessageV1::Unsubscribed { ref key, .. } => {
                // This is the handler logic:
                if let Err(error) = subscribe(op_manager, *key, None).await {
                    tracing::error!(%error, "Failed to subscribe to contract");
                }
                break; // Exits the retry loop for this message
            }
            _ => break, // Handles other cases
        }

Actual Behavior:

When a node receives an Unsubscribed { key, from, .. } message, indicating that the from peer no longer wishes to receive updates for key, the receiving node executes subscribe(op_manager, *key, None).await. It attempts to subscribe itself (or ensure its own subscription) to the contract that the other peer just unsubscribed from.

Crucially, it does not appear to remove the from peer from its local subscriber list for the specified key (e.g., there is no call like op_manager.ring.remove_subscriber(key, from_peer_id)).

Expected Behavior:

Receiving an Unsubscribed message from a peer should typically trigger the removal of that peer from the local node's subscriber list for the specified contract key. This ensures the subscriber list accurately reflects which peers are currently interested in updates.

Impact:

1. Inaccurate Subscriber Lists: The primary issue is that the subscriber list maintained by the Ring (specifically SeedingManager::subscribers) may become inaccurate over time. Peers that have explicitly unsubscribed might remain on the list.
2. Incorrect Update Propagation: Peers that have unsubscribed might still receive UpdateMsg::BroadcastTo messages for the contract, leading to unnecessary network traffic and processing.
3. Unnecessary subscribe Calls: The handler triggers potentially redundant subscribe operations on the receiving node, which might add unnecessary load or network requests.
4. Over-reliance on Disconnect Pruning: This leaves the location-based prune_subscriber (triggered on peer disconnect) as potentially the only mechanism removing these entries, which might be insufficient if the peer doesn't disconnect or changes location properties used for pruning.

Suggestion:

Review the logic within the NetMessageV1::Unsubscribed handler in process_message_v1. Consider changing the implementation to:

1. Remove the call to subscribe(op_manager, *key, None).await.
2. Instead, call the appropriate function on the Ring or SeedingManager to remove the from peer from the subscriber list associated with the key.
3. Verify if the current subscribe call serves any other critical, intended purpose in this specific context that needs to be preserved or handled differently.

[iduartgomez]

@sanity after reading the code, I think we are definitively missing a call to seeding_manager.prune_subscriber after receiving the unsubscribe notification, and I don't think we need to call subscribeagain, cause this is for cleaning up downstream subscribers.

However, I don't think this is the origin of the problems we are currently seeing as unsubcribe events are not even happening, and those only happen after a peer stops seeding a contract (which ain't happening right now).

[iduartgomez]

@claude is this relevant anymore?

[claude[bot]]

Claude finished @iduartgomez's task —— View job

---

Investigating Issue Relevance

• [x] Read current implementation of NetMessageV1::Unsubscribed handler
• [x] Determine if the bug still exists in the codebase
• [x] Provide answer about issue relevance

---

Answer

Yes, this issue is still relevant. The bug described in the issue still exists in the current codebase.

Current State

The NetMessageV1::Unsubscribed handler appears twice in crates/core/src/node/mod.rs:

1. Line 859-863 in process_message_v1():

NetMessageV1::Unsubscribed { ref key, .. } => {
    if let Err(error) = subscribe(op_manager, *key, None).await {
        tracing::error!(%error, "Failed to subscribe to contract");
    }
    break;
}

2. Line 1066-1070 in handle_pure_network_message_v1():

NetMessageV1::Unsubscribed { ref key, .. } => {
    if let Err(error) = subscribe(op_manager, *key, None).await {
        tracing::error!(%error, "Failed to subscribe to contract");
    }
    break;
}

Both handlers have the same incorrect behavior: they call subscribe() instead of removing the unsubscribing peer from the subscriber list.

What Should Happen

When a peer sends an Unsubscribed { key, from, .. } message, the receiving node should:

1. Remove the from peer from its subscriber list for the specified key
2. Not call subscribe() to re-subscribe itself

Implementation Challenges

The Unsubscribed message contains:

• key: ContractKey - the contract being unsubscribed from
• from: PeerId - the peer that unsubscribed

However, the existing SeedingManager::prune_subscriber(loc: Location) method:

• Takes a Location parameter (not PeerId)
• Removes the subscriber from all contracts (not just a specific one)

To properly fix this, we would need to either:

1. Add a new method like SeedingManager::remove_subscriber_from_contract(key: ContractKey, peer: PeerId), or
2. Use the connection manager to look up the location from the peer ID and then implement targeted removal

Recommendation

This bug should be fixed. While @iduartgomez noted that unsubscribe events aren't frequently happening in practice right now, having correct handling is important for the system's integrity and future scenarios where unsubscribe events become more common.

Would you like me to implement a fix for this issue?

---

Job Run | Branch

[sanity]

🤖 Relabeled for consistency

Applied labels:

• T-bug
• P-high
• E-medium
• A-networking
• A-contracts

Reasoning: The submitted code snippet shows that NetMessageV1::Unsubscribed triggers subscribe(op_manager, *key, None).await instead of removing the peer from the subscriber list. That behavior contradicts the expected semantics of an Unsubscribed message and can leave stale subscribers, cause unnecessary broadcasts and extra subscribe calls. The fix is likely localized to the handler: remove the subscribe call and invoke the Ring/SeedingManager path to remove the peer for the given key. This appears to be a real bug with moderate implementation effort and significant network/contract update impact, so classify as a high-priority bug.

Previous labels: ``