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.

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

adjust query to support ordering #107

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

Conversation

45930
Copy link
Contributor

@45930 45930 commented Oct 21, 2024

Fixes: #106

This PR adds sequenceNumber, as well as zkappAccountUpdateIds to the graphQL schema, which will expose enough information for clients to verify the order in which actions were processed by consensus. It also attempts to return actions in that order, and exposes the ordering spec in a unit test.

@45930 45930 force-pushed the 2024-10-bugfix-actions-order branch from 045c9c8 to ddd8b27 Compare October 22, 2024 17:47
@45930 45930 changed the base branch from main to 2024-10-upgrade-o1js October 22, 2024 17:47
Copy link

@deepthiskumar deepthiskumar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have to better understand the design of this API but my initial reaction is-

  1. sequenceNumber makes more sense under transactionInfo since it is a property of the transaction. Also it needs block-height (assuming the API returns canonical blocks only) to disambiguate transactions with same sequence numbers but are part of different blocks. So the client can order by block height and sequence number
  2. Same with zkappAccountUpdateIds since it describes the order of the account updates within a transaction. Also, I think we'll want to expose the index of the account-id that this action corresponds to. A client can then order by block-height + sequence-number + account-update-sequence-number
  3. zkappEventElementIds: Similar to zkappAccountUpdateIds because events/actions are a list of field elements. The API returns an array of field elements for both actions and events but this array may not be in the same order as it was in the account update. So we might want to use the ordering in zkappEventElementIds from zke.element_ids for actions and events respectively. Finally the order for actions/events for an account across a few different blocks will be based on block-height + transaction-sequence-number + account-sequence-number + event-sequence-number.

Base automatically changed from 2024-10-upgrade-o1js to main October 23, 2024 03:54
@45930
Copy link
Contributor Author

45930 commented Oct 23, 2024

Yes, I will have to work on the format. Here's what I can produce on the current branch:

