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

Jump to bottom

Handle the "balances" and "coins_to_spend" indexation errors. #2428

Open
rafal-ch opened this issue Nov 11, 2024 · 0 comments
Open

Handle the "balances" and "coins_to_spend" indexation errors. #2428

rafal-ch opened this issue Nov 11, 2024 · 0 comments

Comments

@rafal-ch
Copy link
Contributor

rafal-ch commented Nov 11, 2024 •
edited
Loading

Originally posted by @rafal-ch in #2383 (comment)

This issue is for tracking the error handling in the indexation module:

For Balances:

At the moment all "amounts" for coins, messages and contracts is stored as u64. When implementing the "balances cache" I noticed that when we calculate the total balances, we use the accumulator which is u64 and we call saturating_add() on it. This may lead to the balances being calculated incorrectly.

In the original PR the accumulator has been changed to u128, which vastly reduces the problem with accumulation (one would require humongous amount of u64s to overflow u128). However, there is still a problem with balance deduction, as we might find ourselves in the situation where, for example, our balance is 100, but we try to reduce it by 200.

This is a fatal error and indicates a serious bug in the logic. Currently, we just log it and continue.

To be considered:

  1. Properly handle the underflow error
  • maybe change the return type of "balances()" from StorageResult to something else, since overflow is not a storage error
  • update integration tests - some of them are written with the assumption that balances calculation is infallible
  1. Consider changing all "amounts" to be u128, so that even a single coin can have a balance greater than u64

For Coins To Spend:

When "coins to spend" index is considered, the possible errors to be handled are:

  1. trying to index the same coin twice
  2. trying to remove the coin that is not in the index
@rafal-ch rafal-ch mentioned this issue Nov 11, 2024
Merged
2 tasks
@rafal-ch rafal-ch changed the title Handle the overflow in "balances" calculation. Handle the "balances" and "coins_to_spend" indexation errors. Nov 28, 2024
@rafal-ch rafal-ch mentioned this issue Dec 13, 2024
Merged
2 tasks
rafal-ch added a commit that referenced this issue Jan 13, 2025
@rafal-ch @xgreenx
Use indexation cache to satisfy "coins to spend" queries (#2463)
4783c2f 
Closes #2391

This PR includes all changes from the [Part 1
PR](#2455), making it
deprecated.

## Description
Changes in this PR:

#### The new `CoinsToSpend` index
* This is the database that stores all coins to spend sorted by the
amounts (i.e. largest-by-value coins first)
* The key consists of several parts
* _Retryable flag_ - to distinguish between retryable messages and other
coins
  * _Address_ (owner)
  * _AssetID_
* _Amount_ - as "big-endian" bytes to leverage the RocksDB key sorting
capabilities
* _Foreign Key_ - this are bytes of the key from either the `Messages`
or `Coins` on-chain databases
    * for messages this is a 32-bytes `Nonce`
    * for coins this is a 34-bytes `UtxoId`
* The value is an instance of `IndexedCoinType` enum, so we know which
on-chain database to query when returning the actual coins
* This index is updated when executor events are processed
* When querying for "coins to spend" the following algorithm is applied:
* First, we get as many "big" coins as required to satisfy _double the
amount_ from the query (respecting `max` and `excluded` params)
* If we have enough coins, but there are still some "slots" in the query
left (because we selected less coins than `max`) we fill the remaining
slots with a **random** number of "dust" coins
* If it happens that the value of selected "dust coins" is able to cover
the value of some of the already selected "big coins", we remove the
latter from the response
* If at any step we encounter a problem (reading from database, integer
conversions, etc.) we bail with an appropriate error

#### Changes to `CoinsQueryError` type
* The `MaxCoinsReached` variant has been removed because in the new
algorithm we never query for more coins than the specified `max`, hence,
without additional effort, we are not able to tell whether the query
could be satisfied if user provided bigger `max`
* The `InsufficientCoins` has been renamed to
`InsufficientCoinsForTheMax` and it now contains the additional `max`
field

#### Off-chain database metadata
* The metadata for off-chain database now contain the additional
`IndexationKind` - `CoinsToSpend`

#### Refactoring
* The `indexation.rs` module was split into separate files, each per
indexation type + errors + some utils.

#### Other
* Integration tests have to be updated to not expect the exact number of
coins to spend in the response (currently, due to randomness, we're not
deterministic in this regard)
* The number of excluded ids in the `coinsToSpend` GraphQL query is now
limited to the maximum number of inputs allowed in transaction.

### Before requesting review
- [X] I have reviewed the code myself
- [X] I have created follow-up issues caused by this PR and linked them
here

### Follow-up issues
* #2498
* #2448
* #2428
* #2499
* #2496

---------

Co-authored-by: Green Baneling <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@rafal-ch