Implement "Verify Local Data" function (EC/amuled/amulegui) - #491

Reviewed and built all three variants (monolithic, daemon, amulegui) — clean follow-up to #381 overall. The CSharedFileList/CSharedFilesRem split mirrors RenameFile, the IsPartFile guard is preserved for remote-GUI users too, and it reuses the existing verify task.

One thing worth simplifying before merge: the daemon handler routes through the GUI-notify layer (CoreNotify_KnownFile_VerifyLocalData → MuleNotify::KnownFile_VerifyLocalData → file->VerifyLocalData()), but ProcessRequest2 already runs on the core, so it can call it directly — the way the sibling EC_OP_SHARED_FILE_SEARCH_KAD_NOTES handler does with file->RequestKadNoteSearch():

case EC_OP_VERIFY_LOCAL_DATA: {
    CMD4Hash hash = request->GetTagByNameSafe(EC_TAG_KNOWNFILE)->GetMD4Data();
    CKnownFile *file = theApp->sharedfiles->GetFileByID(hash);
    if (file)
        theApp->sharedfiles->VerifyLocalData(file);
    response = new CECPacket(EC_OP_NOOP);
    break;
}

That lets you drop KnownFile_VerifyLocalData() in GuiEvents.cpp, its declaration in GuiEvents.h, and the CoreNotify_KnownFile_VerifyLocalData macro — removing the indirection entirely.

(The ECCodes.java update is fine to keep — it's a stale generated file we don't actively maintain, but what you added is correct.)

Everything else looks good — just that one simplification and it's good to merge.

Simplification looks perfect — merged. Thanks for the quick turnaround!

Thanks for the review! Since you are the expert of amuleapi, can I let you handle the development of this function there (if you wish to have it there, of course)?

Thanks @danim7 — I took the amuleapi side, it's up as #528: POST /api/v0/shared/{hash}/verify, reusing your EC_OP_VERIFY_LOCAL_DATA opcode as-is, so no core changes needed.

Two things worth flagging that came out of building it.

First, Verify Local Data currently has no way to return its result to any client. CVerifyLocalDataTask has no OnExit(), emits no notification and no EC event, and doesn't change observable file state — m_corruptedMD4 / m_corruptedAICH get collected and then only formatted into the log line in PrintReport(). That applies equally to the monolithic GUI and amulegui, not just the REST API, so all amuleapi can honestly answer is 202 (re-hash scheduled) and point clients at /logs/amule. The endpoint documents it that way rather than implying more.

Second, it's gated to completed files only — the task bails on IsPartFile(), and the shared-files context menu logs "Verify Local Data on PartFile is currently not supported". The REST endpoint mirrors that with 409 partfile_unsupported, since accepting a partfile would promise a report that never arrives. Extending it to cover partfiles looks worthwhile too, if you see a clean way to check completed chunks against the stored part hashes.

So the natural follow-up is result handling in the UI plus the EC protocol — a completion notification off the task, an EC event carrying the per-file verdict (corrupt MD4 parts / AICH blocks), and the clients surfacing that instead of leaving users to read the log — with partfile support alongside it. Since you implemented the feature, would you want to drive that part? I'll wire up the amuleapi and SSE side once the EC event exists.

Thanks for integrating this functionality @got3nks.

Concerning the following steps, I must say I have some open questions because i'm not 100% convinced on what is the best approach now, specially on the UI. Maybe I could open a fresh new issue for discussion. The next steps could be:

• We shall persist the result of the check in known.met file and store the corrupted MD4 / AICH block numbers. Can we store a list of blocks using tags? One tag per corrupt block, or one single tag containing a count+list of block numbers? Or just use a new file type different than known.met?
• Maybe also persist a timestamp for the last check?
• Re-running a "Verify" will update the list of corrupted blocks / clear the list if the file got fixed compared to a previous check.
• Do not announce corrupted blocks as available to share / do not upload them to other peers.
• Show in the GUI that a given file is corrupt...but how exactly: New column in shared window? Or creating a new area within the "Show File Details" window from the right-click menu?
• Add a recovery workflow...but how? Possible options, but really don't know which one to choose:
  • Option 1: When the shared file is found corrupt, and depending on where in the GUI this is shown to the user, add an option: "download a clean copy" --> it will add the file to download since we have the MD4+size, then copy the good parts from the knwon file to the .part files to reduce the required parts to download, ask for AICH recovery for the bad AICH blocks, and download the bad parts... when finished, just store the newly-downloaded in the Incoming folder as any other file? This will still require a manual action from the user: replace/delete the old-corrupted file with the just-downloaded good copy.
  • Option 2: Ideally, it would be better to download the bad parts in-place and fix the already existing file, but this seems like a big change since the file would not download via the "standard" way: Temp folder --> then Incoming folder. Also, if the file is stored in a read-only location, that would not be possible. The advantage of this option is it will not require any additional user action after the initial "download a clean copy" from the first step.
• Once it is working for amule monolithic, wire the required events / actions in EC protocol and add the corresponding functionality to external clients .
• Also partfile support. For this, we may only check the MD4 hashes of completed parts. If some parts we thought were correct are now found corrupt, then mark them as corrupt, and initiate recovery via AICH requests if we trust a MasterHash.

After writting all of the above, I want to take a minute to look at the bigger picture:

• This is a nice-to-have feature, and it can be quite a lot of work for a rare use-case. The already in master "Verify Local Data" function + the workaround described in manual (forced) rehash [feature] amule-project/amule#27 already serve well.
• I don't want to delay the NAT-T feature by distracting attention to this recovery-workflow. I think NAT-T is the most urgent development and what can really benefit the whole ed2k community

My take:

• we can keep working in this corruption-fix workflow, i will do it during my free time, but let's focus on releasing next version and resuming work on NAT-T first :)

Your thoughts?

On the persistence/recovery design — I think we can lean much harder on what already exists for downloads rather than build a parallel path for shared files.

The partfile engine already does exactly this: when a part fails its hash it's marked corrupt (AddGap() + tracked in m_corrupted_list, persisted in the .part.met), then re-downloaded, with AICH recovering individual 180 KB blocks where we trust a MasterHash. That is the recovery workflow, already written and battle-tested.

So instead of storing corrupt-block lists in known.met plus a new recovery flow: when a shared file is found corrupt, hand it back to that engine — turn it into a partfile with the good parts kept complete and the corrupt parts marked as gaps. Then:

• no new known.met tags — the state already lives in the partfile gap list / m_corrupted_list;
• "don't announce/upload corrupt blocks" is automatic — while it's being fixed it's a partfile, not a complete shared file;
• recovery is just the normal Temp → Incoming flow (good parts preserved, only the bad ones re-fetched + AICH), which effectively merges your Option 1 and Option 2;
• it sidesteps the read-only-location problem from Option 2 — the rebuild happens in Temp, and the good copy lands in Incoming.

The GUI mostly falls out too: the file just appears in the transfer window as a download for the corrupt parts, so we don't strictly need a new column/details area to start. And partfiles are even simpler — they already carry m_corrupted_list, so a re-verify just re-gaps the bad parts and the engine takes over.

