Repository navigation
New Direct Mode - #3630
New Direct Mode#3630ripplebiz wants to merge 5 commits into
Conversation
|
Is this a use case for the blockchain path tracking that you were looking at? Allow one to mine the exact path taken using the discovery db of adverts. First 16 char is all you'd need to add to the blockchain |
No, this is unrelated. |
|
I think there's a case where WIDE can break an otherwise working route. Suppose the path starts with B can forward on behalf of A, replacing A with its own hash and advancing the cursor. This leaves However, B has already marked the packet as seen. When A forwards its copy, B will discard it as a duplicate, since Instead of always advancing the cursor by one, could we check whether the forwarding repeater's own hash appears again later in the remaining path? If it does, we could advance the cursor past that occurrence and continue forwarding, effectively skipping the intermediate hops. We'd also need to account for the skipped hops in the recorded path so it doesn't misrepresent the actual route. Am I missing anything in the duplicate handling here? |
Yes, I think you're correct. That scenario would fail. Seems like removing duplicates of 'B' (in that example) before forwarding could be the fix, although it's a little sketchy if it's a 1-byte path hash. It could be conservative and only remove duplicate path hash (ahead of cursor) only if it's the next hop or the one after that(?) rather than scanning whole path. |
Agreed, with 1-byte path hashes any fix seems like a trade-off. I also considered keeping the repeated B in the path. When B substitutes for A in That would cover both possibilities: the second B is the same repeater, or a different one with a matching prefix. The downside is that duplicate handling would need to distinguish these forwarding stages, including on downstream repeaters. Plus the extra airtime. Might be more complexity than this case warrants. |
|
I've been out of the loop lately so I'm glad to see the path cursor and wide mode are still being considered! An additional thing that's worth considering is having |
Good idea. It would be easy to add that as tuning config item, eg. "set wide.snr.thresh 1.0", but have it default to something like -8.0 |
* repeater: Wide mode, isHashNeighbor() now only considers neighbor nodes with SNR >= threshold
| est_timeout = calcDirectTimeoutMillisFor(t, recipient.out_path_len); | ||
| return MSG_SEND_SENT_DIRECT; | ||
| } | ||
| auto pkt = obtainPacketFor(recipient); |
There was a problem hiding this comment.
Both sendRequest() overloads shadow the outer pkt with auto pkt. The null check and encodeDatagram() use the inner pointer, but stdSend() gets the uninitialized outer one.
Looks like auto just needs to be dropped in both places.
| // a DIRECT login can reply via the stored out_path, as onPeerDataRecv() does for REQ | ||
| ClientInfo* client = acl.getClient(sender.pub_key, PUB_KEY_SIZE); | ||
| bool have_out_path = client != NULL && client->out_path_len != OUT_PATH_UNKNOWN; | ||
| if (client == NULL) { |
There was a problem hiding this comment.
This early return also drops responses to public REGIONS, OWNER and BASIC requests. Those handlers don't create an ACL entry, so a valid request from an unknown client gets no reply.
Could we preserve the reply path for these requests without requiring an ACL entry?
| * \returns true, if path was accepted and that reciprocal path should be sent | ||
| */ | ||
| virtual bool onPeerPathRecv(Packet* packet, int sender_idx, const uint8_t* secret, uint8_t* path, uint8_t path_len, uint8_t extra_type, uint8_t* extra, uint8_t extra_len) { return false; } | ||
| virtual bool onPeerPathRecv(Packet* packet, int sender_idx, const uint8_t* secret, uint8_t* path, uint8_t path_len, uint8_t extra_type, uint8_t* extra, uint8_t extra_len, uint8_t crypto_type) { return false; } |
There was a problem hiding this comment.
Adding crypto_type here leaves SensorMesh::onPeerPathRecv() with the old 8-argument signature in both the header and implementation.
The override now fails to compile. Looks like the sensor override needs updating too. simple_sensor isn't covered by the current PR build matrix.
This is an attempt to carve out a brand new Direct Mode by utilising the as-yet-unused TRANSPORT_DIRECT route type (0x03). ie. Packets with Direct + transport code data.
transport_code[0] is used as such:
transport_code[1] is used as such:
Some of the things this new encoding/scheme is attempting is:
NOTE: The 'wide' mode is inspired from @redxdev and this PR: #2648
Wide sub-mode is fairly simple, where (at each hop) the designated repeater with matching hash forwards, but also all direct neighbors of that repeater also forward. This could be a much more resilient direct mode. New clients could have a strategy, when sending, to use DM_MODE_SEQ_NORMAL (say first 2 attempts), then DM_MODE_SEQ_WIDE (for next attempt), then revert to FLOOD.
Also, if a neighbor repeater forwards it scribbles into the path its own hash, then edits the sub-mode to DM_MODE_SEQ_WIDE_MODIFIED. Dest node can then see if an alternate path was actually taken and use that info.
The _LINK_ID sub-mode is still effectively a reserved sub-mode, and hasn't been properly fleshed-out.
There are still a number of aspects that are yet to be done, like acting on the Link Quality calcs. (just placeholder updateLinkQualityStats() methods ATM)