Skip to content

Conversation

@GeorgeTsagk
Copy link
Member

Description

On the top level, this PR fixes the flake reported in #1670.

The actual moment in our development history where this flake could have been discovered was here #1478

By merging the above PR, we added an extra check to the HTLC validation by the aux invoice manager. That check compared the assets of the HTLC against the assets of the channel. We would draw the channel ID from rapid.Uint64, but due to the nature of rapid draws this would cause conflicts in the channel ID. That lead to failures when checking the HTLC against the channel.

In detail, the channel asset info would source from lndclient.ListChannels which is mocked with the rapid-generated channels. After retrieving the responses we would check if the chanIDs match, which means that in many cases we could grab the wrong channel entry.

Why this is a flake

The current design of the aux invoice manager prop test allows rapid to draw many values randomly. Many of these values are flags that indicate whether a certain type of error should be caused. Due to the large number of draws related to "things that can go wrong" we would rarely run into the case where "nothing is going wrong, except for a chan ID collision".

The above is true at least for our current default number of checks, which is 100. Even without the above fix (and without any rapid failure files present) you can run the tests with -rapid.checks=100 a couple dozens of times before this error pops up. This allowed for the error introduction to be buried sufficiently deep for it to be ignored as a flake onwards.

With the assumption that we don't really want to account for non-sane cases where we have colliding channel IDs, we change the draw to be done from rand.Uint64 instead. This way intentional collisions, which are not relevant, are avoided.

Closes #1670

Given that this PR changes the set of drawn values, the seed referenced in the above issue becomes irrelevant. We can still draw and ignore the old value just to preserve the previously failing test case.

We also use the rfqmath library in the property based checks in order
to get rid of any discrepancies between the expected and actual values.
Previously we would draw the chanIDs for our channels from the rapid
farmework. This worked for a bit, but after introducing some more strict
checks between the HTLC and the assets within the channel it introduced
some rare failure cases. We now draw the value from rand which is
practically impossible to cause collisions on that value.
@GeorgeTsagk GeorgeTsagk requested review from ffranr and guggero July 22, 2025 16:28
@GeorgeTsagk GeorgeTsagk self-assigned this Jul 22, 2025
@coveralls
Copy link

Pull Request Test Coverage Report for Build 16450088324

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 21540 unchanged lines in 160 files lost coverage.
  • Overall coverage decreased (-17.0%) to 39.504%

Files with Coverage Reduction New Missed Lines %
proof/util.go 2 81.63%
tapdb/migrations.go 2 76.06%
address/log.go 3 0.0%
commitment/log.go 3 0.0%
internal/pedersen/commitment.go 3 95.31%
lndservices/log.go 3 0.0%
rfq/log.go 3 0.0%
tapchannel/log.go 3 0.0%
tapdb/sqlc/db_custom.go 3 70.83%
tapsend/allocation.go 3 70.21%
Totals Coverage Status
Change from base Build 16443362480: -17.0%
Covered Lines: 31400
Relevant Lines: 79486

💛 - Coveralls

Copy link
Contributor

@guggero guggero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🎉

@Roasbeef do you still happen to have the .fail file so we could verify this doesn't happen anymore?

@ffranr
Copy link
Contributor

ffranr commented Jul 22, 2025

@GeorgeTsagk This is the rapid fail I have locally. Not sure if this is the same bug.

TestAuxInvoiceManagerProperty_invoice_manager-20250501230723-340947.zip

@github-project-automation github-project-automation bot moved this from 🏗 In progress to 👀 In review in Taproot-Assets Project Board Jul 29, 2025
@ffranr ffranr added this pull request to the merge queue Jul 29, 2025
Merged via the queue into main with commit ab9c51d Jul 29, 2025
32 of 36 checks passed
@github-project-automation github-project-automation bot moved this from 👀 In review to ✅ Done in Taproot-Assets Project Board Jul 29, 2025
@guggero guggero deleted the fix-invoice-prop-tests branch July 29, 2025 12:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: ✅ Done

Development

Successfully merging this pull request may close these issues.

[Test Flake] TestAuxInvoiceManagerProperty: unexpected final asset value

5 participants