Unlock only if lock_result was Ok

[apfitzge]

Problem

• I am adding some earlier filtering of transactions that has different TransactionError, which happens before locking
• We are unlocking some error'd lock results if they do not match the specific set of possible errors that is currently in this function
• With newly introduced filtering, this creates a race condition that leads to multiple write locks being taken concurrently if a batch with an error were unlocked during processing

Summary of Changes

• Only unlock transactions that we actually took locks for

Fixes #

[apfitzge]

Attempted to track down when this error filtering was introduced, and found myself back to at least v0.10.0.
It seems this has always been here, and as we added more pre-lock filtering errors we've just added to this set without reflecting why it was there in the first place.

If we never took the locks for the transaction, we should not release those locks under any conditions. Since we are passing the lock_result, if they are any error we did not take them.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (b814497) 81.8% compared to head (6bc110c) 81.8%.
Report is 3 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master   #34652   +/-   ##
=======================================
  Coverage    81.8%    81.8%           
=======================================
  Files         824      824           
  Lines      222394   222392    -2     
=======================================
+ Hits       181957   181981   +24     
+ Misses      40437    40411   -26     

[brooksprumo]

Seems reasonable to me. This was an interesting one to track down!

I don't personally feel that I know this code well enough to give a unilateral "ship it!". I'll defer to justin (or maybe carl/stephen?).

[mergify]

Backports to the stable branch are to be avoided unless absolutely necessary for fixing bugs, security issues, and perf regressions. Changes intended for backport should be structured such that a minimum effective diff can be committed separately from any refactoring, plumbing, cleanup, etc that are not strictly necessary to achieve the goal. Any of the latter should go only into master and ride the normal stabilization schedule.

[mergify]

Backports to the beta branch are to be avoided unless absolutely necessary for fixing bugs, security issues, and perf regressions. Changes intended for backport should be structured such that a minimum effective diff can be committed separately from any refactoring, plumbing, cleanup, etc that are not strictly necessary to achieve the goal. Any of the latter should go only into master and ride the normal stabilization schedule. Exceptions include CI/metrics changes, CLI improvements and documentation updates on a case by case basis.

[apfitzge]

• Added v1.16 and v1.17 labels. Plan was to introduce a backported PR which does earlier fee-payer validation (before locking), and that will require this fix.

[jstarry]

I also went back into git history and couldn't find a good reason for this. Thanks for cleaning it up!