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

Expose transaction size to tx submission #4926

Merged
merged 2 commits into from
Sep 20, 2024

Conversation

crocodile-dentist
Copy link
Contributor

@crocodile-dentist crocodile-dentist commented Aug 8, 2024

Description

A callback, exposing CBOR-encoded transaction size as it is when transmitted over the network, is provided to
tx submission inbound and outbound handlers. The size returned by this function does not take into account some additional wrapping that happens when a transaction is sent across the network. Namely, it does not include header data for CBOR-in-CBOR encoding (a tag + it's value to indicate a blob payload, and an encodeBytes tag ~ so roughly 3 words) neither the top level header which consists of EncodeListLen tag + it's value, era word tag + era index word, so ~4 words).

IntersectMBO/cardano-ledger#4521
IntersectMBO/ouroboros-consensus#1211

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.

Comment on lines 84 to 88
-> (tx -> SizeInBytes) -- ^ get size of CBOR encoded transaction
-> NodeToNodeVersion
-> ControlMessageSTM m
-> TxSubmissionClient txid tx m ()
txSubmissionOutbound tracer maxUnacked TxSubmissionMempoolReader{..} _version controlMessageSTM =
txSubmissionOutbound tracer maxUnacked TxSubmissionMempoolReader{..} _txSize _version controlMessageSTM =
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the outbound side need the callback, or can we use the sizes from the mempool?

Maybe we should change the mempool interface, to give us just [txid]s rather than [(txid, SizeInBytes)] - so we don't have multiple ways of getting that information, possibly in a non-compatible way.

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 removed the callback from the outbound side. We can keep the interface as-is for now, and if/when the annotated codec gets integrated we can remove SizeInBytes from the mempool API.

Comment on lines 186 to 188
txSubmissionInbound tracer (NumTxIdsToAck maxUnacked) mpReader mpWriter _version =
txSubmissionInbound tracer (NumTxIdsToAck maxUnacked) mpReader mpWriter _txSize _version =
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we verify the tx sizes using an assert-ion? I'd expect that production nodes run without assertions, and this would give us a chance to verify that this works well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea - @karknu commented on slack I think that this maybe should be in an #ifdef.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need CPP for it, assert-tions are only enforced if one compiled the node with -fno-ignore-asserts flag.

@crocodile-dentist crocodile-dentist changed the base branch from master to coot/tx-submission September 20, 2024 09:37
@crocodile-dentist crocodile-dentist force-pushed the mwojtowicz/tx-size branch 2 times, most recently from e0a9467 to 7500a79 Compare September 20, 2024 15:16
@crocodile-dentist crocodile-dentist force-pushed the mwojtowicz/tx-size branch 2 times, most recently from bcf6575 to 0cc240c Compare September 20, 2024 15:25
@crocodile-dentist crocodile-dentist merged commit 7a2f47b into coot/tx-submission Sep 20, 2024
3 of 7 checks passed
@crocodile-dentist crocodile-dentist deleted the mwojtowicz/tx-size branch September 20, 2024 15:32
crocodile-dentist added a commit that referenced this pull request Sep 20, 2024
* Added callback providing tx size to txSubmissionInbound

* changelog update
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tx-submission Issues related to tx-submission protocol
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants