Skip to content

tx-sub: added mempoolLastTicket to MempoolSnapshot #5184

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

crocodile-dentist
Copy link
Contributor

Description

This is just to expose what consensus already tracks so we don't have to traverse the list of transactions to find the last tx index/ticket. This will also come in handy in dmq node to client code.

Checklist

Quality

  • Commit sequence makes sense and have useful messages, see ref.
  • New tests are added and existing tests are updated.
  • Self-reviewed the PR.

Maintenance

  • Linked an issue or added the PR to the current sprint of ouroboros-network project.
  • Added labels.
  • Updated changelog files.
  • The documentation has been properly updated, see ref.

@crocodile-dentist crocodile-dentist self-assigned this Aug 8, 2025
@crocodile-dentist crocodile-dentist requested a review from a team as a code owner August 8, 2025 15:45
@crocodile-dentist crocodile-dentist added tx-submission Issues related to tx-submission protocol dmq-node Issues / PRs related to DMQ Node labels Aug 8, 2025
@github-project-automation github-project-automation bot moved this to In Progress in Ouroboros Network Aug 8, 2025
@@ -131,7 +131,8 @@ getMempoolReader (Mempool mempool) =
\idx -> zipWith f [idx + 1 ..] (toList $ Seq.drop idx seq),
-- why do I need to use `pred`?
mempoolLookupTx = flip Seq.lookup seq . pred,
mempoolHasTx = \txid -> isJust $ find (\tx -> getTxId tx == txid) seq
mempoolHasTx = \txid -> isJust $ find (\tx -> getTxId tx == txid) seq,
mempoolLastTicket = Just $ Seq.length seq
Copy link
Contributor

Choose a reason for hiding this comment

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

At least in main branch, we'll need to subtract 1, as the indexes are 0-based, but not in this branch. So this is not the reason why the test fails.

Should we actually return Nothing if the list is empty?

Suggested change
mempoolLastTicket = Just $ Seq.length seq
mempoolLastTicket = if Seq.length seq > 0 then
then Seq.length seq - 1
else Nothing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've tried this and it didn't work. I've also added a counter to the mempool state because perhaps relying on the length is not correct, but that also didn't work. I will have to look at the logs more carefully as I've ran out of obvious simple tweaks.

Copy link
Contributor Author

@crocodile-dentist crocodile-dentist Aug 11, 2025

Choose a reason for hiding this comment

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

The problem is that this mempoolLastTicket moves the window too much, so we skip tx's. This doesn't matter for tx submission that much where we only ask for a few at a time, but the local dmq protocols will probably transmit more between the peers. We could change mempoolTxIdsAfter to mempoolTxIdsSlice to do what we want.

As an aside, I've noticed that the tx submission server could be more eager in requesting tx id's. The server will often make a request for 1 txid when it could ask for all that the protocol allows. For eg, here is an example of an initial request:
TraceTxInboundDecision (TxDecision {txdTxIdsToAcknowledge = NumTxIdsToAck 0, txdTxIdsToRequest = NumTxIdsToReq 1, txdPipelineTxIds = False, txdTxsToRequest = fromList [], txdTxsToMempool = []})
It only asked for one, when it could have requested for the maximum unacknowledged limit.
Later, after receving the single lone tx, it will send another request for one tx id while acknowledging that prior tx id.
TraceTxInboundDecision (TxDecision {txdTxIdsToAcknowledge = NumTxIdsToAck 1, txdTxIdsToRequest = NumTxIdsToReq 1, txdPipelineTxIds = False, txdTxsToRequest = fromList [], txdTxsToMempool [...]=

[edit] nevermind, this is just from the arbitrary instance for tx decision policy.

@crocodile-dentist crocodile-dentist marked this pull request as draft August 18, 2025 20:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dmq-node Issues / PRs related to DMQ Node tx-submission Issues related to tx-submission protocol
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

2 participants