privatesend|wallet|qt: Improve calculations in CreateDenominated/MakeCollateralAmounts #3657

xdustinface · 2020-08-07T11:36:45Z

While the idea of respecting fees in calculations brought up in #3588 and #3640 makes totally sense i still noticed while reviewing/testing the latter that the estimation of just saying use min fee for 148 bytes for inputs and min fee for 34 bytes for outputs and then sum it up isn’t good enough imo.

It doen’t respect the general tx overhead
It introduces rounding errors with the increasing numbers of inputs/outputs

So even with those estimations we end up in cases where the calculations are slightly off from the expected. This brings inaccuracy to transaction generation and leads to issues (e.g. the. one in TransactionRecord::decomposeTransaction).

As solution this PR introduces CTransactionBuilder in 13fa395eeb0050225ce2f1a77418d1af96137b0e, a class which wraps transaction creation and allows a simple and failproof creation of transactions with inputs defined by CompactTallyItem while also respecting the logics implemented in CWallet::CreateTransaction. For output generation it takes care of key generation, provides methods e.g. to add/try to add outputs, get the remaining amount available, and methods to create/commit the transaction to the wallet. To avoid running into the issues described above it does not calculate a seperate fee for the amount of bytes for input/outputs like it was before. Instead it uses bytes for all calculations and calculates a total fee for the transaction when required. This way it’s possible now to calculate the correct amounts/fees used for the created transactions.

Further on top this PR brings also some small fixes/modifications to the logic behind CreateDenominated and MakeCollateralAmounts in fba14a1 and 8e0191c so please review very carefully there that we can be sure i didn’t mess something here :)

3d00601 finally makes sure #3597 is not longer an issue. As it now should cover all the cases produced by MakeCollateralAmounts correctly.

UdjinM6 · 2020-08-09T21:43:18Z

Pls see https://github.com/UdjinM6/dash/commits/pr3657

tl;dr:

209f43f should be reverted cause it violates discard/dustrelay fee definitions
I think you messed up "finished" conditions (39a60562aa95ef2a4dbd6a49e00c2cc0f61957a5)
The rest is just some refactoring/cleanups.

xdustinface · 2020-08-10T16:49:53Z

209f43f should be reverted cause it violates discard/dustrelay fee definitions

Isn't a partial revert like in 1365db1 enough? This way we can still say 100% sure that the same feerate is used for change dust consideration.

I think you messed up "finished" conditions (39a6056)

Ops, good catch!

The rest is just some refactoring/cleanups.

Picked them all 👍

xdustinface · 2020-08-11T01:39:40Z

@UdjinM6 While reviewing your suggestions more precisely i realized that your refactoring of GetAmountUsed() in 2b01a09 introduced a rounding bug. Reason it didn't respect fees in the beginning was to always have only one fee calculation with the total number of bytes. With it respecting fees you need to call GetFee() twice with subtotals in TryAddOutput and TryAddOutputs https://github.com/dashpay/dash/blob/2b01a09c6e8263095f316961a62b8bfa514ec487/src/privatesend/privatesend-util.cpp#L169-L172 First call is in GetAmountLeft() which calls GetAmountUsed() https://github.com/dashpay/dash/blob/2b01a09c6e8263095f316961a62b8bfa514ec487/src/privatesend/privatesend-util.cpp#L198-L206 This twice calling GetFee() results in a rounding error due to integer cutoffs here https://github.com/dashpay/dash/blob/2b01a09c6e8263095f316961a62b8bfa514ec487/src/policy/feerate.cpp#L28

Lets assume GetBytesTotal() == 200 and nBytesOutput == 34 and the feerate 1234 duff/KB

With 2b01a09 applied:
1234*200/1000 = 246,8 (246 in integer world)
1234*34/1000 = 41,956 (41 in integer world)
= 287 duffs fee

With the initial commits:
1234*234/1000 = 288,756 (288 in integer world)
= 288 duffs fee

With the force push i dropped 2b01a09, 7c30d5d, f90e626 again. The latter two because they bring no drawbacks but IMO without 2b01a09 it just brings more readability to keep them.. still feel free to complain again if you prefer to have them dropped for some reason. There are also the new commits 94529da, 3cb7d2d and 7502fcb

