Mempool: Use Consensus::CheckTxInputs direclty over main::CheckInputs #8346

pull jtimon wants to merge 1 commits into bitcoin:master from jtimon:0.12.99-consensus-mempool-checks changing 2 files +20 −2
  1. jtimon commented at 7:11 PM on July 16, 2016: contributor

    The mempool doesn't need to CheckInputs with fCheckScripts=false, it can call Consensus::CheckTxInputs() directly. We could also remove the parameter from CheckInputs now, since all the remaining callers always pass fCheckScripts=true. But I have left for later, when/if we separate the function in two: one for libconsensus, the other for the multi-threaded validation (both called from ConnectBlock, the former also called from AcceptToMemeoryPool see https://github.com/jtimon/bitcoin/commit/24a09994138267fb132ba69b5128d094aac84a31 ).

    I've tried to do this several times, but usually combined with moveonly commits and more changes. One of those times, @sipa suggested to remove the checks here instead, but I can't remember his reasoning or find a reference. I believe he changed his mind later.

    Ping @NicolasDorier @kanzure

  2. in src/txmempool.cpp:None in c1d83a93bc outdated 
     737 | @@ -737,7 +738,7 @@ void CTxMemPool::check(const CCoinsViewCache *pcoins) const
 738 |              waitingOnDependants.push_back(&(*it));
 739 |          else {
 740 |              CValidationState state;
 741 | -            assert(CheckInputs(tx, state, mempoolDuplicate, false, 0, false, NULL));
 742 | +            assert(Consensus::CheckTxInputs(tx, state, mempoolDuplicate, nSpendHeight));
    paveljanik commented at 8:03 PM on July 16, 2016:

    Hmm, does CheckInputs change state in assert in the original code (i.e. is it against our Assertions should not have side-effects rule)?

    jtimon commented at 9:20 AM on July 17, 2016:

    I didn't know we had just a rule, is there a place where those "rules" are documented? If we do, I suggest we change that rule to at least not apply to CValidationState. In any case, this is not a change in this PR: there's no change in functionality here, it was doing the same before.

    jtimon commented at 11:03 AM on July 17, 2016:

    Thanks. So, yes, it seems these asserts were not complying with that rule. Neither before not after my change. If there's an easy fix to add here, I'm all for it. Should I launch an exception instead ? It doesn't seem right the way CTxMemPool::check is written... Maybe it's enough to rename the offending state local variables to dummyState ?

    MarcoFalke commented at 11:05 AM on July 17, 2016:

    Assign the result to a local scope bool and assert the value of the bool?

    jtimon commented at 11:14 AM on July 17, 2016:

    Oh, yeah, is as simple as that. Happy to code and squash that.

  3. NicolasDorier commented at 3:12 AM on July 17, 2016: contributor

    CheckTxInputs should return true; if it is a coinbase. (agree that in theory it should not happen, but I prefer that we don't change the semantic by mistake)

  4. jtimon commented at 9:18 AM on July 17, 2016: contributor

    @NicolasDorier I'm doing that later in https://github.com/jtimon/bitcoin/commits/0.12.99-consensus , but since you refuse to review that branch...Little steps first, preparations, remember?

  5. jtimon commented at 11:44 AM on July 17, 2016: contributor

    Updated solving @paveljanik 's nit as suggested by @MarcoFalke . Didn't squashed the commit yet though.

  6. in src/txmempool.cpp:None in fa3e82563d outdated 
     656 | @@ -657,6 +657,9 @@ void CTxMemPool::check(const CCoinsViewCache *pcoins) const
 657 |      uint64_t innerUsage = 0;
 658 |  
 659 |      CCoinsViewCache mempoolDuplicate(const_cast<CCoinsViewCache*>(pcoins));
 660 | +    const int64_t nSpendHeight = GetSpendHeight(mempoolDuplicate);
 661 | +    CValidationState state;
 662 | +    bool fCheckResult = false;
    MarcoFalke commented at 11:47 AM on July 17, 2016:

    Would be better to move this into the smaller else {} scopes

    jtimon commented at 12:19 PM on July 17, 2016:

    Sorry, I actually moved the state out. I thought this would be clearer. I will change it back and declare the bool twice in their smaller scopes. Reusing the bool actually takes one extra line.

  7. jonasschnelli added the label Refactoring on Jul 17, 2016
  8. NicolasDorier commented at 3:35 PM on July 17, 2016: contributor

    @jtimon just saying the returns true if coinbase should be part of this commit, not later. Anyway, not blocking for me ACK fa3e82563db63c6e601ccb440a11ad4ba66c9a5b

    EDIT: oh I think you were responding to the first comment I made and deleted shortly before you replied. Don't take note to what I said before, my only complain on this commit is that CheckTxInputs should returns true if it is a coinbase.

  9. jtimon commented at 3:40 PM on July 17, 2016: contributor

    I'm not sure I understand. Which return true if coinbase?

  10. NicolasDorier commented at 3:41 PM on July 17, 2016: contributor

    CheckTxInputs should returns true if it is a coinbase.

  11. NicolasDorier commented at 3:41 PM on July 17, 2016: contributor

    as the main::CheckInputs was doing.

  12. in src/txmempool.cpp:None in fa3e82563d outdated 
     738 | @@ -736,22 +739,22 @@ void CTxMemPool::check(const CCoinsViewCache *pcoins) const
 739 |          if (fDependsWait)
 740 |              waitingOnDependants.push_back(&(*it));
 741 |          else {
 742 | -            CValidationState state;
 743 | -            assert(CheckInputs(tx, state, mempoolDuplicate, false, 0, false, NULL));
 744 | +            fCheckResult = Consensus::CheckTxInputs(tx, state, mempoolDuplicate, nSpendHeight);
    NicolasDorier commented at 3:49 PM on July 17, 2016:

    here, either you should add

    fCheckResult = Consensus::CheckTxInputs(tx, state, mempoolDuplicate, nSpendHeight) || tx.IsCoinbase;

    Or better, CheckTxInputs should do it internally.

    jtimon commented at 2:20 PM on July 18, 2016:

    Oh, yeah again lost in re-write. it's even stated in the Preconditions of Consensus::CheckTxInputs in the doc I'm adding just now!

  13. jtimon force-pushed on Jul 18, 2016
  14. jtimon commented at 2:27 PM on July 18, 2016: contributor

    Updated with more fixes and after squashing.

  16. NicolasDorier commented at 12:56 PM on July 21, 2016: contributor

    utACK 00b4943

  17. btcdrak commented at 12:56 PM on July 21, 2016: contributor

    utACK 00b4943

    Nice proof of work commit there :-p

  18. MarcoFalke commented at 1:17 PM on July 21, 2016: member

    utACK 00b4943c2794fecf93c21d3db814ea7b2e362087

  19. NicolasDorier commented at 8:52 PM on July 28, 2016: contributor

    I confirm this one can get merged before #8259. I'll need to backport to 0.13 #8259 which does not have this PR, but that's trivial change.

  20. sipa commented at 9:30 PM on July 28, 2016: member

    I'm not a fan of having the code in main.cpp while the header is in consensus.h. What is the plan afterwards? Will CheckTxInputs move to consesus/consensus.cpp as well? Will that mean that CCoinsViewCache (on which it depends) also moves to consensus?

  21. NicolasDorier commented at 9:35 PM on July 28, 2016: contributor

    @sipa most likely in the future for libconsensus, we'll need to make an interface and replace CCoinsViewCache dependencies by such interface. CCoinsViewCache would stay out of consensuslib.

    The implementation of the interface in libconsensus will use user provided callbacks, while bitcoin core's implementation would use CCoinViewCache under the hood.

  22. sipa commented at 9:36 PM on July 28, 2016: member

    @NicolasDorier I mean in the immediate future. It's not a nice state that the definition for a function is inside a module where the code can't actually move yet.

  23. NicolasDorier commented at 9:40 PM on July 28, 2016: contributor

    @sipa ah yes, I also think so. I'm not against moving before, but I would prefer moving the declaration after as well.

  24. sipa commented at 9:43 PM on July 28, 2016: member

    I'd rather not move things until they can actually cleanly moved.

  25. jtimon commented at 10:02 PM on July 28, 2016: contributor

    @sipa My plan was to move it later to tx_verify as shown in #8329, but there's many other options. I'm happy moving the declaration to main.h (although I strongly believe the declaration doesn't belong there) or the definition to whatever place outside of main, even if it means moving the declaration outside of consensus/consensus.h. Whatever looks easier to merge in the short term.

  26. Mempool: Use Consensus::CheckTxInputs direclty over main::CheckInputs a6cc299541
  27. jtimon force-pushed on Jul 28, 2016
  28. jtimon commented at 10:52 PM on July 28, 2016: contributor

    Updated with the new declaration in main. Please, the sooner you nick or nack-or-at-least-nack-for-this-pr nit/NACK the new doc, the better.

  29. NicolasDorier commented at 11:04 PM on July 28, 2016: contributor

    utACK a6cc299

  30. sipa commented at 11:04 PM on July 28, 2016: member

    utACK a6cc299541fc9df5af010ce63eb1dd34d8c4b6e2

  31. paveljanik commented at 7:51 PM on July 31, 2016: contributor

    nit: commit message typo: direclty -> directly

  33. sipa merged this on Jul 31, 2016
  34. sipa closed this on Jul 31, 2016
  35. sipa referenced this in commit f798b891bc on Jul 31, 2016
  36. codablock referenced this in commit 38862272e6 on Sep 19, 2017
  37. codablock referenced this in commit 615c6d1c9f on Dec 29, 2017
  38. codablock referenced this in commit 60668fb93d on Jan 8, 2018
  39. zkbot referenced this in commit 84c19b8a87 on Feb 8, 2018
  40. zkbot referenced this in commit e1f3a15fdc on Feb 8, 2018
  41. zkbot referenced this in commit eb3128ab0a on Feb 19, 2018
  42. zkbot referenced this in commit 6db10127a9 on Feb 20, 2018
  43. zkbot referenced this in commit 8487be8360 on Feb 20, 2018
  44. andvgal referenced this in commit 9a8288f372 on Jan 6, 2019
  45. DrahtBot locked this on Sep 8, 2021
Contributors
jtimonpaveljanikMarcoFalkeNicolasDorierinstagibbsbtcdraksipa
Labels
Refactoring
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-17 15:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me