feat: add pagination to GetNetworkAccountIds

[SantiagoPittella]

I'm working on #1299 .

I noticed that we do not store the block in which an account is created, so the typical pagination that we are using in the other endpoint does not work here. We would need some kind of cursor to keep track the position of the overall query for the pagination. The problem is that we cannot count on any of the fields in the accounts schema to be ordered. A solution could be to add a created_at field storing the block number in which the account was deployed.

cc @Mirko-von-Leipzig @drahnr

[bobbinth]

I noticed that we do not store the block in which an account is created, so the typical pagination that we are using in the other endpoint does not work here. We would need some kind of cursor to keep track the position of the overall query for the pagination. The problem is that we cannot count on any of the fields in the accounts schema to be ordered. A solution could be to add a created_at field storing the block number in which the account was deployed.

This is for the accounts table, right? And the proposal is to add created_at field to the accounts table so that we have a stable way to sort accounts (i.e., to avoid accounts getting re-sorted if a new account is added or an existing account is updated) - correct?

[Mirko-von-Leipzig]

I think we can technically rely on rowid here if we wanted since we never delete an account again (for now). That said, I think having the created_at height will be useful at some stage in any case. It can also be useful to short-circuit other queries perhaps e.g. if someone is querying the account state at some height this can be a quicker check than checking all account deltas.

Its a cheap column to add so imo we can do it.

[drahnr]

I think this can be removed?

[SantiagoPittella]

Removed!

[drahnr]

Can we avoid the Option? Nobody should request this without anyways, no?

[Mirko-von-Leipzig]

I actually think we need to retain the original behavior within this method. The ntx builder currently expects this to return all network account IDs.

To retain this behavior for now I would

• revert function signature change
• internally loop over and aggregate the returned pages

As currently used this will only return the first page in the main builder https://github.com/0xMiden/miden-node/pull/1452/changes#diff-630c8daca8771b806879dbee6a3059b92341ac7418c345180f537c911dd272a2R15

@SantiagoPittella ^^

[SantiagoPittella]

I implemented this changes

[drahnr]

future work/probably too much: we might want to use build.rs that generates this const value

[SantiagoPittella]

Should I create an issue for this one?

[drahnr]

I'd like @Mirko-von-Leipzig 's input on it first :)

[Mirko-von-Leipzig]

This is an internal only endpoint, so I think we can get away with simplifying this as much as possible such that we don't need a derived MAX_ROWS at all, but can simply keep it at some arbitrary value.

With a small bit of refactoring in the ntx builder (briefly touched upon in #1478 (comment)), we can simplify this function signature to

pub(crate) fn select_all_network_account_ids(
    conn: &mut SqliteConnection,
    token: ContinuationToken,
) -> Result<(Vec<AccountId>, ContinuationToken), DatabaseError> 

This is because the ntx builder won't need to care about block-delimited pages, and instead only needs a mechanism to get all currently committed network account IDs.

So all we need is an iteration key that retains the order, which could be

ORDER BY rowid                  -- if we don't delete, or
ORDER BY created_at, account_id -- for an agnostic one

and then

struct ContinuationToken(rowid);
// or
struct ContinuationToken(BlockNumber, AccountId);

If we do decide to go the stream route, then ContinuationToken would remain internal only, otherwise it forms part of the protobuf.

[Mirko-von-Leipzig]

Overall looks good, but definitely requires the fix on the ntx client side.

I would also be interested in an experiment where we change the proto to return a stream of AccountId instead. Internally, the store would manage the stream by paginating internally (which no longer needs to be block bound, but can use rowid or any other internally consistent row token). e.g. request a page from db, stream the page, request next page, stream it etc.

Client can recover from a broken stream by initializing stream with Option<AccountI>, the last account ID received. Though we could also just restart from scratch since its an internal only API.

[Mirko-von-Leipzig]

I actually think we need to retain the original behavior within this method. The ntx builder currently expects this to return all network account IDs.

To retain this behavior for now I would

• revert function signature change
• internally loop over and aggregate the returned pages

As currently used this will only return the first page in the main builder https://github.com/0xMiden/miden-node/pull/1452/changes#diff-630c8daca8771b806879dbee6a3059b92341ac7418c345180f537c911dd272a2R15

@SantiagoPittella ^^

[SantiagoPittella]

@Mirko-von-Leipzig I created #1478 to test the stream approach

[bobbinth]

Looks good! Thank you! I left a few comments inline - mostly related to making sure the comments/docs are up to date.

[bobbinth]

Looks good! Thank you!