Only increment current_version_id when an update is applied

[jintekc]

Moves the current_version_id increment out of the "Process updates Array" loop and into "Apply update", so duplicates confirmed via "Confirm Duplicate Update" no longer advance the counter. This keeps current_version_id in lockstep with update_hash_history (fixes the inflated versionId and the false LATE_PUBLISHING on histories like [v2, v2-again, v3]).

Also adds the guard discussed in the issue thread: "Confirm Duplicate Update" now raises INVALID_DID_UPDATE when targetVersionId < 2, so a non-conforming update can't reach the history lookup with a negative index and all resolvers reject it the same way.

Fixes #321

[danpape]

Looks good.

[parasyte]

I'm confused. "Confirm Duplicate Update" confirms a duplicate by raising an error. When the error is raised, processing the updates array (and everything else in the resolver, really) stops. That will never reach the increment.

I agree with the new "if update.targetVersionId is less than 2" condition (we discussed this in #321). But I don't understand the justification for changing the increment. What am I missing?

[jintekc]

"Confirm Duplicate Update" only raises when the hashes differ.

This step confirms that an update with a lower-than-expected targetVersionId is a true duplicate.

Create unsigned_update by removing the proof property from update. Hash unsigned_update with the JSON Document Hashing algorithm and compare it to update_hash_history[update.targetVersionId - 2]. Raise a LATE_PUBLISHING error if the hashes differ.

Issue #321 and this corresponding PR are primarily focused on the case of when hashes match because then the "Confirm Duplicate Update" sub-routine returns successfully and continues executing the outer loop, which would then execute the step "Increment current_version_id." This would be done erroneously since the update was not a true version bump, but rather a confirmed duplicate, likely resulting from an accidental broadcast of the same update more than once.

The whole point of the "Confirm Duplicate Update" step is to allow users the ability to make the mistake of broadcasting an identical update more than once without bricking their DID resolution on a "LATE PUBLISHING" error, since technically its not late publishing. The net side effect of how we wrote it, however, is that it will end up inflating current_version_id if we do not scope that step to the "Apply update" sub-section.

[parasyte]

That's what I'm missing. I've misread my own code! An increment happens in that location, but the result is stored in a separate variable and used for the condition on next line. This error was probably introduced by me when translating this code to the new spec prose. A misfortunate simplification.

Yes, the increment to current_version_id needs to be moved into "Apply update". That is a much closer match to the code that this description was based on.

---

I went through a whole comparison between the code and the old spec it was based on. That revealed a gap in the old spec that I believe the new spec with this PR adequately resolves. It doesn't matter for this PR, so I'll hide the analysis below. It might be useful to future archeologists.

Analysis of old spec...

The code I'm looking at was based on the pandoc version of the spec: https://github.com/dcdpr/did-btcr2/blob/75ae6934458922e70d24d8f3c22db423c7df017f/chapters/CRUD-Operations.md#traverse-bitcoin-blockchain-history.

Step 10 (you'll have to scroll down a page) is the process in question. Step 10.i is update.targetVersionId <= current_version_id, 10.ii is update.targetVersionId == current_version_id + 1, and 10.iii is NOT update.targetVersionId > current_version_id + 1, it is ... + 2! current_version_id gets incremented within the == case.

The way this older spec (and the code that I'm comparing to) was written, the third condition requires the extra increment, and that leaves a gap where update.targetVersionId > current_version_id + 1 is not actually checked. (In short, the old spec and the code are missing else clauses on the latter two conditions. The new spec fixes this ambiguity by stating "Only one of three possible conditions will occur:")

I do believe we need to check that gap (unlike the old spec). And I think that moving the increment into "Apply update" is the right thing to do (matching the old spec).