fix: denomination arithmetic #794

rflechtner · 2024-11-11T10:34:20Z

Adresses issues with precision stemming from integer division in convert_to_fixed and suggests a strategy to mitigate overflow problems on refund_account.

Checklist:

I have verified that the code works
- No panics! (checked arithmetic ops, no indexing array[3] use get(3), ...)
I have verified that the code is easy to understand
- If not, I have left a well-balanced amount of inline comments
I have left the code in a better state
I have documented the changes (where applicable)
- Either PR or Ticket to update the Docs
- Link the PR/Ticket here

pallets/pallet-bonded-coins/src/curves/mod.rs

rflechtner · 2024-11-11T11:31:00Z

pallets/pallet-bonded-coins/src/curves/mod.rs

+			// This WILL overflow if 10^denomination > capacity(float)
+			.checked_div(decimals.checked_to_fixed().ok_or(ArithmeticError::Overflow)?)


The solution comes at the cost of one additional case where overflow can happen:

Previously, overflow could happen where capacity(balance) / 10^denomination > capacity(fixed), which for a u128 balance (capacity ~3.4e+38) and a I75F53 (capacity ~1.8e+22) meant that any denomination >= 17 is safe.

~~With the current solution, overflow will occur where 10^denomination > capacity(float), which for an I75F53 means that we have a hard cap at a denomination of 22.~~

We can possibly soften the hard cap, e.g. by iteratively dividing the remainder by 10 (or some other multiplicative component of decimals which can be represented), but denominations larger than the soft cap would progressively increase the probability of an overflow occurring.

For example, with an I75F53 and a denomination of 23, any amount ~1.8e+22 < x < e+23 would result in a remainder that cannot be represented and thus in an overflow error.

Softened the cap on 694334c, which I think is a better way of scaling a fixed by an integer.
Still, I introduced a constant to limit allowable denominations on e94104e.

UPDATE: Since 08b71c4 we no longer have new cases of overflow introduced with this fix.

rflechtner · 2024-11-11T13:28:18Z

pallets/pallet-bonded-coins/src/lib.rs

+				.checked_mul(&burnt)
+				// This can indeed overflow if (sum_of_issuances - 1) * burnt > capacity(u128)
+				.ok_or(ArithmeticError::Overflow)?


Very large sum_of_issuances make large remainders more likely, which can in turn result in a product that is too large.
We can accept that refunds would sometimes fail, which would require accounts with lower balances to refund first (after which the situation may have changed).
Or we can look for fallback strategies, such as:

saturating multiplication, which could mean that people receive only a fraction of the funds they would be entitled to

scaling, which sacrifices precision

partial refunds, where we only burn the amount of funds for which we can properly compute the collateral to be refunded, and leave the rest untouched.

With 08b71c4 overflow issues are now a thing of the past.

rflechtner · 2024-11-11T14:18:15Z

pallets/pallet-bonded-coins/src/tests/curves/arithmetic.rs

+#[test]
+fn test_convert_to_fixed_too_large_denomination() {
+	let x = u128::MAX; // around 3.4e+38
+	let denomination = 30u8; // I75F53 should handle around 1.8e+22, this can't be represented
+
+	let result = convert_to_fixed::<Test>(x, denomination);
+	assert_ok!(result);
+}


this test fails, as a reminder that the proposed solution is still imperfect

Due to 694334c this test now passes; but not for u128::MAX, which results in an overflowing remainder.

Since 08b71c4 large denominations do not result in an overflow any more. Note that of course truncation can still happen with large denominations if the fixed type is not able to offer enough digits behind the comma.

rflechtner · 2024-11-11T14:23:46Z

pallets/pallet-bonded-coins/src/lib.rs

+			// divide first to avoid overflow, remainder will be handled seperately
+			let divisible_part = total_collateral_issuance


I realised that the approach of handling remainders separately can also improve the calculations of how much collateral is refunded, preventing overflow in many (but not all) cases.

ntn-x2 · 2024-11-12T14:26:40Z

