[Data Service] Implement simple upstream transaction filtering #13699
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
⏱️ 4h 47m total CI duration on this PR
🚨 2 jobs on the last run were significantly faster/slower than expected
|
banool
force-pushed
the
banool/data-service-filters
branch
2 times, most recently
from
1b965b2 to
bb5a4e7
Compare
banool
requested review from
CapCap,
bowenyang007,
just-in-chang,
larry-aptos and
rtso
banool
force-pushed
the
banool/data-service-filters
branch
2 times, most recently
from
e10b908 to
d73b44f
Compare
banool
force-pushed
the
banool/data-service-filters
branch
from
d73b44f to
dd22f7a
Compare
rtso
approved these changes
Jun 20, 2024
| // 2. Push the data to the response channel, i.e. stream the data to the client. | ||
| let current_batch_size = transaction_data.as_slice().len(); | ||
| let end_of_batch_version = transaction_data.as_slice().last().unwrap().version; | ||
| let resp_items = get_transactions_responses_builder( |
Contributor
There was a problem hiding this comment.
[Re: line 524]
Should we have some metrics to measure the filtering, like "bytes after filtering" or "number of transactions filtered"?
See this comment inline on Graphite.
Should we have some metrics to measure the filtering, like "bytes after filtering" or "number of transactions filtered"?
See this comment inline on Graphite.
Contributor
Author
There was a problem hiding this comment.
Great call, done for both!
banool
force-pushed
the
max_aptos-transaction-filter
branch
2 times, most recently
from
ce4b9be to
8af264b
Compare
banool
force-pushed
the
banool/data-service-filters
branch
from
dd22f7a to
1148dc7
Compare
banool
force-pushed
the
max_aptos-transaction-filter
branch
4 times, most recently
from
8482bb7 to
7815ce6
Compare
There are two types of transaction filtering we will support in the future: 1. Per stream configuration: The downstream declares what txns they want to receive. 2. Global configuration: At the data service level we refuse to include full txns for all streams. This PR implements the second of these, using @CapCap's work here: aptos-labs/aptos-indexer-processors#398. Rather than not sending txns at all if they match the blocklist filters, we just omit the writesets and events. Not sending the txns entirely would cause issues with processors, which today assume that they will receive all txns.
banool
force-pushed
the
banool/data-service-filters
branch
from
1148dc7 to
05aa06d
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Contributor
✅ Forge suite
|
Contributor
✅ Forge suite
|
Contributor
✅ Forge suite
|
yuunlimm
reviewed
Jun 21, 2024
Comment on lines
+987
to
+989
| // Note: `is_allowed` means the txn matches the filter, in which case | ||
| // we strip it. | ||
| if txns_to_strip_filter.is_allowed(&txn) { |
Contributor
There was a problem hiding this comment.
nit: is_allowed is a bit confusing, maybe we can rename later?
yuunlimm
approved these changes
Jun 21, 2024
rtso
pushed a commit
that referenced
this pull request
Jun 24, 2024
There are two types of transaction filtering we will support in the future: 1. Per stream configuration: The downstream declares what txns they want to receive. 2. Global configuration: At the data service level we refuse to include full txns for all streams. This PR implements the second of these, using @CapCap's work here: aptos-labs/aptos-indexer-processors#398. Rather than not sending txns at all if they match the blocklist filters, we just omit the writesets and events. Not sending the txns entirely would cause issues with processors, which today assume that they will receive all txns.
rtso
pushed a commit
that referenced
this pull request
Jun 24, 2024
There are two types of transaction filtering we will support in the future: 1. Per stream configuration: The downstream declares what txns they want to receive. 2. Global configuration: At the data service level we refuse to include full txns for all streams. This PR implements the second of these, using @CapCap's work here: aptos-labs/aptos-indexer-processors#398. Rather than not sending txns at all if they match the blocklist filters, we just omit the writesets and events. Not sending the txns entirely would cause issues with processors, which today assume that they will receive all txns.
rtso
pushed a commit
that referenced
this pull request
Jun 24, 2024
There are two types of transaction filtering we will support in the future: 1. Per stream configuration: The downstream declares what txns they want to receive. 2. Global configuration: At the data service level we refuse to include full txns for all streams. This PR implements the second of these, using @CapCap's work here: aptos-labs/aptos-indexer-processors#398. Rather than not sending txns at all if they match the blocklist filters, we just omit the writesets and events. Not sending the txns entirely would cause issues with processors, which today assume that they will receive all txns.
rtso
added a commit
that referenced
this pull request
Jun 24, 2024
* [GRPC] Enable data service ZSTD and update crate that uses old tonic (#13621) * replace println with tracing * [GRPC] Simple Transaction Filtering * Improve transaction filter comments, exports, README, fix lz4 in tests * [Data Service] Implement simple upstream transaction filtering (#13699) There are two types of transaction filtering we will support in the future: 1. Per stream configuration: The downstream declares what txns they want to receive. 2. Global configuration: At the data service level we refuse to include full txns for all streams. This PR implements the second of these, using @CapCap's work here: aptos-labs/aptos-indexer-processors#398. Rather than not sending txns at all if they match the blocklist filters, we just omit the writesets and events. Not sending the txns entirely would cause issues with processors, which today assume that they will receive all txns. --------- Co-authored-by: Max Kaplan <[email protected]> Co-authored-by: yuunlimm <[email protected]> Co-authored-by: CapCap <[email protected]> Co-authored-by: Daniel Porteous <[email protected]>
rtso
pushed a commit
that referenced
this pull request
Jun 24, 2024
There are two types of transaction filtering we will support in the future: 1. Per stream configuration: The downstream declares what txns they want to receive. 2. Global configuration: At the data service level we refuse to include full txns for all streams. This PR implements the second of these, using @CapCap's work here: aptos-labs/aptos-indexer-processors#398. Rather than not sending txns at all if they match the blocklist filters, we just omit the writesets and events. Not sending the txns entirely would cause issues with processors, which today assume that they will receive all txns.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
There are two types of transaction filtering we will support in the future:
This PR implements the second of these, using @CapCap's work here:
aptos-labs/aptos-indexer-processors#398#13715.Rather than not sending txns at all if they match the blocklist filters, we just omit the writesets and events (and payload + signature, like we did with
sender_addresses_to_ignore). Not sending the txns entirely would cause issues with processors, which today assume that they will receive all txns.The txn filtering stuff is a superset of the existing
sender_addresses_to_ignorefunctionality, so I remove that in this PR. I will include how to expresssender_addresses_to_ignoreusing the txn filter stuff in the runbook.Type of Change
Which Components or Systems Does This Change Impact?
How Has This Been Tested?
See updated unit test. We'll try this out in devnet first and release as per the release strategy.
Key Areas to Review
Make sure the configuration approach seems ergonomic.
Checklist