Skip to content

Commit 6eedaa5

Browse files
authored
chore(p2p): remove proposal_tx_collector leftovers (#23276)
ProposalTxCollector doesn't exist anymore. Clean up unused files. Rename bench that is now only testing BatchTxRequester.
1 parent db4bd98 commit 6eedaa5

7 files changed

Lines changed: 35 additions & 462 deletions

File tree

yarn-project/bootstrap.sh

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -245,7 +245,7 @@ function bench_cmds {
245245
echo "$hash BENCH_OUTPUT=bench-out/kv_store.bench.json yarn-project/scripts/run_test.sh kv-store/src/bench/map_bench.test.ts"
246246
echo "$hash BENCH_OUTPUT=bench-out/tx_pool_v2.bench.json yarn-project/scripts/run_test.sh p2p/src/mem_pools/tx_pool_v2/tx_pool_v2_bench.test.ts"
247247
echo "$hash BENCH_OUTPUT=bench-out/tx_validator.bench.json yarn-project/scripts/run_test.sh p2p/src/msg_validators/tx_validator/tx_validator_bench.test.ts"
248-
echo "$hash:ISOLATE=1:CPUS=16:MEM=32g:TIMEOUT=1800 BENCH_OUTPUT=bench-out/p2p_client_proposal_tx_collector.bench.json yarn-project/scripts/run_test.sh p2p/src/client/test/tx_proposal_collector/p2p_client.proposal_tx_collector.bench.test.ts"
248+
echo "$hash:ISOLATE=1:CPUS=16:MEM=32g:TIMEOUT=1800 BENCH_OUTPUT=bench-out/p2p_client_batch_tx_requester.bench.json yarn-project/scripts/run_test.sh p2p/src/client/test/p2p_client.batch_tx_requester.bench.test.ts"
249249
echo "$hash BENCH_OUTPUT=bench-out/tx.bench.json yarn-project/scripts/run_test.sh stdlib/src/tx/tx_bench.test.ts"
250250
echo "$hash:ISOLATE=1:CPUS=10:MEM=16g:LOG_LEVEL=silent BENCH_OUTPUT=bench-out/proving_broker.bench.json yarn-project/scripts/run_test.sh prover-client/src/test/proving_broker_testbench.test.ts"
251251
echo "$hash:ISOLATE=1:CPUS=16:MEM=16g BENCH_OUTPUT=bench-out/avm_bulk_test.bench.json yarn-project/scripts/run_test.sh bb-prover/src/avm_proving_tests/avm_bulk.test.ts"

yarn-project/p2p/src/client/test/tx_proposal_collector/README.md renamed to yarn-project/p2p/src/client/test/p2p_client.batch_tx_requester.bench.README.md

Lines changed: 23 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
1-
# ProposalTxCollector Benchmarks
1+
# BatchTxRequester Benchmarks
22

3-
This benchmark suite measures **how quickly a proposer node can fetch missing transactions from P2P peers** when building a block proposal. It compares two alternative transaction-collection implementations under several controlled "who-has-which-txs" distributions.
3+
This benchmark suite measures **how quickly a proposer node can fetch missing transactions from P2P peers** when building a block proposal under several controlled "who-has-which-txs" distributions.
44

55
## Purpose
66

@@ -10,12 +10,6 @@ This benchmark answers:
1010

1111
- How long does it take to fetch **N missing txs** (N ∈ **{10, 50, 100, 500}**)?
1212
- How do different **peer availability patterns** affect performance?
13-
- Which collector strategy performs better under each pattern?
14-
15-
The suite compares two collectors:
16-
17-
- **`BatchTxRequesterCollector`** (collector type: `batch-requester`)
18-
- **`SendBatchRequestCollector`** (collector type: `send-batch-request`)
1913

2014
## Architecture
2115

@@ -24,7 +18,7 @@ The benchmark runs a small simulated network on localhost:
2418
```
2519
┌─────────────────────────────────────────────────────────────────────┐
2620
│ Test Process (Driver) │
27-
│ p2p_client.proposal_tx_collector.bench.test.ts │
21+
│ p2p_client.batch_tx_requester.bench.test.ts
2822
│ ┌─────────────────────────────────────────────────────────────┐ │
2923
│ │ WorkerClientManager │ │
3024
│ │ (src/testbench/worker_client_manager.ts) │ │
@@ -34,7 +28,7 @@ The benchmark runs a small simulated network on localhost:
3428
│ ▼ ▼ ▼ │
3529
│ ┌───────────┐ ┌───────────┐ ┌───────────┐ │
3630
│ │ Worker 0 │◄──────►│ Worker 1 │◄──────►│ Worker N-1│ │
37-
│ │ (Collector│ P2P │(Responder)│ P2P │(Responder)│ │
31+
│ │(Aggregator│ P2P │(Responder)│ P2P │(Responder)│ │
3832
│ │ Node) │ │ │ │ │ │
3933
│ │ TxPool:[] │ │ TxPool: │ │ TxPool: │ │
4034
│ │ │ │ [txs...] │ │ [txs...] │ │
@@ -54,12 +48,12 @@ Using separate OS processes makes the setup closer to real networking behavior (
5448

5549
The network is intentionally asymmetric:
5650

57-
- **Worker 0 is the collector/proposer node**
51+
- **Worker 0 is the aggregator/proposer node**
5852
- Starts with an **empty tx pool** (`[]`)
59-
- Is the only worker instructed to run the collector for each `BENCH_REQRESP` command
53+
- Is the only worker instructed to run `BatchTxRequester` for each `BENCH_REQRESP` command
6054
- **Workers 1..N-1 are responder peers**
6155
- Locally generate and filter txs according to the distribution pattern
62-
- Respond to req/resp queries made by Worker 0's collector
56+
- Respond to req/resp queries made by Worker 0's `BatchTxRequester`
6357

6458
This models a proposer that has only `txHashes` in a proposal and must fetch the full tx bodies from the network.
6559

@@ -72,7 +66,7 @@ Each benchmark case generates `missingTxCount` mock txs and assigns them to peer
7266
**Every responder peer has every transaction.**
7367

7468
- Simulates the best-case: high replication / high gossip success
75-
- Expectation: collector should quickly succeed; differences mostly reflect collector overhead and batching strategy
69+
- Expectation: the requester should quickly succeed; differences mostly reflect requester overhead and batching strategy
7670

7771
### `sparse`
7872

@@ -81,7 +75,7 @@ Each benchmark case generates `missingTxCount` mock txs and assigns them to peer
8175
Each responder is bucketed and holds txs whose index falls into its bucket or the "next" bucket (striped by tx index).
8276

8377
- Simulates partial propagation, churn, or uneven mempool convergence
84-
- Expectation: collector must query multiple peers and cope with "misses"
78+
- Expectation: the requester must query multiple peers and cope with "misses"
8579

8680
### `pinned-only`
8781

@@ -92,33 +86,13 @@ Each responder is bucketed and holds txs whose index falls into its bucket or th
9286

9387
> **Guardrail:** the pinned peer index must be within `(0, numberOfPeers)` (Worker 0 cannot be pinned).
9488
95-
## Collectors Under Test
96-
97-
### `BatchTxRequesterCollector` (`batch-requester`)
98-
99-
```typescript
100-
new BatchTxRequesterCollector(p2pService, logger, new DateProvider())
101-
```
102-
103-
Uses the P2P service plus internal logic to fetch missing txs, coordinating requests in a batched or staged way.
104-
105-
### `SendBatchRequestCollector` (`send-batch-request`)
106-
107-
```typescript
108-
const maxPeers = 10;
109-
const maxRetryAttempts = Math.max(peerIds.length, 3);
110-
new SendBatchRequestCollector(p2pService, maxPeers, maxRetryAttempts)
111-
```
112-
113-
Explicitly caps the number of peers it will involve (`maxPeers`) and uses a retry budget derived from peer count.
114-
11589
## Test Parameters
11690

11791
| Parameter | Value | Description |
11892
|-----------|-------|-------------|
11993
| `PEERS_PER_RUN` | 30 | Number of worker processes spawned |
12094
| `MISSING_TX_COUNTS` | 10, 50, 100, 500 | Number of missing transactions to fetch |
121-
| `TIMEOUT_MS` | 30,000 ms | Collector timeout per case |
95+
| `TIMEOUT_MS` | 30,000 ms | Per-case timeout for the requester |
12296
| `TEST_TIMEOUT_MS` | 600,000 ms | Overall Jest timeout (10 minutes) |
12397

12498
## Running
@@ -127,13 +101,13 @@ From the p2p package:
127101

128102
```bash
129103
cd yarn-project/p2p
130-
yarn test src/client/test/tx_proposal_collector/p2p_client.proposal_tx_collector.bench.test.ts
104+
yarn test src/client/test/p2p_client.batch_tx_requester.bench.test.ts
131105
```
132106

133107
Or from repo root:
134108

135109
```bash
136-
yarn test p2p_client.proposal_tx_collector.bench.test.ts
110+
yarn test p2p_client.batch_tx_requester.bench.test.ts
137111
```
138112

139113
The benchmark is intentionally long due to spawning many processes and running multiple cases.
@@ -145,23 +119,21 @@ The benchmark is intentionally long due to spawning many processes and running m
145119
If no env vars are set, the suite prints a table:
146120

147121
```
148-
| Collector | Distribution | Missing | Duration (ms) | Fetched | Success |
149-
|---------------------|--------------|---------|---------------|---------|---------|
150-
| batch-requester | pinned-only | 10 | 123 | 10 | Yes |
151-
| send-batch-request | pinned-only | 10 | 145 | 10 | Yes |
122+
| Distribution | Missing | Duration (ms) | Fetched | Success |
123+
|--------------|---------|---------------|---------|---------|
124+
| pinned-only | 10 | 123 | 10 | Yes |
125+
| pinned-only | 50 | 145 | 50 | Yes |
152126
```
153127

154-
Plus a comparison summary stating which collector was faster per `(distribution, missing)` pair.
155-
156128
### JSON metrics (for CI/dashboards)
157129

158130
```bash
159131
BENCH_OUTPUT=/path/results.json yarn test ...
160132
```
161133

162134
Writes JSON metrics like:
163-
- `ProposalTxCollector/<collector>/<distribution>/missing_<N>/duration` (ms)
164-
- `ProposalTxCollector/<collector>/<distribution>/missing_<N>/fetched` (txs)
135+
- `BatchTxRequester/<distribution>/missing_<N>/duration` (ms)
136+
- `BatchTxRequester/<distribution>/missing_<N>/fetched` (txs)
165137

166138
### Markdown file output
167139

@@ -175,14 +147,14 @@ Writes the pretty table + summary to disk.
175147

176148
For each case the benchmark records:
177149

178-
- `durationMs`: wall-clock time spent inside the collector call
179-
- `fetchedCount`: how many txs were returned by the collector
150+
- `durationMs`: wall-clock time spent inside the requester call
151+
- `fetchedCount`: how many txs were returned by the requester
180152
- `success`: `fetchedCount === missingTxCount`
181153

182154
**Guidelines:**
183155

184156
- **Always check `Success` first.** A faster run that fetched fewer txs is not a win.
185-
- Compare collectors **within the same distribution + missing count** only.
157+
- Compare runs **within the same distribution + missing count** only.
186158
- Expect `pinned-only` to highlight pinned-peer behavior (fast if pinned peer is used effectively; slow if the algorithm wastes time sampling other peers).
187159
- Expect `sparse` to be the most "network-like" stress case, since many peers won't have each requested tx.
188160

@@ -193,7 +165,7 @@ Inside each worker, the benchmark intentionally reduces variability:
193165
- **Unlimited rate limits** are installed so the req/resp rate limiter doesn't dominate results
194166
- **Deterministic tx generation** ensures all workers see the same tx set without large IPC payloads
195167

196-
This makes the benchmark better for *comparing collectors* (A vs B), but it is **not** a perfect model of production networking conditions.
168+
This makes the benchmark better for tracking regressions, but it is **not** a perfect model of production networking conditions.
197169

198170
## Limitations
199171

@@ -207,9 +179,7 @@ This benchmark does **not** measure:
207179

208180
| File | Purpose |
209181
|------|---------|
210-
| `p2p_client.proposal_tx_collector.bench.test.ts` | Test suite (cases, distributions, output formatting) |
211-
| `proposal_tx_collector_worker.ts` | Collector-specific worker implementation |
212-
| `proposal_tx_collector_worker_protocol.ts` | IPC message types and serialization |
182+
| `p2p_client.batch_tx_requester.bench.test.ts` | Test suite (cases, distributions, output formatting) |
213183
| `src/testbench/worker_client_manager.ts` | Worker process manager (forking, IPC, orchestration) |
214184
| `src/testbench/p2p_client_testbench_worker.ts` | General testbench worker implementation |
215185
| `src/test-helpers/testbench-utils.ts` | Shared mocks and utilities (InMemoryTxPool, InMemoryAttestationPool, etc.) |

yarn-project/p2p/src/client/test/tx_proposal_collector/p2p_client.proposal_tx_collector.bench.test.ts renamed to yarn-project/p2p/src/client/test/p2p_client.batch_tx_requester.bench.test.ts

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ import {
99
type DistributionPattern,
1010
WorkerClientManager,
1111
testChainConfig,
12-
} from '../../../testbench/worker_client_manager.js';
12+
} from '../../testbench/worker_client_manager.js';
1313

1414
const TEST_TIMEOUT_MS = 600_000; // 10 minutes
1515
jest.setTimeout(TEST_TIMEOUT_MS);
@@ -75,7 +75,7 @@ const CASES: readonly BenchmarkCase[] = BASE_SCENARIOS.flatMap(base =>
7575
})),
7676
);
7777

78-
describe('ProposalTxCollector Benchmarks', () => {
78+
describe('BatchTxRequester Benchmarks', () => {
7979
const results: BenchmarkResult[] = [];
8080

8181
let logger: Logger;
@@ -181,7 +181,7 @@ function toPrettyString(benchResults: BenchmarkResult[]): string {
181181

182182
lines.push('');
183183
lines.push('='.repeat(80));
184-
lines.push('ProposalTxCollector Benchmark Results');
184+
lines.push('BatchTxRequester Benchmark Results');
185185
lines.push('='.repeat(80));
186186
lines.push('');
187187
lines.push('| Distribution | Missing | Duration (ms) | Fetched | Success |');
@@ -212,7 +212,7 @@ function toBenchmarkJSON(benchResults: BenchmarkResult[], indent = 2): string {
212212
const metrics: JsonBenchmarkResult[] = [];
213213

214214
for (const result of benchResults) {
215-
const baseName = `ProposalTxCollector/${result.distribution}/missing_${result.missingTxCount}`;
215+
const baseName = `BatchTxRequester/${result.distribution}/missing_${result.missingTxCount}`;
216216
metrics.push(
217217
{
218218
name: `${baseName}/duration`,

0 commit comments

Comments
 (0)