Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rfq: fix precision issue in HTLC compliance check #1194

Merged
merged 1 commit into from
Nov 18, 2024
Merged

rfq: fix precision issue in HTLC compliance check #1194

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions rfq/order.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -297,6 +297,13 @@ func (c *AssetPurchasePolicy) CheckHtlcCompliance(
return fmt.Errorf("error summing asset balance: %w", err)
}

// Due to rounding errors, we may slightly underreport the incoming
// value of the asset. So we increase it by exactly one asset unit to
// ensure that we do not reject the HTLC in the "inbound amount cannot
// be less than outbound amount" check below.
roundingCorrection := rfqmath.NewBigIntFromUint64(1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we always assume the rounding error is less then 1 asset unit?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, the imprecision of an integer representation of a division can at most be 0.99999......, since we're strictly cutting off any fractional part after the division.

assetAmt = assetAmt.Add(roundingCorrection)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would adding an entire asset unit be acceptable in the scenario where where one asset unit is a considerable amount / value to the user? e.g. the amount represented an 1e8 sats. Or are the rounding errors only expected when the values are de minis?

What other methods might be available to address the source of the rounding errors?

Copy link
Member Author

Choose a reason for hiding this comment

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

just to clarify: this PR doesn't "fix" any rounding related code. It just adjusts a check to allow an HTLC to be forwarded that is affected by the rounding down. The rounding down will always happen, due to the integer nature of taproot assets (see RFQ/decimal display doc for more info).
The decision we can make is: do we force the sender to over pay by one unit so that the ultimate receiver always sees the expected (or even one unit more) balance? Or do we allow under payment of one unit, leading to the scenario where the ultimate receiver sees a final balance lower than expected.

But since this only fixes a check, I don't think we should block this PR and look at the decision holistically.


// Convert the inbound asset amount to millisatoshis and ensure that the
// outgoing HTLC amount is not more than the inbound asset amount.
assetAmtFp := new(rfqmath.BigIntFixedPoint).SetIntValue(assetAmt)
Expand Down
Loading