jsonrpc: introduce coin_index_2 with coins sorted by balance #19822

bmwill · 2024-10-11T17:47:28Z

Description

Introduce a new coin_index_2 index with coins sorted by balance in decreasing value (sorted high to low).

Test plan

How did you test the new or updated feature?

Release notes

Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required.

For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates.

Protocol:
Nodes (Validators and Full nodes):
Indexer:
JSON-RPC:

Introduces a new coin index used in returning paginated coins for the sui_getCoins JSON-RPC method. This changes the pagination order of coins to be returned in order of decreasing value (coins with a larger balance are returned first). In order to accomplish this a new column family (coin_index_2) was added to the jsonrpc indexes rocksdb database, deprecating and obsolescing the old coin_index column family.

Note

On startup a Fullnode will need to initialize this new index by iterating through, and processing the live
object set. This will result in a small period of downtime on node startup.

Once the new index has been initialized the old one will be removed in order to reclaim disk space.

GraphQL:
CLI:
Rust SDK:
REST API:

vercel · 2024-10-11T17:47:38Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
sui-docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Oct 15, 2024 7:13pm

3 Skipped Deployments

Name	Status	Preview	Updated (UTC)
multisig-toolkit	⬜️ Ignored (Inspect)	Visit Preview	Oct 15, 2024 7:13pm
sui-kiosk	⬜️ Ignored (Inspect)	Visit Preview	Oct 15, 2024 7:13pm
sui-typescript-docs	⬜️ Ignored (Inspect)	Visit Preview	Oct 15, 2024 7:13pm

longbowlu · 2024-10-12T21:24:51Z

crates/sui-core/src/par_index_live_object_set.rs

+    fn make_live_object_indexer(&self) -> Self::ObjectIndexer;
+}
+
+/// Represents an instance of a indexer that operates on a subset of the live object set


Suggested change

/// Represents an instance of a indexer that operates on a subset of the live object set

/// Represents an instance of an indexer that operates on a subset of the live object set

longbowlu · 2024-10-12T21:49:45Z

crates/sui-core/src/jsonrpc_index.rs

-                });
+
+        // 1. Remove old coins from the DB by looking at the set of input coin objects
+        let coin_delete_keys = input_coins


@bmwill is there a motivation to change this piece (i.e. not using object_index_changes but using input_coins instead)? It's not immediately clear to me whether this change is correct or not, and we'd better not touch this piece unless absolutely needed

same for using written_coins below

Yes, the way it is written now is actually going to be more correct and less error prone and imo easier to reason about.

I can't tell whether it's more correct or not with my current cooked weekend brain. Did you find errors in the old implementation which makes the new way "more correct"? If not, i'd vote for not touching this very brittle thing. If yes or you feel very confident about the correctness then i won't insist either.

either way i think @sadhansood may want to take a look at this piece as well

Taking another look this seems like it can be simplified even more, and i should cleanup the comments on the written coins side

Looking at the new logic, it seems reasonable. Although it is hard to be 100% sure and my hope is our tests will catch any bugs here. One way we could be absolutely sure is to compare end of epoch account balance of every account between old and new indexing logic on two FNs which are bootstrapped from db snapshots of the same epoch (obviously disconnect network or change sui config to prevent them from processing new checkpoints). This is subjective on how much confidence we have on our tests

longbowlu · 2024-10-12T21:54:29Z

crates/sui-json-rpc/src/coin_api.rs

+                Some(c) => {
+                    let obj = self.internal.get_object(&c).await?.ok_or_else(|| {
+                        SuiRpcInputError::GenericInvalid("cursor not found".to_string())
+                    })?;
+                    let coin = obj.as_coin_maybe().ok_or_else(|| {
+                        SuiRpcInputError::GenericInvalid("cursor is not a coin".to_string())
+                    })?;
+                    (coin_type_tag.to_string(), !coin.balance.value(), c)
+                }


this has the risk of getting inconsistent results across pages, but honestly this is not the only api with this issue and i don't have a better idea how to deal with it

Yeah :(

I had two options:

break the cursor to now take the balance

do this

This seemed the least bad option.

It does highlight that cursors like this should be less defined so that they can change if need be in the future

Remark: Rocksdb actually has a way to support consistent cursor here which is called snapshot (not to be confused with db checkpoint). It's a low overhead point in time view of the db (and millions of such snapshots could be simultaneously kept) which could be used to retrieve consistent data across pages (obviously we cannot hold a snapshot forever and it would need to be timed out)

crates/sui-core/src/jsonrpc_index.rs

crates/sui-core/src/par_index_live_object_set.rs

crates/sui-core/src/jsonrpc_index.rs

sadhansood · 2024-10-13T04:14:21Z

crates/sui-core/src/jsonrpc_index.rs

-                });
+
+        // 1. Remove old coins from the DB by looking at the set of input coin objects
+        let coin_delete_keys = input_coins


Looking at the new logic, it seems reasonable. Although it is hard to be 100% sure and my hope is our tests will catch any bugs here. One way we could be absolutely sure is to compare end of epoch account balance of every account between old and new indexing logic on two FNs which are bootstrapped from db snapshots of the same epoch (obviously disconnect network or change sui config to prevent them from processing new checkpoints). This is subjective on how much confidence we have on our tests

crates/sui-core/src/rest_index.rs

sadhansood · 2024-10-13T05:43:07Z

crates/sui-json-rpc/src/coin_api.rs

+                Some(c) => {
+                    let obj = self.internal.get_object(&c).await?.ok_or_else(|| {
+                        SuiRpcInputError::GenericInvalid("cursor not found".to_string())
+                    })?;
+                    let coin = obj.as_coin_maybe().ok_or_else(|| {
+                        SuiRpcInputError::GenericInvalid("cursor is not a coin".to_string())
+                    })?;
+                    (coin_type_tag.to_string(), !coin.balance.value(), c)
+                }


Remark: Rocksdb actually has a way to support consistent cursor here which is called snapshot (not to be confused with db checkpoint). It's a low overhead point in time view of the db (and millions of such snapshots could be simultaneously kept) which could be used to retrieve consistent data across pages (obviously we cannot hold a snapshot forever and it would need to be timed out)

crates/sui-core/src/par_index_live_object_set.rs

sadhansood

code lgtm, and hopefully with some a/b testing of fetching coins/balance we can see if this works as expected

crates/sui-core/src/jsonrpc_index.rs

Factor out the parallel live object indexing, used for initializing the rest indexes, into more general purpose and reusable logic.

Introduce a new coin_index_2 index with coins sorted by balance in decreasing value (sorted high to low).

Initialize the new coin_index_2 index and clear the old coin_index column family.

Reduce the minimum batch size to write out during indexing the live object set from 256MB to 128MB, these smaller batch sizes resulted in a small improvement to index initialization.

…tion Introduce a meta column family that is used to track initialization of the Index DB itself as well as the status of the various column families themsevles.

vercel bot deployed to Preview – sui-docs October 11, 2024 17:52 View deployment

vercel bot deployed to Preview – sui-docs October 11, 2024 19:12 View deployment

vercel bot deployed to Preview – sui-docs October 11, 2024 20:03 View deployment

bmwill force-pushed the jsonrpc-coin-index branch 2 times, most recently from 37387e2 to 39459f1 Compare October 11, 2024 20:30

bmwill changed the title ~~Jsonrpc coin index~~ jsonrpc: introduce coin_index_2 with coins sorted by balance Oct 11, 2024

bmwill marked this pull request as ready for review October 11, 2024 20:31

vercel bot deployed to Preview – sui-docs October 11, 2024 20:32 View deployment

bmwill requested a review from longbowlu October 11, 2024 20:35

bmwill force-pushed the jsonrpc-coin-index branch from 39459f1 to 7743d1e Compare October 11, 2024 21:52

vercel bot deployed to Preview – sui-docs October 11, 2024 21:53 View deployment

