validation: Pass tx pool reference into CheckSequenceLocks#13783
Merged
maflcko merged 1 commit intobitcoin:masterfrom Oct 27, 2018
Merged
validation: Pass tx pool reference into CheckSequenceLocks#13783maflcko merged 1 commit intobitcoin:masterfrom
maflcko merged 1 commit intobitcoin:masterfrom
Conversation
Member
Author
|
This is required for and split off of #13804 |
Contributor
Reviewers, this pull request conflicts with the following ones:
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. |
Contributor
|
Tested ACK fad3b321. |
maflcko
force-pushed
the
Mf1807-txPoolValidation
branch
from
fad3b32 to
fa7f3fc
Compare
maflcko
force-pushed
the
Mf1807-txPoolValidation
branch
from
fa7f3fc to
fa7edd4
Compare
maflcko
force-pushed
the
Mf1807-txPoolValidation
branch
2 times, most recently
from
fd69e5b to
fa287b8
Compare
maflcko
force-pushed
the
Mf1807-txPoolValidation
branch
from
fa287b8 to
fa511e8
Compare
Member
Author
|
@ryanofsky It seems the commit you reviewed is unrelated to this pull request? |
Contributor
utACK fa511e8, pasted the wrong hash previously |
promag
reviewed
Sep 17, 2018
| 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); |
Contributor
There was a problem hiding this comment.
Unrelated to this PR, but couldn't this AssertLockHeld be removed?
Member
Author
There was a problem hiding this comment.
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.
Contributor
There was a problem hiding this comment.
On the other hand, the annotation is incomplete, missing pool.cs?
Member
Author
There was a problem hiding this comment.
True, but should be done in a separate pull, since the changes required are non-trivial (more than one line)
Contributor
There was a problem hiding this comment.
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?
maflcko
pushed a commit
that referenced
this pull request
Oct 27, 2018
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
jasonbcox
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Oct 11, 2019
Summary: `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. Backport of Bitcoin Core PR13783 bitcoin/bitcoin#13783 Test Plan: ``` make check-all ``` Reviewers: Fabien, #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D4178
jonspock
pushed a commit
to jonspock/devault
that referenced
this pull request
Dec 27, 2019
Summary: `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. Backport of Bitcoin Core PR13783 bitcoin/bitcoin#13783 Test Plan: ``` make check-all ``` Reviewers: Fabien, #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D4178
jonspock
pushed a commit
to devaultcrypto/devault
that referenced
this pull request
Dec 29, 2019
Summary: `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. Backport of Bitcoin Core PR13783 bitcoin/bitcoin#13783 Test Plan: ``` make check-all ``` Reviewers: Fabien, #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D4178
Munkybooty
pushed a commit
to Munkybooty/dash
that referenced
this pull request
Jul 26, 2021
…uenceLocks 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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
CheckSequenceLocksis called from ATMP and the member functionCTxMemPool::removeForReorgwithout passing in the tx pool object that is used in those function's scope and instead using the global::mempoolinstance.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)