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

txSize reports the incorrect size #1535

Closed
mrBliss opened this issue Jan 30, 2020 · 1 comment · Fixed by #1540
Closed

txSize reports the incorrect size #1535

mrBliss opened this issue Jan 30, 2020 · 1 comment · Fixed by #1540
Assignees
Labels
bug Something isn't working byron ledger integration byron Required for a Byron mainnet: replace the old core nodes with cardano-node consensus issues related to ouroboros-consensus mempool
Milestone

Comments

@mrBliss
Copy link
Contributor

mrBliss commented Jan 30, 2020

txSize for ByronBlock includes the encoding overhead for the different GenTx constructors:

  data GenTx ByronBlock
    = ByronTx             !Utxo.TxId                !(Utxo.ATxAux             ByteString)
    | ByronDlg            !Delegation.CertificateId !(Delegation.ACertificate ByteString)
    | ByronUpdateProposal !Update.UpId              !(Update.AProposal        ByteString)
    | ByronUpdateVote     !Update.VoteId            !(Update.AVote            ByteString)
    deriving (Eq, Generic)
    deriving NoUnexpectedThunks via UseIsNormalForm (GenTx ByronBlock)

  txSize tx =
        1 {- encodeListLen -}
      + 1 {- tag -}
      + (fromIntegral . Strict.length $ mempoolPayloadRecoverBytes tx')
    where
      tx' = toMempoolPayload tx

This means that we're over-estimating transaction sizes.

What it should be:

  txSize =
     fromIntegral . Strict.length . mempoolPayloadRecoverBytes . toMempoolPayload

The byronBlockEncodingOverhead might have to be revisited and it might have to take the number of transactions an argument.

Important: we should check that the size we're reporting matches the expectation.

@mrBliss mrBliss added bug Something isn't working consensus issues related to ouroboros-consensus byron ledger integration mempool byron Required for a Byron mainnet: replace the old core nodes with cardano-node labels Jan 30, 2020
@mrBliss mrBliss added this to the S6 2020-02-13 milestone Jan 30, 2020
@dcoutts
Copy link
Contributor

dcoutts commented Jan 30, 2020

Yes I think this is correct, it's only the +2 that's wrong here. Otherwise it's counting the raw tx size, which is right.

The byronBlockEncodingOverhead might have to be revisited and it might have to take the number of transactions an argument.

Probably not, unless we want to make it very precise. The encoding for the block tx list is at most 3 bytes (since there'll never be more than 64k txs in a block).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working byron ledger integration byron Required for a Byron mainnet: replace the old core nodes with cardano-node consensus issues related to ouroboros-consensus mempool
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants