wallet: add cached m_is_ibd to remove Chain::isInitialBlockDownload #17484

ariard · 2019-11-14T22:15:08Z

Dependency on #15719

Actually we lock the chain every-time a resend or transaction is broadcast to evaluate if node is currently in-or-out of IBD. This PR proposes instead to rely on updatedBlockTip to refresh IBD status, therefore increasing asynchronicity between wallet-node and should be a slight performance improvement.

achow101 · 2019-11-15T00:08:16Z

It let call sethseed while being disconnected, which may be blameworthty but as createwallet let you already generate a HD wallet without checking for IBD, we should go either way or the other, i.e can generate HD-seed without out-of-IBD or can't generate HD-seed without out-of-IBD. Thoughts ?

The reason that you had to be out of IBD in order to set an HD seed is because by setting a new HD seed, no new keys would ever be generated from the old seed. So if there were a transaction that involved a key that hadn't been generated yet and was in a block after where you were currently synced, setting a new HD seed would mean that you don't detect that transaction. That would be bad.

With createwallet, the difference is that it's a completely new wallet with no history at all so it is safe to make a new wallet when you are behind.

DrahtBot · 2019-11-15T00:52:05Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

Use shared pointers only in validation interface #18354 (Use shared pointers only in validation interface by bvbfan)
wallet: always do avoid partial spends if fees are within a specified range #14582 (wallet: always do avoid partial spends if fees are within a specified range by kallewoof)

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.

ariard · 2019-11-15T02:26:57Z

So if there were a transaction that involved a key that hadn't been generated yet and was in a block after where you were currently synced

You're talking about a key derived from the old seed here ? I didn't dig in code but if I understand when we set a new seed we discard checking keys for old one right ? Node is getting out of IBD at tip N - DEFAULT_TIP_AGE(~144), which means you can set a new seed at N - 144, but you're not going to see a tx with an old key in block N - 100 for example, so is this mechanism reliable ?

achow101 · 2019-11-15T20:25:36Z

You're talking about a key derived from the old seed here ? I didn't dig in code but if I understand when we set a new seed we discard checking keys for old one right ?

Yes, I'm talking about keys derived from the old seed. Any key that has already been derived and added to the wallet (i.e. it was in the keypool or already marked as used) will still be checked for. Setting a new seed doesn't change that. Setting a new seed only makes it so that no new keys will be derived from the old seed.

Node is getting out of IBD at tip N - DEFAULT_TIP_AGE(~144), which means you can set a new seed at N - 144, but you're not going to see a tx with an old key in block N - 100 for example, so is this mechanism reliable ?

I think it's reasonably reliable as I think it is unlikely that someone used the entire keypool in 144 blocks.

jonatack

Concept ACK.

First review pass to get started before digging deeper into the approach. Built, ran tests and bitcoind.

Some implementation thoughts below; please ignore them until the PR has more Concept/Approach ACKs. Additional thoughts:

It might be clearer to merge the last two commits which appear to make overlapping changes
Consider if you can use const
I'd like to test this further with some embedded printing or logging
Does the current test coverage suffice for these changes?

jonatack · 2019-11-18T12:08:12Z

src/wallet/wallet.h

perhaps: "Whether or not the chain is in initial block download."

jonatack · 2019-11-18T12:23:07Z

src/wallet/wallet.h

Thanks for adding this documentation. Suggestion (if I understand correctly that othersaid means autrement dit, which translates to in other words):

- /* Flag to indicate chain is still in initial block download. Set it to - * false to let wallet generating new seed without being connected to a node. - * Othersaid, a wallet evaluates chain being out of initial block download - * until it's proven false. + /* Flag to indicate whether or not the chain is still in initial block + * download. Defaults to false to allow the wallet to generate new seeds + * without being connected to a node. In other words, the wallet assumes the + * chain is out of initial block download unless shown otherwise.

jonatack · 2019-11-18T12:36:35Z

src/wallet/rpcwallet.cpp

Perhaps make this change in the last commit where the other removals of isInitialBlockDownload take place?

IIUC, the commit is misnamed and could better be called something like "Remove Chain::isInitialBlockDownload`

jonatack · 2019-11-18T12:38:36Z

src/wallet/wallet.cpp

Perhaps make this change in the commit "Add m_is_ibd in CWallet` (or merge the two commits, IIUC)

re: #17484 (comment)

In commit "Remove Chain::isReadyToBroadcast method" (a5a674857d0ae6d535e4725337968492c821c635)

Perhaps make this change in the commit "Add m_is_ibd in CWallet` (or merge the two commits, IIUC)

Change does make sense (and is necessary) in this commit due to isReadyToBroadcast call a few lines up. But it would be good to add a comment here mentioning this is also needed to avoid spamming nodes with old transactions.

ariard · 2019-11-19T17:22:36Z

Thanks @achow101 for your insights, I'm going to hold-on this PR once the wallet init its state asynchronously like thanks to change here https://github.com/ariard/bitcoin/commits/2019-08-rescan-index-refactor so it would avoid behavior change on the point you raised.

Thanks for your review @jonatack, will fix comments once PR gets Concept ACK.

jnewbery · 2019-11-25T21:10:12Z

Concept ACK.

@achow101 to be clear, is the failure scenario as follows:

user has an HD wallet
user makes a backup from the wallet
user hands out more keys then they have in their keypool (currently 1000, previously 100) and receives payments to keys handed out after the initial keypool was drained
user restores original backup wallet on unsync'ed node, then calls sethdseed before the node has synced. The payments to the keys handed out after the initial keypool was drained are lost.

(1) must be an HD wallet because restoring a non-HD wallet backup that has drained its initial keypool keys always results in funds loss
(3) must drain the initial keypool because setting a new HD seed does not remove the initial keypool from the wallet. It simply marks them as not in the keypool

Not allowing sethdseed to be called before we're out of IBD isn't perfect protection against this. If many addresses (more than in the initial keypool) have been handed out, the user could still receive a payment to one of those addresses after they've changed their hdseed and lose the payment.

@ariard - is it possible to change this so m_is_ibd initializes to true and then call Chain::isInitialBlockDownload() once on wallet start-up?

achow101 · 2019-11-25T22:27:34Z

For some context about the IBD and sethdseed stuff: #12560 (comment)

is the failure scenario as follows:

Yes.

jnewbery · 2019-11-25T22:53:56Z

@achow101 Thanks for the cross-reference 🙏

can we either check that we're fully synced before allowing an HD master rotate or keep around old HD keys for key derivation? I'd prefer the second

Keeping the old key for key derivation definitely seems like the correct fix. How complex is that (and does descriptor wallets do that automatically?)

achow101 · 2019-11-25T23:35:42Z

Keeping the old key for key derivation definitely seems like the correct fix. How complex is that

I don't think it would be too complex. Although I think it would be a fairly large change. We would need to keep track of the old hd seeds and keep track of where we've derived up to for a particular seed. And a bunch of things that do things on the seed would need to be changed to do things on any seed. We would have to add several more records to the wallet.

Alternatively we could have sethdseed make a new LegacyScriptPubKeyMan with the seed, but that may also require a bit of re-architecting as the primary assumption about LegacyScriptPubKeyMans will change. But we would have to wait for the full ScriptPubKeyMan and CWallet separation before that can happen.

and does descriptor wallets do that automatically?

Not the current implementation. But it also would be pretty trivial to do it.

ariard · 2019-11-26T22:52:30Z

@jnewbery

is it possible to change this so m_is_ibd initializes to true and then call Chain::isInitialBlockDownload() once on wallet start-up?

Yes but that removes PR interest of drying up the Chain interface. I think best it to wait wallet registering its sync state with the Chain::interface (https://github.com/ariard/bitcoin/commits/2019-08-rescan-index-refactor) at start-up so after rescan done wallet knows it's out of IBD.

can we either check that we're fully synced before allowing an HD master rotate or keep around old HD keys for key derivation? I'd prefer the second

I think by robustness we should go for the second option. Reading https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Wallet-Class-Structure-Changes, best option is to have multiple Scri[tPubKeyMan or one with multiple hdChain ?

ryanofsky · 2019-12-02T23:06:50Z

Reading https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Wallet-Class-Structure-Changes, best option is to have multiple Scri[tPubKeyMan or one with multiple hdChain ?

I think you could have separate LegacyScriptPubKeyMan instances for each hdseed, but this would probably be more work than necessary, and not very related to what this PR is trying to do.

I think the minimal fix to prevent running out of keys for old hd seeds would be to do some extra checking and topping up inside LegacyScriptPubKeyMan::MarkUnusedAddresses. Add a check there to see if the affected key has metadata associated with an inactive hd seed. If it does and the key number in the metadata is close enough to the highest generated key on that hd path, generate more keys on the path and add them to the wallet. I don't think this would require very much code. To work efficiently the LegacyScriptPubKeyMan instance would need to have a new map from KeyOriginInfo hd path to the maximum generated key number on that path.

Actually, I think you would need to do what I just described even if you did want to instantiate different LegacyScriptPubKeyMan instances for each seed, because inactive hd seeds aren't going to have any pool data, so m_pool_key_to_index data will be empty and existing topup code won't work for them. So you'd need this new LegacyScriptPubKeyMan::MarkUnusedAddresses code anyway.

If it is desirable to have separate LegacyScriptPubKeyMan instances in the wallet, I think that might not be too hard to implement. The trickiest part would just be loading them. In walletdb.cpp as keys and scripts are loaded you would have to examine their metadata to figure out what seed they were associated with and load them into the appropriate keyman instance based on that seed.

achow101 · 2019-12-05T21:54:59Z

I've dealt with the sethdseed stuff in #17681

ariard · 2019-12-05T23:58:54Z

Whoa awesome, will review #17681 !

ryanofsky

Code review ACK a5a674857d0ae6d535e4725337968492c821c635 with caveat that I only did cursory check to verify that CMainSignals::UpdatedBlockTip fInitialDownload value is the same value given by ChainstateActive().IsInitialBlockDownload()

Would be great to see this PR and #17443 rebased and merged as followup to #16426!

ryanofsky · 2020-05-01T02:06:27Z

src/wallet/wallet.cpp

In commit "Add m_is_ibd in CWallet" (dca4246dac0f4fa6369a71ee787b2bf1f6415c6d)

Calling chain lock not unnecessary here (but harmless)

ryanofsky · 2020-05-01T02:27:32Z

src/interfaces/chain.h

In commit "Remove Chain::isReadyToBroadcast method" (a5a674857d0ae6d535e4725337968492c821c635)

Implementation and meaning of isReadyToBroadcast method is changing a lot, so I would rename it to something like "isInitializing" or "isLoading" and update the comment.

Also commit title could be changed to mention removing isInitialBlockDownload

ryanofsky · 2020-05-01T02:34:18Z

src/wallet/wallet.cpp

re: #17484 (comment)

In commit "Remove Chain::isReadyToBroadcast method" (a5a674857d0ae6d535e4725337968492c821c635)

Perhaps make this change in the commit "Add m_is_ibd in CWallet` (or merge the two commits, IIUC)

Change does make sense (and is necessary) in this commit due to isReadyToBroadcast call a few lines up. But it would be good to add a comment here mentioning this is also needed to avoid spamming nodes with old transactions.

ryanofsky · 2020-05-01T02:42:25Z

src/wallet/rpcwallet.cpp

In commit "Add m_is_ibd in CWallet" (dca4246dac0f4fa6369a71ee787b2bf1f6415c6d)

Is the commit message correct? It says there is a behavior change here, but there doesn't seem to be one if this is still raising an error.

ariard · 2020-05-07T09:29:24Z

Thanks @ryanofsky for call-up on this and #17443! I will rebase them tomorrow-ish but I think for this one it would be better for achow one to get first.

…keys from them as needed 1ed52fb Remove IBD check in sethdseed (Andrew Chow) b1810a1 Test that keys from inactive seeds are generated (Andrew Chow) c93082e Generate new keys for inactive seeds after marking used (Andrew Chow) 45f2f6a Determine inactive HD seeds from key metadata and track them in LegacyScriptPubKeyMan (Andrew Chow) b59b450 have GenerateNewKey and DeriveNewChildKey take a CHDChain as an argument (Andrew Chow) Pull request description: Largely implements the suggestion from #17484 (comment). After `sethdseed` is called, the CHDChain for the old seed is kept in the wallet. It is kept on the file as a new `inactivehdseed` record and in memory in a map `m_inactive_hd_seeds`. In `LegacyScriptPubKeyMan::MarkUnusedAddresses` we check each used key's metadata for whether it was derived from an inactive seed. If it is, we then check to see how many keys after that key were derived from the inactive seed. If that number does not match the keypool parameter, we derive more keys from the inactive seed until it does match. This way we won't miss transactions belonging to keys outside of the range of the keypool initially. The indexes and internal-ness of a key is gotten by checking it's key origin data. Because of this change, we no longer need to wait for IBD to finish before `sethdseed` can work so that check is also removed. A test case for this is added as well which fails on master. ACKs for top commit: ryanofsky: Code review ACK 1ed52fb. Changes since last review: various commit message, code comment, log message, error checking improvements, and fix for topping up inactive seeds if wallet isn't reloaded after calling sethdseed and test for this ariard: Code Review ACK 1ed52fb jonatack: ACK 1ed52fb thanks for addressing the previous review feedback; would be happy to see the new review questions answered and feedback addressed and re-ack. Tree-SHA512: e658ae0e1dab94be55d2b62cdda506c94815e73a6881533fd30d41cc77477f82fee2095144957a3a1df0c129e256bdd7b7abe3737d515f393610446cae4edf1c

…derive keys from them as needed 1ed52fb Remove IBD check in sethdseed (Andrew Chow) b1810a1 Test that keys from inactive seeds are generated (Andrew Chow) c93082e Generate new keys for inactive seeds after marking used (Andrew Chow) 45f2f6a Determine inactive HD seeds from key metadata and track them in LegacyScriptPubKeyMan (Andrew Chow) b59b450 have GenerateNewKey and DeriveNewChildKey take a CHDChain as an argument (Andrew Chow) Pull request description: Largely implements the suggestion from bitcoin#17484 (comment). After `sethdseed` is called, the CHDChain for the old seed is kept in the wallet. It is kept on the file as a new `inactivehdseed` record and in memory in a map `m_inactive_hd_seeds`. In `LegacyScriptPubKeyMan::MarkUnusedAddresses` we check each used key's metadata for whether it was derived from an inactive seed. If it is, we then check to see how many keys after that key were derived from the inactive seed. If that number does not match the keypool parameter, we derive more keys from the inactive seed until it does match. This way we won't miss transactions belonging to keys outside of the range of the keypool initially. The indexes and internal-ness of a key is gotten by checking it's key origin data. Because of this change, we no longer need to wait for IBD to finish before `sethdseed` can work so that check is also removed. A test case for this is added as well which fails on master. ACKs for top commit: ryanofsky: Code review ACK 1ed52fb. Changes since last review: various commit message, code comment, log message, error checking improvements, and fix for topping up inactive seeds if wallet isn't reloaded after calling sethdseed and test for this ariard: Code Review ACK 1ed52fb jonatack: ACK 1ed52fb thanks for addressing the previous review feedback; would be happy to see the new review questions answered and feedback addressed and re-ack. Tree-SHA512: e658ae0e1dab94be55d2b62cdda506c94815e73a6881533fd30d41cc77477f82fee2095144957a3a1df0c129e256bdd7b7abe3737d515f393610446cae4edf1c

fjahr · 2020-05-23T12:17:36Z

Concept ACK, will do full review when this is rebased.

…s_ibd This is only used in next commit.

ariard · 2020-06-04T00:52:43Z

Rebased the PR but this is not worthy to review now, after thinking a bit more I want to avoid behavior changes and for so, it would be better to get #15719 and its follow-up first to always ensure we are in-sync with node wrt to IBD after wallet startup.

This current PR may provoke issue if a privacy leak if user broadcast a transaction without being at tip.

Updated PR description in consequence.

ryanofsky

re: #17484 (comment)
Seems reasonable to delay this PR until wallet startup works better. In case it's not clear to others, the change in behavior here is that with this PR the wallet may now incorrectly assume the node is in IBD when it's loaded (before it gets an updatedBlockTip notification indicating otherwise). This can cause it to choose transaction lock times differently and be a privacy leak.

But code review ACK 20eb0da aside from this concern. Changes since last review: rebase, adding m_is_ibd comment, moving resend ibd check to earlier commit.

ryanofsky · 2020-06-12T02:52:58Z

src/wallet/wallet.cpp

    { // cs_wallet scope
        LOCK(cs_wallet);

+        if (m_is_ibd) return;


In commit "[wallet] Add m_is_ibd in CWallet" (797ec9d)

I do think it would be helpful to have comment here saying this is skipped during ibd to avoid spam.

Also the comment above for isReadyToBroadcast() is incorrect after commit "[interfaces] Remove Chain::isReadyToBroadcast" (20eb0da), and should be updated to not mention IBD since the IBD check isn't there anymore

DrahtBot · 2020-08-17T05:35:59Z

🐙 This pull request conflicts with the target branch and needs rebase.

_{Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".}

…ve keys from them as needed Summary: 1ed52fbb4d81f7b7634fd4fb6d1d00e1478129dc Remove IBD check in sethdseed (Andrew Chow) b1810a145a601a8064e4094350cfb6ddafbdb4d8 Test that keys from inactive seeds are generated (Andrew Chow) c93082ece40b1c72f05b3e2085c022c09eaa4d65 Generate new keys for inactive seeds after marking used (Andrew Chow) 45f2f6a0e8514a0438a87554400bf73cbb90707f Determine inactive HD seeds from key metadata and track them in LegacyScriptPubKeyMan (Andrew Chow) b59b4504abf96cec860badfed2ac793ae5d40ced have GenerateNewKey and DeriveNewChildKey take a CHDChain as an argument (Andrew Chow) Pull request description: Largely implements the suggestion from bitcoin/bitcoin#17484 (comment). After `sethdseed` is called, the CHDChain for the old seed is kept in the wallet. It is kept on the file as a new `inactivehdseed` record and in memory in a map `m_inactive_hd_seeds`. In `LegacyScriptPubKeyMan::MarkUnusedAddresses` we check each used key's metadata for whether it was derived from an inactive seed. If it is, we then check to see how many keys after that key were derived from the inactive seed. If that number does not match the keypool parameter, we derive more keys from the inactive seed until it does match. This way we won't miss transactions belonging to keys outside of the range of the keypool initially. The indexes and internal-ness of a key is gotten by checking it's key origin data. Because of this change, we no longer need to wait for IBD to finish before `sethdseed` can work so that check is also removed. A test case for this is added as well which fails on master. --- Backport of Core [[bitcoin/bitcoin#17681 | PR17681]] Test Plan: CC=clang CXX=clang++ cmake .. -GNinja -DENABLE_SANITIZERS=thread ninja all check check-functional cmake .. -GNinja -DCMAKE_BUILD_TYPE=Debug ninja all check check-functional Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Subscribers: PiRK Differential Revision: https://reviews.bitcoinabc.org/D9196

DrahtBot · 2021-12-15T11:22:20Z

There hasn't been much activity lately and the patch still needs rebase. What is the status here?

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

maflcko · 2021-12-15T11:31:51Z

Looks like this depends on #15719

DrahtBot · 2022-03-21T13:07:44Z

There hasn't been much activity lately and the patch still needs rebase. What is the status here?

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

ariard changed the title ~~CWallet: add cached m_is_ibd to remove isInitialBlockDownload~~ CWallet: add cached m_is_ibd to remove Chain::isInitialBlockDownload Nov 14, 2019

fanquake added the Wallet label Nov 14, 2019

maflcko changed the title ~~CWallet: add cached m_is_ibd to remove Chain::isInitialBlockDownload~~ wallet: add cached m_is_ibd to remove Chain::isInitialBlockDownload Nov 15, 2019

jonatack reviewed Nov 18, 2019

View reviewed changes

achow101 mentioned this pull request Dec 5, 2019

wallet: Keep inactive seeds after sethdseed and derive keys from them as needed #17681

Merged

DrahtBot added the Needs rebase label Jan 30, 2020

ariard mentioned this pull request Apr 23, 2020

Reverse cs_main, cs_wallet lock order and reduce cs_main locking #16426

Merged

ryanofsky approved these changes May 1, 2020

View reviewed changes

ryanofsky reviewed May 1, 2020

View reviewed changes

[interfaces] Extend Chain::Notifications::updatedBlockTip with bool i…

693bbc6

…s_ibd This is only used in next commit.

ariard mentioned this pull request Jun 4, 2020

Wallet passive startup #15719

Closed

Antoine Riard added 2 commits June 3, 2020 20:47

[wallet] Add m_is_ibd in CWallet

797ec9d

[interfaces] Remove Chain::isReadyToBroadcast

20eb0da

ariard force-pushed the 2019-11-wallet-remove-isibd branch from a5a6748 to 20eb0da Compare June 4, 2020 00:50

DrahtBot removed the Needs rebase label Jun 4, 2020

This was referenced Jun 4, 2020

Drop checkFinalTx and use Median Time Past to check finality of wallet transactions #17443

Closed

wallet: always do avoid partial spends if fees are within a specified range #14582

Merged

Use shared pointers only in validation interface #18354

Closed

ryanofsky approved these changes Jun 12, 2020

View reviewed changes

DrahtBot added the Needs rebase label Aug 17, 2020

achow101 closed this Oct 12, 2022

bitcoin locked and limited conversation to collaborators Oct 12, 2023

wallet: add cached m_is_ibd to remove Chain::isInitialBlockDownload #17484

wallet: add cached m_is_ibd to remove Chain::isInitialBlockDownload #17484

Uh oh!

Conversation

ariard commented Nov 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

achow101 commented Nov 15, 2019

Uh oh!

DrahtBot commented Nov 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

ariard commented Nov 15, 2019

Uh oh!

achow101 commented Nov 15, 2019

Uh oh!

jonatack left a comment • 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!

jonatack Nov 18, 2019 • 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!

ariard commented Nov 19, 2019

Uh oh!

jnewbery commented Nov 25, 2019

Uh oh!

achow101 commented Nov 25, 2019

Uh oh!

jnewbery commented Nov 25, 2019

Uh oh!

achow101 commented Nov 25, 2019

Uh oh!

ariard commented Nov 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky commented Dec 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

achow101 commented Dec 5, 2019

Uh oh!

ariard commented Dec 5, 2019

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ariard commented May 7, 2020

Uh oh!

fjahr commented May 23, 2020

Uh oh!

ariard commented Jun 4, 2020

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Aug 17, 2020

Uh oh!

DrahtBot commented Dec 15, 2021

Uh oh!

maflcko commented Dec 15, 2021

ariard commented Nov 14, 2019 •

edited

Loading

DrahtBot commented Nov 15, 2019 •

edited

Loading

jonatack left a comment •

edited

Loading

jonatack Nov 18, 2019 •

edited

Loading

ariard commented Nov 26, 2019 •

edited

Loading

ryanofsky commented Dec 2, 2019 •

edited

Loading