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

Introduce "already validated" flag for ledger layer and use it #440

Closed
edsko opened this issue Apr 12, 2019 · 3 comments · Fixed by #862
Closed

Introduce "already validated" flag for ledger layer and use it #440

edsko opened this issue Apr 12, 2019 · 3 comments · Fixed by #862
Assignees
Labels
byron ledger integration consensus issues related to ouroboros-consensus

Comments

@edsko
Copy link
Contributor

edsko commented Apr 12, 2019

The ledger DB will frequently apply the ledger rules to blocks that have previously been validated; we are not necessarily interested in checking whether the block is valid yes or no (we know that it is), but just need the new ledger state (which we didn't store for memory reasons). It indicates this through PrevApplied; however, the current integration with the ledger layer does not actually allow for this parameter, nor does the ledger layer itself (to be the best of my knowledge). It is important that this is changed (UpdateLedger, integration in the LgrDB part of the ChainDB implementation), otherwise applying blocks will be significantly more costly than it needs to be.

/cc @nc6

@edsko edsko added the consensus issues related to ouroboros-consensus label Apr 12, 2019
@edsko edsko added this to the Consensus_IntegrateLedger milestone Apr 12, 2019
@edsko
Copy link
Contributor Author

edsko commented May 24, 2019

This is partly in place. In the mempool we have

class UpdateLedger b => ApplyTx b where
  ...

  -- | Apply transaction we have not previously seen before
  applyTx :: LedgerConfig b
          -> GenTx b
          -> LedgerState b
          -> Except (ApplyTxErr b) (LedgerState b)

  -- | Re-apply a transaction
  --
  -- When we re-apply a transaction to a potentially different ledger state
  -- expensive checks such as cryptographic hashes can be skipped, but other
  -- checks (such as checking for double spending) must still be done.
  reapplyTx :: LedgerConfig b
            -> GenTx b
            -> LedgerState b
            -> Except (ApplyTxErr b) (LedgerState b)

  -- | Re-apply a transaction to the very same state it was applied in before
  --
  -- In this case no error can occur.
  --
  -- See also 'ldbConfReapply' for comments on implementing this function.
  reapplyTxSameState :: LedgerConfig b
                     -> GenTx b
                     -> LedgerState b
                     -> LedgerState b

For blocks this 3-way split is unnecessary, because there can be no difference between reapply and reapplySameState: blocks can only be applied in a single specific state. For this reason the LedgerDB does

data LedgerDbConf m l r b e = LedgerDbConf {
      ...

      -- | Apply a block (passed by value)
    , ldbConfApply   :: b -> l -> Either e l

      -- | Apply a previously applied block
      --
      -- Since a block can only be applied to a single, specific, ledger state,
      -- if we apply a previously applied block again it will be applied in the
      -- very same ledger state, and therefore can't possibly fail.
      --
      -- NOTE: During testing it might be useful to implement 'ledgerDbReapply'
      -- using the code that /does/ do all checks, and throw an exception if
      -- any of them fail (as this would indicate a bug in the code somewhere).
      -- We do not have sufficient context in the ledger DB to /verify/ that
      -- when we reapply a block we are applying it to the correct ledger.
    , ldbConfReapply :: b -> l -> l

However, we don't yet model this in UpdateLedger. We should. Moreover, we will need support from the ledger layer. /cc @nc6 and @ruhatch .

@edsko
Copy link
Contributor Author

edsko commented May 24, 2019

@mrBliss
Copy link
Contributor

mrBliss commented Jun 17, 2019

Ideally, this is done before we do system-level benchmarks. It must be done for mainnet. Especially for the mempool, otherwise we'd re-evaluate every tx every time.

iohk-bors bot added a commit that referenced this issue Aug 2, 2019
862: [#440] Implement "already validated" flag for block and tx application r=intricate a=intricate

Closes #440 

Co-authored-by: Luke Nadur <[email protected]>
iohk-bors bot added a commit that referenced this issue Aug 6, 2019
862: [#440] Implement "already validated" flag for block and tx application r=mrBliss a=intricate

Closes #440 

880: Use record type for output of protocol tests r=mrBliss a=nfrisby

`broadcastNetwork` currently just returns a map. In tickets #231 etc and related debugging/investigations, I've wanted programmatic access to more information from the run.

This PR is a general refinement that simplifies the types within each per-protocol family member's test code and makes it easier to add information to the general test framework.

Co-authored-by: Luke Nadur <[email protected]>
Co-authored-by: Nicolas Frisby <[email protected]>
@iohk-bors iohk-bors bot closed this as completed in #862 Aug 6, 2019
@mrBliss mrBliss removed this from the Consensus_IntegrateByronLedger milestone Nov 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
byron ledger integration consensus issues related to ouroboros-consensus
Projects
None yet
3 participants