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

[Indexer] Add transactions by address indexing #9076

Merged
merged 3 commits into from
Jul 20, 2023
Merged

Conversation

bowenyang007
Copy link
Contributor

@bowenyang007 bowenyang007 commented Jul 13, 2023

Description

It's nice to have a dedicated table to get all transactions that touch an address instead of a view. I'm adding this to the coin_processor because the coin processor already needs to process every transaction. We don't want this to be in default processor currently because we've had instances where default processor takes forever to run migrations and it stalls explorer and wallet.

What qualifies as a transaction associated with an address:

  1. account signed or is part of a multi sig / multi agent
  2. account resources / events are directly modified
  3. account owns an object where resources / events are modified (only 1 degree)

I called out a bunch of TODOs here. Notably what we're currently unable to do:

  • Recursively find owner of an object
  • Find an owner of an object that was deleted
  • Find owner of a table, recursively or not

P.S. This PR also fixes a bug where we're not actually recording object deletes.

Test Plan

image

@bowenyang007 bowenyang007 changed the base branch from main to fungible_tokens July 13, 2023 07:39
@bowenyang007 bowenyang007 requested review from junkil-park, kent-white, movekevin and larry-aptos and removed request for msmouse and saharct July 13, 2023 07:42
@bowenyang007 bowenyang007 added CICD:build-images when this label is present github actions will start build+push rust images from the PR. CICD:build-indexer-images labels Jul 13, 2023
@bowenyang007 bowenyang007 force-pushed the fungible_tokens branch 3 times, most recently from 24a39cf to 9157f6f Compare July 14, 2023 04:26
@bowenyang007 bowenyang007 force-pushed the address_transactions branch from 5a0e9c3 to 56eecdf Compare July 14, 2023 04:33
Base automatically changed from fungible_tokens to main July 14, 2023 05:04
@bowenyang007 bowenyang007 force-pushed the address_transactions branch from 8ddaa67 to 204191b Compare July 14, 2023 05:09
pub fn from_transaction(
transaction: &Transaction,
) -> anyhow::Result<HashMap<AccountTransactionPK, Self>> {
let (events, wscs, signatures, txn_version) = match transaction {
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like a fair amount of common code. Is there a common library to call to do this?

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 will need to be cleaned up but for now I don't have time to refactor all that code.

Copy link
Contributor

Choose a reason for hiding this comment

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

Gotcha. Just the OCD me saying :P

@@ -120,35 +108,42 @@ impl Object {
/// This should never really happen since it's very difficult to delete the entire resource group
Copy link
Contributor

@movekevin movekevin Jul 15, 2023

Choose a reason for hiding this comment

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

This is not true. Unnamed token objects can be burnt and deleted very thoroughly. For most use cases using aptos_token, we'll see clean deletions so let's make sure we handle this case well (can be in a separate PR though).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea I've seen this already so I'll change the comments.

@@ -157,4 +152,44 @@ impl Object {
Ok(None)
}
}

/// This is actually not great because object owner can change. The best we can do now though
fn get_object_owner(
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if this can lead to some performance/failure issue for the processor if the data is stored in remote databases (e.g. in Hive). If a processor has to query the object owner multiple times, this means multiple remote calls

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't delete that often so it should be fine for now.

retried += 1;
match CurrentObjectQuery::get_by_address(object_address, conn) {
Ok(res) => {
return Ok(CurrentObject {
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought we also planned to do this for table items?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did you read the summary lol. We can't do this for table items performantly.

@sionescu sionescu requested a review from a team as a code owner July 18, 2023 17:17
@bowenyang007 bowenyang007 force-pushed the address_transactions branch 2 times, most recently from fa96bee to cfbe271 Compare July 18, 2023 20:14
larry-aptos added a commit to aptos-labs/aptos-indexer-processors that referenced this pull request Jul 19, 2023
@bowenyang007 bowenyang007 enabled auto-merge (squash) July 19, 2023 18:00
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

/// We will consider all transactions that modify a resource or event associated with a particular account.
/// We will do 1 level of redirection for now (e.g. if it's an object, we will record the owner as account address).
/// We will also consider transactions that the account signed or is part of a multi sig / multi agent.
/// TODO: recursively find the parent account of an object
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be done in post-processing instead of in the processor?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we might not have a choice lol.

) -> anyhow::Result<Option<(Self, CurrentObject)>> {
if delete_resource.resource.to_string() == "0x1::object::ObjectCore" {
if delete_resource.resource.to_string() == "0x1::object::ObjectGroup" {
Copy link
Contributor

Choose a reason for hiding this comment

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

What's object group and why do we need to change this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the old thing wasn't working hahaha. I assuemd it would be ObjectCore but actually when it's deleted it's ObjectGroup. It was just buggy before.

Copy link
Contributor

Choose a reason for hiding this comment

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

that's weird that the resource type is different for writes and deletes.

@@ -524,12 +526,54 @@ impl TransactionProcessor for DefaultTransactionProcessor {
}

// TODO, merge this loop with above
// Moving object handling here because we need a single object
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the difference between this new way and the old way of getting the objects? They look like they're doing the same thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are. If you notice though that I'm passing all_objects into the from_delete_resource function. That's the main difference. I mean I could've made it work as before as well but the right thing to do is to use a general forloop to avoid the duplicate work so I just refactored this at the same time.

@bowenyang007 bowenyang007 force-pushed the address_transactions branch from 5ed6317 to a302422 Compare July 19, 2023 20:23
auto-merge was automatically disabled July 20, 2023 04:04

Pull request was closed

@bowenyang007 bowenyang007 force-pushed the address_transactions branch from a302422 to 0cdd064 Compare July 20, 2023 04:04
@bowenyang007 bowenyang007 removed CICD:build-images when this label is present github actions will start build+push rust images from the PR. CICD:build-indexer-images labels Jul 20, 2023
@bowenyang007 bowenyang007 reopened this Jul 20, 2023
@bowenyang007 bowenyang007 enabled auto-merge (squash) July 20, 2023 04:27
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions
Copy link
Contributor

✅ Forge suite compat success on aptos-node-v1.5.1 ==> 4839a2081aa8bf5a997472283c0bd35e5ba99560

Compatibility test results for aptos-node-v1.5.1 ==> 4839a2081aa8bf5a997472283c0bd35e5ba99560 (PR)
1. Check liveness of validators at old version: aptos-node-v1.5.1
compatibility::simple-validator-upgrade::liveness-check : committed: 4627 txn/s, latency: 6907 ms, (p50: 7100 ms, p90: 9600 ms, p99: 12300 ms), latency samples: 175860
2. Upgrading first Validator to new version: 4839a2081aa8bf5a997472283c0bd35e5ba99560
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 1873 txn/s, latency: 15487 ms, (p50: 19100 ms, p90: 21900 ms, p99: 22300 ms), latency samples: 91800
3. Upgrading rest of first batch to new version: 4839a2081aa8bf5a997472283c0bd35e5ba99560
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 1849 txn/s, latency: 15609 ms, (p50: 18800 ms, p90: 21900 ms, p99: 22300 ms), latency samples: 92460
4. upgrading second batch to new version: 4839a2081aa8bf5a997472283c0bd35e5ba99560
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 3222 txn/s, latency: 9303 ms, (p50: 10100 ms, p90: 12600 ms, p99: 13200 ms), latency samples: 135340
5. check swarm health
Compatibility test for aptos-node-v1.5.1 ==> 4839a2081aa8bf5a997472283c0bd35e5ba99560 passed
Test Ok

@github-actions
Copy link
Contributor

✅ Forge suite realistic_env_max_load success on 4839a2081aa8bf5a997472283c0bd35e5ba99560

two traffics test: inner traffic : committed: 6072 txn/s, latency: 6431 ms, (p50: 6000 ms, p90: 8700 ms, p99: 14700 ms), latency samples: 2635660
two traffics test : committed: 100 txn/s, latency: 3138 ms, (p50: 3100 ms, p90: 3800 ms, p99: 4500 ms), latency samples: 1860
Max round gap was 1 [limit 4] at version 1252087. Max no progress secs was 5.090366 [limit 10] at version 1252087.
Test Ok

@github-actions
Copy link
Contributor

✅ Forge suite framework_upgrade success on aptos-node-v1.5.1 ==> 4839a2081aa8bf5a997472283c0bd35e5ba99560

Compatibility test results for aptos-node-v1.5.1 ==> 4839a2081aa8bf5a997472283c0bd35e5ba99560 (PR)
Upgrade the nodes to version: 4839a2081aa8bf5a997472283c0bd35e5ba99560
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 4594 txn/s, latency: 7185 ms, (p50: 7500 ms, p90: 9900 ms, p99: 13000 ms), latency samples: 169980
5. check swarm health
Compatibility test for aptos-node-v1.5.1 ==> 4839a2081aa8bf5a997472283c0bd35e5ba99560 passed
Test Ok

@bowenyang007 bowenyang007 merged commit f49a1dc into main Jul 20, 2023
@bowenyang007 bowenyang007 deleted the address_transactions branch July 20, 2023 05:06
ngkuru pushed a commit that referenced this pull request Jul 20, 2023
* make changes to old indexer

* apply change to grpc processors

* remove unecessary log
ngkuru pushed a commit that referenced this pull request Jul 20, 2023
* make changes to old indexer

* apply change to grpc processors

* remove unecessary log
gedigi pushed a commit that referenced this pull request Aug 2, 2023
* make changes to old indexer

* apply change to grpc processors

* remove unecessary log
xbtmatt pushed a commit that referenced this pull request Aug 13, 2023
* make changes to old indexer

* apply change to grpc processors

* remove unecessary log
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.

4 participants