pallets/pallet-bonded-coins/src/lib.rs

 		type MaxStringLength: Get<u32>;

+		/// The maximum denomination that bonded currencies can use. To ensure
+		/// safe operation, this should be set so that 10^MaxDenomination <


If you want to make sure that only these values are specified here, you can add an integrity test (available with the try-runtime feature) that only checks the values of the Config implementations for any given runtime, according to the specified rules. You can check an example here: https://github.com/paritytech/polkadot-sdk/blob/a875f1803253e469288f1a905e73861fb600bb28/polkadot/runtime/parachains/src/configuration.rs#L1258.

This is to protect against malformed runtime-injected values inside a config trait, not too much about user-provided values.

The new solution does not really put a limit on the maximum denomination - as long as 10^denomination can be represented in a U256 - but if the denomination is larger than the precision of the fixed, we will experience truncation of balance amounts during scaling, which I documented here.

I also added an integrity test to check this, as suggested: 3a2306b

pallets/pallet-bonded-coins/src/lib.rs

Co-authored-by: Adel Golghalyani <48685760+Ad96el@users.noreply.github.com>

…ows U256

rflechtner added 3 commits November 11, 2024 10:38

test: denomination arithmetic

9988d89

fix: avoid dropping decimals in fixed conversion

36f4d07

fix: adress overflow issues on refund_account

9807ea3

rflechtner mentioned this pull request Nov 11, 2024

test: mint and burn #791

Merged

6 tasks

rflechtner self-assigned this Nov 11, 2024

rflechtner commented Nov 11, 2024

View reviewed changes

pallets/pallet-bonded-coins/src/curves/mod.rs Show resolved Hide resolved

rflechtner commented Nov 11, 2024

View reviewed changes

pallets/pallet-bonded-coins/src/curves/mod.rs Outdated Show resolved Hide resolved

rflechtner commented Nov 11, 2024

View reviewed changes

refactor: convert_to_fixed

4b8d224

rflechtner commented Nov 11, 2024

View reviewed changes

rflechtner added 2 commits November 12, 2024 10:51

fix: mitigate overflow on denominations exceeding the capacity of fixed

694334c

feat: constant to limit maximum allowed denomination

e94104e

rflechtner marked this pull request as ready for review November 12, 2024 14:16

rflechtner requested a review from ntn-x2 November 12, 2024 14:16

ntn-x2 reviewed Nov 12, 2024

View reviewed changes

ntn-x2 approved these changes Nov 13, 2024

View reviewed changes

Ad96el approved these changes Nov 13, 2024

View reviewed changes

rflechtner mentioned this pull request Nov 14, 2024

refactor: use U256 for infallible scaling #796

Merged

6 tasks

rflechtner and others added 4 commits November 14, 2024 13:13

refactor: use U256 for infallible scaling (#796)

08b71c4

Co-authored-by: Adel Golghalyani <48685760+Ad96el@users.noreply.github.com>

chore: document safe denomination limit

bf42e60

feat: add integrity test hook

3a2306b

test: additional integrity test to check for denomination that overfl…

a86d1c0

…ows U256

rflechtner merged commit 6b3dcf5 into pallet_bonded_coins Nov 14, 2024
1 of 7 checks passed

rflechtner deleted the fix-denomination-arithmetic branch November 14, 2024 14:12

		// This WILL overflow if 10^denomination > capacity(float)
		.checked_div(decimals.checked_to_fixed().ok_or(ArithmeticError::Overflow)?)

		// divide first to avoid overflow, remainder will be handled seperately
		let divisible_part = total_collateral_issuance

fix: denomination arithmetic #794

fix: denomination arithmetic #794

Uh oh!

Conversation

rflechtner commented Nov 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist:

Uh oh!

Uh oh!

Uh oh!

rflechtner Nov 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rflechtner Nov 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rflechtner Nov 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

rflechtner commented Nov 11, 2024 •

edited

Loading

rflechtner Nov 11, 2024 •

edited

Loading

rflechtner Nov 14, 2024 •

edited

Loading

rflechtner Nov 11, 2024 •

edited

Loading