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

Revert AuthorityStore deprecated functions to fix AuthorityStore::multi_get_transaction_checkpoint #4164

Merged
merged 6 commits into from
Nov 26, 2024

Conversation

bingyanglin
Copy link
Contributor

@bingyanglin bingyanglin commented Nov 20, 2024

Description of change

Together with @semenov-vladyslav to investigate the AuthorityStore deprecated codes again, and in order to enable get the CheckpointSequenceNumber by the transaction digest, the old deprecated codes need to be reverted, because there is no other tables storing the relationship between the digest and epochId, or digest and CheckpointSequenceNumber in old epochs. Also the deprecated_multi_get_transaction_checkpoint/deprecated_get_transaction_checkpoint /deprecated_insert_finalized_transactions/executed_transactions_to_checkpoint/checkpoint_cache are coupled together. The easiest/safest way might be just to revert them back.

Investigation

The error is triggered in JSON RPC ReadApi impl of get_transaction_block: the API response contains checkpoint sequence number and timestamp (of the checkpoint) which are fetched from AuthorityPerpetualTables::executed_transactions_to_checkpoint DB table via a number of traits and wrappers. The table is not used for any other purposes and was marked as deprecated (together with a number of related functions and trait methods that support this table) in favor of AuthorityEpochTables::executed_transactions_to_checkpoint which gets cleared after each epoch. The reason for deprecation is unclear. Maybe, the JSON RPC ReadApi was expected to change (eg. get rid of checkpoint sequence number), or the implementation was expected to change and not use the mentioned table.

  1. Right now we do not wish to change JSON RPC ReadApi.
  2. We can't reimplement it another way (eg. fetch checkpoint info from another table); there is AuthorityState::rest_index that might have the required info (it seems to be duplicated), but it is only optionally enabled, so we can't rely on it unconditionally.
  3. The access to the table is hidden with traits and wrappers, so it's not possible to simplify access to it and still get rid of methods marked as deprecated.

So the only reasonable option is to revert the RP that removed the table and deprecated API. And rename functions to make it clearer that the checkpoint info is fetched from perpetual table and add a few comments explaining these few implementation details.

As for the testnet the simplest solutions is to resync nodes from genesis.

Links to any relevant issues

Fixed #4154

Type of change

  • Bug fix (a non-breaking change which fixes an issue)

How the change has been tested

Local Testing

Passed the testing cargo nextest run -p iota-json-rpc-tests -E "test(#get_transaction_block)" locally.

 Nextest run ID a45927f8-cf75-464b-a4ea-a5110d5a6d8e with nextest profile: default
    Starting 1 test across 10 binaries (148 tests skipped)
        PASS [  14.656s] iota-json-rpc-tests::read_api get_transaction_block

Sync from Genesis Testing

  1. Prepare empty db/live folder
  2. Build this branch and run cargo run --release --bin iota-node -- --config-path fullnode.yaml
  3. Successfully sync without crashing
  4. Query the old checkpoint and it returns checkpoints belonging to epoch 0.
curl 'http://iota-test-node-0.iota.works:9000/' \
 -H 'content-type: application/json' \
 --data-raw '{"jsonrpc":"2.0","id":12,"method":"iotax_queryTransactionBlocks","params": [
   {
     "filter": {"FromAddress": "0x0000000000000000000000000000000000000000000000000000000000000000"},
     "options": {"showInput": true}
   },
   null,
   10,
   false
 ]}' | json_pp | grep checkpoint
  1. Get the correct output
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 11637  100 11375  100   262  26006    599 --:--:-- --:--:-- --:--:-- 26629
            "checkpoint" : "0",
            "checkpoint" : "4",
            "checkpoint" : "1",
            "checkpoint" : "2",
            "checkpoint" : "2",
            "checkpoint" : "6",
            "checkpoint" : "8",
            "checkpoint" : "11",
            "checkpoint" : "13",
            "checkpoint" : "16",

Sync in the middle of the epoch testing

  1. Use the iotaledger/iota-node:v0.7.2-rc image to sync to the last checkpoint.
  2. Shut the node down, and remain all the live folder
  3. Apply this reverting change, build and run cargo run --release --bin iota-node -- --config-path fullnode.yaml
  4. Successfully sync without crashing
  5. Query the old checkpoint and it works
  curl 'http://iota-test-node-0.iota.works:9000/' \
  -H 'content-type: application/json' \
  --data-raw '{"jsonrpc":"2.0","id":12,"method":"iotax_queryTransactionBlocks","params": [
    {
      "filter": {"FromAddress": "0x0000000000000000000000000000000000000000000000000000000000000000"},
      "options": {"showInput": true}
    },
    "7FoK3eUdLeRhpsyCyMqEJ5coKX2A4eAhrA3AqKWJWvex",
    2,
    false
  ]}' | json_pp | grep checkpoint
  1. Get the correct output
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  2301  100  1998  100   303   3116    472 --:--:-- --:--:-- --:--:--  3584
            "checkpoint" : "1075228",
            "checkpoint" : "1075228",

