refactor: Keep mempool interface in validation #19629

maflcko · 2020-07-30T16:26:58Z

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, ...)

promag

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?

maflcko · 2020-07-30T18:33:59Z

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

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

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

promag · 2020-07-30T22:55:06Z

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.

maflcko · 2020-07-31T04:52:48Z

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)

DrahtBot · 2020-07-31T11:46:36Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

Introduce deploymentstatus #19438 (Introduce deploymentstatus by ajtowns)
refactor: Replace RecursiveMutex with Mutex in CTxMemPool #19306 (refactor: Replace RecursiveMutex with Mutex in CTxMemPool by hebasto)
refactor: Make CCheckQueue RAII-styled #18731 (refactor: Make CCheckQueue RAII-styled by hebasto)
net: Add NAT-PMP port forwarding support #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.

darosior

ACK faab637

darosior · 2020-07-31T13:09:15Z

src/validation.cpp

@@ -2508,22 +2512,23 @@ bool CChainState::DisconnectTip(BlockValidationState& state, const CChainParams&
    if (!FlushStateToDisk(chainparams, state, FlushStateMode::IF_NEEDED))
        return false;

-    if (disconnectpool) {
+    if (disconnectpool && m_mempool) {
+        AssertLockHeld(m_mempool->cs);


Isn't the asserion redundant with line 2491 ?

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

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.

practicalswift · 2020-08-01T10:43:06Z

Concept ACK

jnewbery · 2020-08-01T11:18:29Z

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 · 2020-08-02T14:24:25Z

@jnewbery would you drop if (m_mempool) checks?

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

jnewbery

Code review ACK faab637

@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.

jnewbery · 2020-08-02T17:49:42Z

src/validation.cpp

@@ -2866,7 +2878,8 @@ bool CChainState::ActivateBestChain(BlockValidationState &state, const CChainPar
        LimitValidationInterfaceQueue();

        {
-            LOCK2(cs_main, ::mempool.cs); // Lock transaction pool for at least as long as it takes for connectTrace to be consumed
+            LOCK(cs_main);
+            LOCK(m_mempool ? &m_mempool->cs : nullptr); // Lock transaction pool for at least as long as it takes for connectTrace to be consumed


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

bitcoin/src/sync.h

Line 163 in a787428

UniqueLock(Mutex* pmutexIn, const char* pszName, const char* pszFile, int nLine, bool fTry = false) EXCLUSIVE_LOCK_FUNCTION(pmutexIn)

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:

bitcoin/src/sync.h

Line 165 in a787428

if (!pmutexIn) return;

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

bitcoin/src/sync.h

Lines 174 to 178 in a787428

~UniqueLock() UNLOCK_FUNCTION()

{

if (Base::owns_lock())

LeaveCritical();

}

and the base class destructor does nothing.

promag · 2020-08-02T18:19:46Z

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

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

maflcko · 2020-08-04T10:02:12Z

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.

hebasto · 2020-08-06T04:14:46Z

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

--- a/src/validation.h
+++ b/src/validation.h
@@ -645,7 +645,7 @@ public:
                       CCoinsViewCache& view, const CChainParams& chainparams, bool fJustCheck = false) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
 
     // Apply the effects of a block disconnection on the UTXO set.
-    bool DisconnectTip(BlockValidationState& state, const CChainParams& chainparams, DisconnectedBlockTransactions* disconnectpool) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
+    bool DisconnectTip(BlockValidationState& state, const CChainParams& chainparams, DisconnectedBlockTransactions* disconnectpool) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_mempool->cs);
 
     // Manual block validity manipulation:
     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 · 2020-08-06T09:53:35Z

--- a/src/validation.h
+++ b/src/validation.h
@@ -645,7 +645,7 @@ public:
                       CCoinsViewCache& view, const CChainParams& chainparams, bool fJustCheck = false) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
 
     // Apply the effects of a block disconnection on the UTXO set.
-    bool DisconnectTip(BlockValidationState& state, const CChainParams& chainparams, DisconnectedBlockTransactions* disconnectpool) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
+    bool DisconnectTip(BlockValidationState& state, const CChainParams& chainparams, DisconnectedBlockTransactions* disconnectpool) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_mempool->cs);
 
     // Manual block validity manipulation:
     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 · 2020-08-06T10:04:58Z

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.

ajtowns

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.

src/validation.cpp

ajtowns · 2020-08-06T20:46:51Z

src/validation.cpp

@@ -2508,22 +2512,23 @@ bool CChainState::DisconnectTip(BlockValidationState& state, const CChainParams&
    if (!FlushStateToDisk(chainparams, state, FlushStateMode::IF_NEEDED))
        return false;

-    if (disconnectpool) {
+    if (disconnectpool && m_mempool) {
+        AssertLockHeld(m_mempool->cs);


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.

promag · 2020-08-14T23:59:57Z

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.

maflcko · 2020-08-15T11:55:38Z

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

src/interfaces/txmempool.h

hebasto · 2020-08-15T15:08:37Z

Trying to test thread safety annotations with the following patch:

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

and got:

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

Could be related to https://clang.llvm.org/docs/ThreadSafetyAnalysis.html#no-alias-analysis

src/interfaces/txmempool.cpp

hebasto · 2020-08-15T17:03:00Z

Another test of thread safety annotations with the following patch:

--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -2595,7 +2595,6 @@ public:
 bool CChainState::ConnectTip(BlockValidationState& state, const CChainParams& chainparams, CBlockIndex* pindexNew, const std::shared_ptr<const CBlock>& pblock, ConnectTrace& connectTrace, DisconnectedBlockTransactions &disconnectpool)
 {
     AssertLockHeld(cs_main);
-    AssertLockHeld(m_txmempool->m_mutex);
 
     assert(pindexNew->pprev == m_chain.Tip());
     // Read block from disk.
--- a/src/validation.h
+++ b/src/validation.h
@@ -689,7 +689,7 @@ public:
 
 private:
     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);
-    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);
+    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);
 
     void InvalidBlockFound(CBlockIndex* pindex, const BlockValidationState& state) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
     CBlockIndex* FindMostWorkChain() EXCLUSIVE_LOCKS_REQUIRED(cs_main);

and this line

bitcoin/src/validation.cpp

Line 2640 in face161

m_txmempool->removeForBlock(blockConnecting.vtx, pindexNew->nHeight);

does not raise a thread safety warning as one could expect due to this annotation:

bitcoin/src/interfaces/txmempool.cpp

Line 35 in face161

    
           void removeForBlock(const std::vector<CTransactionRef>& vtx, unsigned int block_height) override EXCLUSIVE_LOCKS_REQUIRED(m_mutex)

maflcko · 2020-08-16T06:29:17Z

Addressed feedback by @hebasto

hebasto · 2020-08-16T06:49:47Z

Tested fa1d3da 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 · 2020-08-16T06:56:43Z

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

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

hebasto · 2020-08-16T07:10:51Z

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

fa1d3da + #19668

interfaces/txmempool.cpp:38:9: warning: calling function 'AssertLockHeldInternal<AnnotatedMixin<std::recursive_mutex> >' requires holding mutex 'm_txmempool.cs' exclusively [-Wthread-safety-analysis]
        AssertLockHeld(m_txmempool.cs);
        ^
./sync.h:79:28: note: expanded from macro 'AssertLockHeld'
#define AssertLockHeld(cs) AssertLockHeldInternal(#cs, __FILE__, __LINE__, cs)
                           ^
interfaces/txmempool.cpp:39:21: warning: calling function 'removeForBlock' requires holding mutex 'm_txmempool.cs' exclusively [-Wthread-safety-analysis]
        m_txmempool.removeForBlock(vtx, block_height);
                    ^
2 warnings generated.
validation.cpp:379:5: warning: calling function 'AssertLockHeldInternal<AnnotatedMixin<std::recursive_mutex> >' requires holding mutex 'mempool.cs' exclusively [-Wthread-safety-analysis]
    AssertLockHeld(mempool.cs);
    ^
./sync.h:79:28: note: expanded from macro 'AssertLockHeld'
#define AssertLockHeld(cs) AssertLockHeldInternal(#cs, __FILE__, __LINE__, cs)
                           ^
validation.cpp:397:21: warning: calling function 'removeRecursive' requires holding mutex 'mempool.cs' exclusively [-Wthread-safety-analysis]
            mempool.removeRecursive(**it, MemPoolRemovalReason::REORG);
                    ^
validation.cpp:409:13: warning: calling function 'UpdateTransactionsFromBlock' requires holding mutex 'mempool.cs' exclusively [-Wthread-safety-analysis]
    mempool.UpdateTransactionsFromBlock(vHashUpdate);
            ^
validation.cpp:412:13: warning: calling function 'removeForReorg' requires holding mutex 'mempool.cs' exclusively [-Wthread-safety-analysis]
    mempool.removeForReorg(&::ChainstateActive().CoinsTip(), ::ChainActive().Tip()->nHeight + 1, STANDARD_LOCKTIME_VERIFY_FLAGS);
            ^
validation.cpp:414:5: warning: calling function 'LimitMempoolSize' requires holding mutex 'mempool.cs' exclusively [-Wthread-safety-analysis]
    LimitMempoolSize(mempool, gArgs.GetArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE) * 1000000, std::chrono::hours{gArgs.GetArg("-mempoolexpiry", DEFAULT_MEMPOOL_EXPIRY)});
    ^
validation.cpp:2521:9: warning: calling function 'AssertLockHeldInternal<AnnotatedMixin<std::recursive_mutex> >' requires holding mutex 'm_txmempool->txmempool().cs' exclusively [-Wthread-safety-analysis]
        AssertLockHeld(m_txmempool->txmempool()->cs);
        ^
./sync.h:79:28: note: expanded from macro 'AssertLockHeld'
#define AssertLockHeld(cs) AssertLockHeldInternal(#cs, __FILE__, __LINE__, cs)
                           ^
validation.cpp:2529:39: warning: calling function 'removeRecursive' requires holding mutex 'm_txmempool->txmempool().cs' exclusively [-Wthread-safety-analysis]
            m_txmempool->txmempool()->removeRecursive(**it, MemPoolRemovalReason::REORG);
                                      ^
7 warnings generated.

maflcko · 2020-08-16T07:41:31Z

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 · 2020-08-17T21:11:59Z

@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 (1bc8e8e) + #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 · 2020-08-18T10:27:30Z

@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 (e6e277f) + #19668::

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

hebasto · 2020-08-18T11:09:24Z

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

This is not required with suggested patch.

maflcko · 2020-08-28T07:43:24Z

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.

jnewbery · 2020-08-28T08:14:50Z

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?

maflcko · 2020-08-28T08:45:30Z

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

maflcko added Refactoring Validation labels Jul 30, 2020

maflcko changed the title ~~Pass mempool reference to chainstate constructor~~ Pass mempool pointer to chainstate constructor Jul 30, 2020

maflcko mentioned this pull request Jul 30, 2020

Remove mempool global #19556

Merged

promag reviewed Jul 30, 2020

View reviewed changes

darosior approved these changes Jul 31, 2020

View reviewed changes

DrahtBot mentioned this pull request Jul 31, 2020

refactor: Make CCheckQueue RAII-styled #18731

Closed

DrahtBot mentioned this pull request Aug 2, 2020

assumeutxo #15606

Closed

18 tasks

jnewbery reviewed Aug 2, 2020

View reviewed changes

DrahtBot mentioned this pull request Aug 3, 2020

refactor: Replace RecursiveMutex with Mutex in CTxMemPool #19306

Closed

hebasto reviewed Aug 4, 2020

View reviewed changes

ajtowns reviewed Aug 6, 2020

View reviewed changes

maflcko added the Waiting for author label Aug 15, 2020

maflcko force-pushed the 2007-nomem branch from faab637 to fa3f232 Compare August 15, 2020 11:12

sync: Add AssertLockHeld(MutexType*) helper

fab9c62

maflcko changed the title ~~Pass mempool pointer to chainstate constructor~~ refactor: Keep mempool interface in validation Aug 15, 2020

maflcko force-pushed the 2007-nomem branch from fa3f232 to fabe5ba Compare August 15, 2020 11:43

maflcko removed the Waiting for author label Aug 15, 2020

maflcko force-pushed the 2007-nomem branch from fabe5ba to face161 Compare August 15, 2020 12:19

hebasto requested changes Aug 15, 2020

View reviewed changes

src/interfaces/txmempool.h Outdated Show resolved Hide resolved

src/interfaces/txmempool.h Outdated Show resolved Hide resolved

hebasto reviewed Aug 15, 2020

View reviewed changes

src/interfaces/txmempool.cpp Outdated Show resolved Hide resolved

Keep mempool interface in chainstate for validation

fa1d3da

maflcko force-pushed the 2007-nomem branch from face161 to fa1d3da Compare August 16, 2020 06:26

This was referenced Aug 20, 2020

Introduce deploymentstatus #19438

Merged

net: Add NAT-PMP port forwarding support #18077

Merged

maflcko added the Waiting for author label Aug 27, 2020

maflcko closed this Aug 28, 2020

maflcko deleted the 2007-nomem branch August 28, 2020 07:43

maflcko mentioned this pull request Aug 28, 2020

Pass mempool reference to chainstate constructor #19826

Merged

fanquake removed the Waiting for author label Aug 31, 2020

maflcko mentioned this pull request Aug 31, 2020

Remove mempool global from interfaces #19848

Merged

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

	~UniqueLock() UNLOCK_FUNCTION()
	{
	if (Base::owns_lock())
	LeaveCritical();
	}

refactor: Keep mempool interface in validation #19629

refactor: Keep mempool interface in validation #19629

Conversation

maflcko commented Jul 30, 2020 • edited Loading

promag left a comment

Choose a reason for hiding this comment

maflcko commented Jul 30, 2020

promag commented Jul 30, 2020

maflcko commented Jul 31, 2020

DrahtBot commented Jul 31, 2020 • edited Loading

Conflicts

darosior left a comment

Choose a reason for hiding this comment

darosior Jul 31, 2020

Choose a reason for hiding this comment

maflcko Jul 31, 2020

Choose a reason for hiding this comment

ajtowns Aug 6, 2020

Choose a reason for hiding this comment

practicalswift commented Aug 1, 2020

jnewbery commented Aug 1, 2020

promag commented Aug 2, 2020 • edited Loading

jnewbery left a comment

Choose a reason for hiding this comment

jnewbery Aug 2, 2020

Choose a reason for hiding this comment

promag commented Aug 2, 2020

hebasto left a comment

Choose a reason for hiding this comment

maflcko commented Aug 4, 2020

hebasto commented Aug 6, 2020

darosior commented Aug 6, 2020

hebasto commented Aug 6, 2020

ajtowns left a comment

Choose a reason for hiding this comment

ajtowns Aug 6, 2020

Choose a reason for hiding this comment

promag commented Aug 14, 2020

maflcko commented Aug 15, 2020

hebasto commented Aug 15, 2020

hebasto commented Aug 15, 2020

maflcko commented Aug 16, 2020

hebasto commented Aug 16, 2020

hebasto commented Aug 16, 2020

hebasto commented Aug 16, 2020

maflcko commented Aug 16, 2020 • edited Loading

hebasto commented Aug 17, 2020 • edited Loading

hebasto commented Aug 18, 2020 • edited Loading

hebasto commented Aug 18, 2020

maflcko commented Aug 28, 2020

jnewbery commented Aug 28, 2020

maflcko commented Aug 28, 2020

maflcko commented Jul 30, 2020 •

edited

Loading

DrahtBot commented Jul 31, 2020 •

edited

Loading

promag commented Aug 2, 2020 •

edited

Loading

maflcko commented Aug 16, 2020 •

edited

Loading

hebasto commented Aug 17, 2020 •

edited

Loading

hebasto commented Aug 18, 2020 •

edited

Loading