UdjinM6 · 2020-08-11T09:43:54Z

Makes sense. Still don't see a reason for CoinControl::m_discard_feerate though abbb195ff5.

xdustinface · 2020-08-11T10:46:36Z

Makes sense. Still don't see a reason for CoinControl::m_discard_feerate though abbb195.

Okaaay... just one more time a bit more detailed in case you didn't get the reasoning i have in mind for it: I can't say for sure that there is no chance for the code down in the deeps of GetDiscardRate() to give different results when it's called twice with some time elapsed in between, can you? So for me thats the reason to have CCoinControl::m_discard_rate as it makes sure we still use the same feerate in both cases (CTransactionBuilder::IsDust and CWallet::CreateTransaction). What may happen if the results are different is that CTransactionBuilder::Commit fails. Sure, then it tries it one more time but what if there is only one utxo left with a dust change, this continues to happen and the mixing starts to stuck because of it? I know, it sounds not very likely 😆 but at the end it's a potential unexpected scenario we could avoid with few lines of code. I rather prefer to prevent such edge cases if i know about them. But if you are sure that can't happen or if you think this is an acceptable case just give me your 👍 and i will drop it.

UdjinM6

a couple of nits

src/privatesend/privatesend-util.cpp

src/wallet/wallet.cpp

UdjinM6 · 2020-08-11T11:26:22Z

Oh, by 👍 I meant that it makes sense and I understand the reason now 🙈 pls ignore my previous suggestion and drop 5fb050b

xdustinface · 2020-08-11T11:29:20Z

Oh, by 👍 I meant that it makes sense and I understand the reason now 🙈 pls ignore my previous suggestion and drop 5fb050b

Ahhh ops, i took it as the "go drop it thumbs up" 😅

UdjinM6 · 2020-08-13T14:20:44Z

Pls see https://github.com/UdjinM6/dash/commits/pr3657 (starting with ~~703922203ecfa8f18a2c0c9ed853d91ab7c3fc36~~8b19c551fd85840aeadeaf64f04ffe655cd6072f)

EDIT: tl;dr: change outputs should be handled automatically, size calculations need to be more precise, some minor tweaks (off-by-one, types, logs, comments)

xdustinface · 2020-08-13T21:42:46Z

Overall nice suggestions but im not a big fan of 703922203ecfa8f18a2c0c9ed853d91ab7c3fc36 yet, convince me :D

Why does it matter in this case if the change is not handled automatically? The idea of doing it manually was to have two outputs with the same amount in this case as this imo reduces potential false positive detection even more for the "make collateral amounts tx type".

Edit: Also there is no "Make sure its not denominated" anymore for the change if its done automatically as the amount is not set through getAdjustedAmount

UdjinM6 · 2020-08-14T10:58:32Z

Why does it matter in this case if the change is not handled automatically?

Added f6f0f8ac72. Try mixing with --paytxfee=0.001 for example and you'll see that it fails without 7039222 - when feerate is much greater than discard feerate, wallet tries to create new change output in such cases and fails with "not enough funds".

Also there is no "Make sure its not denominated" anymore for the change if its done automatically as the amount is not set through getAdjustedAmount.

Right, it's not really used after 7039222 (wasn't ever needed in case3, the smallest denom is 10x larger than GetCollateralAmount) and it can be dropped together with UpdateAmount ced2939491.

xdustinface · 2020-08-14T18:59:56Z

Nice catch with the extra byte due to the compact serialize size 👍 I added some commits related to that to wrap it up a bit and to also respect this when trying to add outputs in TryAddOutput and TryAddOutputs. Let me know what you think.

8b19c551fd85840aeadeaf64f04ffe655cd6072f is also a good catch! But i modified it a bit in 05ee734, thoughts?

And i picked the others except 703922203ecfa8f18a2c0c9ed853d91ab7c3fc36. But i will have a better look at it and your points again later if i get the chance or tomorrow!

src/privatesend/privatesend-client.cpp

src/wallet/wallet.cpp

PastaPastaPasta

Did a limited code review, didn't review everything.

Big takeaway so far:
I'm not a huge fan of adding ~320 LOC, but that's probably okay.
CTransactionBuilder really needs a good amount of unit tests

src/privatesend/privatesend-client.cpp

PastaPastaPasta · 2020-08-16T15:35:52Z

src/privatesend/privatesend-client.cpp

+        CTransactionBuilderOutput* out = txBuilder.AddOutput();
+        out->UpdateAmount(getAdjustedAmount(txBuilder.GetAmountLeft()));
+
+        assert(CPrivateSend::IsCollateralAmount(out->GetAmount()));


These asserts kindof scare me tbh...

Why exactly?

I could imagine a situation where somehow something gets changed in such a way that it's too much or too little to pass this assert, and this isn't caught in testing. I would rather log an error, and return false than assert and crash the application. You could maybe use something like

dash/src/validation.cpp

Line 1546 in fea0036

bool AbortNode(const std::string& strMessage, const std::string& userMessage="")

if you want to somewhat gracefully shutdown the application

thoughts @UdjinM6 ?

@UdjinM6 I would like your thoughts here

Hm imo its nothing to worry about, we have unit tests and

dash/src/privatesend/privatesend-client.cpp

Line 1442 in 9a7b36a

if (!txBuilder.CouldAddOutput(CPrivateSend::GetCollateralAmount())) {

dash/src/privatesend/privatesend-client.cpp

Line 1460 in 9a7b36a

} else if (txBuilder.CouldAddOutputs({CPrivateSend::GetCollateralAmount(), CPrivateSend::GetCollateralAmount()})) {

imo it's ok

src/privatesend/privatesend-util.cpp

PastaPastaPasta · 2020-08-16T15:48:30Z

src/privatesend/privatesend-util.cpp

+CAmount CTransactionBuilder::GetAmountLeft(const CAmount nAmountInitial, const CAmount nAmountUsed, const CAmount nFee)
+{
+    return nAmountInitial - nAmountUsed - nFee;
+}


I'm not sure I understand the point of this function at this point is it's just a glorified subtraction function...

Jup its just a subtraction helper. https://github.com/dashpay/dash/blob/e7dc9da05c9e6cf6be4173ad65aecbc611cd1992/src/privatesend/privatesend-util.h#L119 imo it improves the readability of e.g.

https://github.com/dashpay/dash/blob/e7dc9da05c9e6cf6be4173ad65aecbc611cd1992/src/privatesend/privatesend-util.cpp#L169-L174

As it wraps the subtrahends in GetAmountLeft() so you can just see by reading the wrappers name what the point of the return is. I don't see any drawback by doing it like this as the compiler should be smart enough to take care of it, but i neither will fight for keeping it lol.. so if you see any problems with this, just complain again.

src/privatesend/privatesend-util.cpp

PastaPastaPasta · 2020-08-16T15:52:35Z

src/privatesend/privatesend-util.cpp

+
+    fKeepKeys = true;
+
+    strResult = wtx.GetHash().ToString();
+


I would say remove a few of these empty lines, probably line 300 at least

xdustinface · 2020-08-24T11:16:21Z

Added f6f0f8a, ee0e3ee, e42c43f
Dropped 8d4dcd2 (moved it into a4620e4), 6801983 (not longer needed if #3668 gets merged), part of dfebc25

@UdjinM6 are you good now with #3668 instead of 7039222?

xdustinface · 2020-08-26T03:03:36Z

Rebased on develop + e755205

UdjinM6

Seems to be working as expected (together with #3668 )

ACK

xdustinface · 2020-08-29T19:17:35Z

see https://github.com/PastaPastaPasta/dash/commits/pr_3657

Picked 87b7431, 2543a4c and bcdcb13 (squashed the 3 typo commits) 👍

About the others:

71d49f9bd57dc0937358001f5638f1c506b114dc and 6c34988801d640a91d624a3ab8cc7ae70ab0cf58: does it really bring any improvement / make a difference to use explicit casts there? IMO this just blows it up unnecessarily. We will never reach anything near INT_MAX in any of those cases, do we?

eaeb50e7a61dd6125f9cde76977e76770fed29be: why?

PastaPastaPasta · 2020-08-29T19:21:53Z

see https://github.com/PastaPastaPasta/dash/commits/pr_3657

Picked 87b7431, 2543a4c and bcdcb13 (squashed the 3 typo commits) +1

About the others:

71d49f9 and 6c34988: does it really bring any improvement / make a difference to use explicit casts there? IMO this just blows it up unnecessarily. We will never reach anything near INT_MAX in any of those cases, do we?

eaeb50e: why?

eaeb50e resolves a clang-tidy warning

71d49f9#diff-7d55c5a40c548772705e07a8dff7a49eR188-R189 L189 specifically resolves a clang-tidy, and the rest imo it's good standard to validate when going from unsigned int -> int or long -> int or really any narrowing conversion, that we aren't having something weird happen. I think the explicit cast on L249 also resolves clang-tidy warning

6c34988 resolves clang-tidy warning

Signed-off-by: pasta <[email protected]>

estimateSmartFee handles it if negative Signed-off-by: pasta <[email protected]>

xdustinface · 2020-08-29T19:44:56Z

Ok, seems like im just not a big fan of some of those clang-tidy recommendations lol.. not the first time i noticed that haha.. and i really wonder if we should always follow them.. especially eaeb50e is one i can't understand at all 😆 anyway, they don't do anything bad so i just picked them.

PastaPastaPasta · 2020-08-30T04:56:39Z

Ok, seems like im just not a big fan of some of those clang-tidy recommendations lol.. not the first time i noticed that haha.. and i really wonder if we should always follow them.. especially eaeb50e is one i can't understand at all laughing anyway, they don't do anything bad so i just picked them.

see https://stackoverflow.com/a/46292715

xdustinface · 2020-08-30T10:39:55Z

see https://stackoverflow.com/a/46292715

It's not that i didn't get that it doesn't make a difference :D I just don't see the point why clang-tidy prefers to introduce inconsistencies between declaration and definition. I mean

static CAmount GetAmountLeft(CAmount, CAmount, CAmount);

also works, but you also don't leave variable names out of the declaration only because they are ignored by the compiler and they don't have an impact on the definition, or is this also something clang-tidy recommends?

src/privatesend/privatesend-util.h

Co-authored-by: UdjinM6 <[email protected]>

PastaPastaPasta · 2020-08-30T18:21:48Z

see https://stackoverflow.com/a/46292715

It's not that i didn't get that it doesn't make a difference :D I just don't see the point why clang-tidy prefers to introduce inconsistencies between declaration and definition. I mean
static CAmount GetAmountLeft(CAmount, CAmount, CAmount);
also works, but you also don't leave variable names out of the declaration only because they are ignored by the compiler and they don't have an impact on the definition, or is this also something clang-tidy recommends?

Seems like most of the thought process is the fact that something is const doesn't change how someone is going to call that function, whereas knowing the names is pretty valuable

UdjinM6

utACK

src/privatesend/privatesend-util.h

PastaPastaPasta · 2020-08-31T20:28:17Z

I think I'm ready to ACK once those comments become doxygen

PastaPastaPasta

ACK, code review, as well as mixed a good bit on testnet

…CollateralAmounts (dashpay#3657) * wallet: Add m_dust_feerate to CCoinControl / Use it in CreateTransaction * privatesend: Introduce CTransactionBuilder/CTransactionBuilderOutput Builder classes for transactions from type Inputs: Defined by CompactTallyItem Outputs: Simple outputs with lose reserve keys This takes fully takes care of fee calculations and makes sure calculations are the same like those happening when actually create the transaction with CreateTransaction. * privatesend: Improve amount/fee calculation in CreateDenominated * privatesend: Improve amount/fee calculation in MakeCollateralAmounts * qt: Fix decomposeTransaction's MakeCollateralAmounts detection Align it with the three cases in CPrivateSendClientSession::MakeCollateralAmounts * Refactor GetFee The fee rate is always coinControl.m_feerate, also it's not used outside so should be a private method * Simplify nBytesOutput calculations * Drop unused GetBytesOutput() * Make Clear(), GetBytesTotal() and GetAmountUsed() private * Drop unused GetCoinControl() * Make ToString() const * Refactor `CTransactionBuilder::Commit()` * Reorder cases in decomposeTransaction * Fix "finished" conditions in CreateDenominated * Fix typo * wallet|privatesend: Refactor CCoinControl's m_dust_feerate -> m_discard_feerate * privatesend: Drop unused member CTransactionBuilder::dustFeeRate * privatesend: Improve CTransactionBuilder's readability * privatesend: Make the static CTransactionBuilder::GetAmountLeft private * wallet: Recover previous code style * Update src/privatesend/privatesend-util.cpp Co-authored-by: UdjinM6 <[email protected]> * Tweak CTransactionBuilder to respect potential compact size diffs * Tweak GetFee param type * Trivial log/comments tweaks * privatesend: Fix countPossibleOutputs - Fix off by one issue - Respect max outputs threshold * privatesend: Use GetSizeOfCompactSizeDiff in CTransactionBuilder * Apply suggestions from code review Co-authored-by: PastaPastaPasta <[email protected]> * privatesend: Rename TryAdd to CouldAdd in CTransactionBuilder * wallet: Reset m_discard_feerate in CCoinControl::SetNull * Respect `-paytxfee` and `settxfee` * privatesend: Check for denominated amount only where really required * qt: Remove obsolete IsCollateralAmount() checks * privatesend: Don't accept negative amounts in CTransactionBuilder * privatesend: Remove unused CConnman parameter * use emplace_back instead of push_back Signed-off-by: pasta <[email protected]> * fix typos Signed-off-by: pasta <[email protected]> * make GetAmount const Signed-off-by: pasta <[email protected]> * privatesend: Explicit capture __func__ in needMoreOutputs lambda * privatesend: Update CTransactionBuilder::UpdateAmount * remove const on parameter in declaration Signed-off-by: pasta <[email protected]> * handle unsigned int -> int conversions a bit better Signed-off-by: pasta <[email protected]> * explicitly cast to int from unsigned int. estimateSmartFee handles it if negative Signed-off-by: pasta <[email protected]> * Make CTransactionBuilderOutput::GetScript const Co-authored-by: UdjinM6 <[email protected]> * privatesend: Update comments to follow doxygen * privatesend: Add class descriptions Co-authored-by: UdjinM6 <[email protected]> Co-authored-by: PastaPastaPasta <[email protected]> Co-authored-by: pasta <[email protected]>

xdustinface mentioned this pull request Aug 7, 2020

More accurate fee/amount calculations in MakeCollateralAmounts #3640

Closed

xdustinface force-pushed the pr-privatesend-transaction-builder branch from efb87d2 to 3d00601 Compare August 7, 2020 15:18

xdustinface force-pushed the pr-privatesend-transaction-builder branch from ab18ddd to 7502fcb Compare August 11, 2020 01:32

UdjinM6 mentioned this pull request Aug 11, 2020

Add missing change output fee in CreateDenominated #3641

Closed

UdjinM6 added this to the 16 milestone Aug 11, 2020

UdjinM6 added the backport-candidate-16.x label Aug 11, 2020

UdjinM6 reviewed Aug 11, 2020

View reviewed changes

src/privatesend/privatesend-util.cpp Outdated Show resolved Hide resolved

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

xdustinface force-pushed the pr-privatesend-transaction-builder branch from 5fb050b to 7502fcb Compare August 11, 2020 11:30

UdjinM6 reviewed Aug 14, 2020

View reviewed changes

src/privatesend/privatesend-client.cpp Outdated Show resolved Hide resolved

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

PastaPastaPasta reviewed Aug 16, 2020

View reviewed changes

xdustinface mentioned this pull request Aug 22, 2020

wallet: Fix and improve CWallet::CreateTransaction #3668

Merged

xdustinface force-pushed the pr-privatesend-transaction-builder branch from e7dc9da to e42c43f Compare August 24, 2020 11:14

PastaPastaPasta mentioned this pull request Aug 24, 2020

trivial: return bool instead of int in CPrivateSendClientOptions #3660

Merged

xdustinface force-pushed the pr-privatesend-transaction-builder branch from 2f5ede3 to e755205 Compare August 26, 2020 03:01

xdustinface mentioned this pull request Aug 26, 2020

test: Implement unit tests for CTransactionBuilder #3677

Merged

UdjinM6 previously approved these changes Aug 26, 2020

View reviewed changes

xdustinface mentioned this pull request Aug 29, 2020

bitcoin#9680: Unify CWalletTx construction #3680

Merged

xdustinface added 2 commits August 29, 2020 21:04

privatesend: Explicit capture __func__ in needMoreOutputs lambda

aacb056

privatesend: Update CTransactionBuilder::UpdateAmount

7814ff3

xdustinface force-pushed the pr-privatesend-transaction-builder branch from b857b1c to 7814ff3 Compare August 29, 2020 19:05

PastaPastaPasta added 3 commits August 29, 2020 21:31

remove const on parameter in declaration

531a917

Signed-off-by: pasta <[email protected]>

handle unsigned int -> int conversions a bit better

a7d81d9

Signed-off-by: pasta <[email protected]>

explicitly cast to int from unsigned int.

f740612

estimateSmartFee handles it if negative Signed-off-by: pasta <[email protected]>

UdjinM6 reviewed Aug 30, 2020

View reviewed changes

src/privatesend/privatesend-util.h Outdated Show resolved Hide resolved

Make CTransactionBuilderOutput::GetScript const

9a7b36a

Co-authored-by: UdjinM6 <[email protected]>

UdjinM6 previously approved these changes Aug 30, 2020

View reviewed changes

PastaPastaPasta requested changes Aug 31, 2020

View reviewed changes

src/privatesend/privatesend-util.h Outdated Show resolved Hide resolved

xdustinface added 2 commits September 1, 2020 01:56

privatesend: Update comments to follow doxygen

c30df0d

privatesend: Add class descriptions

fb2c6d0

xdustinface dismissed UdjinM6’s stale review via fb2c6d0 August 31, 2020 23:58

PastaPastaPasta approved these changes Sep 1, 2020

View reviewed changes

UdjinM6 merged commit a206af4 into dashpay:develop Sep 1, 2020

UdjinM6 removed the backport-candidate-16.x label Sep 14, 2020

xdustinface mentioned this pull request Nov 20, 2020

[0.16.0.0-rc2] Mixing TXes of type Payment to yourself show up in the Overview tab #3597

Closed

qwizzie mentioned this pull request Jan 1, 2021

v0.17.0.0 RC2 PrivateSend Denominate Transactions visible in Overview tab - Recent Transactions #3910

Closed

privatesend|wallet|qt: Improve calculations in CreateDenominated/MakeCollateralAmounts #3657

privatesend|wallet|qt: Improve calculations in CreateDenominated/MakeCollateralAmounts #3657

Uh oh!

Conversation

xdustinface commented Aug 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

UdjinM6 commented Aug 9, 2020

Uh oh!

xdustinface commented Aug 10, 2020

Uh oh!

xdustinface commented Aug 11, 2020

Uh oh!

UdjinM6 commented Aug 11, 2020

Uh oh!

xdustinface commented Aug 11, 2020

Uh oh!

UdjinM6 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

UdjinM6 commented Aug 11, 2020

Uh oh!

xdustinface commented Aug 11, 2020

Uh oh!

UdjinM6 commented Aug 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xdustinface commented Aug 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

UdjinM6 commented Aug 14, 2020

Uh oh!

xdustinface commented Aug 14, 2020

Uh oh!

Uh oh!

Uh oh!

PastaPastaPasta left a comment

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!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

xdustinface commented Aug 24, 2020

Uh oh!

xdustinface commented Aug 26, 2020

Uh oh!

UdjinM6 left a comment

Choose a reason for hiding this comment

Uh oh!

xdustinface commented Aug 29, 2020

Uh oh!

PastaPastaPasta commented Aug 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xdustinface commented Aug 7, 2020 •

edited

Loading

UdjinM6 commented Aug 13, 2020 •

edited

Loading

xdustinface commented Aug 13, 2020 •

edited

Loading

PastaPastaPasta commented Aug 29, 2020 •

edited

Loading