feat(mempool): poll-reconcile single timeline for broadcast (replaces TTL TxnCache) - #808
feat(mempool): poll-reconcile single timeline for broadcast (replaces TTL TxnCache)#808nekomoto911 wants to merge 7 commits into
Conversation
read_timeline was returning MultiBucketTimelineIndexIds with id_per_bucket = vec![0; out.len()], which is the wrong shape: length must be fee/ranking bucket count, not batch size, and values must be max timeline_ids per bucket. Architecture A intentionally does not implement timeline progress (TxnCache + TTL instead). Return an empty id_per_bucket as an honest stub so zip/update do not invent fake (0,0) ranges or pretend fee-bucket cursors exist. Closes Galxe/gravity-audit#1090
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d961dd2a71
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // `vec![0; out.len()]`. See gravity-audit#1090. | ||
| // When timeline is re-enabled: return per-fee-bucket max timeline_ids | ||
| // with fixed length broadcast_buckets.len(). | ||
| (out, MultiBucketTimelineIndexIds { id_per_bucket: vec![] }) |
There was a problem hiding this comment.
In the gaptos broadcast path, determine_broadcast_batch() passes each read_timeline() cursor to MempoolMessageId::from_timeline_ids(); with an empty new_timeline_id, the old/new zip emits no ranges, so every non-empty fresh batch to a peer is sent as MempoolMessageId([]). When two broadcasts overlap for the same peer (for example, tick interval shorter than network RTT), an ACK for the first batch can remove the sent_messages entry for the later batch in process_broadcast_ack(), causing the later ACK/backoff/retry signal to be ignored and bypassing peer backpressure. Please preserve a non-colliding message id, even if real timeline progress remains stubbed.
Useful? React with 👍 / 👎.
Replace Arch-A TxnCache/Snapshot/ObservedTopology with BroadcastIndex: per-sender_bucket monotonic TimelineIndex + bodies/hash_to_pos, throttled maybe_reconcile from get_broadcast_txns. Stub read_timeline/timeline_range until later tasks; keep get_batch path unchanged.
nekomoto911
changed the title
…Store Rename BroadcastIndex to TransactionStore and field names to match aptos core_mempool (transactions, timeline_index, hash_index, TimelineIndex:: timeline_id/timeline). Document each member against the aptos counterpart and Gravity poll-reconcile differences.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 83b4312787
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| ); | ||
| }; | ||
| out.push((txn.clone(), 0)); // ready_time_ms = 0 | ||
| last_included = Some(id); |
There was a problem hiding this comment.
With the new real cursor, including a txn here makes the returned new_timeline_id advance, and shared-mempool advances the peer state as soon as the batch is sent; after an ACK with retry=false, future reads for that peer start after this id. That is unsafe for Gravity's receiver path because Mempool::add_txn() calls TxPool::add_external_txn(), and the real implementation in bin/gravity_node/src/mempool.rs spawns pool.add_external_transaction(...) and returns true before the insert result is known, only logging later recoverable failures such as nonce gap/low fee/local config. Reproduce by sending one broadcast to a peer whose reth pool rejects in that spawned task: the sender receives a successful ACK, keeps the txn locally, but the peer cursor is already past it so it is never offered to that peer again. Please either make receiver admission/ACK reflect the actual pool insert result or preserve a retry/TTL path for ACKed-but-not-admitted txns.
Useful? React with 👍 / 👎.
…drain, no rebroadcast Cover T1-A/B/C (range keeps sent_messages, max_broadcasts backpressure, expired via timeline_range), T2 (no Fresh rebroadcast after drain), T3 (R6 multi-sender cover in ceil(S/count) ticks), and T5 unit (Failover before=now-500ms filters fresh admits). Simulates gaptos filter/pending logic against Gravity CoreMempoolTrait without network harness.
…try, T3 scale App-layer Instant clock sims for WAN-like ACK delay (T4-A/B/C), immediate-ACK drain + multi-TTL no-rebroadcast (T2-B), backoff PeerNotScheduled and Retry range path (T1-D/E), larger R6 cover (T3-B), and Failover before mid/delay-0 edge (T5-B). pfn_chain Phase3 comments: 5s is historical Arch-A; P99 is safety net not 500ms first-alt proof. No production logic changes.
1 participant
Description
Replace architecture-A mempool broadcast progress (global
TxnCache+ TTL four-state + empty cursor stub) with poll-reconcile + per-sender_bucketmonotonic timeline, so gaptos shared-mempool can use realread_timelinecursors andtimeline_range*for ACK / in-flight / retransmit.Ground truth remains
TxPool::get_broadcast_txns(periodic reconcile). Packing path (get_batch→best_txns) is unchanged. No gaptosnetwork.rsedits.Also supersedes the earlier audit#1090 empty-cursor-only approach on this branch: instead of leaving timeline unimplemented, we implement a single logical timeline per sender bucket with fee-slot-shaped cursors (
id_per_bucket.len() == broadcast_buckets.len(), progress only in slot0).Issue Number: closes Galxe/gravity-audit#1090 (cursor contract); broader progress-model change beyond the stub fix
How Has This Been Tested?
Unit (
aptos-mempoollib)read_timeline: fee-slot-shaped cursor, incremental cursor, count truncation, empty batch no advance,beforeInstant filter, bucket isolationtimeline_range/timeline_range_of_message: MessageId window re-materialize, leave-then-range, multi-bucket flattenget_batchbyte-budget test keptE2E
./gravity_e2e/run_test.sh pfn_chain— PASS (Phase 0–3; black-hole p99 ~1.7–2.2s vs 18s SLA ceiling)./gravity_e2e/run_test.sh vfn— PASS (3 tests)Local notes (not in repo): acceptance checklist under
_local/wiki/mempool-broadcast/(untracked).Key Areas to Review
reconcilevsget_broadcast_txns— leave/admit order, stable id while hash stays, re-enter new id + Instant; throttleMEMPOOL_SNAPSHOT_MAX_AGE_MS(default 20ms).read_timeline—Excluded(cursor0),count,before(admit Instant >= t→ break);cursor_fromonly advances[0]; empty batch does not advance.timeline_range*— fee slot 0 only; stdMutexnot reentrant (timeline_range_with_indexhelper); missing body skip.Mutex<BroadcastIndex>for&selftrait methods; real contention remains outersmp.mempool(same as before).beforemodel.Type of Change
Which Components or Systems Does This Change Impact?
Out of scope (follow-ups)
Checklist
Summary of commits on branch
d961dd2a— empty cursor stub (audit#1090)d7e568d1— poll-reconcile timeline index; remove TTL TxnCachea388d114— realread_timeline(cursor / count / before)5454c8c0— realtimeline_range*for ACK retransmit path