refactor: drop boost::signals2 in validationinterface #18524

ryanofsky · 2020-04-04T16:03:18Z

Stop using boost::signals2 internally in validationinterface. Replace with std::list and Add/Remove/Clear/Iterate helper functions.

Motivation for change is to reduce dependencies and avoid issues happening with boost versions before 1.59: #18517, #18471

Stop using boost::signals2 internally in validationinterface. Replace with std::list and Add/Remove/Clear/Iterate helper functions. Motivation for change is to reduce dependencies and avoid issues happening with boost versions before 1.59: bitcoin#18517 bitcoin#18471

practicalswift · 2020-04-04T16:37:44Z

Concept ACK: less boost is better

naumenkogs · 2020-04-04T16:39:07Z

Concept ACK: it solved the issue I had.

hebasto · 2020-04-04T16:40:04Z

Concept ACK.

ryanofsky · 2020-04-04T16:55:00Z

Updated ba8312c -> ad067a9 (pr/nosig.1 -> pr/nosig.2, compare) avoiding unneeded shared_ptr copies and cleaning up typedefs
Updated ad067a9 -> 01639a2 (pr/nosig.2 -> pr/nosig.3, compare) removing last typedef

promag

Tested that this fixes the issue. I also prefer dropping boost signals2.

I was going a different way. From https://stackoverflow.com/a/2265979

I suggest you go the other way around and connect a dummy slot, then disconnect it when your "real" slot is invoked. Connecting another slot will clean up stale connections, so your slot should be released.

promag · 2020-04-04T17:15:53Z

src/validationinterface.cpp

+    void Add(std::shared_ptr<CValidationInterface> callbacks)
+    {
+        LOCK(m_mutex);
+        auto inserted = m_map.emplace(callbacks.get(), m_list.end());


So we add the same twice?

re: #18524 (comment)

So we add the same twice?

There's two emplaces because of the map and list (if that's the question)

I mean this is allowing adding the same callbacks. Why not assert(inserted.second)?

re: #18524 (comment)

I mean this is allowing adding the same callbacks. Why not assert(inserted.second)?

Unit tests fail if the unregister function is not idempotent, so it seems good to me that the register function is idempotent as well. I could imagine an idempotent API here making calling code simpler, even though I could also imagine an assert catching potential bugs. I think asserts tend to work a better at short range within a module instead of being used to make an API rigid in an attempt to catch external bugs. But I don't think there is a right answer here and would be ok with a PR that took a different approach.

Personally I think these shouldn't be idempotent because makes sense to call just once. For instance std::fstream::open is not idempotent, fails if file is already opened. I'd rather fix unit tests. Just an opinion and should not prevent this change going forward.

re: #18524 (comment)

Personally I think these shouldn't be idempotent because makes sense to call just once. For instance std::fstream::open is not idempotent, fails if file is already opened. I'd rather fix unit tests. Just an opinion and should not prevent this change going forward.

Feel free to open a followup. If I wanted to change calling code not to use the same pointer more than once, I would change these to return bool and have calling code assert that they return true, rather having these crash on cases they can reasonably handle and making assumptions about calling code. I do find API's like std::map's insert/erase that are idempotent convenient for avoiding boilerplate code, so that's another reason I like the keeping the boost behavior beyond not wanting to increase scope of this PR and not liking asserts across an api boundary

promag · 2020-04-04T17:16:57Z

src/validationinterface.cpp

+
+    template<typename F> void Iterate(F&& f)
+    {
+        WAIT_LOCK(m_mutex, lock);


How about local copy and then iterate it lock free?

re: #18524 (comment)

How about local copy and then iterate it lock free?

I'm not sure what approach could be entirely lock free since the list is global and can be modified from any thread.

I just implemented something simple that can be changed and optimized in the future. The lock is not held when calling CValidation interface methods. It avoids copying so allocations aren't needed just to iterate the list. Assumption is that the list is iterated frequently and modified infrequently

So at best case it would be lock free I think.

Oops, I wanted to delete my own comment, but it seems I misclicked and deleted @promag's. Restoring from mail:

Assumption is that the list is iterated frequently and modified infrequently

Right. What I had in mind is 2 lists and an atomic book list_changed. On iterating it would then sync the list with the mutex locked.

:) I was wondering what happened. So draft:

if (list_changed) { lock; iterate_list = list; list_changed = false } for (f : iterate_list) f()

re: #18524 (comment)

Right. What I had in mind is 2 lists and an atomic book list_changed. On iterating it would then sync the list with the mutex locked.

I don't understand the suggestion from the pseudocode. I don't love the idea of having multiple lists, but even if you have them, I don't see how you avoid locks copying the list if you do create copies, or avoid needing a condition variable to delay modifying the list if you don't create copies. I'm probably just making an incorrect assumption about what you want to accomplish here. Feel free to post sample code, or just change the implementation in an alternate PR or followup PR. I'd be especially interested if it's a simplification and not just an optimization.

Is it not possible that there are two invocations of Iterate simultaneously?

re: #18524 (comment)

Is it not possible that there are two invocations of Iterate simultaneously?

It shouldn't be the most common thing but should be possible because notifications like CMainSignals::BlockChecked and CMainSignals::NewPoWValidBlock run on calling thread. Other notifications are sent from the scheduler so shouldn't happen simultaneously.

Is it not possible that there are two invocations of Iterate simultaneously?

My previous suggestion isn't possible in this case.

hebasto

Tested 01639a2 on Ubuntu 16.04.6 LTS, it works as expected.

The end of output:

...
2020-04-04T20:13:51Z FlushStateToDisk: write coins cache to disk (0 coins, 0kB) started
2020-04-04T20:13:51Z FlushStateToDisk: write coins cache to disk (0 coins, 0kB) completed (0.00s)
2020-04-04T20:13:51Z [default wallet] Releasing wallet
2020-04-04T20:13:51Z Shutdown: done

The only concern is about failed CentOS 7 build on Travis.

src/validationinterface.cpp

ryanofsky

Updated 01639a2 -> 3d463ad (pr/nosig.3 -> pr/nosig.4, compare) to fix centos 7 compiler error

ryanofsky · 2020-04-04T23:13:39Z

src/validationinterface.cpp

+    void Add(std::shared_ptr<CValidationInterface> callbacks)
+    {
+        LOCK(m_mutex);
+        auto inserted = m_map.emplace(callbacks.get(), m_list.end());


re: #18524 (comment)

So we add the same twice?

There's two emplaces because of the map and list (if that's the question)

ryanofsky · 2020-04-04T23:25:24Z

src/validationinterface.cpp

+
+    template<typename F> void Iterate(F&& f)
+    {
+        WAIT_LOCK(m_mutex, lock);


re: #18524 (comment)

How about local copy and then iterate it lock free?

I'm not sure what approach could be entirely lock free since the list is global and can be modified from any thread.

I just implemented something simple that can be changed and optimized in the future. The lock is not held when calling CValidation interface methods. It avoids copying so allocations aren't needed just to iterate the list. Assumption is that the list is iterated frequently and modified infrequently

src/validationinterface.cpp

hebasto · 2020-04-05T11:18:57Z

src/validationinterface.cpp

+    void Clear()
+    {
+        LOCK(m_mutex);
+        for (auto it = m_list.begin(); it != m_list.end();) {
+            if (--it->count == 0) it = m_list.erase(it); else ++it;
+        }
+        m_map.clear();
+    }


It follows from the code of the Clear() function that m_list could be non-empty when the function returns.

Is it consistent state if m_list.empty() == false and m_map.empty() == true ?

re: #18524 (comment)

It follows from the code of the Clear() function that m_list could be non-empty when the function returns.

Is it consistent state if m_list.empty() == false and m_map.empty() == true ?

Yes, this happens when the reference count of one or more callbacks in the list is nonzero, which means they are currently being called, and will be removed by the iterate function when they are done being called, clearing the list.

ryanofsky · 2020-04-05T12:33:20Z

Updated 3d463ad -> b5fea24 (pr/nosig.4 -> pr/nosig.5, compare) to destroy scheduler before callback list (no real change but makes more sense logically)

Also I'm noticing random appveyor test failures with OSError: [WinError 10048] Only one usage of each socket address (protocol/network address/port) is normally permitted https://ci.appveyor.com/project/DrahtBot/bitcoin/builds/31959888#L3892, I'm assuming not related to the PR

hebasto · 2020-04-05T13:37:08Z

src/validationinterface.cpp

+            m_list.emplace_back();
+            inserted.first->second = std::prev(m_list.end());
+        }
+        inserted.first->second->callbacks = std::move(callbacks);


Why callbacks are moved into the m_list always, but not only when an insertion into m_map occurs?

re: #18524 (comment)

Why callbacks are moved into the m_list always, but not only when an insertion into m_map occurs?

Shouldn't make a difference in practice, but it seemed better to me to call operator=(&&) and do empty destroy on the RHS object consistently instead of having different call sequences depending on a basically unrelated condition. Also, this will probably never matter but maybe always updating could be useful and worth guaranteeing in the future if there's internal property of the pointer worth updating like a new custom deleter.

hebasto

ACK b5fea24

maflcko · 2020-04-05T22:33:11Z

ACK b5fea24

sipa · 2020-04-05T23:23:24Z

src/validationinterface.cpp

-    boost::signals2::signal<void (const CBlockIndex *, const std::shared_ptr<const CBlock>&)> NewPoWValidBlock;
-
+    Mutex m_mutex;
+    struct Entry { std::shared_ptr<CValidationInterface> callbacks; int count = 1; };


Am I correct in stating the invariants here?

There are two types of callbacks, ones that are registered (in m_map) and ones that are not (because they're still being executed while being deleted).

m_list contains all callbacks of both types

entry.count is equal to the number of current executions of that entry, plus 1 if it's registered. It cannot be 0 (because that would imply being unregistered, and not being executed).

Perhaps it's worth spelling these out.

Am I correct in stating the invariants here?

Yes, that's all correct and well stated. I'll add this as a comment

ryanofsky · 2020-04-05T23:25:36Z

One way to test this change is to load and unload wallets repeatedly like in #18362. Each time a wallet is loaded and unloaded, a CValidationInterface instance gets registered and unregistered. If this is done during a sync when there are lots of UpdateTip and BlockConnected callbacks, it can be a good way to mix registrations with notifications and try to trigger deadlocks & segfaults we've previously seen here. Some command lines I used for testing this:

src/bitcoind -testnet -debug=1 -debugexclude=libevent -server=1 -printtoconsole

src/bitcoin-cli -testnet createwallet w1
src/bitcoin-cli -testnet unloadwallet w1
src/bitcoin-cli -testnet createwallet w2
src/bitcoin-cli -testnet unloadwallet w2

while src/bitcoin-cli -testnet loadwallet w1 && src/bitcoin-cli -testnet unloadwallet w1; do true; done

while src/bitcoin-cli -testnet loadwallet w2 && src/bitcoin-cli -testnet unloadwallet w2; do true; done

src/validationinterface.cpp

promag · 2020-04-05T23:39:55Z

Out of curiosity, have you considered a read write lock?

src/validationinterface.cpp

ryanofsky

Updated b5fea24 -> c3a4716 (pr/nosig.5 -> pr/nosig.6, compare) adding suggested comments and renaming a few things to match comments.

Out of curiosity, have you considered a read write lock?

I was thinking about one in the context of the solution you were proposing. If we just used a read-write lock in the most straightforward way here without reference counts registering and unregistering would block while callbacks were executing which would be changing behavior from boost signals and might lead to external deadlocks

src/validationinterface.cpp

sipa · 2020-04-06T00:46:39Z

src/validationinterface.cpp

+        auto inserted = m_map.emplace(callbacks.get(), m_list.end());
+        if (inserted.second) {
+            m_list.emplace_back();
+            inserted.first->second = std::prev(m_list.end());


I think these lines can be combined into inserted.first->second = m_list.emplace(m_list.end());.

re: #18524 (comment)

I think these lines can be combined into inserted.first->second = m_list.emplace(m_list.end());.

Thanks, switched to this

sipa · 2020-04-06T00:49:40Z

src/validationinterface.cpp

-    boost::signals2::signal<void (const CBlock&, const BlockValidationState&)> BlockChecked;
-    boost::signals2::signal<void (const CBlockIndex *, const std::shared_ptr<const CBlock>&)> NewPoWValidBlock;
-
+    Mutex m_mutex;


Do all of these member variables/types need to be public?

re: #18524 (comment)

Do all of these member variables/types need to be public?

No none do, added private/public sections

sipa · 2020-04-06T02:43:11Z

ACK 9617600

promag

There is one little difference from boost signals2 when some validation interface is registered in the middle of some Iterate which results in a different order of callback execution. However I don't see how this could be a problem.

promag · 2020-04-06T09:07:44Z

src/validationinterface.cpp

+    void Add(std::shared_ptr<CValidationInterface> callbacks)
+    {
+        LOCK(m_mutex);
+        auto inserted = m_map.emplace(callbacks.get(), m_list.end());


Personally I think these shouldn't be idempotent because makes sense to call just once. For instance std::fstream::open is not idempotent, fails if file is already opened. I'd rather fix unit tests. Just an opinion and should not prevent this change going forward.

ryanofsky

Updated 9617600 -> d6815a2 (pr/nosig.7 -> pr/nosig.8, compare) just adding public/private, simplifying emplace, and making erase calls more consistent

re: #18524 (review)

There is one little difference from boost signals2 when some validation interface is registered in the middle of some Iterate which results in a different order of callback execution. However I don't see how this could be a problem.

Curious what the boost behavior is when registrations change during a call. Seems like it would have to make a snapshot copy or use a kind of versioning to be able to ignore additions / removals

ryanofsky · 2020-04-06T12:19:51Z

src/validationinterface.cpp

-    boost::signals2::signal<void (const CBlock&, const BlockValidationState&)> BlockChecked;
-    boost::signals2::signal<void (const CBlockIndex *, const std::shared_ptr<const CBlock>&)> NewPoWValidBlock;
-
+    Mutex m_mutex;


re: #18524 (comment)

Do all of these member variables/types need to be public?

No none do, added private/public sections

ryanofsky · 2020-04-06T12:20:03Z

src/validationinterface.cpp

+    void Add(std::shared_ptr<CValidationInterface> callbacks)
+    {
+        LOCK(m_mutex);
+        auto inserted = m_map.emplace(callbacks.get(), m_list.end());


re: #18524 (comment)

Personally I think these shouldn't be idempotent because makes sense to call just once. For instance std::fstream::open is not idempotent, fails if file is already opened. I'd rather fix unit tests. Just an opinion and should not prevent this change going forward.

Feel free to open a followup. If I wanted to change calling code not to use the same pointer more than once, I would change these to return bool and have calling code assert that they return true, rather having these crash on cases they can reasonably handle and making assumptions about calling code. I do find API's like std::map's insert/erase that are idempotent convenient for avoiding boilerplate code, so that's another reason I like the keeping the boost behavior beyond not wanting to increase scope of this PR and not liking asserts across an api boundary

ryanofsky · 2020-04-06T12:20:11Z

src/validationinterface.cpp

+        auto inserted = m_map.emplace(callbacks.get(), m_list.end());
+        if (inserted.second) {
+            m_list.emplace_back();
+            inserted.first->second = std::prev(m_list.end());


re: #18524 (comment)

I think these lines can be combined into inserted.first->second = m_list.emplace(m_list.end());.

Thanks, switched to this

maflcko · 2020-04-06T12:57:16Z

ACK d6815a2

promag · 2020-04-06T13:05:33Z

Curious what the boost behavior is when registrations change during a call. Seems like it would have to make a snapshot copy or use a kind of versioning to be able to ignore additions / removals

Yes, that's the case, slots are called without any lock using a local list.

promag

ACK d6815a2.

Tested it fixes #18517. Some nits for your consideration if you happen to push again.

While reviewing I thought it could make sense to define MainSignalsInstance destructor that would assert nothing is registered.

promag · 2020-04-06T13:43:07Z

src/validationinterface.cpp


    explicit MainSignalsInstance(CScheduler *pscheduler) : m_schedulerClient(pscheduler) {}
+
+    void Register(std::shared_ptr<CValidationInterface> callbacks)


nit, could avoid incrementing usage count - receive reference.

promag · 2020-04-06T13:45:12Z

src/validationinterface.cpp

+    //! count is equal to the number of current executions of that entry, plus 1
+    //! if it's registered. It cannot be 0 because that would imply it is
+    //! unregistered and also not being executed (so shouldn't exist).
+    struct ListEntry { std::shared_ptr<CValidationInterface> callbacks; int count = 1; };


nit int count{1};

promag · 2020-04-06T13:46:16Z

src/validationinterface.cpp

+        LOCK(m_mutex);
+        auto it = m_map.find(callbacks);
+        if (it != m_map.end()) {
+            if (!--it->second->count) m_list.erase(it->second);


nit, == 0.

laanwj · 2020-04-06T14:25:34Z

ACK d6815a2

I like the general direction of this (to move away from boost::signals2), and as it works around a problem with boost I think this is the preferable way to fix this (both in 0.20 and master).

While reviewing I thought it could make sense to define MainSignalsInstance destructor that would assert nothing is registered.

FWIW asserts in destructors have turned out to be pretty terrible in generating shutdown crashes in unexpected conditions such as errors. So I'm not sure I like this.

hebasto · 2020-04-06T14:29:48Z

re-ACK d6815a2

maflcko · 2020-04-06T14:53:42Z

FWIW asserts in destructors have turned out to be pretty terrible in generating shutdown crashes in unexpected conditions such as errors. So I'm not sure I like this.

It could be something that is only enabled on --enable-debug for tests, something like #16136

ryanofsky · 2020-04-06T15:31:52Z

Thanks for suggestions and reviews! I was pushing back on some suggestions that would have expanded scope of this PR or prevented it from being a refactor, but I'm working on a followup PR to implement these.

sipa · 2020-04-06T21:21:04Z

src/validationinterface.cpp

+    void Clear()
+    {
+        LOCK(m_mutex);
+        for (auto it = m_list.begin(); it != m_list.end();) {


Is it correct that this iterates over the entire list? I think it should only iterate over entries that are in the map (the count of those that are already unregistered shouldn't be decremented further).

Suggested fix here: #18551

Bug in MainSignalsInstance::Clear could cause validation interface callbacks to be deleted during execution if UnregisterAllValidationInterfaces was called more than once. Bug was introduced in bitcoin#18524 and is fixed by bitcoin#18551

Summary: d6815a2313158862d448733954a73520f223deb6 refactor: drop boost::signals2 in validationinterface (Russell Yanofsky) Pull request description: Stop using boost::signals2 internally in validationinterface. Replace with std::list and Add/Remove/Clear/Iterate helper functions. Motivation for change is to reduce dependencies and avoid issues happening with boost versions before 1.59: bitcoin/bitcoin#18517, bitcoin/bitcoin#18471 ACKs for top commit: MarcoFalke: ACK d6815a2313158862d448733954a73520f223deb6 laanwj: ACK d6815a2313158862d448733954a73520f223deb6 hebasto: re-ACK d6815a2313158862d448733954a73520f223deb6 promag: ACK d6815a2313158862d448733954a73520f223deb6. Tree-SHA512: 4fc0f14a8446e8616cc142af6c3d36815f3254525d30348ba8e4d4bc74c249a5a8c9bc119bdd1be7ebd7abe0b784bc0c5551a3e156a766890cb2fdd891a95919 Backport of Core [[bitcoin/bitcoin#18524 | PR18524]] Depends on D6652 Test Plan: `ninja check check-functional` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D6653

Bug in MainSignalsInstance::Clear could cause validation interface callbacks to be deleted during execution if UnregisterAllValidationInterfaces was called more than once. Bug was introduced in bitcoin/bitcoin#18524 and is fixed by bitcoin/bitcoin#18551

ryanofsky mentioned this pull request Apr 4, 2020

[wallet] Node process hangs after SIGINT #18517

Closed

ryanofsky force-pushed the pr/nosig branch from ba8312c to ad067a9 Compare April 4, 2020 16:53

DrahtBot added Refactoring Validation labels Apr 4, 2020

ryanofsky force-pushed the pr/nosig branch from ad067a9 to 01639a2 Compare April 4, 2020 17:04

promag reviewed Apr 4, 2020

View reviewed changes

maflcko added this to the 0.20.0 milestone Apr 4, 2020

hebasto reviewed Apr 4, 2020

View reviewed changes

maflcko reviewed Apr 4, 2020

View reviewed changes

src/validationinterface.cpp Outdated Show resolved Hide resolved

ryanofsky force-pushed the pr/nosig branch from 01639a2 to 3d463ad Compare April 4, 2020 23:30

ryanofsky commented Apr 4, 2020

View reviewed changes

bitcoin deleted a comment from promag Apr 4, 2020

hebasto reviewed Apr 5, 2020

View reviewed changes

ryanofsky force-pushed the pr/nosig branch from 3d463ad to b5fea24 Compare April 5, 2020 12:31

hebasto reviewed Apr 5, 2020

View reviewed changes

hebasto approved these changes Apr 5, 2020

View reviewed changes

sipa reviewed Apr 5, 2020

View reviewed changes

src/validationinterface.cpp Outdated Show resolved Hide resolved

ryanofsky force-pushed the pr/nosig branch from b5fea24 to c3a4716 Compare April 6, 2020 00:04

sipa reviewed Apr 6, 2020

View reviewed changes

src/validationinterface.cpp Outdated Show resolved Hide resolved

ryanofsky commented Apr 6, 2020

View reviewed changes

src/validationinterface.cpp Outdated Show resolved Hide resolved

sipa reviewed Apr 6, 2020

View reviewed changes

promag reviewed Apr 6, 2020

View reviewed changes

laanwj added the Bug label Apr 6, 2020

ryanofsky force-pushed the pr/nosig branch from 9617600 to d6815a2 Compare April 6, 2020 12:45

ryanofsky commented Apr 6, 2020

View reviewed changes

promag reviewed Apr 6, 2020

View reviewed changes

laanwj merged commit fdeb445 into bitcoin:master Apr 6, 2020

ryanofsky mentioned this pull request Apr 6, 2020

qa: Test shared validation interface #18471

Closed

sipa reviewed Apr 6, 2020

View reviewed changes

promag mentioned this pull request Apr 9, 2020

boost::signals2 disconnections don't block for slots to finish executing (& CValidationInterface lifecycle races) #18065

Closed

ryanofsky mentioned this pull request Apr 27, 2020

miner: Avoid stack-use-after-return in validationinterface #18742

Merged

str4d mentioned this pull request Dec 6, 2021

Backport upstream PRs that remove Boost usage zcash/zcash#4819

Open

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


		explicit MainSignalsInstance(CScheduler *pscheduler) : m_schedulerClient(pscheduler) {}

		void Register(std::shared_ptr<CValidationInterface> callbacks)

refactor: drop boost::signals2 in validationinterface #18524

refactor: drop boost::signals2 in validationinterface #18524

Uh oh!

Conversation

ryanofsky commented Apr 4, 2020

Uh oh!

practicalswift commented Apr 4, 2020

Uh oh!

naumenkogs commented Apr 4, 2020

Uh oh!

hebasto commented Apr 4, 2020

Uh oh!

ryanofsky commented Apr 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag Apr 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky commented Apr 5, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Apr 5, 2020

Uh oh!

sipa Apr 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

ryanofsky commented Apr 4, 2020 •

edited

Loading

promag Apr 4, 2020 •

edited

Loading

sipa Apr 5, 2020 •

edited

Loading

sipa Apr 6, 2020 •

edited

Loading

ryanofsky left a comment •

edited

Loading