Skip to content
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

LocalTxMonitor: Add support for GetMeasures #4918

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

fraser-iohk
Copy link

@fraser-iohk fraser-iohk commented Jul 26, 2024

Description

This PR adds a new message to LocalTxMonitor, GetMeasures, which is similar to GetSize (returning the current mempool transaction count, the size of those transactions, and the total mempool capacity), but returns information about several other metrics in addition to the byte size and capacity of the mempool. GetMeasures currently returns size and capacity information for transaction byte size, ledger's execution units (both their memory and execution steps), and the reference scripts size, but can easily be extended to support any new metric types in future while remaining backwards compatible.

This change works towards ouroboros-consensus#1178, and has a dependent PR in ouroboros-consensus

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.

@fraser-iohk fraser-iohk force-pushed the fraser-iohk/localtxmonitor-measures branch from 9488451 to d6eb7bb Compare September 4, 2024 16:55
@fraser-iohk fraser-iohk force-pushed the fraser-iohk/localtxmonitor-measures branch from d6eb7bb to 0a00293 Compare December 11, 2024 14:56
@fraser-iohk fraser-iohk marked this pull request as ready for review December 12, 2024 11:09
@fraser-iohk fraser-iohk requested a review from a team as a code owner December 12, 2024 11:09
@fraser-iohk fraser-iohk force-pushed the fraser-iohk/localtxmonitor-measures branch 3 times, most recently from 1007422 to e415888 Compare December 12, 2024 11:28
@fraser-iohk fraser-iohk added consensus issues related to ouroboros-consensus local-tx-monitor LocalTxMonitor protocol labels Dec 12, 2024
Copy link
Contributor

@coot coot left a comment

Choose a reason for hiding this comment

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

I think that the function codecLocalTxMonitorId needs to be updated too. It doesn't handle:

  • MsgGetMeasures,
  • MsgReplyGetMeasures,
  • MsgGetSizes, and
  • MsgReplyGetSizes.

Maybe we are missing a test for it, or we don't generate these messages in the appropriate arbitrary instance.

Please also update the network-spec doc in ./docs/network-specs/miniprotocols.tex. I added Local Tx-Monitor there recently (you might need to rebase on top of the main branch).

@fraser-iohk fraser-iohk force-pushed the fraser-iohk/localtxmonitor-measures branch from 76cb305 to a342b40 Compare January 23, 2025 12:19
@fraser-iohk fraser-iohk requested a review from coot January 23, 2025 12:27
Copy link
Contributor

@coot coot left a comment

Choose a reason for hiding this comment

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

LGTM

@fraser-iohk fraser-iohk force-pushed the fraser-iohk/localtxmonitor-measures branch 2 times, most recently from fb73a86 to 788630d Compare January 27, 2025 12:37
- adds a new pair of LocalTxMonitor messages: GetMeasures and ReplyGetMeasures
- add a property test `codecIdM` for `codecLocalTxMonitorId`
- "parametrised" -> "parameterised"
@fraser-iohk fraser-iohk force-pushed the fraser-iohk/localtxmonitor-measures branch from 788630d to a859c04 Compare January 27, 2025 12:45
@fraser-iohk fraser-iohk enabled auto-merge January 28, 2025 13:24
@fraser-iohk fraser-iohk requested a review from a team as a code owner January 28, 2025 13:28
@fraser-iohk fraser-iohk force-pushed the fraser-iohk/localtxmonitor-measures branch from 80d9606 to a859c04 Compare January 29, 2025 12:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
consensus issues related to ouroboros-consensus local-tx-monitor LocalTxMonitor protocol
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

Local Tx-Monitor should allow to query all the mempool dimensions
3 participants