Change checklist

Tick the boxes that are relevant to your changes, and delete any items that are not.

  • I have followed the contribution guidelines for this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • I have checked that new and existing unit tests pass locally with my changes

@bingyanglin bingyanglin added the node Issues related to the Core Node team label Nov 20, 2024
@bingyanglin bingyanglin self-assigned this Nov 20, 2024
@bingyanglin bingyanglin changed the title Revert AuthorityStore deprecated functions to fix AuthorityStore ::multi_get_transaction_checkpoint Revert AuthorityStore deprecated functions to fix AuthorityStore::multi_get_transaction_checkpoint Nov 20, 2024
@bingyanglin bingyanglin marked this pull request as ready for review November 20, 2024 14:36
@bingyanglin bingyanglin requested review from a team as code owners November 20, 2024 14:36
@bingyanglin bingyanglin marked this pull request as draft November 20, 2024 14:43
@muXxer
Copy link
Contributor

muXxer commented Nov 20, 2024

As discussed in the call, if those things are actually needed and not deprecated, please remove all "deprecated" comments and function names, and add a proper description why the database table is needed.

Then please sync the state from genesis with an old commit of the codebase, then apply this fix and restart the node to check if the node crashes, or if the newly added table works fine.

@alexsporn We should discuss if a "migration step or tool" is needed to fill that info in the perpetual table, or if the lack of the info for the old transactions is not that big of a problem. In my opinion it would get pruned at some point anyway, or nodes that want to keep all history could simply resync from genesis after applying that fix, to have the fixed state.

@bingyanglin
Copy link
Contributor Author

bingyanglin commented Nov 20, 2024

Then please sync the state from genesis with an old commit of the codebase, then apply this fix and restart the node to check if the node crashes, or if the newly added table works fine.

Thanks @muXxer for the test plan, I addressed them and did 3 different testings in 7347932 locally. All of them passed, and I put the details here. @semenov-vladyslav will add comments and do renaming, then I will test the new commit again. Also need to check if we can query the tx digest which belongs to the previous epoch.
Updated: d32c948 also passed the three same scenarios.

@semenov-vladyslav semenov-vladyslav marked this pull request as ready for review November 21, 2024 11:10
Copy link
Contributor

@kodemartin kodemartin left a comment

Choose a reason for hiding this comment

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

The test patch documented in #4154 is not actually merged on develop. I will push a more polished version promptly here if you don't mind.

Copy link
Contributor

@kodemartin kodemartin left a comment

Choose a reason for hiding this comment

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

Added the test with 60c8182. Ran it locally, so once the CI passes this is good to go

@muXxer
Copy link
Contributor

muXxer commented Nov 21, 2024

After an in person discussion with @miker83z we decided to investigate first how this information is fetched via graphql RPC, before we reintroduce these mappings in the database just for the json-rpc. Maybe there is a way without using the table.

@semenov-vladyslav
Copy link
Contributor

Graphql uses iota-indexer to process queries, it is the job of iota-indexer to maintain the DB, iota-indexer runs iota-node with rest_api enabled which in turn enables AuthorityState::rest_index , which maintains its own DB that stores the proper table. It's possible to fetch checkpoint from a DB maintained by rest_index, but it is not always enabled, its type is Option<Arc<RestIndexStore>> and is enabled from config flag.

@kodemartin
Copy link
Contributor

kodemartin commented Nov 21, 2024

After an in person discussion with @miker83z we decided to investigate first how this information is fetched via graphql RPC, before we reintroduce these mappings in the database just for the json-rpc. Maybe there is a way without using the table.

GraphQL uses the indexer database, as does the Indexer RPC that return the expected information. The node tables do not enter the equation.

@semenov-vladyslav semenov-vladyslav force-pushed the core-node/feat/revert-authority-store-pr branch from 60c8182 to f26162b Compare November 22, 2024 10:39
@bingyanglin bingyanglin merged commit 29eda32 into develop Nov 26, 2024
38 of 39 checks passed
@bingyanglin bingyanglin deleted the core-node/feat/revert-authority-store-pr branch November 26, 2024 13:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
node Issues related to the Core Node team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[iota-core]: AuthorityStore::multi_get_transaction_checkpoint returns only current epoch checkpoints
4 participants