[GRPC] Simple Transaction Filtering #13715

CapCap · 2024-06-14T23:46:28Z

Description

an MVP version of the transaction filtering crate, to performantly filter out API streams of transactions

This PR: aptos-labs/aptos-indexer-processors#398 but in aptos-core as otherwise there are dependency issues

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: aptos-labs/aptos-indexer-processors#392

Type of Change

Which Components or Systems Does This Change Impact?

How Has This Been Tested?

Key Areas to Review

Checklist

I have read and followed the CONTRIBUTING doc
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I identified and added all stakeholders and component owners affected by this change as reviewers
I tested both happy and unhappy path of the functionality
I have made corresponding changes to the documentation

trunk-io · 2024-06-14T23:46:32Z

⏱️ 34h 28m total CI duration on this PR

Job	Cumulative Duration	Recent Runs
rust-targeted-unit-tests	4h 46m	🟥 🟥 🟥 ⬜ 🟩 (+8 more)
rust-smoke-tests	4h 45m	🟩 🟩 🟩 ⬜ 🟩 (+4 more)
test-fuzzers	4h 8m	🟩 🟩 🟩 🟩 🟩 (+2 more)
execution-performance / single-node-performance	3h 36m	🟩 🟩 🟩 🟩 🟩 (+4 more)
rust-move-tests	2h 39m	🟩 🟩 🟩 ⬜ 🟩 (+9 more)
forge-framework-upgrade-test / forge	2h 8m	🟩 🟩 🟩 🟩 🟩 (+3 more)
rust-images / rust-all	2h 3m	🟩 🟩 🟩 ⬜ 🟩 (+4 more)
forge-e2e-test / forge	1h 53m	🟩 🟩 🟩 🟩 🟩 (+3 more)
forge-compat-test / forge	1h 40m	🟩 🟩 🟩 🟩 🟩 (+3 more)
rust-lints	1h 21m	🟩 🟩 🟩 🟥 🟩 (+8 more)
run-tests-main-branch	59m	🟩 🟩 🟩 🟩 🟩 (+8 more)
cli-e2e-tests / run-cli-tests	51m	🟩 🟩 🟩 🟩 🟩 (+3 more)
rust-build-cached-packages	47m	🟩 🟩 🟩 🟩 🟩 (+4 more)
test-target-determinator	39m	🟩 🟩 🟩 🟩 🟩 (+4 more)
execution-performance / test-target-determinator	37m	🟩 🟩 🟩 🟩 🟩 (+4 more)
check	33m	🟩 🟩 🟩 🟩 🟩 (+4 more)
general-lints	25m	🟩 🟩 🟩 🟩 🟩 (+8 more)
check-dynamic-deps	15m	🟩 🟩 🟩 🟩 🟩 (+9 more)
node-api-compatibility-tests / node-api-compatibility-tests	9m	🟩 🟩 🟩 🟩 🟩 (+3 more)
semgrep/ci	6m	🟩 🟩 🟩 🟩 🟩 (+9 more)
file_change_determinator	3m	🟩 🟩 🟩 🟩 🟩 (+9 more)
file_change_determinator	2m	🟩 🟩 🟩 🟩 🟩 (+9 more)
file_change_determinator	2m	🟩 🟩 🟩 🟩 🟩 (+4 more)
permission-check	53s	🟩 🟩 🟩 🟩 🟩 (+9 more)
permission-check	47s	🟩 🟩 🟩 🟩 🟩 (+9 more)
permission-check	45s	🟩 🟩 🟩 🟩 🟩 (+9 more)
permission-check	42s	🟩 🟩 🟩 🟩 🟩 (+9 more)
permission-check	34s	🟩 🟩 🟩 🟩 🟩 (+4 more)
determine-docker-build-metadata	29s	🟩 🟩 🟩 🟩 🟩 (+4 more)

🚨 3 jobs on the last run were significantly faster/slower than expected

Job	Duration	vs 7d avg
rust-targeted-unit-tests	24m	13m
forge-framework-upgrade-test / forge	18m	13m
execution-performance / test-target-determinator	3m	5m

_{settings ⋅ feedback ⋅ docs ⋅ learn more about trunk.io}

banool

Just a few extra comments, I left most of my initial feedback in #13715.

banool · 2024-06-19T17:55:52Z

Cargo.toml

@@ -619,7 +622,7 @@ libfuzzer-sys = "0.4.6"
 libsecp256k1 = "0.7.0"
 log = "0.4.17"
 lru = "0.7.5"
-lz4 = "1.24.0"
+lz4 = "1.25.0"


I'm assuming this doesn't cause issues with anything else in aptos-core?

I looked at the changelog, seems fine.

banool · 2024-06-19T17:58:04Z

ecosystem/indexer-grpc/transaction-filter/src/filters/user_transaction.rs

+#[serde(deny_unknown_fields)]
+#[derive(derive_builder::Builder)]
+#[builder(setter(strip_option), default)]
+pub struct UserTransactionFilter {


I just took this library for a spin and immediately tried to do things like UserTransactionFilter::builder() and UserTransactionFilter::sender(), before ultimately looking at the derive_builder docs and realizing I want UserTransactionFilterBuilder.

Two things:

Could we maybe mention this in the doc comments?

Maybe something less heavyweight / more obvious would be to add a builder method as the first method for all of these structs that just returns a new ThingBuilder. Maybe derive_builder can even help with that.

banool · 2024-06-19T17:59:49Z

ecosystem/indexer-grpc/transaction-filter/src/lib.rs

+pub mod filters;
+pub mod traits;
+
+// re-export for convenience


In practice I had to do these imports too:

use aptos_transaction_filter::{ boolean_transaction_filter::APIFilter, filters::UserTransactionFilterBuilder, };

Maybe more should be exposed at the top level by default. I doubt we'll see name clashes.

banool · 2024-06-19T18:01:18Z

ecosystem/indexer-grpc/transaction-filter/src/traits.rs

+    /**
+     * This is a convenience method to allow for the error to be annotated with the filter type/path at each level
+     * This is the public API for checking the validity of a filter!
+     * Example output looks like:
+     * ```text
+     *   FilterError: This is a test error!.
+     *   Trace Path:
+     *   transaction_filter::traits::test::InnerStruct:   {"a":"test"}
+     *   core::option::Option<transaction_filter::traits::test::InnerStruct>:   {"a":"test"}
+     *   transaction_filter::traits::test::OuterStruct:   {"inner":{"a":"test"}}
+     *  ```
+     **/


Needs to be /// for it to be a doc comment. Maybe intentional that it isn't one? Seems like it should be though.

banool · 2024-06-19T18:02:29Z

ecosystem/indexer-grpc/transaction-filter/src/traits.rs

+        let res = outer.is_valid();
+        assert!(res.is_err());
+        let error = res.unwrap_err();
+        assert_eq!(error.to_string(), "Filter Error: This is a test error!\nTrace Path:\naptos_transaction_filter::traits::test::InnerStruct:   {\"a\":\"test\"}\ncore::option::Option<aptos_transaction_filter::traits::test::InnerStruct>:   {\"a\":\"test\"}\naptos_transaction_filter::traits::test::OuterStruct:   {\"inner\":{\"a\":\"test\"}}");


Nit, to make this more readable can we use a raw string or indoc or something for the assertion?

banool · 2024-06-19T18:03:23Z

ecosystem/indexer-grpc/transaction-filter/README.md

+
+Or, if you prefer, as yaml:
+
+```yaml


Now that this is using just 0.8.x of serde_yaml does this still work? I assume so, looks fine, just calling it out in case.

rtso · 2024-06-20T18:47:29Z

Moving my approval over here from aptos-labs/aptos-indexer-processors#398

banool · 2024-06-21T10:11:27Z

Seeing this error in the tests in CI but not locally (with both normal cargo test and cargo nextest), investigating:

thread 'boolean_transaction_filter::test::test_query_parsing' panicked at ecosystem/indexer-grpc/transaction-filter/src/test_lib/mod.rs:13:10:
Lz4 decompression failed.: Custom { kind: Other, error: LZ4Error("ERROR_frameType_unknown") }
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Update: I see this PR updates lz4 to 1.25 whereas in the original repo and here it is 1.24. I'll try going back to 1.24.

Update: No cigar, debugging further here: https://aptos-org.slack.com/archives/C04PF1X2UKY/p1718969817705389.

github-actions · 2024-06-21T19:26:13Z

✅ Forge suite `compat` success on `f648076a280621dbfd4e73b1ca83e3a3f52878ed` ==> `7815ce66262cf8c8aea068a7a946c81f12e3c194`

Compatibility test results for f648076a280621dbfd4e73b1ca83e3a3f52878ed ==> 7815ce66262cf8c8aea068a7a946c81f12e3c194 (PR)
1. Check liveness of validators at old version: f648076a280621dbfd4e73b1ca83e3a3f52878ed
compatibility::simple-validator-upgrade::liveness-check : committed: 6380.833218447245 txn/s, latency: 4968.537102734664 ms, (p50: 3700 ms, p90: 7200 ms, p99: 21100 ms), latency samples: 243540
2. Upgrading first Validator to new version: 7815ce66262cf8c8aea068a7a946c81f12e3c194
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 3041.2986503720504 txn/s, latency: 10248.077649952396 ms, (p50: 9900 ms, p90: 14000 ms, p99: 14200 ms), latency samples: 126040
3. Upgrading rest of first batch to new version: 7815ce66262cf8c8aea068a7a946c81f12e3c194
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 3352.3550846367266 txn/s, latency: 9338.037786259541 ms, (p50: 9400 ms, p90: 13900 ms, p99: 14400 ms), latency samples: 138860
4. upgrading second batch to new version: 7815ce66262cf8c8aea068a7a946c81f12e3c194
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 4567.644934348592 txn/s, latency: 7127.8354590007175 ms, (p50: 6900 ms, p90: 11700 ms, p99: 15400 ms), latency samples: 167320
5. check swarm health
Compatibility test for f648076a280621dbfd4e73b1ca83e3a3f52878ed ==> 7815ce66262cf8c8aea068a7a946c81f12e3c194 passed
Test Ok

github-actions · 2024-06-21T19:28:37Z

✅ Forge suite `realistic_env_max_load` success on `7815ce66262cf8c8aea068a7a946c81f12e3c194`

two traffics test: inner traffic : committed: 8560.836349155526 txn/s, latency: 4583.0977881087165 ms, (p50: 4400 ms, p90: 5700 ms, p99: 10500 ms), latency samples: 3695480
two traffics test : committed: 99.96357407620748 txn/s, latency: 2105.3232558139534 ms, (p50: 2000 ms, p90: 2500 ms, p99: 4900 ms), latency samples: 1720
Latency breakdown for phase 0: ["QsBatchToPos: max: 0.218, avg: 0.214", "QsPosToProposal: max: 0.253, avg: 0.243", "ConsensusProposalToOrdered: max: 0.306, avg: 0.289", "ConsensusOrderedToCommit: max: 0.362, avg: 0.353", "ConsensusProposalToCommit: max: 0.650, avg: 0.642"]
Max round gap was 1 [limit 4] at version 887356. Max no progress secs was 5.223288 [limit 15] at version 887356.
Test Ok

github-actions · 2024-06-21T19:31:44Z

✅ Forge suite `framework_upgrade` success on `f648076a280621dbfd4e73b1ca83e3a3f52878ed` ==> `7815ce66262cf8c8aea068a7a946c81f12e3c194`

Compatibility test results for f648076a280621dbfd4e73b1ca83e3a3f52878ed ==> 7815ce66262cf8c8aea068a7a946c81f12e3c194 (PR)
Upgrade the nodes to version: 7815ce66262cf8c8aea068a7a946c81f12e3c194
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 805.8213486186796 txn/s, submitted: 806.4880513340288 txn/s, failed submission: 0.6667027153491558 txn/s, expired: 0.6667027153491558 txn/s, latency: 3720.6169194704908 ms, (p50: 2900 ms, p90: 7200 ms, p99: 10000 ms), latency samples: 72520
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 869.2011523456093 txn/s, submitted: 871.2789240245231 txn/s, failed submission: 2.077771678913807 txn/s, expired: 2.077771678913807 txn/s, latency: 3715.3979150066402 ms, (p50: 1900 ms, p90: 8100 ms, p99: 15100 ms), latency samples: 75300
5. check swarm health
Compatibility test for f648076a280621dbfd4e73b1ca83e3a3f52878ed ==> 7815ce66262cf8c8aea068a7a946c81f12e3c194 passed
Upgrade the remaining nodes to version: 7815ce66262cf8c8aea068a7a946c81f12e3c194
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 897.6780824617266 txn/s, submitted: 899.5526386245284 txn/s, failed submission: 1.8745561628018304 txn/s, expired: 1.8745561628018304 txn/s, latency: 3767.7312190028715 ms, (p50: 2400 ms, p90: 7500 ms, p99: 13300 ms), latency samples: 76620
Test Ok

CapCap requested a review from a team June 14, 2024 23:46

CapCap force-pushed the max_aptos-transaction-filter branch 3 times, most recently from ade83ae to 1c4d87c Compare June 15, 2024 05:25

CapCap enabled auto-merge (squash) June 15, 2024 05:25