Simplest MVP transaction filtering #398

CapCap · 2024-06-11T18:57:44Z

an MVP version of the transaction filtering crate.

Performance

On a random 3mb proto of 1000txns during taptos, running the "everything" filter (looped) gives:

BENCH: Took 13.225167ms for 1000000 transactions (13ns each)

Random 10mb proto from April 25th with "everything" filter:

BENCH: Took 21.815375ms for 1000000 transactions (21ns each)

Graffio 100mb proto ("everything" filter, but with or):

BENCH: Took 31.559333ms for 1000000 transactions (31ns each)

More advanced feature complete version here: #392

rust/transaction-filter/src/filter_operator.rs

rtso

lgtm, just a small comment about the not

bowenyang007 · 2024-06-11T19:56:44Z

How do we plan to share this crate with aptos-core?

CapCap · 2024-06-11T19:57:38Z

@bowenyang007 sorry I don't understand the question

bowenyang007 · 2024-06-11T20:01:43Z

Data service needs to use this right? I'm curious if we're going to copy over the code or upload it to crates.io.

A few additional questions:

Can we add a readme? Particularly I'm not sure from the PR what exactly is being filtered or how it's meant to be used.
How are the proto fixtures supposed to work? How do we upgrade the proto?
Can we have comments?

CapCap · 2024-06-11T20:03:42Z

@bowenyang007 I figured we'd do it the way we do everything else, i.e;

transaction-filter = { git = "https://github.com/aptos-labs/aptos-indexer-processors.git", rev = "4541add3fd29826ec57f22658ca286d2d6134b93" }

I can move stuff to the readme and add an example, sure

banool

Lots of small things, mostly around how the builders work in certain cases. Overall structure still looks great, molto benne.

rust/Cargo.toml

rust/processor/src/grpc_stream.rs

rust/transaction-filter/Cargo.toml

rust/transaction-filter/README.md

banool · 2024-06-13T08:56:22Z

rust/transaction-filter/src/filters/event.rs

+        if !self.data.is_allowed(&item.data) {
+            return false;
+        }
+
+        true


Nit, could be collapsed to just returning self.data.is_allowed(&item.data) directly.

It could! I left it as-is to make the eventual merge of advanced filtering a bit easier haha, but I can change this too

I don't mind so much, explicit is good also.

rust/transaction-filter/src/filters/event.rs

banool · 2024-06-13T08:58:45Z

rust/transaction-filter/src/filters/move_module.rs

+#[derive(Clone, Debug, Default, Deserialize, PartialEq, Serialize)]
+#[serde(deny_unknown_fields)]
+#[derive(derive_builder::Builder)]
+#[builder(setter(into, strip_option), default)]


I wonder if default is a footgun in this case, it might be best to have people explicitly set (even to None) all the fields for this one? I can see people forgetting to set address and only setting module for example. Not too opinionated, up to you.

Same comment for the entry function filter and the like.

AFAIR the issue is builder requires default unfortunately.

I'm not an expert in the library though and there are a lot of config options; the biggest thing I wish we could do is call the validation function when calling "build", and have that return our error type- maybe there is, maybe I can do it without a lot of boiler plate, I can take a look

banool · 2024-06-13T09:02:39Z

rust/transaction-filter/src/filters/user_transaction.rs

+                if !(self.address.is_allowed(&module.address)
+                    && self.function.is_allowed(&module.name))


Not 100% sure I'm understanding this one right. In this block, address or function could be None. I believe None by default resolves to false for is_allowed based on the trait below. Which I don't think is what we want here, if the user hasn't set a value for address then we shouldn't evaluate it as a filter?

Is there a unit test that explicitly checks this case where only one of address or function is set that can put my mind at ease hahah.

Ah I get it, is_allowed if false if the value is None but true if the filter is None, perfect. In which case, I suppose if self.address.is_some() || self.function.is_some() is just an optimization, we don't actually need it?

Exactly- we don't explicitly need it, it's just an optimization to save on some ops if we don't need it

banool · 2024-06-13T09:09:00Z

rust/transaction-filter/src/filters/user_transaction.rs

+#[derive(Clone, Debug, Default, Deserialize, PartialEq, Serialize)]
+#[serde(deny_unknown_fields)]
+#[derive(derive_builder::Builder)]
+#[builder(setter(strip_option), default)]


I figure this shouldn't be default, because there is only one field so you must set it otherwise it'll always be invalid.

It's required for the builder derive to allow partial completions- otherwise if you only set one field, and even though the overall builder is valid, it yells at you for fields being unset

I think there is some improvements we can make with it, I tried not to get too deep into it to avoid this PRs continued growth hahaha

rust/Cargo.toml

@CapCap

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.

@CapCap

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.

@CapCap

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.

@CapCap

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.

@CapCap

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.

@CapCap

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.

@CapCap

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.

@CapCap

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.

@CapCap

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.

@CapCap

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.

@CapCap

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.

@CapCap

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.

@CapCap

* [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]>

@CapCap

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.

CapCap requested a review from a team June 11, 2024 19:12

rtso reviewed Jun 11, 2024

View reviewed changes

rust/transaction-filter/src/filter_operator.rs Outdated Show resolved Hide resolved

rtso approved these changes Jun 11, 2024

View reviewed changes

bowenyang007 requested a review from banool June 11, 2024 19:57

CapCap force-pushed the simplest_transaction_filter branch 2 times, most recently from a28c57c to 13a63d8 Compare June 13, 2024 05:17

CapCap requested a review from a team June 13, 2024 05:34

banool requested changes Jun 13, 2024

View reviewed changes

banool reviewed Jun 14, 2024

View reviewed changes

rust/Cargo.toml Show resolved Hide resolved

banool mentioned this pull request Jun 14, 2024

[Data Service] Implement simple upstream transaction filtering aptos-labs/aptos-core#13699

Merged

21 tasks

CapCap force-pushed the simplest_transaction_filter branch 6 times, most recently from f1d3f28 to 8f62517 Compare June 14, 2024 23:32

CapCap mentioned this pull request Jun 14, 2024

[GRPC] Simple Transaction Filtering aptos-labs/aptos-core#13715

Merged

21 tasks

banool force-pushed the simplest_transaction_filter branch from 8f62517 to 020a761 Compare June 21, 2024 12:06

Simplest MVP transaction filtering

5fc070b

banool force-pushed the simplest_transaction_filter branch from 020a761 to 5fc070b Compare June 21, 2024 12:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplest MVP transaction filtering #398

Simplest MVP transaction filtering #398

CapCap commented Jun 11, 2024 •

edited

Loading

rtso left a comment

bowenyang007 commented Jun 11, 2024

CapCap commented Jun 11, 2024

bowenyang007 commented Jun 11, 2024

CapCap commented Jun 11, 2024 •

edited

Loading

banool left a comment

banool Jun 13, 2024

CapCap Jun 13, 2024

banool Jun 14, 2024

banool Jun 13, 2024

CapCap Jun 13, 2024

banool Jun 13, 2024

banool Jun 13, 2024

CapCap Jun 13, 2024

banool Jun 13, 2024

CapCap Jun 13, 2024

		if !(self.address.is_allowed(&module.address)
		&& self.function.is_allowed(&module.name))

Simplest MVP transaction filtering #398

Are you sure you want to change the base?

Simplest MVP transaction filtering #398

Conversation

CapCap commented Jun 11, 2024 • edited Loading

Performance

rtso left a comment

Choose a reason for hiding this comment

bowenyang007 commented Jun 11, 2024

CapCap commented Jun 11, 2024

bowenyang007 commented Jun 11, 2024

CapCap commented Jun 11, 2024 • edited Loading

banool left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CapCap commented Jun 11, 2024 •

edited

Loading

CapCap commented Jun 11, 2024 •

edited

Loading