Skip to content

chore(mempool): user batch followups#2034

Merged
Mirko-von-Leipzig merged 10 commits intomainfrom
mirko/user-batch-fixups
May 7, 2026
Merged

chore(mempool): user batch followups#2034
Mirko-von-Leipzig merged 10 commits intomainfrom
mirko/user-batch-fixups

Conversation

@Mirko-von-Leipzig
Copy link
Copy Markdown
Collaborator

@Mirko-von-Leipzig Mirko-von-Leipzig commented May 4, 2026

Addresses the minor #1995 follow-up tasks, others have issues created now.

Closes #1995

@Mirko-von-Leipzig Mirko-von-Leipzig added the no changelog This PR does not require an entry in the `CHANGELOG.md` file label May 4, 2026
@Mirko-von-Leipzig Mirko-von-Leipzig marked this pull request as ready for review May 4, 2026 12:58
for tx in &txs {
assert!(self.by_tx.insert(*tx, batch).is_none());
}
assert!(self.by_batch.insert(batch, txs).is_none());
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we want the assert here? Even if we expect this API to never be used with the same inserts more than once, I would think this type should be factored to be robust against that.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its more that this is a helper function/struct, and in the caller's scope a double insert would constitute a logical bug.

So its not that the BatchTxMap cannot be hardened against this, its that this indicates a larger !@#!%$ up in the mempool in general. And this is a convenient place to do an assert on this.


fn contains_tx(&self, tx: &TransactionId) -> bool {
self.by_tx.contains_key(tx)
}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be worth adding quick unit tests for this type

Copy link
Copy Markdown
Collaborator

@sergerad sergerad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but I'm not super familiar with mempool stack. Have added a comment about the BatchTxMap impl which I think is worth making robust against multiple inserts.

@Mirko-von-Leipzig Mirko-von-Leipzig merged commit c27bd14 into main May 7, 2026
20 checks passed
@Mirko-von-Leipzig Mirko-von-Leipzig deleted the mirko/user-batch-fixups branch May 7, 2026 14:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no changelog This PR does not require an entry in the `CHANGELOG.md` file

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants