refactor: Keep mempool interface in validation

MarcoFalke commented at 4:26 pm on July 30, 2020: member

Next step toward #19556

Instead of relying on the mempool global, each chainstate is given an optional mempool to keep up to date with the tip (block connections, disconnections, reorgs, …)

MarcoFalke added the label Refactoring on Jul 30, 2020

MarcoFalke added the label Validation on Jul 30, 2020

MarcoFalke renamed this:
~~Pass mempool reference to chainstate constructor~~
Pass mempool pointer to chainstate constructor
on Jul 30, 2020

promag commented at 5:12 pm on July 30, 2020: member

Concept ACK, and code change looks good.

Would be nice to have a case with no mempool, even to ease review. Do you have a followup with this?

MarcoFalke commented at 6:33 pm on July 30, 2020: member

Hmm, I think it is hard to write a meaningful test case right now. Locally I tested with

 0diff --git a/src/init.cpp b/src/init.cpp
 1index 50d25e672e..7fb8b3aaa7 100644
 2--- a/src/init.cpp
 3+++ b/src/init.cpp
 4@@ -1563,7 +1563,7 @@ bool AppInitMain(const util::Ref& context, NodeContext& node)
 5             const int64_t load_block_index_start_time = GetTimeMillis();
 6             try {
 7                 LOCK(cs_main);
 8-                chainman.InitializeChainstate(node.mempool);
 9+                chainman.InitializeChainstate(nullptr);
10                 chainman.m_total_coinstip_cache = nCoinCacheUsage;
11                 chainman.m_total_coinsdb_cache = nCoinDBCache;
12

Though, if you have a test commit, I am happy to cherry-pick it here.

promag commented at 10:55 pm on July 30, 2020: member

I think it would be less problematic if you just pass the mempool reference (not pointer) and defer the optional change to when it’s needed - blocksonly or withoutmempool PR.

MarcoFalke commented at 4:52 am on July 31, 2020: member

That was my initial draft, but it would also mean that all changed lines will need to change again. Is there any reason to increase the git blame stack for those lines? See also #19556 (review)

DrahtBot commented at 11:46 am on July 31, 2020: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#19438 (Introduce deploymentstatus by ajtowns)
#19306 (refactor: Replace RecursiveMutex with Mutex in CTxMemPool by hebasto)
#18731 (refactor: Make CCheckQueue RAII-styled by hebasto)
#18077 (net: Add NAT-PMP port forwarding support by hebasto)

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.

in src/validation.cpp:2516 in faab637301 outdated

2511@@ -2508,22 +2512,23 @@ bool CChainState::DisconnectTip(BlockValidationState& state, const CChainParams&
2512     if (!FlushStateToDisk(chainparams, state, FlushStateMode::IF_NEEDED))
2513         return false;
2514 
2515-    if (disconnectpool) {
2516+    if (disconnectpool && m_mempool) {
2517+        AssertLockHeld(m_mempool->cs);

darosior commented at 1:09 pm on July 31, 2020:

Isn’t the asserion redundant with line 2491 ?

MarcoFalke commented at 4:21 pm on July 31, 2020:

Locally it wouldn’t compile for me with clang as soon as I remove it

ajtowns commented at 8:46 pm on August 6, 2020:

The AssertLockHeld here is silencing the compile time thread safety check, and will need to be LockAssertion lock(m_mempool->cs) after #19668 . It’s not redundant with the earlier assertion, because the silencing is scoped to the block, which ends immediately for the if (m_mempool) AssertLockHeld(m_mempool->cs); line.

darosior approved

darosior commented at 1:40 pm on July 31, 2020: member

ACK faab637301a336c318ea8ee0187a74a2c7a92ef3

practicalswift commented at 10:43 am on August 1, 2020: contributor

Concept ACK

jnewbery commented at 11:18 am on August 1, 2020: member

That was my initial draft, but it would also mean that all changed lines will need to change again. Is there any reason to increase the git blame stack for those lines? See also #19556 (comment)

I totally agree with this. If the intention is to eventually allow CChainState to not have a mempool, then it makes sense to have the constructor take a pointer, and then assert that it’s not nullptr. To make mempool optional, you just need to change the internal implementation of CChainState rather than the interface and all call sites.

promag commented at 2:24 pm on August 2, 2020: member

@jnewbery would you drop if (m_mempool) checks?

edit: @MarcoFalke sorry for being pedantic, feel free to ignore.

in src/validation.cpp:2882 in faab637301 outdated

2877@@ -2866,7 +2878,8 @@ bool CChainState::ActivateBestChain(BlockValidationState &state, const CChainPar
2878         LimitValidationInterfaceQueue();
2879 
2880         {
2881-            LOCK2(cs_main, ::mempool.cs); // Lock transaction pool for at least as long as it takes for connectTrace to be consumed
2882+            LOCK(cs_main);
2883+            LOCK(m_mempool ? &m_mempool->cs : nullptr); // Lock transaction pool for at least as long as it takes for connectTrace to be consumed

jnewbery commented at 5:49 pm on August 2, 2020:

For those scratching their heads over whether LOCK(nullptr) is ok, I think it’s fine. It constructs a UniqueLock using this constructor:

https://github.com/bitcoin/bitcoin/blob/a78742830aa35bf57bcb0a4730977a1e5a1876bc/src/sync.h#L163

Which calls the default constructor (1 in https://en.cppreference.com/w/cpp/thread/unique_lock/unique_lock) for the base std::unique_lock<Mutex> class and then exits:

https://github.com/bitcoin/bitcoin/blob/a78742830aa35bf57bcb0a4730977a1e5a1876bc/src/sync.h#L165

In the destructor, the call to owns_lock() returns false (since the unique_lock doesn’t have a locked mutex):

https://github.com/bitcoin/bitcoin/blob/a78742830aa35bf57bcb0a4730977a1e5a1876bc/src/sync.h#L174-L178

and the base class destructor does nothing.

jnewbery commented at 5:55 pm on August 2, 2020: member

Code review ACK faab637301a336c318ea8ee0187a74a2c7a92ef3

@jnewbery would you drop if (m_mempool) checks?

I think it’s fine like this. My alternative approach would have been to leave those out and keep the logic the same, but add an assert m_mempool != nullptr to the constructor, but it’s not a hard review, so I think it’s fine to change the logic now.

promag commented at 6:19 pm on August 2, 2020: member

My alternative approach would have been to leave those out and keep the logic the same, but add an assert m_mempool != nullptr to the constructor

Same thing here. But it’s fine as it is.

hebasto commented at 9:25 am on August 4, 2020: member

Concept ACK.

Could conditional locking stuff hinder effective usage of Thread Safety Analysis (especially when CTxMemPool::cs will transit from RecursiveMutex to Mutex)?

https://clang.llvm.org/docs/ThreadSafetyAnalysis.html#no-conditionally-held-locks

MarcoFalke commented at 10:02 am on August 4, 2020: member

Without a mempool, the mempool lock won’t exist and thus can not be taken. Though, this shouldn’t affect anything outside of validation. E.g. changing cs to a non-recursive mutex or lock annotations outside of validation should be unaffected.

laanwj added this to the "Blockers" column in a project

hebasto commented at 4:14 am on August 6, 2020: member

Without a mempool, the mempool lock won’t exist and thus can not be taken. Though, this shouldn’t affect anything outside of validation. E.g. changing cs to a non-recursive mutex or lock annotations outside of validation should be unaffected.

I guess it won’t work. For example, this patch

 0--- a/src/validation.h
 1+++ b/src/validation.h
 2@@ -645,7 +645,7 @@ public:
 3                       CCoinsViewCache& view, const CChainParams& chainparams, bool fJustCheck = false) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
 4 
 5     // Apply the effects of a block disconnection on the UTXO set.
 6-    bool DisconnectTip(BlockValidationState& state, const CChainParams& chainparams, DisconnectedBlockTransactions* disconnectpool) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
 7+    bool DisconnectTip(BlockValidationState& state, const CChainParams& chainparams, DisconnectedBlockTransactions* disconnectpool) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_mempool->cs);
 8 
 9     // Manual block validity manipulation:
10     bool PreciousBlock(BlockValidationState& state, const CChainParams& params, CBlockIndex* pindex) LOCKS_EXCLUDED(cs_main);

causes -Wthread-safety-analysis warnings regardless of if (m_mempool) AssertLockHeld(m_mempool->cs); and LOCK(m_mempool ? &m_mempool->cs : nullptr); before the DisconnectTip() calls.

I think a such disadvantage should be avoided.

darosior commented at 9:53 am on August 6, 2020: member

 0--- a/src/validation.h
 1+++ b/src/validation.h
 2@@ -645,7 +645,7 @@ public:
 3                       CCoinsViewCache& view, const CChainParams& chainparams, bool fJustCheck = false) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
 4 
 5     // Apply the effects of a block disconnection on the UTXO set.
 6-    bool DisconnectTip(BlockValidationState& state, const CChainParams& chainparams, DisconnectedBlockTransactions* disconnectpool) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
 7+    bool DisconnectTip(BlockValidationState& state, const CChainParams& chainparams, DisconnectedBlockTransactions* disconnectpool) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_mempool->cs);
 8 
 9     // Manual block validity manipulation:
10     bool PreciousBlock(BlockValidationState& state, const CChainParams& params, CBlockIndex* pindex) LOCKS_EXCLUDED(cs_main);

causes -Wthread-safety-analysis warnings regardless of if (m_mempool) AssertLockHeld(m_mempool->cs); and LOCK(m_mempool ? &m_mempool->cs : nullptr); before the DisconnectTip() calls.

I think a such disadvantage should be avoided.

Is such an annotation in the header file dereferencing a pointer that may be NULL valid ?

hebasto commented at 10:04 am on August 6, 2020: member

Is such an annotation in the header file dereferencing a pointer that may be NULL valid ?

Not sure about annotation validity, but Clang accepts it.

I don’t want to lose ability to use EXCLUSIVE_LOCKS_REQUIRED(m_mempool->cs) annotations.

in src/validation.cpp:373 in faab637301 outdated

369@@ -370,9 +370,10 @@ static bool IsCurrentForFeeEstimation() EXCLUSIVE_LOCKS_REQUIRED(cs_main)
370  * and instead just erase from the mempool as needed.
371  */
372 
373-static void UpdateMempoolForReorg(DisconnectedBlockTransactions& disconnectpool, bool fAddToMempool) EXCLUSIVE_LOCKS_REQUIRED(cs_main, ::mempool.cs)
374+static void UpdateMempoolForReorg(CTxMemPool& mempool, DisconnectedBlockTransactions& disconnectpool, bool fAddToMempool) EXCLUSIVE_LOCKS_REQUIRED(cs_main)

ajtowns commented at 8:18 pm on August 6, 2020:

Should just be removing the :: but still requiring mempool.cs to already be locked. Keeping the annotation here means adding LockAnnotation lock(mempool->cs); in a bunch of places that call UpdateMempoolForReorg.

MarcoFalke commented at 11:45 am on August 15, 2020:

Done

ajtowns commented at 9:35 pm on August 6, 2020: member

If we want to support not having a mempool, it might be better to virtualize CTxMemPool, so that validation expects a TxMemPoolInterface that has a cs lock to let thread safety annotations all make sense, and a bunch of virtual functions that are either implemented by CTxMemPool as is, or by dummy functions in a NoMemPool class.

If this PR just had m_mempool as a reference rather than a pointer, that would be a bunch of changes when supporting an optional mempool, though I think it would mostly be limited to the class definitions and the function arguments, which is maybe slightly less than every call site.

The conditional locking in the current patch seems pretty fragile to me, but not wrong at least.

promag commented at 11:59 pm on August 14, 2020: member

If we want to support not having a mempool, it might be better to virtualize CTxMemPool, so that validation expects a TxMemPoolInterface that has a cs lock to let thread safety annotations all make sense, and a bunch of virtual functions that are either implemented by CTxMemPool as is, or by dummy functions in a NoMemPool class.

Sounds like a nice approach.

MarcoFalke added the label Waiting for author on Aug 15, 2020

MarcoFalke force-pushed on Aug 15, 2020

sync: Add AssertLockHeld(MutexType*) helper fab9c6259a

MarcoFalke renamed this:
~~Pass mempool pointer to chainstate constructor~~
refactor: Keep mempool interface in validation
on Aug 15, 2020

MarcoFalke force-pushed on Aug 15, 2020

MarcoFalke commented at 11:55 am on August 15, 2020: member

Added back compile-time thread safety annotations as requested by @ajtowns and @hebasto

MarcoFalke removed the label Waiting for author on Aug 15, 2020

MarcoFalke force-pushed on Aug 15, 2020

in src/interfaces/txmempool.h:56 in face161701 outdated

51+    void check(const CCoinsViewCache& coins) override { invalidCall(); }
52+    void removeForBlock(const std::vector<CTransactionRef>& vtx, unsigned int block_height) override { invalidCall(); }
53+
54+private:
55+    void invalidCall() { Assert(false); }
56+};

hebasto commented at 2:36 pm on August 15, 2020:

face16170176c83d04c68454c91eaf42c80e6ec7 There is no need to place this class in the header, right?

MarcoFalke commented at 6:25 am on August 16, 2020:

Thx, moved to cpp file

in src/interfaces/txmempool.h:55 in face161701 outdated

50+    void transactionUpdated() override { invalidCall(); }
51+    void check(const CCoinsViewCache& coins) override { invalidCall(); }
52+    void removeForBlock(const std::vector<CTransactionRef>& vtx, unsigned int block_height) override { invalidCall(); }
53+
54+private:
55+    void invalidCall() { Assert(false); }

hebasto commented at 2:38 pm on August 15, 2020:

face16170176c83d04c68454c91eaf42c80e6ec7 nit: custom Assert has no benefits in this case

0    void invalidCall() { assert(false); }

MarcoFalke commented at 6:27 am on August 16, 2020:

I prefer Assert over assert because it comes with the following benefits:

Can not accidentally be disabled via NDEBUG
No need to update the code if more features get added to Assert, which are not part of assert

hebasto commented at 6:30 am on August 16, 2020:

Probably, it is worth to update Developer Notes :)

hebasto changes_requested

hebasto commented at 3:08 pm on August 15, 2020: member

Trying to test thread safety annotations with the following patch:

0--- a/src/interfaces/txmempool.cpp
1+++ b/src/interfaces/txmempool.cpp
2@@ -34,7 +34,6 @@ public:
3 
4     void removeForBlock(const std::vector<CTransactionRef>& vtx, unsigned int block_height) override EXCLUSIVE_LOCKS_REQUIRED(m_mutex)
5     {
6-        AssertLockHeld(m_txmempool.cs);
7         m_txmempool.removeForBlock(vtx, block_height);
8     }

and got:

0interfaces/txmempool.cpp:37:21: warning: calling function 'removeForBlock' requires holding mutex 'm_txmempool.cs' exclusively [-Wthread-safety-analysis]
1        m_txmempool.removeForBlock(vtx, block_height);
2                    ^
31 warning generated.

in src/interfaces/txmempool.cpp:35 in face161701 outdated

30+    void check(const CCoinsViewCache& coins) override
31+    {
32+        m_txmempool.check(&coins);
33+    }
34+
35+    void removeForBlock(const std::vector<CTransactionRef>& vtx, unsigned int block_height) override EXCLUSIVE_LOCKS_REQUIRED(m_mutex)

hebasto commented at 3:23 pm on August 15, 2020:

0    void removeForBlock(const std::vector<CTransactionRef>& vtx, unsigned int block_height) override EXCLUSIVE_LOCKS_REQUIRED(m_txmempool.cs)

MarcoFalke commented at 6:28 am on August 16, 2020:

Annotation can not be added to cpp files (or at least they will be ignored). Moved annotation to header file.

hebasto commented at 6:35 am on August 16, 2020:

Annotation can not be added to cpp files (or at least they will be ignored).

I don’t think so. https://clang.llvm.org/docs/ThreadSafetyAnalysis.html:

Attributes must be attached to named declarations, such as classes, methods, and data members.

MarcoFalke commented at 7:02 am on August 16, 2020:

Just logically, if validation does not include the txmempool.cpp file, how would it know about the annotation?

And practically you observed that it didn’t work: #19629 (comment)

hebasto commented at 5:03 pm on August 15, 2020: member

Another test of thread safety annotations with the following patch:

 0--- a/src/validation.cpp
 1+++ b/src/validation.cpp
 2@@ -2595,7 +2595,6 @@ public:
 3 bool CChainState::ConnectTip(BlockValidationState& state, const CChainParams& chainparams, CBlockIndex* pindexNew, const std::shared_ptr<const CBlock>& pblock, ConnectTrace& connectTrace, DisconnectedBlockTransactions &disconnectpool)
 4 {
 5     AssertLockHeld(cs_main);
 6-    AssertLockHeld(m_txmempool->m_mutex);
 7 
 8     assert(pindexNew->pprev == m_chain.Tip());
 9     // Read block from disk.
10--- a/src/validation.h
11+++ b/src/validation.h
12@@ -689,7 +689,7 @@ public:
13 
14 private:
15     bool ActivateBestChainStep(BlockValidationState& state, const CChainParams& chainparams, CBlockIndex* pindexMostWork, const std::shared_ptr<const CBlock>& pblock, bool& fInvalidFound, ConnectTrace& connectTrace) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_txmempool->m_mutex);
16-    bool ConnectTip(BlockValidationState& state, const CChainParams& chainparams, CBlockIndex* pindexNew, const std::shared_ptr<const CBlock>& pblock, ConnectTrace& connectTrace, DisconnectedBlockTransactions& disconnectpool) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_txmempool->m_mutex);
17+    bool ConnectTip(BlockValidationState& state, const CChainParams& chainparams, CBlockIndex* pindexNew, const std::shared_ptr<const CBlock>& pblock, ConnectTrace& connectTrace, DisconnectedBlockTransactions& disconnectpool) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
18 
19     void InvalidBlockFound(CBlockIndex* pindex, const BlockValidationState& state) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
20     CBlockIndex* FindMostWorkChain() EXCLUSIVE_LOCKS_REQUIRED(cs_main);

and this line https://github.com/bitcoin/bitcoin/blob/face16170176c83d04c68454c91eaf42c80e6ec7/src/validation.cpp#L2640

does not raise a thread safety warning as one could expect due to this annotation: https://github.com/bitcoin/bitcoin/blob/face16170176c83d04c68454c91eaf42c80e6ec7/src/interfaces/txmempool.cpp#L35

Keep mempool interface in chainstate for validation fa1d3da12e

MarcoFalke force-pushed on Aug 16, 2020

MarcoFalke commented at 6:29 am on August 16, 2020: member

Addressed feedback by @hebasto

hebasto commented at 6:49 am on August 16, 2020: member

Tested fa1d3da12e6d0712bf6934d6df9e6d5d0e43801d with #19629 (comment). Still fail. It seems required to move data member m_txmempool from the implementation class to the base one, changing its type to a pointer, and providing EXCLUSIVE_LOCKS_REQUIRED(m_txmempool->cs) annotation.

hebasto commented at 6:56 am on August 16, 2020: member

Tested fa1d3da12e6d0712bf6934d6df9e6d5d0e43801d with #19629 (comment). Looks ok:

0validation.cpp:2639:18: warning: calling function 'removeForBlock' requires holding mutex 'm_txmempool->m_mutex' exclusively [-Wthread-safety-analysis]
1    m_txmempool->removeForBlock(blockConnecting.vtx, pindexNew->nHeight);
2                 ^
31 warning generated.

hebasto commented at 7:10 am on August 16, 2020: member

FWIW, I found a patch set from #19668 very useful to test correctness of thread safety annotations. Currently, fa1d3da12e6d0712bf6934d6df9e6d5d0e43801d, a bunch of -Wthread-safety-analysis warnings are generated.

 0interfaces/txmempool.cpp:38:9: warning: calling function 'AssertLockHeldInternal<AnnotatedMixin<std::recursive_mutex> >' requires holding mutex 'm_txmempool.cs' exclusively [-Wthread-safety-analysis]
 1        AssertLockHeld(m_txmempool.cs);
 2        ^
 3./sync.h:79:28: note: expanded from macro 'AssertLockHeld'
 4#define AssertLockHeld(cs) AssertLockHeldInternal(#cs, __FILE__, __LINE__, cs)
 5                           ^
 6interfaces/txmempool.cpp:39:21: warning: calling function 'removeForBlock' requires holding mutex 'm_txmempool.cs' exclusively [-Wthread-safety-analysis]
 7        m_txmempool.removeForBlock(vtx, block_height);
 8                    ^
 92 warnings generated.
10validation.cpp:379:5: warning: calling function 'AssertLockHeldInternal<AnnotatedMixin<std::recursive_mutex> >' requires holding mutex 'mempool.cs' exclusively [-Wthread-safety-analysis]
11    AssertLockHeld(mempool.cs);
12    ^
13./sync.h:79:28: note: expanded from macro 'AssertLockHeld'
14#define AssertLockHeld(cs) AssertLockHeldInternal(#cs, __FILE__, __LINE__, cs)
15                           ^
16validation.cpp:397:21: warning: calling function 'removeRecursive' requires holding mutex 'mempool.cs' exclusively [-Wthread-safety-analysis]
17            mempool.removeRecursive(**it, MemPoolRemovalReason::REORG);
18                    ^
19validation.cpp:409:13: warning: calling function 'UpdateTransactionsFromBlock' requires holding mutex 'mempool.cs' exclusively [-Wthread-safety-analysis]
20    mempool.UpdateTransactionsFromBlock(vHashUpdate);
21            ^
22validation.cpp:412:13: warning: calling function 'removeForReorg' requires holding mutex 'mempool.cs' exclusively [-Wthread-safety-analysis]
23    mempool.removeForReorg(&::ChainstateActive().CoinsTip(), ::ChainActive().Tip()->nHeight + 1, STANDARD_LOCKTIME_VERIFY_FLAGS);
24            ^
25validation.cpp:414:5: warning: calling function 'LimitMempoolSize' requires holding mutex 'mempool.cs' exclusively [-Wthread-safety-analysis]
26    LimitMempoolSize(mempool, gArgs.GetArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE) * 1000000, std::chrono::hours{gArgs.GetArg("-mempoolexpiry", DEFAULT_MEMPOOL_EXPIRY)});
27    ^
28validation.cpp:2521:9: warning: calling function 'AssertLockHeldInternal<AnnotatedMixin<std::recursive_mutex> >' requires holding mutex 'm_txmempool->txmempool().cs' exclusively [-Wthread-safety-analysis]
29        AssertLockHeld(m_txmempool->txmempool()->cs);
30        ^
31./sync.h:79:28: note: expanded from macro 'AssertLockHeld'
32#define AssertLockHeld(cs) AssertLockHeldInternal(#cs, __FILE__, __LINE__, cs)
33                           ^
34validation.cpp:2529:39: warning: calling function 'removeRecursive' requires holding mutex 'm_txmempool->txmempool().cs' exclusively [-Wthread-safety-analysis]
35            m_txmempool->txmempool()->removeRecursive(**it, MemPoolRemovalReason::REORG);
36                                      ^
377 warnings generated.

