Skip to content
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 support for ignoring nodes with is_ignored field in NodeInfo #5319

Merged
merged 7 commits into from
Nov 12, 2024

Conversation

mdesmedt
Copy link
Contributor

This adds the is_ignored field to NodeInfo structs. It functions similarly to the existing is_favorite field. The field gets checked in Router::perhapsHandleReceived and when set the packet is dropped.

This will enable clients to configure their node to drop packets from other nodes. Much in the same way as LoRaConfig.ignore_incoming but without being limited to 3 entries.

This change depends on the protobuf changes in PR meshtastic/protobufs#621

Implements my feature request here: #5297

@CLAassistant
Copy link

CLAassistant commented Nov 12, 2024

CLA assistant check
All committers have signed the CLA.

@thebentern
Copy link
Contributor

Really great solution to this issue. I think we'll probably end up deprecating the ignore_incoming after this gets integrated.

Copy link
Member

@GUVWAF GUVWAF left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, although maybe we should already have it migrate from the ignore_incoming array to this bool? Then we won't be sticking forever with two possible configurations.

@GUVWAF
Copy link
Member

GUVWAF commented Nov 12, 2024

Hmm, now that I think twice about this, I think this would be non-ideal, because usually you also don't want ignored nodes in your node list. However, if you delete the node, it will be un-ignored.

@ianmcorvidae
Copy link
Contributor

I suppose that with the node ignored you probably also won't see updates from it, ultimately making its last-seen time later and later and eventually pushing it out of nodedb, which would then lose the ignored state. We'd need to treat this like favorites and prioritize keeping them around, I guess, which is admittedly a little weird.

Would it be nuts to expand the nodedb by the number of ignored nodes, but trim as much information out of the ignored node entries as possible so it isn't space-prohibitive? I guess it really only needs to hold onto the nodenum and the is_ignored flag.

@mdesmedt
Copy link
Contributor Author

Hmm, now that I think twice about this, I think this would be non-ideal, because usually you also don't want ignored nodes in your node list. However, if you delete the node, it will be un-ignored.

Yes, although first I thought having the is_ignored field in the NodeDB is primarily a UI consideration (the apps could choose to list ignored nodes in a different part of the UI if desired), perhaps using the NodeDB is actually not ideal.

Maintaining a dynamically sized uint32 array shouldn't be that difficult. Now that I understand the project a little better, instead of this approach, perhaps it should just be a new repeated uint32 field in DeviceState, like node_db_lite is.

The set_ignored_node/remove_ignored_node admin messages can still be used. All that's needed is a way to query the list over proto. Maybe something as straightforward as a pair of get/response admin messages entries for A. the size of the ignore list and B. a query by index? Let me know if you would like me to try to code this approach instead, and I can submit a different PR.

@thebentern
Copy link
Contributor

thebentern commented Nov 12, 2024

I think the nodes you ignore being in the nodedb to show up in the list is actually a feature for apps to build around rather than a drawback. You can actually see who you've blocked on your mesh and remove it if you made a mistake. Apps can present this like blocking features in messaging clients. Keep in mind this probably won't get used extensively. We identified that 3 is not enough, but I doubt most meshes will ever block more than possibly 10 nodes.

When we ignore a node, we may want to remove a bunch of the unnecessary information. We don't need telemetry, position, etc. I think having the long name / shortname is actually part of the value-add of this feature though. I'm likely to remember that I blocked "Bob's big boy" because he was spamming rangetest message. 😅

@GUVWAF
Copy link
Member

GUVWAF commented Nov 12, 2024

Okay, that's fair, although the apps would then need to disable the option to remove it when it's ignored, and we'd still need to avoid cycling them out of the NodeDB.

@thebentern
Copy link
Contributor

Okay, that's fair, although the apps would then need to disable the option to remove it when it's ignored, and we'd still need to avoid cycling them out of the NodeDB.

Exactly. We want to treat them as favorites in terms of the devices not pushing them out when new nodes roll in and @garthvh @andrekir should be able to guard against a user deleting them after they are ignored. Assuming we could do something like force a user to unignore the node before removing them? I think that would go a long way at preventing confusion.

@thebentern thebentern requested a review from GUVWAF November 12, 2024 12:37
@thebentern
Copy link
Contributor

thebentern commented Nov 12, 2024

I have pushed the change to no longer consider is_ignored a boring node. :-)

Update: Also cleared out metrics, position, and user key upon ignore.

@thebentern thebentern merged commit 2ec3958 into meshtastic:master Nov 12, 2024
47 checks passed
@mdesmedt mdesmedt deleted the is_ignored branch November 12, 2024 20:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants