feat(seismic): add opt-in ECSD address screening#331
feat(seismic): add opt-in ECSD address screening#331aegis-cipherowl wants to merge 4 commits intoSeismicSystems:seismicfrom
Conversation
This commit introduces a new `ScreeningArgs` struct for configuring address screening in the Seismic node. The `SeismicNode` struct is updated to optionally include screening arguments, allowing for integration with an ECSD sidecar for address validation. The CLI is also modified to accept these new arguments, enhancing the node's capabilities for address screening during transaction processing. Additionally, various dependencies are updated in the Cargo files to support these changes.
Resolves all CI check failures in PR SeismicSystems#331: 1. **rustfmt**: Reformatted .expect() call in node.rs to multi-line format per nightly rustfmt preferences 2. **protoc dependency**: Added protobuf-compiler installation to 5 CI jobs (warnings, clippy, unit-test, integration-test, viem) required by tonic-build for proto compilation 3. **clippy expect_used**: Added #[allow(clippy::expect_used)] annotations with justification: - calldata.rs: array length checked immediately before - node.rs: fail_mode validated by clap value_parser All .expect() calls are justified and fail-fast is intentional.
Eliminates the protobuf-compiler (protoc) dependency by checking in the generated gRPC client/server code, simplifying the build process and CI configuration. Changes: - Add crates/seismic/txpool/src/screening/proto.rs (407 lines) Generated code from ecsd.proto with regeneration instructions - Remove crates/seismic/txpool/build.rs No longer need tonic-build at compile time - Remove [build-dependencies] from Cargo.toml Eliminates tonic-build = "0.12" dependency - Update client.rs and mock_ecsd.rs Use super::proto instead of include_proto! macro - Remove protoc installation from CI Deleted protoc setup from 5 jobs: warnings, clippy, unit-test, integration-test, viem Benefits: - No protoc installation required for developers or CI - Faster builds (no proto compilation step) - Simpler CI configuration (removed 10 lines across 5 jobs) - Works on all platforms without protobuf-compiler package - Generated code is version-controlled and reviewable The proto source file remains at crates/seismic/txpool/proto/ecsd.proto for reference. If it changes, proto.rs can be regenerated following the instructions in its header comment. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
daltoncoder
left a comment
daltoncoder
left a comment
There was a problem hiding this comment.
Looks good to me. Going to test some more on some of our testnets. A couple comments:
| Result<tonic::Response<BatchCheckResponse>, tonic::Status>, | ||
| tokio::time::error::Elapsed, | ||
| > = { | ||
| let mut client = self.inner.client.lock().await; |
There was a problem hiding this comment.
This Mutex lock is held for duration of call. This could really effect how many transactions a second we can process. The underlying tonic client is clone and supports multiplexing so i think dropping the mutex and doing this here instead would increase our maximum throughput greatly.
let mut client = self.inner.client.clone();
tokio::time::timeout(self.inner.timeout, client.batch_check_addresses(request)).await| valid_tx.into_transaction(), | ||
| InvalidPoolTransactionError::Consensus( | ||
| InvalidTransactionError::SeismicTx(format!( | ||
| "address screening: flagged addresses {flagged:?}" |
There was a problem hiding this comment.
this error will propagate back through rpc. So just wanted to make sure that is okay. If your blacklist is private this could allow people to submit transactions and figure out exactly which addresses are blacklisted. If thats the case we should probably just make this error super generic "txn rejected"
Fixes 43 clippy warnings across calldata.rs, client.rs, and proto.rs: calldata.rs: - Combine trait bounds: T: PoolTransaction + alloy_consensus::Transaction - Add #[allow(clippy::indexing_slicing)] for length-checked slicing operations (all slices are validated before use) client.rs: - Make builder methods const: timeout() and fail_mode() proto.rs (generated code): - Add module-level #[allow] for generated code patterns: unreachable_pub, missing_const_for_fn, doc_markdown - Add Eq to all PartialEq derives (ExtendedHealthRequest, ExtendedHealthResponse, BatchCheckRequest, BatchCheckResponse) - Change visibility to pub (needed for benches to access) benches/calldata_extraction.rs: - Replace std::iter::repeat().take() with repeat_n() (clippy::manual_repeat_n) screening.rs tests: - Replace #[should_panic] with explicit error checking (panic message from clap doesn't match expected string exactly) All changes maintain functionality while satisfying strict clippy lints enabled in CI (-D warnings with custom lint configuration).
cdrappi
left a comment
cdrappi
left a comment
There was a problem hiding this comment.
Hey guys. I stopped reviewing the whole thing after finding one super critical issue, detailed below
TLDR: we have some work to get our seismic tx pipeline in a spot where you can actually screen on encrypted calldata. right now, this code as executed on seismic transactions will not screen anything, defeating the purpose entirely
Right now, this is more of an us problem than a you problem; when we've got our ducks in line we will circle back with some instructions on correctly wiring in Seismic transactions to screening
| } => { | ||
| // Extract all screenable addresses | ||
| let extraction_start = Instant::now(); | ||
| let addresses = extract_addresses(valid_tx.transaction()); |
There was a problem hiding this comment.
if valid_tx is a seismic transaction here, we will fail to extract addresses on calldata: it'll run encrypted calldata through the screening. that means it will miss function selectors, address inputs, etc.
in our node, we currently decrypt in the block executor. in light of this audit discovery – independent but very related – we need to think of a plan that both:
- prevents users from DOS'ing us with invalid ciphertext, and
- allows us to screen the ciphertext itself, ideally in the mempool
You should be able to see that issue on the aegis-cipherowl account (invited this to Seismic org as a guest). If you'd like us to add another account, let me know and we can give you access to the repo with the issue
Also, since I've added aegis-cipherowl to our org, CI workflows should run automatically without one of us approving (at least after you accept the invite)
There was a problem hiding this comment.
i think we have some work to do first to get our calldata encryption in the right spot; my main man @HenryMBaldwin is on this starting this afternoon. so for now, i'd hang tight (maybe aside from addressing @daltoncoder's comments)
3 participants
Summary
Adds operator-enforced compliance screening for Seismic validators via integration with CipherOwl's ECSD (Ethereum Compliance Screening Daemon). This is an opt-in policy layer that screens transaction addresses against a compliance blocklist before pool admission.
ScreeningTransactionValidatorwrapper, NOT part of protocol consensus rulesEither<A, B>to maintain pool type compatibility whether screening is enabled or disabledKey Features
Address Extraction
Extracts addresses from transactions for screening:
transfer,approve,transferFromsafeTransferFromvariantssafeTransferFrom,safeBatchTransferFromPerformance: ~18ns per extraction (negligible CPU overhead)
gRPC Client
open(permissive) orclosed(restrictive)Performance: ~470µs avg latency with real ECSD Docker
CLI Configuration
Security Fix
closeinstead ofclosed) now fail fast at startupunwrap_or(Open)could silently use permissive mode when operator intended restrictive modeArchitecture
Performance Benchmarks
Benchmarked against real ECSD Docker container:
Files Changed
New Files (15)
Proto & Build:
crates/seismic/txpool/proto/ecsd.proto- gRPC service definitioncrates/seismic/txpool/build.rs- Proto compilationScreening Module (6 files):
crates/seismic/txpool/src/screening/mod.rscrates/seismic/txpool/src/screening/calldata.rs- Address extractioncrates/seismic/txpool/src/screening/client.rs- gRPC clientcrates/seismic/txpool/src/screening/metrics.rs- Prometheus metricscrates/seismic/txpool/src/screening/validator.rs- Validator wrappercrates/seismic/txpool/src/screening/README.md- DocumentationBenchmarks (4 files):
crates/seismic/txpool/benches/calldata_extraction.rscrates/seismic/txpool/benches/screening.rscrates/seismic/txpool/benches/mock_ecsd.rscrates/seismic/txpool/benches/README.md- DocumentationCLI Args:
crates/node/core/src/args/screening.rs- CLI configurationModified Files (8)
Cargo.toml,Cargo.lock- Added tonic, prost dependenciesbin/seismic-reth/src/main.rs- Wired up ScreeningArgscrates/node/core/src/args/mod.rs- Exported ScreeningArgscrates/seismic/node/Cargo.toml- Added futures-utilcrates/seismic/node/src/node.rs- Conditional screening with Eithercrates/seismic/txpool/Cargo.toml- Screening dependenciescrates/seismic/txpool/src/lib.rs- Updated pool type aliasTest Plan
Unit Tests: 27 tests passing (18 screening-specific + existing)
Benchmarks: All benchmarks passing with real ECSD Docker
Integration: Full compilation and type checking
Manual Testing (post-merge):
docker run -p 9090:9090 cipherowl/ecsd:latestProduction Validation:
Documentation
src/screening/README.md)benches/README.md)Security Considerations
value_parserto reject invalid fail-mode values at startup (prevents silent misconfiguration)Migration Impact
Eithermaintains pool type compatibility🤖 Generated with Claude Code