Create indexation cache for "coins to spend" queries #2455
Conversation
rafal-ch
changed the title
rafal-ch
requested review from
xgreenx,
Dentosal and
MitchTurner
as code owners
…ssage()` test" This reverts commit c95ea5b.
…l_2391_coins_to_spend_cache
…l_2391_coins_to_spend_cache
| offset += Address::LEN; | ||
| arr[offset..offset + AssetId::LEN].copy_from_slice(asset_id_bytes); | ||
| offset += AssetId::LEN; | ||
| arr[offset..offset + u8::BITS as usize / 8].copy_from_slice(&NON_RETRYABLE_BYTE); |
There was a problem hiding this comment.
I think it will make more sense if the type of the coin Message/Coin will be after amount, before Nonce/UtxoId. In this case we also will sort messages by amount.
There was a problem hiding this comment.
This assumes we change the structure of the index key, because with the current approach we cannot switch places since this byte is used as a part of prefix when querying the index. Let's huddle this out.
| use crate::graphql_api::indexation; | ||
|
|
||
| use self::indexation::coins_to_spend::{ | ||
| NON_RETRYABLE_BYTE, |
There was a problem hiding this comment.
In another comment I mentioned, that we don't need to include retryable messages into the coins to spend query. So we can remove it.
But you need to know the difference between coin and message, so I think we need to use this 1 byte for the Message or Coin enum representation.
There was a problem hiding this comment.
In the complete PR I use the "value" in the column to distinguish between Coins and Messages (to be able to read data from the proper on-chain DB).
| offset += u64::BITS as usize / 8; | ||
| arr[offset..offset + Nonce::LEN].copy_from_slice(nonce_bytes); | ||
| offset += Nonce::LEN; | ||
| arr[offset..].copy_from_slice(&indexation::coins_to_spend::MESSAGE_PADDING_BYTES); |
There was a problem hiding this comment.
Hmm, how well RocksDB works with the dynamic sized keys? Maybe based on the type of the message/coin we could use 32/34 bytes for Nonce/UtxoId types and during decoding decide what type to return?
Just a thought, if it is hard to support or implement, I'm okay with the current padding approach =)
There was a problem hiding this comment.
Actually, in the complete PR I decided to remove the padding approach and use variable length keys. I don't know about any performance implications on RocksDB side. Also, we never query for a large amounts of data (255 items at most with the current limits), so we should be good. Taking this into consideration I though it's not worth "wasting" additional two bytes for every indexed message.
| type Key = Self::OwnedKey; | ||
| type OwnedKey = CoinsToSpendIndexKey; | ||
| type Value = Self::OwnedValue; | ||
| type OwnedValue = u8; |
There was a problem hiding this comment.
Why do we need 1 byte here? If you don't use it, we can just use () type.
There was a problem hiding this comment.
Ah, I see, it is IndexedCoinType. Why not to use this type here instead of u8? Also, in the comment above I said that maybe we could use retryable and non-retryable byte to track message/coin type.
There was a problem hiding this comment.
I think it will be simpler to review if the second part was actually the first, and vice versa=)
Because the second part only requires you to have sorted backward and forward iterators. You could update the current algorithm to work with this iterator(you could use iter and iter().rev() from the vector) and, in the next PR, replace the sorted vector with the new indexation. In this case, we don't need to have todo! in the code and it is easier to review the final variant of the feature.
Yes, that's true. Also because after I started implementing the actual usage of the index, I noticed that a couple of things implemented here need to be adjusted. I'll prepare a single PR with the complete feature which will also include responses to the comments you placed here. |
|
Closing now in favor of #2463 |
This is the 1/2 PR to fix the #2391
It is stacked on top of #2383 (balances cache)
Description
The scope of this PR is to build the proper index upon processing the coin related events.
The follow-up PR will contain the actual usage of the index, hence there are a couple of TODOs left that mention this follow-up PR.
Before requesting review