{
    "data": {
        "actions": [
            {
                "blockInfo": {
                    "stateHash": "3NKZqA81s8iHMQc4vcJxrcx6epVB7C9vBD8oV7K3EpCZzvfebCtP",
                    "timestamp": "1729695587000",
                    "height": 69,
                    "parentHash": "3NLAneCAdNdj2u6DW5ycvUAvuJZP4mMain7eLWEU6QvL9ZTmZ53i",
                    "chainStatus": "canonical",
                    "distanceFromMaxBlockHeight": 36,
                    "globalSlotSinceGenesis": 132
                },
                "actionState": {
                    "actionStateOne": "25619375377485241455325506385026113155326956491463643967090964080969742671789",
                    "actionStateTwo": "25079927036070901246064867767436987657692091363973573142121686150614948079097",
                    "actionStateThree": "25079927036070901246064867767436987657692091363973573142121686150614948079097",
                    "actionStateFour": "25079927036070901246064867767436987657692091363973573142121686150614948079097",
                    "actionStateFive": "25079927036070901246064867767436987657692091363973573142121686150614948079097"
                },
                "actionData": [
                    {
                        "data": [
                            "24650",
                            "1",
                            "63073",
                            "4611450100234868895111341341933539196411650948711574062334330094836938200670",
                            "1"
                        ],
                        "accountUpdateId": "27",
                        "transactionInfo": {
                            "status": "applied",
                            "hash": "5JvAynd7NAHEiMqCpgZD85WCzLniBv18eSEJyuvNVK8pQL8Fi5kx",
                            "memo": "E4YM2vTHhWEg66xpj52JErHUBU4pZ1yageL4TVDDpTTSsv8mK6YaH"
                        },
                        "sequenceNumber": 2,
                        "zkappAccountUpdateIds": [
                            27,
                            28
                        ],
                        "zkappEventElementIds": [
                            23
                        ]
                    },
                    {
                        "data": [
                            "24651",
                            "1",
                            "63073",
                            "4611450100234868895111341341933539196411650948711574062334330094836938200670",
                            "1"
                        ],
                        "accountUpdateId": "28",
                        "transactionInfo": {
                            "status": "applied",
                            "hash": "5JvAynd7NAHEiMqCpgZD85WCzLniBv18eSEJyuvNVK8pQL8Fi5kx",
                            "memo": "E4YM2vTHhWEg66xpj52JErHUBU4pZ1yageL4TVDDpTTSsv8mK6YaH"
                        },
                        "sequenceNumber": 2,
                        "zkappAccountUpdateIds": [
                            27,
                            28
                        ],
                        "zkappEventElementIds": [
                            24
                        ]
                    },
                    {
                        "data": [
                            "73587",
                            "1",
                            "92297",
                            "8253926607854573680301530978054957179571865656842078211022112667228198387920",
                            "1"
                        ],
                        "accountUpdateId": "25",
                        "transactionInfo": {
                            "status": "applied",
                            "hash": "5JumftuGBSY8x4yHpqvpg4gDWUnEpuZ8335Nxr5FgsXxmUsqmX8J",
                            "memo": "E4YM2vTHhWEg66xpj52JErHUBU4pZ1yageL4TVDDpTTSsv8mK6YaH"
                        },
                        "sequenceNumber": 1,
                        "zkappAccountUpdateIds": [
                            25,
                            26
                        ],
                        "zkappEventElementIds": [
                            21
                        ]
                    },
                    {
                        "data": [
                            "73588",
                            "1",
                            "92297",
                            "8253926607854573680301530978054957179571865656842078211022112667228198387920",
                            "1"
                        ],
                        "accountUpdateId": "26",
                        "transactionInfo": {
                            "status": "applied",
                            "hash": "5JumftuGBSY8x4yHpqvpg4gDWUnEpuZ8335Nxr5FgsXxmUsqmX8J",
                            "memo": "E4YM2vTHhWEg66xpj52JErHUBU4pZ1yageL4TVDDpTTSsv8mK6YaH"
                        },
                        "sequenceNumber": 1,
                        "zkappAccountUpdateIds": [
                            25,
                            26
                        ],
                        "zkappEventElementIds": [
                            22
                        ]
                    },
                    {
                        "data": [
                            "55455",
                            "1",
                            "86171",
                            "476632679643188528985438056980162363837707337312210785567256027534077014652",
                            "0"
                        ],
                        "accountUpdateId": "23",
                        "transactionInfo": {
                            "status": "applied",
                            "hash": "5Ju8GoXZWFqBKTAv3b1iMbc77yUhZEoVc5RwD5aj3VQw5eCkHwU9",
                            "memo": "E4YM2vTHhWEg66xpj52JErHUBU4pZ1yageL4TVDDpTTSsv8mK6YaH"
                        },
                        "sequenceNumber": 0,
                        "zkappAccountUpdateIds": [
                            23,
                            24
                        ],
                        "zkappEventElementIds": [
                            19
                        ]
                    },
                    {
                        "data": [
                            "55456",
                            "1",
                            "86171",
                            "476632679643188528985438056980162363837707337312210785567256027534077014652",
                            "0"
                        ],
                        "accountUpdateId": "24",
                        "transactionInfo": {
                            "status": "applied",
                            "hash": "5Ju8GoXZWFqBKTAv3b1iMbc77yUhZEoVc5RwD5aj3VQw5eCkHwU9",
                            "memo": "E4YM2vTHhWEg66xpj52JErHUBU4pZ1yageL4TVDDpTTSsv8mK6YaH"
                        },
                        "sequenceNumber": 0,
                        "zkappAccountUpdateIds": [
                            23,
                            24
                        ],
                        "zkappEventElementIds": [
                            20
                        ]
                    }
                ]
            },
            {
                "blockInfo": {
                    "stateHash": "3NLEZvkc7Z1mRdFkh1bSppnKo8aZrB6VGe83J7UdKpL4PuLjtjqB",
                    "timestamp": "1729695707000",
                    "height": 70,
                    "parentHash": "3NKZqA81s8iHMQc4vcJxrcx6epVB7C9vBD8oV7K3EpCZzvfebCtP",
                    "chainStatus": "canonical",
                    "distanceFromMaxBlockHeight": 35,
                    "globalSlotSinceGenesis": 138
                },
                "actionState": {
                    "actionStateOne": "7365288285578028013264727183932035337741887015276045401228215954698411077529",
                    "actionStateTwo": "25619375377485241455325506385026113155326956491463643967090964080969742671789",
                    "actionStateThree": "25079927036070901246064867767436987657692091363973573142121686150614948079097",
                    "actionStateFour": "25079927036070901246064867767436987657692091363973573142121686150614948079097",
                    "actionStateFive": "25079927036070901246064867767436987657692091363973573142121686150614948079097"
                },
                "actionData": [
                    {
                        "data": [
                            "73850",
                            "1",
                            "11895",
                            "8253926607854573680301530978054957179571865656842078211022112667228198387920",
                            "1"
                        ],
                        "accountUpdateId": "33",
                        "transactionInfo": {
                            "status": "applied",
                            "hash": "5JuunjPt2xffChmddFDzdreN3TtTsADs1AiW4cgPXkYoFERyKPAi",
                            "memo": "E4YM2vTHhWEg66xpj52JErHUBU4pZ1yageL4TVDDpTTSsv8mK6YaH"
                        },
                        "sequenceNumber": 3,
                        "zkappAccountUpdateIds": [
                            33,
                            34
                        ],
                        "zkappEventElementIds": [
                            29
                        ]
                    },
                    {
                        "data": [
                            "73851",
                            "1",
                            "11895",
                            "8253926607854573680301530978054957179571865656842078211022112667228198387920",
                            "1"
                        ],
                        "accountUpdateId": "34",
                        "transactionInfo": {
                            "status": "applied",
                            "hash": "5JuunjPt2xffChmddFDzdreN3TtTsADs1AiW4cgPXkYoFERyKPAi",
                            "memo": "E4YM2vTHhWEg66xpj52JErHUBU4pZ1yageL4TVDDpTTSsv8mK6YaH"
                        },
                        "sequenceNumber": 3,
                        "zkappAccountUpdateIds": [
                            33,
                            34
                        ],
                        "zkappEventElementIds": [
                            30
                        ]
                    },
                    {
                        "data": [
                            "73237",
                            "1",
                            "52294",
                            "476632679643188528985438056980162363837707337312210785567256027534077014652",
                            "0"
                        ],
                        "accountUpdateId": "31",
                        "transactionInfo": {
                            "status": "applied",
                            "hash": "5JtuWFstMC6m3wgeoX8E5Nh1ZdwRdGXvhcrVbGB247Ph72KGpL23",
                            "memo": "E4YM2vTHhWEg66xpj52JErHUBU4pZ1yageL4TVDDpTTSsv8mK6YaH"
                        },
                        "sequenceNumber": 1,
                        "zkappAccountUpdateIds": [
                            31,
                            32
                        ],
                        "zkappEventElementIds": [
                            27
                        ]
                    },
                    {
                        "data": [
                            "73238",
                            "1",
                            "52294",
                            "476632679643188528985438056980162363837707337312210785567256027534077014652",
                            "0"
                        ],
                        "accountUpdateId": "32",
                        "transactionInfo": {
                            "status": "applied",
                            "hash": "5JtuWFstMC6m3wgeoX8E5Nh1ZdwRdGXvhcrVbGB247Ph72KGpL23",
                            "memo": "E4YM2vTHhWEg66xpj52JErHUBU4pZ1yageL4TVDDpTTSsv8mK6YaH"
                        },
                        "sequenceNumber": 1,
                        "zkappAccountUpdateIds": [
                            31,
                            32
                        ],
                        "zkappEventElementIds": [
                            28
                        ]
                    },
                    {
                        "data": [
                            "35093",
                            "1",
                            "39852",
                            "4611450100234868895111341341933539196411650948711574062334330094836938200670",
                            "1"
                        ],
                        "accountUpdateId": "29",
                        "transactionInfo": {
                            "status": "applied",
                            "hash": "5JtrkeMGmJFJQkVk27EtUsmzkLeiCinetkiwvVBfuHXrMqrwJvj2",
                            "memo": "E4YM2vTHhWEg66xpj52JErHUBU4pZ1yageL4TVDDpTTSsv8mK6YaH"
                        },
                        "sequenceNumber": 0,
                        "zkappAccountUpdateIds": [
                            29,
                            30
                        ],
                        "zkappEventElementIds": [
                            25
                        ]
                    },
                    {
                        "data": [
                            "35094",
                            "1",
                            "39852",
                            "4611450100234868895111341341933539196411650948711574062334330094836938200670",
                            "1"
                        ],
                        "accountUpdateId": "30",
                        "transactionInfo": {
                            "status": "applied",
                            "hash": "5JtrkeMGmJFJQkVk27EtUsmzkLeiCinetkiwvVBfuHXrMqrwJvj2",
                            "memo": "E4YM2vTHhWEg66xpj52JErHUBU4pZ1yageL4TVDDpTTSsv8mK6YaH"
                        },
                        "sequenceNumber": 0,
                        "zkappAccountUpdateIds": [
                            29,
                            30
                        ],
                        "zkappEventElementIds": [
                            26
                        ]
                    }
                ]
            }
        ]
    }
}

@45930
Copy link
Contributor Author

45930 commented Oct 23, 2024

It looks like all the data is there, but it can be better-organized.

@deepthiskumar, do you know the relationship between events and actions? It seems like each transaction here has one event id, and 2 action ids. My test script does generate 2 actions per transaction, so I expected that, but I'm not sure why only one event would be sent.

@45930
Copy link
Contributor Author

45930 commented Oct 23, 2024

Also it needs block-height (assuming the API returns canonical blocks only) to disambiguate transactions with same sequence numbers but are part of different blocks

This is already available, as blockInfo.height. In my example JSON, you can see the blocks 69 and 70 are present in the response. Within each block, there are transactions with sequence [0, 1, 2] on one, and [0, 1, 3] on the other. Then within each transaction, there are 2 events each. The accountUpdateId property is on the action itself, and the zkappAccountUpdateIds is an array showing the correct order on the transactionInfo.

I could add a property, accountUpdateIndex and remove the property accountUpdateId and handle that logic in the resolver, or we could send both the accountUpdateId of the action, and the zkappAccountUpdateIds of the transaction and allow clients to do the logic on their side.

@45930
Copy link
Contributor Author

45930 commented Oct 23, 2024

So we could have a response like

{
    "data": [
        "24651",
        "1",
        "63073",
        "4611450100234868895111341341933539196411650948711574062334330094836938200670",
        "1"
    ],
    "accountUpdateId": "28",
    "transactionInfo": {
        "status": "applied",
        "hash": "5JvAynd7NAHEiMqCpgZD85WCzLniBv18eSEJyuvNVK8pQL8Fi5kx",
        "memo": "E4YM2vTHhWEg66xpj52JErHUBU4pZ1yageL4TVDDpTTSsv8mK6YaH",
        "sequenceNumber": 2,
        "zkappAccountUpdateIds": [
            27,
            28
        ],
        "zkappEventElementIds": [
            24
        ]
    }
},

