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

validation: Pass tx pool reference into CheckSequenceLocks #13783

Merged
merged 1 commit into from Oct 27, 2018

Conversation

Projects
None yet
4 participants
@MarcoFalke
Copy link
Member

commented Jul 27, 2018

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)

@MarcoFalke

This comment has been minimized.

Copy link
Member Author

commented Jul 30, 2018

This is required for and split off of #13804

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Aug 1, 2018

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.

@promag

This comment has been minimized.

Copy link
Member

commented Aug 1, 2018

Tested ACK fad3b321.

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1807-txPoolValidation branch Aug 25, 2018

@DrahtBot DrahtBot removed the Needs rebase label Aug 26, 2018

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1807-txPoolValidation branch Aug 29, 2018

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1807-txPoolValidation branch 2 times, most recently Sep 10, 2018

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1807-txPoolValidation branch to fa511e8 Sep 11, 2018

@ryanofsky
Copy link
Contributor

left a comment

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

@MarcoFalke

This comment has been minimized.

Copy link
Member Author

commented Sep 17, 2018

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

@ryanofsky

This comment has been minimized.

Copy link
Contributor

commented Sep 17, 2018

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

utACK fa511e8, pasted the wrong hash previously

@promag
Copy link
Member

left a comment

utACK fa511e8.

@@ -361,10 +361,10 @@ bool TestLockPointValidity(const LockPoints* lp)
return true;
}

bool CheckSequenceLocks(const CTransaction &tx, int flags, LockPoints* lp, bool useExistingLockPoints)
bool CheckSequenceLocks(const CTxMemPool& pool, const CTransaction& tx, int flags, LockPoints* lp, bool useExistingLockPoints)
{
AssertLockHeld(cs_main);

This comment has been minimized.

Copy link
@promag

promag Sep 17, 2018

Member

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

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Sep 17, 2018

Author Member

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.

This comment has been minimized.

Copy link
@promag

promag Sep 17, 2018

Member

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

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Sep 17, 2018

Author Member

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

This comment has been minimized.

Copy link
@promag

promag Oct 8, 2018

Member

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?

@MarcoFalke MarcoFalke merged commit fa511e8 into bitcoin:master Oct 27, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@MarcoFalke MarcoFalke deleted the MarcoFalke:Mf1807-txPoolValidation branch Oct 27, 2018

MarcoFalke added a commit that referenced this pull request Oct 27, 2018

Merge #13783: validation: Pass tx pool reference into CheckSequenceLocks
fa511e8 Pass tx pool reference into CheckSequenceLocks (MarcoFalke)

Pull request description:

  `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)

Tree-SHA512: f0804588c7d29bb6ff05ec14f22a16422b89ab31ae714f38cd07f811d7dc7907bfd14e799c4c1c3121144ff22711019bbe9212b39e2fd4531936a4119950fa49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.