MarcoFalke commented at 7:41 am on August 16, 2020: member

Yes, if #19668 is merged, the AsserLockHeld in interfaces/txmempool must be replaced by LockAssertion

changing its type to a pointer

I don’t think this works. Or at least it would lead to the initial solution I had (keep a mempool pointer in validation). I am happy to revert to the initial solution or take over a patch, if you have one that compiles on clang with and without –enable-debug

hebasto commented at 9:11 pm on August 17, 2020: member

@MarcoFalke

I don’t think this works. … if you have one that compiles on clang with and without –enable-debug

Mind looking at https://github.com/hebasto/bitcoin/commits/pr19629-0816-FORK ?

The same branch, rebased on top of the master (1bc8e8eae2dc4b47ef67afc6880ea48b8e84a086) + #19668:

https://github.com/hebasto/bitcoin/commits/pr19629-0816-REBASED

LimitMempoolSize() is a bit messed, but it is resolvable, I think.

UPDATE: the improved patch version is available.

hebasto commented at 10:27 am on August 18, 2020: member

@MarcoFalke Please have a look at the updated branch: https://github.com/hebasto/bitcoin/commits/pr19629-0816-FORKv2:

mess with LimitMempoolSize() is fixed
TxMempool is a pure interface now

The same branch, rebased on top of the master (e6e277f9ed4da7aff9b7b39a7838bada0c3e572a) + #19668::

https://github.com/hebasto/bitcoin/commits/pr19629-0816-REBASEDv2

hebasto commented at 11:09 am on August 18, 2020: member

Yes, if #19668 is merged, the AsserLockHeld in interfaces/txmempool must be replaced by LockAssertion

This is not required with suggested patch.

MarcoFalke removed this from the "Blockers" column in a project

MarcoFalke added the label Waiting for author on Aug 27, 2020

MarcoFalke commented at 7:43 am on August 28, 2020: member

Thanks for the review everyone. Though, I think this is getting a bit out of hand and trying to do too much things in one go. Closing for now.

MarcoFalke closed this on Aug 28, 2020

MarcoFalke deleted the branch on Aug 28, 2020

jnewbery commented at 8:14 am on August 28, 2020: member

I really thought the original implementation was fine and is an improvement over the current code. Were there actually any problems with it, or is this just a case of the best being the enemy of good and us not making a good change because it’s not perfect?

MarcoFalke commented at 8:45 am on August 28, 2020: member

Opened alternative in #19826 . The mempool interface idea can be explored later, if needed.

fanquake removed the label Waiting for author on Aug 31, 2020

DrahtBot locked this on Feb 15, 2022

refactor: Keep mempool interface in validation #19629

Conflicts