Process eth_sendRawTransaction txs correctly#900
Conversation
|
Claude finished @ZanCorDX's task in 1m 52s —— View job Review of PR #900
Overall: Clean, well-scoped change that adds an Issues flagged (see inline comments):
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| #[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize, Deserialize)] | ||
| pub struct RawTx { | ||
| pub tx: Bytes, | ||
| pub is_private: bool, |
There was a problem hiding this comment.
is_private is a required field for deserialization. If any existing serialized RawTx data is read (e.g., from sources other than the SQLite store, which got a version bump), deserialization will fail because old data won't have this field.
Consider adding #[serde(default)] to make this backward-compatible — false is the correct default for legacy data.
| pub is_private: bool, | |
| #[serde(default)] | |
| pub is_private: bool, |
| /// Since we get txs from mempool and also from builder's endpoint eth_sendRawTransaction | ||
| /// MempoolTx is modeling both (is_private <=> came internally through eth_sendRawTransaction). | ||
| /// @Pending rename to something like SingleTx. | ||
| #[derive(Debug, Clone, PartialEq, Eq, Hash)] |
There was a problem hiding this comment.
MempoolTx derives PartialEq, Eq, and Hash, and now is_private participates in all three. This means the same transaction received from both mempool (is_private: false) and eth_sendRawTransaction (is_private: true) will be considered different orders and could produce different hash values.
If these duplicates can reach the order pool, it could lead to double-inclusion attempts during block building. Is this a scenario that's already prevented upstream, or should Hash/PartialEq exclude is_private (e.g., via manual impls that only compare tx_with_blobs)?
| @@ -1367,10 +1374,9 @@ mod test { | |||
| fn test_create_sim_value_tx_non_mempool_coinbase_profit() { | |||
There was a problem hiding this comment.
The test only covers the public mempool case (is_private: false). Given that the bundle test above covers both is_private variants, consider adding a private tx variant here too for completeness — verifying that a private standalone tx is also handled correctly by create_sim_value.
|
Claude finished @ZanCorDX's task in 1m 29s —— View job
Review of PR #900 (post-sync)The "backwards compatibility" commit addresses the The bundle test was updated to cover both Remaining issues:
|
| /// Since we get txs from mempool and also from builder's endpoint eth_sendRawTransaction | ||
| /// MempoolTx is modeling both (is_private <=> came internally through eth_sendRawTransaction). | ||
| /// @Pending rename to something like SingleTx. | ||
| #[derive(Debug, Clone, PartialEq, Eq, Hash)] |
There was a problem hiding this comment.
The derived PartialEq and Hash include is_private, so two MempoolTx values with the same underlying transaction but different is_private flags will compare as unequal and hash differently. This isn't a correctness issue for the order pool (which deduplicates via OrderId based on tx hash alone), but it's semantically surprising — the same transaction shouldn't be "different" based on how it arrived. Consider a manual Hash/PartialEq impl that delegates only to tx_with_blobs, or at minimum add a comment explaining why the derived behavior is acceptable.
1 participant
📝 Summary
We distinguish between mempool txs and txs from eth_sendRawTransaction so MempoolTxsDetector treats them correctly.
✅ I have completed the following steps:
make lintmake test