bmwill force-pushed the jsonrpc-coin-index branch from 7743d1e to 1e51100 Compare October 12, 2024 20:19

vercel bot deployed to Preview – sui-docs October 12, 2024 20:21 View deployment

longbowlu reviewed Oct 12, 2024

View reviewed changes

sblackshear requested a review from sadhansood October 12, 2024 22:02

bmwill force-pushed the jsonrpc-coin-index branch from 1e51100 to 2009290 Compare October 13, 2024 00:52

vercel bot deployed to Preview – sui-docs October 13, 2024 00:54 View deployment

sadhansood reviewed Oct 13, 2024

View reviewed changes

vercel bot deployed to Preview – sui-docs October 15, 2024 15:24 View deployment

sadhansood approved these changes Oct 15, 2024

View reviewed changes

crates/sui-core/src/jsonrpc_index.rs Outdated Show resolved Hide resolved

longbowlu reviewed Oct 15, 2024

View reviewed changes

crates/sui-core/src/jsonrpc_index.rs Show resolved Hide resolved

bmwill added 8 commits October 15, 2024 14:12

db_tool: remove loaded_child_object_versions search match arm

bee69bf

jsonrpc: move indexes from sui-storage to sui-core

5a065da

core: factor out parallel live object indexing

018e69d

Factor out the parallel live object indexing, used for initializing the rest indexes, into more general purpose and reusable logic.

jsonrpc: introduce coin_index_2 with coins sorted by balance

5ab6e35

Introduce a new coin_index_2 index with coins sorted by balance in decreasing value (sorted high to low).

jsonrpc: initialize new coin_index_2

7da9517

Initialize the new coin_index_2 index and clear the old coin_index column family.

jsonrpc: add test for sorted coin balances

521b112

jsonrpc, rest: reduce batch size to 128MB before writting

a052358

Reduce the minimum batch size to write out during indexing the live object set from 256MB to 128MB, these smaller batch sizes resulted in a small improvement to index initialization.

jsonrpc: introduce a meta column family for tracking index initializa…

a487e8c

…tion Introduce a meta column family that is used to track initialization of the Index DB itself as well as the status of the various column families themsevles.

bmwill force-pushed the jsonrpc-coin-index branch from e543142 to a487e8c Compare October 15, 2024 19:12

vercel bot deployed to Preview – sui-docs October 15, 2024 19:13 View deployment

bmwill merged commit 71f9203 into MystenLabs:main Oct 16, 2024
46 checks passed

bmwill deleted the jsonrpc-coin-index branch October 16, 2024 01:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

jsonrpc: introduce coin_index_2 with coins sorted by balance #19822

jsonrpc: introduce coin_index_2 with coins sorted by balance #19822

bmwill commented Oct 11, 2024 •

edited

Loading

vercel bot commented Oct 11, 2024 •

edited

Loading

longbowlu Oct 12, 2024

longbowlu Oct 12, 2024

bmwill Oct 12, 2024 •

edited

Loading

longbowlu Oct 12, 2024

bmwill Oct 13, 2024

sadhansood Oct 13, 2024

longbowlu Oct 12, 2024

bmwill Oct 12, 2024

sadhansood Oct 13, 2024

sadhansood Oct 13, 2024

sadhansood Oct 13, 2024

sadhansood left a comment

	/// Represents an instance of a indexer that operates on a subset of the live object set
	/// Represents an instance of an indexer that operates on a subset of the live object set

jsonrpc: introduce coin_index_2 with coins sorted by balance #19822

jsonrpc: introduce coin_index_2 with coins sorted by balance #19822

Conversation

bmwill commented Oct 11, 2024 • edited Loading

Description

Test plan

Release notes

vercel bot commented Oct 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmwill Oct 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sadhansood left a comment

Choose a reason for hiding this comment

bmwill commented Oct 11, 2024 •

edited

Loading

vercel bot commented Oct 11, 2024 •

edited

Loading

bmwill Oct 12, 2024 •

edited

Loading