fix: utxo consolidation works without prev key #4903
Merged
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Pull Request
Checklist
Summary
@j4m1ef0rd noticed that consolidation test didn't work on localnet, so I had a look (it is part of full bouncer, but not run by default in PRs). Looks like at some point the code was changed resulting in consolidation being skipped if the previous key isn't available (which it is not the case unless you've rotated multiple times on localnet).
Arguably, we don't care about this edge case, but it is still techincally a bug and it is annoying that the test stopped working on localnet (it wasn't a problem in full bouncer because we rotate more than once there), so I fix it here.
Update: This turned out to be more work than I originally thought, but I decided that I might as well finish what I started. Specifically, some changes were required in unit tests, which I found a bit messy. Some of them expliclty asserted that consolidation shouldn't happen if no previous key is set, which was simply "how thing worked" rather than "how things should work". "Consolidation" of old utxos still only happens when previous key is provided. (I really wish we didn't merge the two mechanisms into the same function.)
I also noticed that "consolidation" of old utxos worked even for "stale" utxos (corresponding to the key older than previous). We should only "consolidate" old uxtos that belong to the previous key because we shouldn't assume that we can sign anything older than that (and we already discard such utxos in practice). I changed the code to only "consolidate" old utxos from previous key (in addition to normal consolidation of current key utxos).
This fix probably doesn't matter in practice since old utxos would be discarded elsewhere, but the original behaviour made it more difficult to reason about tests, where we expect "stale" utxos to be discarded rather than "consolidated" (the original tests were hiding this due to the consolidation function not always being called), and the code is now more consistent with "telling the story" about how stale utxos are handled.