STR 2042 mempool generalised priority ordering#1405
STR 2042 mempool generalised priority ordering#1405krsnapaudel wants to merge 3 commits intomainfrom
Conversation
krsnapaudel
force-pushed
the
STR-2042
branch
from
5368d83 to
9c2acd9
Compare
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## main #1405 +/- ##
===========================================
- Coverage 75.10% 64.90% -10.21%
===========================================
Files 799 798 -1
Lines 74349 74417 +68
===========================================
- Hits 55841 48298 -7543
- Misses 18508 26119 +7611
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 239 files with indirect coverage changes 🚀 New features to boost your workflow:
|
|
Commit: 3a1710f SP1 Execution Results
|
bewakes
left a comment
bewakes
left a comment
There was a problem hiding this comment.
Looks good in general, some comments.
delbonis
left a comment
delbonis
left a comment
There was a problem hiding this comment.
A lot of this should be rethought since there's a lot of modelling problems with it.
crates/ol/mempool/src/types.rs
Outdated
| /// Compute the transaction ID by hashing the SSZ-encoded transaction data. | ||
| /// | ||
| /// This follows the established pattern: SSZ encode → SHA256 hash. | ||
| /// The hash is computed from canonical transaction content (payload + attachment), excluding | ||
| /// mempool-local metadata such as `timestamp_micros`. | ||
| /// | ||
| /// The hash of [`OLMempoolTransaction`] should equal the hash of | ||
| /// [`OLTransaction`](strata_ol_chain_types_new::OLTransaction) without accumulator_proofs, | ||
| /// and also equal the hash of `RpcOLTransaction` | ||
| /// when properly converted. | ||
| pub fn compute_txid(&self) -> OLTxId { | ||
| let encoded = ssz::Encode::as_ssz_bytes(self); | ||
| let preimage = MempoolTxIdPreimage { | ||
| payload: &self.payload, | ||
| attachment: &self.attachment, | ||
| }; | ||
| let encoded = ssz::Encode::as_ssz_bytes(&preimage); | ||
| let hash_bytes = hash::raw(&encoded); | ||
| OLTxId::from(hash_bytes) | ||
| } |
There was a problem hiding this comment.
This is not a txid, don't call it a txid.
Also:
This follows the established pattern: SSZ encode → SHA256 hash.
This is not really an established pattern, we should generally be using the SSZ tree hash for things like this.
There was a problem hiding this comment.
@delbonis I have left compute_txid in place returning OLTxId. The ID is now computed via SSZ tree-hash over canonical transaction content (payload + attachment), same hashing approach used for all other protocol types. It's a content-addressed identifier for the mempool-form transaction. Is your objection to name of method and return type or how id is computed?
There was a problem hiding this comment.
My objection was that you defined a new type and then used the flat hash of that encoded message as a txid, which was completely arbitrary.
I think this new approach is fine for now. I reviewed it and retract this point.
krsnapaudel
force-pushed
the
STR-2042
branch
2 times, most recently
from
f63263c to
f646252
Compare
| impl<H: TreeHashDigest> TreeHash<H> for OLMempoolTxPayload { | ||
| fn tree_hash_type() -> TreeHashType { | ||
| TreeHashType::Container | ||
| } | ||
|
|
||
| fn tree_hash_packed_encoding(&self) -> PackedEncoding { | ||
| unreachable!("Union should never be packed") | ||
| } | ||
|
|
||
| fn tree_hash_packing_factor() -> usize { | ||
| unreachable!("Union should never be packed") | ||
| } | ||
|
|
||
| fn tree_hash_root(&self) -> H::Output { | ||
| match self { | ||
| Self::GenericAccountMessage(inner) => { | ||
| let root = <_ as TreeHash<H>>::tree_hash_root(inner); | ||
| mix_in_selector_with_hasher::<H>(&root, 0u8).expect("valid selector") | ||
| } | ||
| Self::SnarkAccountUpdate(inner) => { | ||
| let root = <_ as TreeHash<H>>::tree_hash_root(inner); | ||
| mix_in_selector_with_hasher::<H>(&root, 1u8).expect("valid selector") | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Why are you defining tree hash on this type manually? If this is naturally SSZ-able we should be able to do #[derive(TreeHash)]. If not, then we shouldn't be implementing treehash on it in the first place and should be more careful about how the types are being modeled.
There was a problem hiding this comment.
There's something really weird going on here with these types. It's a pretty big red flag to me that for the mempool which treats transactions mostly as black boxes (aside from target, seqno, etc.) is defining special mempool representations of transaction types with their own serialization and treehashing. Can you see if it's possible to use the canonical transaction types, sure maybe with containers with extra metadata, or if not explain why this is necessary?
There was a problem hiding this comment.
As discussed in Slack, I will wait for a separate PR to remove mempool-specific tx types. Since they should have ssz support, we will not need manual TreeHash impls as well.
| #[derive(Clone, Debug, Encode, Decode, PartialEq, Eq)] | ||
| pub struct OLMempoolTransaction { | ||
| /// Transaction payload (generic message or snark account update). | ||
| pub payload: OLMempoolTxPayload, | ||
|
|
||
| /// Transaction attachment (min_slot, max_slot constraints). | ||
| pub attachment: TransactionAttachment, | ||
|
|
||
| /// Insertion timestamp in microseconds since UNIX epoch. | ||
| pub timestamp_micros: u64, | ||
| } |
There was a problem hiding this comment.
This type's purpose is confused / at odds with itself because it contains two intrinsic components of the transaction itself alongside extrinsic information (timestamp). These are different levels of abstraction and they should be organized in a way that reflects that.
| let mut leaves = [0u8; 64]; | ||
| leaves[..32].copy_from_slice(payload_root.as_ref()); | ||
| leaves[32..].copy_from_slice(attachment_root.as_ref()); | ||
| let root = merkle_root_with_hasher::<Sha256Hasher>(&leaves, 2); | ||
| OLTxId::from(Buf32::from(root.0)) |
There was a problem hiding this comment.
Actually no, I didn't quite see that this is how this was implemented. Even if this is mathematically correct, you should just be directly calling out to the transaction types instead of reimplementing low-level hashing operations! This is how we get incompatibilities across different areas of the codebase, since different components use bespoke versions of operations like "compute txid". We should be defining "computing a txid" in one singular way, in one place, and everything else just calls that.
| /// | ||
| /// `txid` is provided for deterministic tie-breaking when two transactions otherwise share the | ||
| /// same priority. | ||
| fn compute_priority(tx: &OLMempoolTransaction, txid: OLTxId) -> Self::Priority; |
There was a problem hiding this comment.
Is there a reason txid is being passed separately with tx? Since txid can be derived from tx, the definition of this method should only have tx as a parameter imo.
There was a problem hiding this comment.
The way it is now leaves room for passing one tx and a different txid not corresponding to that tx.
| /// This captures only intrinsic validity linkages between transactions. Today the only intrinsic | ||
| /// linkage is per-account sequencing for snark account updates. | ||
| #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] | ||
| pub(crate) enum PackageKey { |
| Self::Snark { | ||
| timestamp_micros: t1, | ||
| txid: tx1, | ||
| .. | ||
| }, | ||
| Self::Snark { | ||
| timestamp_micros: t2, | ||
| txid: tx2, | ||
| .. |
There was a problem hiding this comment.
Can we define the timestamp_micros() and txid() methods in FifoPriorityKey so that this matching becomes cleaner? The destructuring is quite verbose and repititive here.
3 participants
Description
Generalizes OL mempool transaction ordering behind a pluggable
MempoolPriorityPolicytrait, while preserving current FIFO behavior unchanged.MempoolPriorityPolicytrait (ordering.rs)Priority: Ord + Copy + Debug + Send + Sync + 'static— used asBTreeMapkeys in the priority index.compute_priority(tx, txid) -> Self::Priority— computes priority from the transaction and its ID.FifoPrioritydefault implementationReproduces current timestamp-FIFO semantics via
FifoPriorityKey.Intrinsic vs extrinsic ordering (
ordering.rs)PackageKey::SnarkAcctUpdate(AccountId)— scaffolding for grouping same-account snark txs.IntrinsicOrderwithPartialOrd— comparable only within the same package (same-accountseq_no).FifoPriorityKey::Ordapplies intrinsic order first, falls back to FIFO timestamp cross-package.txid tiebreaker (correctness fix)
All
FifoPriorityKeyvariants include atxidfield. Without it, two transactions with the same timestamp would silently overwrite each other in theBTreeMap.Transaction ID (
types.rs)compute_txid()uses SSZ tree-hash over canonical content (payload + attachment), excludingtimestamp_micros. ManualTreeHashimpls onOLMempoolSnarkAcctUpdateTxPayloadandOLMempoolTxPayloadmatch the generated pattern from.ssztypes.Generic propagation
MempoolEntry<P>,MempoolServiceState<SP, Prio>,MempoolService<SP, Prio>,MempoolBuilder<Prio>— default= FifoPriorityonly onMempoolBuilder(public entry point).load_from_db()Retains timestamp-based pre-sort for
seq_noreconstruction correctness; explicitly documented as independent of the active priority policy.This PR was created with help from Claude Code and Codex.
Type of Change
Notes to Reviewers
Is this PR addressing any specification, design doc or external reference document?
If yes, please add relevant links:
Checklist
Related Issues
STR-2042