Should the transactions retrieved from the mempool match those from a forged block?

[dnadales]

The comment in forgeBlock states that all the transactions passed to it could be included up to the maximum block size. We use this function in the forge loop, where after forging a block we encounter the following comment:

-- NOTE: we are tracing the transactions we retrieved from the Mempool,
-- not the transactions actually /in the block/. They should always
-- match, if they don't, that would be a bug.

Is this comment correct? It seems to contradict the fact that all transactions that we get from a snapshot are added up to the maximum block size.

[dcoutts]

Perhaps I'm missing something but I don't see the contradiction. In the absence of bugs these are the same thing.

Note that it's tracing this

        trace $ TraceAdoptedBlock currentSlot newBlock txs

where you might expect that txs is extracted from the newBlock, but for the reason the (full version of the) comment explains, the context doesn't allow extracting the txs from the block. So instead it's using the txs that were used to create the block.

        -- Actually produce the block
        newBlock <- lift $
          Block.forgeBlock blockForging
            ...
            txs
            ...

As the comment says, in the absence of bugs these will be the same thing.

[dnadales]

But what would happen if the size of txs exceeds the block size by a large margin? In such case, txs and the transactions inside newBlock will not be the same, and this is not due to a bug, is it? 🤔

[dcoutts]

Suppose the newBlock does not contain all of the txs, what is the consequence?

The main thing that the txs is used for is tracing, so that would cause some confusion but not any behaviour changes.

It's also used in an exceptional case:

              -- We just produced a block that is invalid according to the
              -- ledger in the ChainDB, while the mempool said it is valid.
              -- There is an inconsistency between the two!
              --
              -- Remove all the transactions in that block, otherwise we'll
              -- run the risk of forging the same invalid block again. This
              -- means that we'll throw away some good transactions in the
              -- process.
              lift $ removeTxs mempool (map (txId . txForgetValidated) txs)

The consequence there would be dropping too many txs but that's not a big deal for an exceptional case.

On re-reading the code, I think the comment is confusing and/or wrong.

The txs is all the txs in the mempool, but only a prefix of them will be included into newBlock (up to the block size limit). The comment "They should always match" is misleading since is suggests the sequences will be equal, though perhaps it was intended to mean that the prefix in the block will match the sequence from the mempool, in the sense that the individual transactions will match, is just that we don't include them all.

The tracing is indeed confusing since it includes both the new block and all the txs from the mempool.

[dnadales]

Submitted a small PR to clarify this.