Util: Allow scheduler to be mocked #18037

amitiuttarwar · 2020-01-31T05:52:00Z

This PR is to support functional tests by allowing the scheduler to be mocked via the RPC.

It adds a MockForward method to the scheduler class that iterates through the task queue and reschedules them to be delta_seconds sooner.

This is currently used to support functional testing of the "unbroadcast" set tracking in #18038. If this patch is accepted, it would also be useful to simplify the code in #16698.

amitiuttarwar · 2020-01-31T05:52:14Z

Some relevant info about the analysis of the cost (maintenance burden) vs benefit (supporting a better tested & simpler codebase) of these changes

pros:

these changes add a function to the scheduler with unit test coverage
the function would only be used when running regtest
intent is for this to support easier functional testing for other features
currently, two explicit use cases

cons:

all code adds maintenance burden
c++ code specifically to support testing should only be added when necessary
the scheduler mock is not required for functionally testing the current use cases. workarounds are possible

explanation of the two use cases & workarounds.

P2P: Mempool tracks locally submitted transactions to improve wallet privacy #18038 uses the mock to trigger the next attempt at initial broadcast, which is scheduled for every 10 minutes. Alternatively, the REATTEMPT_BROADCAST_INTERVAL constant that indicates scheduler frequency could be defined as a chain param and made more frequent for regtest (thanks @MarcoFalke for the suggestion).
In PR Mempool: rework rebroadcast logic to improve privacy #16698, specifically commit baf7de2, the mock would allow deleting some of the c++ code (but still be testable.) In the patch right now, the scheduler periodically calls MaybeResendWalletTxs which calls ResubmitWalletTransactionsToMempool which uses nNextResend to track time. nNextResend implements a mockable time so can be tested from the functional tests. If the scheduler mock is supported, the scheduler could directly invoke the resubmit function & drop the member var to maintain time tracking.

JeremyRubin · 2020-01-31T06:09:40Z

utACK 58c4c19.

This seems like a generally useful test harness, and the code looks basically correct to me. I think the pointer to the scheduler is a little crass, but it isn't a bad option considering the scheduler is created as a static in init.cpp (someone can clean it up later if they make the schedule a non static).

The time delta technically could underflow, but I don't think that's the base time being subtracted from is a time point (and we're far from the unix epoch time). If this weren't just testing harness, it might be worth it to check for underflow, but since we'd basically be having to actively try to shoot ourselves in the foot during a test to get an underflow this usage is fine.

Great work!

fanquake · 2020-01-31T06:24:43Z

Concept ACK.

Looks like there's an issue with the new mockforward unit test, as most of the Travis instances are failing:

test/scheduler_tests.cpp(158): Entering test case "mockforward"
test/scheduler_tests.cpp(188): error: in "scheduler_tests/mockforward": check num_tasks == 1 has failed [3 != 1]
test/scheduler_tests.cpp(191): error: in "scheduler_tests/mockforward": check counter == 2 has failed [0 != 2]
test/scheduler_tests.cpp(197): error: in "scheduler_tests/mockforward": check delta > 2*60 && delta < 3*60 has failed
test/scheduler_tests.cpp(158): Leaving test case "mockforward"; testing time: 6321us
test/scheduler_tests.cpp(13): Leaving test suite "scheduler_tests"; testing time: 146946us

DrahtBot · 2020-01-31T08:40:11Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

P2P: Mempool tracks locally submitted transactions to improve wallet privacy #18038 (P2P: Mempool tracks locally submitted transactions to improve privacy by amitiuttarwar)
refactor: Remove mempool global from net #17997 (refactor: Remove mempool global from net by MarcoFalke)
Testschains: Many regtests with different genesis and default datadir #17037 (Testschains: Many regtests with different genesis and default datadir by jtimon)
Tests: Chainparams: Make regtest almost fully customizable #17032 (Tests: Chainparams: Make regtest almost fully customizable by jtimon)
Chainparams: Rename IsTestChain() to AllowAcceptNonstd() #16770 (Chainparams: Rename IsTestChain() to AllowAcceptNonstd() by jtimon)
Log RPC parameters (arguments) if -debug=rpcparams #16365 (Log RPC parameters (arguments) if -debug=rpcparams by LarryRuane)

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.

maflcko · 2020-01-31T09:12:59Z

No objection, but some observations:

In light of the test failures on some platforms, we obviously need to make sure this is doing the right thing. Timestamp handling isn't obviously trivial especially in combination with condition variables. See also the numerous scheduler crashed that no one can explain: scheduler: crash after releasing wallet #16307, client 0.18.0 crashes when computer wakes up from hibernation #16027, bitcoind aborts with boost exception in WSL guest #14200, ...
"Mocking" code so that tests run the mocked code and production runs a different code might defeat the whole point of having the tests in the first place. Tests (especially functional tests) are supposed to test the production code (paths) as closely as possible.

So my personal preference would be to go with some minimally invasive and simple approach like the chainparam. But if everyone else likes this mock approach because it could be used for other testing needs as well, then I won't object having this merged.

src/rpc/misc.cpp

promag

Concept ACK.

Some minor comments otherwise change LGTM.

src/scheduler.cpp

src/node/context.h

src/rpc/misc.cpp

src/test/scheduler_tests.cpp

amitiuttarwar · 2020-02-05T17:00:54Z

I addressed review comments & tests are now passing CI. Added checks in RPC function & scheduler function to ensure reasonable values / no underflow.

This PR has received 3 ACKs - utACK from @JeremyRubin & concept ACKs from @promag @fanquake. Would appreciate re-review 🙏🏽

JeremyRubin · 2020-02-05T18:39:36Z

re-utACK ecb0ad5

promag

@amitiuttarwar sure, will review later.

promag · 2020-02-05T18:51:16Z

src/rpc/misc.cpp

@@ -570,6 +600,7 @@ static const CRPCCommand commands[] =

    /* Not shown in help */
    { "hidden",             "setmocktime",            &setmocktime,            {"timestamp"}},
+    { "hidden",             "mockscheduler",          &mockscheduler,          {"delta_time"}},


Follow up, would be cool to skip this when !regtest and in mockscheduler would be assert(Params().MineBlocksOnDemand()).

I think it's reasonable for @amitiuttarwar to just copy exactly what setmocktime is doing in this regard.

Huh? static const commands[]= will get done prior to working out what chain is in use, so you'd have to do a much more serious rework to remove it from the list of rpcs entirely.

It's a hidden RPC so I don't think this matters much.

@ajtowns I was thinking that CRPCTable::execute could take into account some CRPCCommand flag and then it would result in RPC_METHOD_NOT_FOUND.

promag

FWIW there's no coverage for mockscheduler RPC, just a simple call in the test framework would be nice.

src/scheduler.cpp

src/test/scheduler_tests.cpp

JeremyRubin · 2020-02-06T02:32:02Z

@promag I think it's reasonable that the mockscheduler RPC doesn't have coverage presently -- @amitiuttarwar has a larger project that this testing harness was extracted from as a reviewable chunk, a later PR will be using it extensively to test rebroadcasting logic (see #18038). I think it's reasonable that the coverage in RPC tests lives there & there isn't a separate one just testing the testing rpc, but feel free to disagree.

amitiuttarwar · 2020-02-06T20:15:16Z

thanks for review @promag!

RE testing mockscheduler RPC- as @JeremyRubin mentioned, my next PR uses it here , so the happy path will be tested. But if you prefer separate explicit test coverage, I can add.

I'll incorporate style nits if I push & invalidate current ACKs

src/scheduler.cpp

src/rpc/misc.cpp

sipa · 2020-02-06T22:31:35Z

src/rpc/misc.cpp

@@ -570,6 +600,7 @@ static const CRPCCommand commands[] =

    /* Not shown in help */
    { "hidden",             "setmocktime",            &setmocktime,            {"timestamp"}},
+    { "hidden",             "mockscheduler",          &mockscheduler,          {"delta_time"}},


It's a hidden RPC so I don't think this matters much.

amitiuttarwar · 2020-02-07T18:19:58Z

changes:

mockscheduler rpc updated to use new m_is_mockable_chain chainparams property instead of MineBlocksOnDemand
added checks to mockscheduler and MockForward methods to ensure delta_seconds is within acceptable range

other comments:

considered adding functional test to test mockscheduler rpc, but testing the success case would require actually invoking something scheduled from the c++ code, making it look extremely similar to the PR that provides context for & builds on this one. link to test. So I don't think it makes sense to test independently.

So I believe all existing review comments are addressed.

This PR is ready for review :)

amitiuttarwar · 2020-02-07T19:01:43Z

s390x travis build is failing with "Disk space is too low!" error. everything else is green

appveyor wallet_resendwallettransactions.py failed, seems unlikely that its related but not impossible since MaybeResendWalletTxs is called by the scheduler. investigating.

maflcko

ACK cf04e02 only some style nits 🔵

Show signature and timestamp

Signature:

Timestamp of file with hash 6c8ec4d2d3fd87097e17c0ce53d45761554026de152c054c527f36f2fdc7f39b -

src/scheduler.cpp

src/scheduler.h

src/test/scheduler_tests.cpp

src/chainparams.cpp

src/rpc/misc.cpp

src/init.cpp

amitiuttarwar · 2020-02-11T23:55:07Z

Thank you for the review @MarcoFalke ! I addressed your concerns in a commit (with one open question), currently opened as a PR on my fork: amitiuttarwar#2, will make as a follow up PR if this one gets accepted.

Tried to investigate the appveyor failure and cannot see how it could possibly be related to my changes.

maflcko · 2020-02-18T01:00:00Z

ACK 8bca30e, only change is some style fixups 🕓

Show signature and timestamp

Signature:

Timestamp of file with hash 0370e988925c0bd1dc04b6ff2d771181e26d2b0eb3b28b69eb3c0a0976243393 -

8bca30e [rpc] expose ability to mock scheduler via the rpc (Amiti Uttarwar) 7c8b6e5 [lib] add scheduler to node context (Amiti Uttarwar) 930d837 [test] add chainparams property to indicate chain allows time mocking (Amiti Uttarwar) 1cd43e8 [test] unit test for new MockForward scheduler method (Amiti Uttarwar) a6f6359 [util] allow scheduler to be mocked (Amiti Uttarwar) Pull request description: This PR is to support functional tests by allowing the scheduler to be mocked via the RPC. It adds a `MockForward` method to the scheduler class that iterates through the task queue and reschedules them to be `delta_seconds` sooner. This is currently used to support functional testing of the "unbroadcast" set tracking in #18038. If this patch is accepted, it would also be useful to simplify the code in #16698. ACKs for top commit: MarcoFalke: ACK 8bca30e, only change is some style fixups 🕓 Tree-SHA512: 2a97fe8ade2b7fd1fb5cdfa1dcafb3227a377d7a847e3845a228bc119eb77824b4aefa43d922a06d583939b22725e223f308cf092961048079d36f6b1d9a639b

8bca30e [rpc] expose ability to mock scheduler via the rpc (Amiti Uttarwar) 7c8b6e5 [lib] add scheduler to node context (Amiti Uttarwar) 930d837 [test] add chainparams property to indicate chain allows time mocking (Amiti Uttarwar) 1cd43e8 [test] unit test for new MockForward scheduler method (Amiti Uttarwar) a6f6359 [util] allow scheduler to be mocked (Amiti Uttarwar) Pull request description: This PR is to support functional tests by allowing the scheduler to be mocked via the RPC. It adds a `MockForward` method to the scheduler class that iterates through the task queue and reschedules them to be `delta_seconds` sooner. This is currently used to support functional testing of the "unbroadcast" set tracking in bitcoin#18038. If this patch is accepted, it would also be useful to simplify the code in bitcoin#16698. ACKs for top commit: MarcoFalke: ACK 8bca30e, only change is some style fixups 🕓 Tree-SHA512: 2a97fe8ade2b7fd1fb5cdfa1dcafb3227a377d7a847e3845a228bc119eb77824b4aefa43d922a06d583939b22725e223f308cf092961048079d36f6b1d9a639b

jonasschnelli · 2020-02-19T17:39:35Z

This PR seems to have made bitcoinbuilds.org pretty unhappy:

  what():  boost::condition_variable::do_wait_until failed in pthread_cond_timedwait: Invalid argument
unknown location(0): fatal error: in "scheduler_tests/mockforward": signal: SIGABRT (application abort requested)
test/scheduler_tests.cpp(172): last checkpoint

Example build:
https://bitcoinbuilds.org/index.php?job=5c006a9e-b8e6-4932-a6a8-3ead3d1fc188

Any ideas why we see a SIGABRT?

I think it happens at std::thread scheduler_thread([&]() { scheduler.serviceQueue(); }); in the scheduler tests.

maflcko · 2020-02-19T18:02:36Z

@jonasschnelli See #18174

jonasschnelli · 2020-02-19T18:05:30Z

@MarcoFalke: I guess you where trying to refer to #18174. Looking into that now...

jonasschnelli · 2020-02-19T18:26:14Z

Master (Linux 64 depends tests) fails when using ccache cache (seems to be reproducible 100%):
https://bitcoinbuilds.org/index.php?job=c1d3a0f5-7d47-40e6-9c9b-cac397c5ff30

Master succeeds when clearing the ccache cache (but keeping the dependency cache):
https://bitcoinbuilds.org/index.php?job=f28f0356-d431-4428-832b-49bc870042f5

…hain 2455aa5 [rpc] changed MineBlocksOnDemand to IsMockableChain (Gloria Zhao) Pull request description: Change: Update the if statement in `setmocktime` to use `IsMockableChain` chainparams function (aka `m_is_mockable_chain`) instead of `MineBlocksOnDemand` Rationale: It's a more appropriate check for whether or not chain is in RegTest, as [discussed](bitcoin#18037 (comment)) in bitcoin#18037 ACKs for top commit: MarcoFalke: ACK 2455aa5 🙇 jonatack: ACK 2455aa5 Tree-SHA512: 1d8c8b7ff0b3c1bcbf5755194969b6664fe05a35003375ad08d18e34bcefd2df4f64d0e60078a10bbef3c8f469a9b9d07db467089b55c14cf532304bc965bffc

Summary: Add MockForward method to the scheduler that mimics going into the future by rescheduling all items on the taskQueue to be sooner. This is a partial backport of Core [[bitcoin/bitcoin#18037 | PR18037]] : bitcoin/bitcoin@a6f6359 Test Plan: ninja all check-all Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D6499

Summary: This is a partial backport of Core [[bitcoin/bitcoin#18037 | PR18037]] : bitcoin/bitcoin@1cd43e8 Depends on D6499 Test Plan: ninja all check-all Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D6500

Summary: Thi is a partial backport of Core [[bitcoin/bitcoin#18037 | PR18037]] : bitcoin/bitcoin@930d837 Depends on D6500 Test Plan: ninja all check-all Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D6501

Summary: - also update test setup & access point in denial of service test This is a partial backport of Core [[bitcoin/bitcoin#18037 | PR18037]] : bitcoin/bitcoin@7c8b6e5 Depends on D6501 Test Plan: ninja all check-all Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D6502

Summary: This is a partial abckport of Core [[bitcoin/bitcoin#18037 | PR18037]] : bitcoin/bitcoin@8bca30e Depends on D6502 and D6498 Test Plan: ninja all check-all Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D6503

Summary: Add MockForward method to the scheduler that mimics going into the future by rescheduling all items on the taskQueue to be sooner. This is a partial backport of Core [[bitcoin/bitcoin#18037 | PR18037]] : bitcoin/bitcoin@a6f6359 Test Plan: ninja all check-all Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D6499

Summary: This is a partial backport of Core [[bitcoin/bitcoin#18037 | PR18037]] : bitcoin/bitcoin@1cd43e8 Depends on D6499 Test Plan: ninja all check-all Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D6500

8bca30e [rpc] expose ability to mock scheduler via the rpc (Amiti Uttarwar) 7c8b6e5 [lib] add scheduler to node context (Amiti Uttarwar) 930d837 [test] add chainparams property to indicate chain allows time mocking (Amiti Uttarwar) 1cd43e8 [test] unit test for new MockForward scheduler method (Amiti Uttarwar) a6f6359 [util] allow scheduler to be mocked (Amiti Uttarwar) Pull request description: This PR is to support functional tests by allowing the scheduler to be mocked via the RPC. It adds a `MockForward` method to the scheduler class that iterates through the task queue and reschedules them to be `delta_seconds` sooner. This is currently used to support functional testing of the "unbroadcast" set tracking in bitcoin#18038. If this patch is accepted, it would also be useful to simplify the code in bitcoin#16698. ACKs for top commit: MarcoFalke: ACK 8bca30e, only change is some style fixups 🕓 Tree-SHA512: 2a97fe8ade2b7fd1fb5cdfa1dcafb3227a377d7a847e3845a228bc119eb77824b4aefa43d922a06d583939b22725e223f308cf092961048079d36f6b1d9a639b

Summary: > Change: Update the if statement in setmocktime to use IsMockableChain chainparams function (aka m_is_mockable_chain) instead of MineBlocksOnDemand > > Rationale: It's a more appropriate check for whether or not chain is in RegTest, as discussed in [[bitcoin/bitcoin#18037 | PR18037]] This is a backport of Core [[bitcoin/bitcoin#18263 | PR18263]] Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D8783

…hain 2455aa5 [rpc] changed MineBlocksOnDemand to IsMockableChain (Gloria Zhao) Pull request description: Change: Update the if statement in `setmocktime` to use `IsMockableChain` chainparams function (aka `m_is_mockable_chain`) instead of `MineBlocksOnDemand` Rationale: It's a more appropriate check for whether or not chain is in RegTest, as [discussed](bitcoin#18037 (comment)) in bitcoin#18037 ACKs for top commit: MarcoFalke: ACK 2455aa5 🙇 jonatack: ACK 2455aa5 Tree-SHA512: 1d8c8b7ff0b3c1bcbf5755194969b6664fe05a35003375ad08d18e34bcefd2df4f64d0e60078a10bbef3c8f469a9b9d07db467089b55c14cf532304bc965bffc

fanquake added the Utils/log/libs label Jan 31, 2020

amitiuttarwar mentioned this pull request Jan 31, 2020

P2P: Mempool tracks locally submitted transactions to improve wallet privacy #18038

Merged

kristapsk reviewed Feb 1, 2020

View reviewed changes

src/rpc/misc.cpp Outdated Show resolved Hide resolved

promag reviewed Feb 3, 2020

View reviewed changes

src/scheduler.cpp Outdated Show resolved Hide resolved

src/node/context.h Show resolved Hide resolved

src/rpc/misc.cpp Show resolved Hide resolved

src/rpc/misc.cpp Outdated Show resolved Hide resolved

src/rpc/misc.cpp Outdated Show resolved Hide resolved

promag reviewed Feb 3, 2020

View reviewed changes

src/test/scheduler_tests.cpp Outdated Show resolved Hide resolved

amitiuttarwar force-pushed the 2020-01-mock-scheduler branch 2 times, most recently from 88bc0fd to ecb0ad5 Compare February 5, 2020 05:43

promag reviewed Feb 5, 2020

View reviewed changes

promag reviewed Feb 6, 2020

View reviewed changes

src/scheduler.cpp Outdated Show resolved Hide resolved

src/test/scheduler_tests.cpp Outdated Show resolved Hide resolved

src/test/scheduler_tests.cpp Outdated Show resolved Hide resolved

sipa reviewed Feb 6, 2020

View reviewed changes

amitiuttarwar force-pushed the 2020-01-mock-scheduler branch from ecb0ad5 to cf04e02 Compare February 7, 2020 17:10

maflcko approved these changes Feb 9, 2020

View reviewed changes

maflcko reviewed Feb 10, 2020

View reviewed changes

src/init.cpp Outdated Show resolved Hide resolved

This was referenced Feb 11, 2020

Testschains: Many regtests with different genesis and default datadir #17037

Closed

Tests: Chainparams: Make regtest almost fully customizable #17032

Closed

amitiuttarwar mentioned this pull request Feb 11, 2020

2020 01 mock scheduler followup amitiuttarwar/bitcoin#2

Closed

This was referenced Feb 11, 2020

Chainparams: Rename IsTestChain() to AllowAcceptNonstd() #16770

Closed

Log RPC parameters (arguments) if -debug=rpcparams #16365

Closed

maflcko merged commit 8bca30e into bitcoin:master Feb 18, 2020

amitiuttarwar mentioned this pull request Feb 18, 2020

WIP test: make mockscheduler test more reliable #18174

Closed

amitiuttarwar mentioned this pull request Feb 28, 2020

scheduler test causing pthread_cond_timedwait: Invalid argument error #18227

Closed

glozow mentioned this pull request Mar 4, 2020

rpc: change setmocktime check to use IsMockableChain #18263

Merged

MarkLTZ mentioned this pull request Apr 4, 2020

Bitcoin PR tracking litecoinz-core/litecoinz#14

Closed

kwvg added a commit to kwvg/dash that referenced this pull request Jan 9, 2022

merge bitcoin#18037: Allow scheduler to be mocked

43f4443

kwvg added a commit to kwvg/dash that referenced this pull request Jan 11, 2022

merge bitcoin#18037: Allow scheduler to be mocked

31e8208

kwvg mentioned this pull request Jan 11, 2022

merge bitcoin#15931, #16839, #17192, #17407, #18037, #17997, partial #15639, #17989: deglobalisation and mining rpc backports dashpay/dash#4357

Closed

kwvg added a commit to kwvg/dash that referenced this pull request Jan 25, 2022

merge bitcoin#18037: Allow scheduler to be mocked

09a517b

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

Util: Allow scheduler to be mocked #18037

Util: Allow scheduler to be mocked #18037

Uh oh!

Conversation

amitiuttarwar commented Jan 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amitiuttarwar commented Jan 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JeremyRubin commented Jan 31, 2020

Uh oh!

fanquake commented Jan 31, 2020

Uh oh!

DrahtBot commented Jan 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

maflcko commented Jan 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

amitiuttarwar commented Feb 5, 2020

Uh oh!

JeremyRubin commented Feb 5, 2020

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

promag Feb 5, 2020

Choose a reason for hiding this comment

Uh oh!

JeremyRubin Feb 6, 2020

Choose a reason for hiding this comment

Uh oh!

ajtowns Feb 6, 2020

Choose a reason for hiding this comment

Uh oh!

sipa Feb 6, 2020

Choose a reason for hiding this comment

Uh oh!

promag Feb 6, 2020

Choose a reason for hiding this comment

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JeremyRubin commented Feb 6, 2020

Uh oh!

amitiuttarwar commented Feb 6, 2020

Uh oh!

Uh oh!

Uh oh!

sipa Feb 6, 2020

Choose a reason for hiding this comment

Uh oh!

amitiuttarwar commented Feb 7, 2020

Uh oh!

amitiuttarwar commented Feb 7, 2020

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

amitiuttarwar commented Jan 31, 2020 •

edited

Loading

amitiuttarwar commented Jan 31, 2020 •

edited

Loading

DrahtBot commented Jan 31, 2020 •

edited

Loading

maflcko commented Jan 31, 2020 •

edited

Loading

maflcko commented Feb 19, 2020 •

edited

Loading