meshtastic / firmware

Meshtastic device firmware
https://meshtastic.org
GNU General Public License v3.0
3.64k stars 910 forks source link

Add support for ignoring nodes with `is_ignored` field in NodeInfo #5319

Closed mdesmedt closed 1 week ago

mdesmedt commented 2 weeks ago

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 https://github.com/meshtastic/protobufs/pull/621

Implements my feature request here: https://github.com/meshtastic/firmware/issues/5297

CLAassistant commented 2 weeks ago

CLA assistant check
All committers have signed the CLA.

thebentern commented 2 weeks ago

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

GUVWAF commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

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.