The one genuinely new piece is a clean "un-complete" path: turning a shared CKnownFile into a CPartFile with the good parts pre-marked (we don't currently go complete → partfile). But that's smaller and more contained than a bespoke persist + recover + GUI stack.

One thing worth deciding up front, though: whether we recover into a copy or invalidate the shared file in place. Converting in place is risky if the file has no sources — we'd leave it stuck as an incompletable partfile, and the user's complete-but-slightly-corrupt copy can't return to Incoming without the missing parts. Recovering into a separate partfile and only swapping the original once the good copy finishes is safer: a sourceless or failed recovery just leaves the original intact and still shared. The cost is temporary extra disk — or we gate the in-place convert on actually finding sources first.

One more on the UI side: right now Verify Local Data has no user feedback at all — CVerifyLocalDataTask just collects the corrupt MD4/AICH lists and logs them (PrintReport → AddLogLine); nothing surfaces in the GUI.

While a check runs we could reuse an existing status rather than invent one — it's effectively a re-hash, so PS_HASHING fits. One caveat: that status is only rendered in the transfer/download list today, not in the Shared Files window — and that's exactly where a shared file lives during the check (it only moves to the transfer view if it's found corrupt and we convert it to a partfile). So PS_HASHING (or a "checking" indicator) needs wiring into the shared view too, since that's where the file is shown until the check completes. For files that do get converted, the transfer window already displays hashing/completing status and progress, so that side comes for free.

Thanks for your detailed answer. It contains a lot of valuable information on how to continue developing this feature.

Just one question: if I'm understanding your comment right, the recovery workflow is automatically initiated if the "verify local data" finds any corruption? It is not really what I had in mind: if I identify a wrong file, maybe I have a backup somewhere else, and I don't need to download it from the network. Or maybe I had an issue with my disk/filesystem, I run a test on the whole library, but I don't want it to automatically add hundreds of files to download in case they are wrong. That's is why I wanted to separate the check from the recover.

One thing worth deciding up front, though: whether we recover into a copy or invalidate the shared file in place. Converting in place is risky if the file has no sources — we'd leave it stuck as an incompletable partfile, and the user's complete-but-slightly-corrupt copy can't return to Incoming without the missing parts. Recovering into a separate partfile and only swapping the original once the good copy finishes is safer: a sourceless or failed recovery just leaves the original intact and still shared. The cost is temporary extra disk — or we gate the in-place convert on actually finding sources first.

If we take this approach, I think we shall recover into a copy, so the user can still access the complete-but-slightly-corrupt file he has. He can always delete the file if he doesn't want to pay the temporary extra disk cost.

The hashing progress from your second comment seems like a nice addition. I may begin with that point while I try to think of a convincing UI/UX (maybe 2 options: one for verify-only, one for verify+fix...?).

Good — separating the check from the recovery is the right call, and it changes the design in a way I think is cleaner. Let me lay out where I've landed.

The verdict lives on the shared file, persisted. A verify run records its result on the CKnownFile and writes it to known.met: a "corrupted" flag, the corrupt-part list, and a last-check timestamp. One tag holding the serialized part list (plus the flag/timestamp), not one tag per block. Re-running Verify refreshes the list, or clears the flag if the file now checks out. This is the piece I earlier thought we could skip by leaning on the partfile gap list — but that only holds if we auto-convert; since we're deliberately not auto-converting, the state has to persist on the still-complete file, and known.met is its natural home.

Surfacing it in the Shared Files view. Note the existing PS_* values are partfile statuses — a complete CKnownFile reports PS_COMPLETE unconditionally, and the shared view renders no status at all today. So rather than reuse the partfile status enum, this is a dedicated corrupted flag on the file, shown as a small state indicator (icon/column) in the shared list. That keeps "complete-but-corrupt" from being confused with a download state.

amuled becomes aware of corrupted shared files. This is the key consequence of keeping the file complete-and-shared instead of auto-converting it: the upload path has to consult the persisted corrupt-part list and refuse to serve those parts (still happily serving the good ones). In the auto-convert design that fell out for free — a partfile just doesn't advertise its gaps — but here we suppress the bad parts explicitly from the same list we persisted. Net effect is the same (no corrupt data goes out), without pulling the file out of the share.

Recovery is a separate, explicit action. A context-menu entry — "Recover corrupted parts" — enabled only when the file is flagged corrupt. It hands the file to the existing partfile engine, recovering into a copy: good parts kept complete, corrupt parts marked as gaps, then the normal Temp → Incoming flow re-fetches only the bad parts (with AICH recovering individual blocks where we trust a MasterHash). The original complete-but-corrupt file stays intact and shared until the good copy finishes, so a sourceless or failed recovery costs nothing but the click. This is your verify-only-vs-verify+fix split, made concrete: Verify only ever marks; fixing is always a deliberate second step, per file.

So the flow end to end: Verify marks + persists + suppresses the bad parts from upload; the shared view shows the flag; the user decides, per file, whether to Recover, which spins up a clean copy via the engine we already have.

Sequencing-wise I agree with your priorities — NAT-T and the next release come first; this proceeds in the background. The hashing-progress indicator you mentioned starting with is a good standalone first step (it just needs wiring into the shared view too, since that's where the file sits during the check). Partfile support and the EC/amuleapi/SSE surface come after the monolithic path works.

I like this design, thanks for the feedback. Will try to do it at some point if I find time :)