We have enough info here to tell us within the block that it's in, it belongs to transaction 2, and it is the second update in that sequence. That seems good enough to me, as long as that is the correct way to order things.

@45930
Copy link
Contributor Author

45930 commented Oct 23, 2024

For pending blocks, it looks like we follow the longest chain rule here:

SELECT
  id, state_hash, parent_hash, parent_id, height, global_slot_since_genesis, global_slot_since_hard_fork, timestamp, chain_status, ledger_hash, last_vrf_output
FROM
  blocks b
WHERE
  height = (SELECT max(height) FROM blocks)

We can probably handle filtering for canonical blocks in o1js. For reducing actions, I think we can ignore pending blocks by default, but for random exploratory queries, people might be interested to see pending data as well.

@45930 45930 force-pushed the 2024-10-bugfix-actions-order branch from d8f0b60 to 030c60f Compare October 23, 2024 19:51
@@ -0,0 +1,66 @@
import { createYoga, createSchema } from 'graphql-yoga';
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This whole file may be better to remove. It's just a script to make a graphql request against the local API.

@@ -157,4 +157,26 @@ class ActionsService implements IActionsService {
}
return actions;
}

sortActions(actions: Action[]): Action[] {
return actions.sort((a, b) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since actions are returned grouped by block, this is focused on ordering the actions within a block.

I'm not sure the sort order is correct. Maybe it's reversed, but we should get it right, then we will have access to the unit test as a spec.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@deepthiskumar it's worth noting that you and gregor both mentioned a sub-order to the account update in o1-labs/o1js#1872

It is your point 4, and Gregor said "It's extremely important that when reordering the actions here, the order within the same account update is maintained".

This sort only looks at sequence number and account update id. But with my JSON comment on this PR and this code, we should be able to get on the same page more easily. If there's a third sorting lever we need here, then I can add it, but it's not clear to me what it is.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the third sorting lever is the element ID index from zkappEventElementIds

/* fundNewAccount = */ false
);
} catch (error) {
console.error(error);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This console was helpful in debugging. Errors were swallowed otherwise.

@45930 45930 marked this pull request as ready for review October 23, 2024 20:33
@deepthiskumar
Copy link

@deepthiskumar, do you know the relationship between events and actions? It seems like each transaction here has one event id, and 2 action ids. My test script does generate 2 actions per transaction, so I expected that, but I'm not sure why only one event would be sent.

Those are two separate fields in the transaction. Events are more for recording information corresponding to an account update. They don't change any on-chain state. Actions on the other hand are committed to the account (action_state field in an account). They

Can you generate at least two account updates (excluding the fee payer account update) each having two or more events and two or more actions? I think that should cover all the cases.

@deepthiskumar
Copy link

deepthiskumar commented Oct 24, 2024

I could add a property, accountUpdateIndex and remove the property accountUpdateId and handle that logic in the resolver, or we could send both the accountUpdateId of the action, and the zkappAccountUpdateIds of the transaction and allow clients to do the logic on their side.

Adding the index or a sequence field makes more sense to me than exposing account ID that is not consistent across DBs. If one were to run the API on two different DBs (or existing DB was updated during migration) then they could get different values for the same field. But with a sequence number or index they should be consistent across all archive DBs. Imo none of the database ID fields should be exposed to the client to ensure the client side application doesn't get tightly coupled to the database instance serving the API

@45930
Copy link
Contributor Author

45930 commented Oct 24, 2024

Adding the index or a sequence field makes more sense to me than exposing account ID that is not consistent across DBs. If one were to run the API on two different DBs (or existing DB was updated during migration) then they could get different values for the same field. But with a sequence number or index they should be consistent across all archive DBs.

This fundamentally makes sense to me, but just to confirm, there is not literally an account update index value in the DB, it's something that needs to be calculated right? e.g. as I do here? The only thing I worry about is the added compute cost to calculate this every time. It seems quite cheap, but perhaps at scale it's not acceptable.

If that's the right solution for the long term, it's also a possibility to do a quick fix now and a migration of the archive DB in the future to better serve this use case.

@45930
Copy link
Contributor Author

45930 commented Oct 24, 2024

@boray is going to advance this PR while I'm on PTO, so to do a quick summary of the changes I suggest based on your feedback:

  • Add accountUpdateIndexNumber to the graphQL schema
    • the index of the accountUpdateID in zkappAccountUpdateIds
  • Add eventElementIndexNumber to the graphQL schema
    • the index of the eventElementId in zkappEventElementIds
  • Add eventElement as the third layer of sorting in the sortAcitons function
  • Generate a test case where an action has more than one event

Important

Don't remove any values from the graphQL schema, because it should continue to work with older o1js versions which will make the old query. We can consider the db identifier fields deprecated though.

@deepthiskumar
Copy link

This fundamentally makes sense to me, but just to confirm, there is not literally an account update index value in the DB, it's something that needs to be calculated right? e.g. as I do here? The only thing I worry about is the added compute cost to calculate this every time. It seems quite cheap, but perhaps at scale it's not acceptable.

If that's the right solution for the long term, it's also a possibility to do a quick fix now and a migration of the archive DB in the future to better serve this use case.

Yes, the index needs to be computed given the current schema. It is an option to do in the archive processor and have a new table to store the sequence number and the corresponding element ID. It is not clear to me if it is a better option (another huge table to join vs looking up in arrays)

@@ -41,6 +41,9 @@ export type TransactionInfo = {
hash: string;
memo: string;
authorizationKind: string;
sequenceNumber: number;
zkappAccountUpdateIds: number[];
zkappEventElementIds: number[];

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'll want another similar field for Events (this is for Actions) to include as part of the Events data, no?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need that, assuming events will not be sorted?

@deepthiskumar
Copy link

There's actually another order that I didn't account for in my previous messages. I've updated now o1-labs/o1js#1872 (comment)
So, a zkapp transaction is a list of account updates, each account update can have a list of action/events, and finally each event/action is an array of field elements.

@45930
Copy link
Contributor Author

45930 commented Nov 15, 2024

Please note that the current branch has failing tests, but CI is green. I will try to resolve that in the next commit, but this is still WIP.

@45930 45930 force-pushed the 2024-10-bugfix-actions-order branch 2 times, most recently from 4b3b73f to d453a59 Compare November 20, 2024 23:57
@45930 45930 force-pushed the 2024-10-bugfix-actions-order branch from d453a59 to 8822808 Compare November 20, 2024 23:58
@45930 45930 force-pushed the 2024-10-bugfix-actions-order branch from 8822808 to 3c0750e Compare November 21, 2024 00:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Archive node API does not expose enough information by which to order actions
3 participants