validation: Pass tx pool reference into CheckSequenceLocks #13783

pull MarcoFalke wants to merge 1 commits into bitcoin:master from MarcoFalke:Mf1807-txPoolValidation changing 4 files +8 −8
  1. MarcoFalke commented at 6:28 PM on July 27, 2018: member

    CheckSequenceLocks is called from ATMP and the member function CTxMemPool::removeForReorg without passing in the tx pool object that is used in those function's scope and instead using the global ::mempool instance.

    This fix should be refactoring only, since currently there is only one (global) tx pool in normal operation. Though, it fixes hard to track down issues in future settings where more than one mempool exists at a time. (E.g. for tests, rpc or p2p tx relay purposes)

  2. MarcoFalke added the label Refactoring on Jul 27, 2018
  3. MarcoFalke added the label Validation on Jul 27, 2018
  4. MarcoFalke commented at 8:46 PM on July 30, 2018: member

    This is required for and split off of #13804

  5. DrahtBot commented at 11:22 AM on August 1, 2018: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->Reviewers, this pull request conflicts with the following ones:

    • #13864 (validation: Document where we are intentionally ignoring bool return values from validation related functions by practicalswift)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

  6. promag commented at 12:41 PM on August 1, 2018: member

    Tested ACK fad3b321.

  7. DrahtBot cross-referenced this on Aug 3, 2018 from issue validation: Document where we are intentionally ignoring bool return values from validation related functions by practicalswift
  8. MarcoFalke cross-referenced this on Aug 24, 2018 from issue WIP: Transaction Pool Layer by MarcoFalke
  9. DrahtBot added the label Needs rebase on Aug 25, 2018
  10. MarcoFalke force-pushed on Aug 25, 2018
  11. DrahtBot removed the label Needs rebase on Aug 26, 2018
  12. MarcoFalke force-pushed on Aug 29, 2018
  13. MarcoFalke force-pushed on Sep 10, 2018
  14. MarcoFalke force-pushed on Sep 10, 2018
  15. Pass tx pool reference into CheckSequenceLocks fa511e8dad
  16. MarcoFalke force-pushed on Sep 11, 2018
  17. ryanofsky commented at 8:46 PM on September 12, 2018: contributor

    ~utACK fac95398366f644911b58f1605e6bc37fb76782d~. Change is simple and looks good if needed for #13804.

  18. MarcoFalke commented at 6:58 PM on September 17, 2018: member

    @ryanofsky It seems the commit you reviewed is unrelated to this pull request?

  19. ryanofsky commented at 7:10 PM on September 17, 2018: contributor

    @ryanofsky It seems the commit you reviewed is unrelated to this pull request?

    utACK fa511e8dad87ddee7bf03b82f2ed69e546021004, pasted the wrong hash previously

  20. in src/validation.cpp:366 in fa511e8dad 
     360 | @@ -361,10 +361,10 @@ bool TestLockPointValidity(const LockPoints* lp)
 361 |      return true;
 362 |  }
 363 |  
 364 | -bool CheckSequenceLocks(const CTransaction &tx, int flags, LockPoints* lp, bool useExistingLockPoints)
 365 | +bool CheckSequenceLocks(const CTxMemPool& pool, const CTransaction& tx, int flags, LockPoints* lp, bool useExistingLockPoints)
 366 |  {
 367 |      AssertLockHeld(cs_main);
    promag commented at 9:38 PM on September 17, 2018:

    Unrelated to this PR, but couldn't this AssertLockHeld be removed?

    MarcoFalke commented at 10:07 PM on September 17, 2018:

    Good point, but I think we want to keep these for now, since not all compilers support these compile-time annotations and sometimes they have to be disabled due to shortcomings.

    promag commented at 10:38 PM on September 17, 2018:

    On the other hand, the annotation is incomplete, missing pool.cs?

    MarcoFalke commented at 10:50 PM on September 17, 2018:

    True, but should be done in a separate pull, since the changes required are non-trivial (more than one line)

    promag commented at 6:13 PM on October 8, 2018:

    In order to add that annotation txmempool.h must be included in validation.h and I'm not sure if that is correct because of the circular dependency "txmempool -> validation -> txmempool". I don't know what is the plan here but I think mempool should not depend on validation or am I wrong?

  21. promag commented at 9:38 PM on September 17, 2018: member

    utACK fa511e8.

  22. promag cross-referenced this on Oct 10, 2018 from issue Add compile time checking for cs_main locks which we assert at run time by practicalswift
  23. fanquake cross-referenced this on Oct 10, 2018 from issue Remove redundant run time assertions for locks already checked at compile time by practicalswift
  24. MarcoFalke merged this on Oct 27, 2018
  25. MarcoFalke closed this on Oct 27, 2018
  26. MarcoFalke deleted the branch on Oct 27, 2018
  27. MarcoFalke referenced this in commit efaf2d85e3 on Oct 27, 2018
  28. jasonbcox referenced this in commit 6518f3585e on Oct 11, 2019
  29. jonspock referenced this in commit bb61b81193 on Dec 27, 2019
  30. jonspock referenced this in commit df27323a67 on Dec 29, 2019
  31. Munkybooty referenced this in commit 0cc7852056 on Jul 22, 2021
  32. Munkybooty referenced this in commit 47aaacb2fc on Jul 22, 2021
  33. Munkybooty referenced this in commit 8eab98a823 on Jul 23, 2021
  34. Munkybooty referenced this in commit c7d1cbf30d on Jul 23, 2021
  35. Munkybooty referenced this in commit 14af9c67b6 on Jul 26, 2021
  36. bitcoin locked this on Sep 8, 2021
Contributors
MarcoFalkeDrahtBotpromagryanofsky
Labels
RefactoringValidation
Linked (view graph)
#13804 WIP: Transaction Pool Layer#14443 Remove redundant run time assertions for locks already checked at compile time#14444 Add compile time checking for cs_main locks which we assert at run time
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-05-19 06:54 UTC