wallet: Remove IsMine from migration code #30328

achow101 · 2024-06-24T16:40:47Z

The legacy wallet IsMine code will be removed with the legacy wallet, but is still partially needed for migration. Instead of using IsMine directly in migration, equivalent checks are performed by migration.

Builds on #26596

DrahtBot · 2024-06-24T16:40:50Z

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/30328.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	theStack

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#31423 (wallet: migration, don't create spendable wallet from a watch-only legacy wallet by furszy)
#28710 (Remove the legacy wallet and BDB dependency by achow101)

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.

achow101 · 2024-07-11T19:30:23Z

Ready for review now that #26596 has been merged.

src/wallet/scriptpubkeyman.cpp

brunoerg · 2024-07-18T20:41:25Z

src/wallet/scriptpubkeyman.cpp

+        case TxoutType::MULTISIG:
+        {
+            if (ctx == ScriptContext::P2WSH) {
+                std::vector<valtype> keys(sols.begin() + 1, sols.begin() + sols.size() - 1);


In 905e22b "wallet: Remove IsMine from migration": Just a question, but could it be a vector of CPubKey then use IsCompressed?

HaveKeys uses std::vector<valtype>

src/wallet/scriptpubkeyman.cpp

DrahtBot · 2024-07-22T11:32:50Z

🚧 At least one of the CI tasks failed.
_{Debug: https://github.com/bitcoin/bitcoin/runs/27341718423}

Hints

Make sure to run all tests locally, according to the documentation.

The failure may happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the
affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

theStack

Concept ACK, left two code-deduplication nits below

src/wallet/scriptpubkeyman.cpp

hodlinator

Commit message in d5d994c incorrectly states:
"This reverts commit bbb1d51."

I believe that commit ended up being merged as b231f4d.

DrahtBot · 2024-08-03T15:00:08Z

🚧 At least one of the CI tasks failed.
_{Debug: https://github.com/bitcoin/bitcoin/runs/27341718760}

Hints

Make sure to run all tests locally, according to the documentation.

The failure may happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the
affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

hodlinator · 2024-09-12T09:22:37Z

#30328 (review) still applies to 705d9eb instead of d5d994c.

(Clarified linked comment a bit).

laanwj

Must admit that i'm not versed enough in the subtle specifics with regard to validity and spendability of scripts, to be sure all of this is correct. But where i've checked, the behavior matches the IsMine-based implementation. Overall changes LGTM.

src/wallet/scriptpubkeyman.cpp

laanwj · 2024-11-22T10:58:24Z

src/wallet/scriptpubkeyman.cpp

+        // to "spks", we can simply check whether this P2PK or P2PKH script is in "spk" to determine spendability.
+        case TxoutType::PUBKEY:
+        case TxoutType::PUBKEYHASH:
+            if (spks.count(script) > 0) {


Doing both inserting into and reading from spks here makes it somewhat harder to be confident that the behavior is independent from the specific order of processing mapScripts, but the comment above helps.

src/wallet/scriptpubkeyman.cpp

furszy · 2024-11-27T18:08:07Z

Found a few ways to crash migration while reviewing these PR that also occur in master: #31374, #31378. I think we should focus on those before moving forward with this final IsMine killing refactoring. They also include further test coverage that would be nice to have here.

test/functional/wallet_migration.py

murchandamus · 2024-12-06T20:34:29Z

test/functional/wallet_migration.py

+        wallet.unloadwallet()
+
+    def test_disallowed_p2wsh(self):
+        self.log.info("Test that P2WSH output scripts with invalid witnessScripts are not migrated")


Why is it important to test that an invalid script that we can import for historical reasons is not migrated? Are we testing that these artifacts do not cause the migration to fail rather than them not being migrated?

Because invalid scripts can exist in the legacy wallet, but can cause issues (crashes, loading failure, etc.) in both migration and descriptor wallets. So we need to test that migration doesn't fail, and that those scripts do not end up in the descriptor wallet.

src/wallet/scriptpubkeyman.cpp

murchandamus · 2024-12-06T22:21:11Z

src/wallet/scriptpubkeyman.cpp

+        // Bare multisigs are never considered spendable
+        case TxoutType::MULTISIG:
+        {
+            Assert(solutions.size() >= 2);


Errr, can a multisig be 0 of 0? If so, wouldn’t that only have a solution size of 1?

The Solver returns TxoutType::NONSTANDARD for 0-of-n, so this code cannot be reached in that case.

murchandamus · 2024-12-06T22:27:39Z

src/wallet/scriptpubkeyman.cpp

+            if (!HaveKeys(keys, *this)) {
+                break;
            }


At this point, the witness program, witness script, or redeemscript is in our mapScript, and we might have enough keys for the quorum but just not all of the keys.

Why does this require all the keys to be present in the wallet rather than just the threshold?

To maintain strict compatibility with LegacyScriptPubKeyMan::IsMine(). I don't think we should change this behavior in this PR.

src/wallet/scriptpubkeyman.cpp

theStack

LGTM, the PR is in a much clearer state now with all the newly introduced comments and the P2WSH scripts loop removed.

theStack · 2024-12-10T15:13:37Z

test/functional/wallet_migration.py

+        comp_eckey = ECKey()
+        comp_eckey.generate(compressed=True)
+        comp_pubkey = comp_eckey.get_pubkey().get_bytes().hex()


in commit 883030c: could use the generate_keypair helper both here and for the uncompressed variant below

Suggested change

comp_eckey = ECKey()

comp_eckey.generate(compressed=True)

comp_pubkey = comp_eckey.get_pubkey().get_bytes().hex()

comp_eckey_wif, comp_pubkey = generate_keypair(compressed=True, wif=True)

then the ECKey and byte_to_base58 imports are not needed anymore

theStack · 2024-12-10T16:21:16Z

src/wallet/scriptpubkeyman.cpp

+        case TxoutType::MULTISIG:
+        {
+            if (ctx == ScriptContext::P2WSH) {
+                std::vector<std::vector<unsigned char>> keys(solutions.begin() + 1, solutions.begin() + solutions.size() - 1);


in commit 19dc8a3: same as in the MULTISIG Solver case below (in the mapScripts loop), could also add an assertion about the solutions size here (or maybe even introduce some get_multisig_keys_from_solutions lambda to further deduplicate)

Suggested change

std::vector<std::vector<unsigned char>> keys(solutions.begin() + 1, solutions.begin() + solutions.size() - 1);

Assert(solutions.size() >= 2);

std::vector<std::vector<unsigned char>> keys(solutions.begin() + 1, solutions.begin() + solutions.size() - 1);

theStack · 2024-12-10T16:24:50Z

src/wallet/scriptpubkeyman.cpp

+            // P2WSH-Multisig output scripts are spendable if the P2WSH output script is also in mapScripts,
+            // and all keys are compressed
+            CScript ms_wsh = GetScriptForDestination(WitnessV0ScriptHash(script));
+            if (HaveCScript(CScriptID(ms_wsh)) && all_keys_compressed(keys)) {


nit: could use is_valid_script here to not repeat the logic (as also done in the PUBKEY/PUBKEYHASH cases above):

Suggested change

if (HaveCScript(CScriptID(ms_wsh)) && all_keys_compressed(keys)) {

if (HaveCScript(CScriptID(ms_wsh)) && is_valid_script(script, ScriptContext::P2WSH)) {

As IsMine will be removed, the relevant components of IsMine are inlined into the migration functions.

This reverts commit b231f4d.

If the `releases` directory exists, but still only a subset of the necessary previous release binaries are available, the test fails by throwing an exception (sometimes leading to follow-up exceptions like "AssertionError: [node 0] Error: no RPC connection") and printing out a stack trace, which can be confusing and at a first glance suggests that the node crashed or some alike. Improve this by checking and printing out *all* of the missing release binaries and skip the test in this case. Noticed while testing bitcoin#30328. Can be tested by e.g. $ ./test/get_previous_releases -b $ rm -rf ./releases/v28.0/ $ ./build/test/functional/wallet_migration.py

sipa · 2024-12-12T18:59:21Z

Very concepty comment (after IRL discussion with @achow101 and @furszy)

Since this code is effectively introducing an analogue of the existing IsMine() logic, it would be useful to add tests that can help convince reviewers of equivalence between the two (especially since the existing IsMine semDamn miners digging into the floor above us!antics are notoriously unintuitive (sorry)).

I would suggest a fuzz test that constructs a legacy SPKM (populated with some keys, redeemscripts, watchscripts), then generates the set of scriptPubKeys to watch using the migration code, checks that all the scripts in that set are IsMine() according to the old code, and then also generate a number of other scriptPubKeys, and test that IsMine() is true for them if and only if they're in the set. A complication is that IsMine may return true for things we cannot actually migrate, but I believe that's fixable by in the test ignoring IsMine() output for anything that cannot be InferDescriptor'd to a valid/roundtrippable descriptor.

I'm happy to help brainstorming how such a fuzz could work.

achow101 · 2024-12-13T22:08:26Z

Following our discussion yesterday on additional edge cases, I have concluded that this approach is no longer viable as it has a high likelihood of missing further edge cases. I have opened #31495 which uses the suggested alternative approach of filtering the upper bound of scripts with IsMine().

…issing If the `releases` directory exists, but still only a subset of the necessary previous release binaries are available, the test fails by throwing an exception (sometimes leading to follow-up exceptions like "AssertionError: [node 0] Error: no RPC connection") and printing out a stack trace, which can be confusing and at a first glance suggests that the node crashed or some alike. Improve this by checking and printing out *all* of the missing release binaries and failing with an explicit error in this case. Also add an info on how to download previous releases binaries. Noticed while testing bitcoin#30328. Can be tested by e.g. $ ./test/get_previous_releases.py -b $ rm -rf ./releases/v28.0/ $ ./build/test/functional/wallet_migration.py

… binaries is missing 1ea7e45 test: raise explicit error if any of the needed release binaries is missing (Sebastian Falbesoner) Pull request description: If the `releases` directory exists, but still only a subset of the necessary previous release binaries are available, the test fails by throwing an exception (sometimes leading to follow-up exceptions like `AssertionError: [node 0] Error: no RPC connection`) and printing out a stack trace, which can be confusing and at a first glance suggests that the node crashed or some alike. Improve this by checking and printing out *all* of the missing release binaries and failing with an explicit error in this case. Also add an info on how to download previous releases binaries. Noticed while testing #30328. Can be tested by e.g. ``` $ rm -rf ./releases $ ./test/get_previous_releases.py -b $ rm -rf ./releases/v28.0/ $ ./build/test/functional/wallet_migration.py ``` master: <details> <summary>Long test fail output</summary> ``` ... 2024-12-10T18:48:54.067000Z TestFramework (ERROR): Assertion failed Traceback (most recent call last): File "/home/thestack/bitcoin/test/functional/test_framework/test_framework.py", line 590, in start_nodes node.start(extra_args[i], *args, **kwargs) File "/home/thestack/bitcoin/test/functional/test_framework/test_node.py", line 257, in start self.process = subprocess.Popen(self.args + extra_args, env=subp_env, stdout=stdout, stderr=stderr, cwd=cwd, **kwargs) File "/usr/lib/python3.10/subprocess.py", line 971, in __init__ self._execute_child(args, executable, preexec_fn, close_fds, File "/usr/lib/python3.10/subprocess.py", line 1863, in _execute_child raise child_exception_type(errno_num, err_msg, err_filename) FileNotFoundError: [Errno 2] No such file or directory: '/home/thestack/bitcoin/releases/v28.0/bin/bitcoind' During handling of the above exception, another exception occurred: Traceback (most recent call last): File "/home/thestack/bitcoin/test/functional/test_framework/test_framework.py", line 131, in main self.setup() File "/home/thestack/bitcoin/test/functional/test_framework/test_framework.py", line 315, in setup self.setup_network() File "/home/thestack/bitcoin/test/functional/test_framework/test_framework.py", line 409, in setup_network self.setup_nodes() File "/home/thestack/bitcoin/./build/test/functional/wallet_migration.py", line 54, in setup_nodes self.start_nodes() File "/home/thestack/bitcoin/test/functional/test_framework/test_framework.py", line 595, in start_nodes self.stop_nodes() File "/home/thestack/bitcoin/test/functional/test_framework/test_framework.py", line 610, in stop_nodes node.stop_node(wait=wait, wait_until_stopped=False) File "/home/thestack/bitcoin/test/functional/test_framework/test_node.py", line 396, in stop_node self.stop(wait=wait) File "/home/thestack/bitcoin/test/functional/test_framework/test_node.py", line 215, in __getattr__ assert self.rpc_connected and self.rpc is not None, self._node_msg("Error: no RPC connection") AssertionError: [node 0] Error: no RPC connection 2024-12-10T18:48:54.118000Z TestFramework (INFO): Stopping nodes Traceback (most recent call last): File "/home/thestack/bitcoin/./build/test/functional/wallet_migration.py", line 1097, in <module> WalletMigrationTest(__file__).main() File "/home/thestack/bitcoin/test/functional/test_framework/test_framework.py", line 159, in main exit_code = self.shutdown() File "/home/thestack/bitcoin/test/functional/test_framework/test_framework.py", line 331, in shutdown self.stop_nodes() File "/home/thestack/bitcoin/test/functional/test_framework/test_framework.py", line 610, in stop_nodes node.stop_node(wait=wait, wait_until_stopped=False) File "/home/thestack/bitcoin/test/functional/test_framework/test_node.py", line 396, in stop_node self.stop(wait=wait) File "/home/thestack/bitcoin/test/functional/test_framework/test_node.py", line 215, in __getattr__ assert self.rpc_connected and self.rpc is not None, self._node_msg("Error: no RPC connection") AssertionError: [node 0] Error: no RPC connection [node 0] Cleaning up leftover process ... ``` </details> PR: ``` ... 2025-01-01T20:26:27.999000Z TestFramework (INFO): PRNG seed is: 4570383538468804512 2025-01-01T20:26:28.000000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_lz66_zcu 2025-01-01T20:26:28.003000Z TestFramework (ERROR): Binary not found: /home/thestack/bitcoin/releases/v28.0/bin/bitcoind 2025-01-01T20:26:28.003000Z TestFramework (ERROR): Binary not found: /home/thestack/bitcoin/releases/v28.0/bin/bitcoin-cli 2025-01-01T20:26:28.003000Z TestFramework (INFO): Previous releases binaries can be downloaded via `test/get_previous_releases.py -b`. 2025-01-01T20:26:28.003000Z TestFramework (ERROR): Assertion failed Traceback (most recent call last): File "/home/thestack/bitcoin/test/functional/test_framework/test_framework.py", line 131, in main self.setup() File "/home/thestack/bitcoin/test/functional/test_framework/test_framework.py", line 315, in setup self.setup_network() File "/home/thestack/bitcoin/test/functional/test_framework/test_framework.py", line 409, in setup_network self.setup_nodes() File "/home/thestack/bitcoin/./build/test/functional/wallet_migration.py", line 50, in setup_nodes self.add_nodes(self.num_nodes, versions=[ File "/home/thestack/bitcoin/test/functional/test_framework/test_framework.py", line 537, in add_nodes raise AssertionError("At least one release binary is missing") AssertionError: At least one release binary is missing 2025-01-01T20:26:28.061000Z TestFramework (INFO): Stopping nodes ... ``` ACKs for top commit: fjahr: re-ACK 1ea7e45 kevkevinpal: ACK [1ea7e45](1ea7e45) maflcko: lgtm ACK 1ea7e45 achow101: ACK 1ea7e45 pablomartin4btc: tACK 1ea7e45 Tree-SHA512: b621c3ce044ca8fc8715a4f4b1f96a8592a470c319a64444cced9ba692d315cfd4885a066679bf377b19136fa3530d9cff6f18894a45aa9c716d39b12719baa0

…to cover edge cases af76664 test: Test migration of a solvable script with no privkeys (Ava Chow) 17f01b0 test: Test migration of taproot output scripts (Ava Chow) 1eb9a2a test: Test migration of miniscript in legacy wallets (Ava Chow) e8c3efc wallet migration: Determine Solvables with CanProvide (Ava Chow) fa1b7cd migration: Skip descriptors which do not parse (Ava Chow) 440ea1a legacy spkm: use IsMine() to extract watched output scripts (Ava Chow) b777e84 legacy spkm: Move CanProvide to LegacyDataSPKM (Ava Chow) b1ab927 tests: Test migration of additional P2WSH scripts (Ava Chow) c39b3cf test: Extra verification that migratewallet migrates (Ava Chow) Pull request description: The legacy wallet `IsMine()` is essentially a black box that would tell us whether the wallet is watching an output script. In order to migrate legacy wallets to descriptor wallets, we need to be able to compute all of the output scripts that a legacy wallet would watch. The original approach for this was to understand `IsMine()` and write a function which would be its inverse. This was partially done in the original migration code, and attempted to be completed in #30328. However, further analysis of `IsMine()` has continued to reveal additional edge cases which make writing an inverse function increasingly difficult to verify correctness. This PR instead changes migration to utilize `IsMine()` to produce the output scripts by first computing a superset of all of the output scripts that `IsMine()` would watch and testing each script against `IsMine()` to filter for the ones that actually are watched. The superset is constructed by computing all possible output scripts for the keys and scripts in the wallet - for keys, every key could be a P2PK, P2PKH, P2WPKH, and P2SH-P2WPKH; for scripts, every script could be an output script, the redeemScript of a P2SH, the witnessScript of a P2WSH, and the witnessScript of a P2SH-P2WSH. Additionally, the legacy wallet can contain scripts that are redeemScripts and witnessScripts, while not watching for any output script utilizing that script. These are known as solvable scripts and are migrated to a separate "solvables" wallet. The previous approach to identifying these solvables was similar to identifying output scripts - finding known solvable conditions and computing the scripts. However, this also can miss scripts, so the solvables are now identified in a manner similar to the output scripts but using the function `CanProvide()`. Using the same superset as before, all output scripts which are `ISMINE_NO` are put through `CanProvide()` which will perform a dummy signing and then a key lookup to determine whether the legacy wallet could provide any solving data for the output script. The scripts that pass will have their descriptors inferred and the script included in the solvables wallet. The main downside of this approach is that `IsMine()` and `CanProvide()` can no longer be deleted. They will need to be refactored to be migration only code instead in #28710. Lastly, I've added 2 test cases for the edge cases that prompted this change of approach. In particular, miniscript witnessScripts and `rawtr()` output scripts are solvable and signable in a legacy wallet, although never `ISMINE_SPENDABLE`. ACKs for top commit: sipa: Code review ACK af76664; I did not review the tests in detail. brunoerg: code review ACK af76664 rkrux: ACK af76664 Tree-SHA512: 7f58a90de6f38fe9801fb6c2a520627072c8d66358652ad0872ff59deb678a82664b99babcfd874288bebcb1487d099a77821f03ae063c2b4cbf2d316e77d141

DrahtBot added the Wallet label Jun 24, 2024

achow101 mentioned this pull request Jun 24, 2024

wallet: Migrate legacy wallets to descriptor wallets without requiring BDB #26596

Merged

achow101 force-pushed the migrate-inline-ismine branch from 744613e to 574b840 Compare July 1, 2024 18:48

achow101 marked this pull request as draft July 8, 2024 17:29

achow101 mentioned this pull request Jul 11, 2024

Proposed Timeline for Legacy Wallet and BDB removal #20160

Closed

48 tasks

achow101 force-pushed the migrate-inline-ismine branch from 574b840 to d5d994c Compare July 11, 2024 19:29

achow101 marked this pull request as ready for review July 11, 2024 19:29

brunoerg reviewed Jul 18, 2024

View reviewed changes

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

brunoerg reviewed Jul 18, 2024

View reviewed changes

src/wallet/scriptpubkeyman.cpp Show resolved Hide resolved

DrahtBot added the CI failed label Jul 22, 2024

DrahtBot removed the CI failed label Jul 22, 2024

theStack reviewed Jul 24, 2024

View reviewed changes

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

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

hodlinator reviewed Jul 26, 2024

View reviewed changes

DrahtBot added the CI failed label Aug 3, 2024

achow101 force-pushed the migrate-inline-ismine branch from d5d994c to b6c2aad Compare September 3, 2024 16:30

achow101 force-pushed the migrate-inline-ismine branch from b6c2aad to 705d9eb Compare September 10, 2024 20:09

DrahtBot removed the CI failed label Sep 11, 2024

achow101 force-pushed the migrate-inline-ismine branch 2 times, most recently from 34751ae to ed5608a Compare October 16, 2024 11:33

laanwj approved these changes Nov 22, 2024

View reviewed changes

theStack reviewed Nov 25, 2024

View reviewed changes

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

DrahtBot mentioned this pull request Nov 27, 2024

wallet: fix crash during watch-only wallet migration #31374

Merged

furszy mentioned this pull request Nov 27, 2024

wallet: fix crash during migration due to invalid multisig descriptors #31378

Draft

DrahtBot mentioned this pull request Dec 5, 2024

wallet: migration, avoid creating spendable wallet from a watch-only legacy wallet #31423

Merged

achow101 force-pushed the migrate-inline-ismine branch 2 times, most recently from fea8925 to 348e5aa Compare December 5, 2024 19:13

murchandamus reviewed Dec 6, 2024

View reviewed changes

achow101 force-pushed the migrate-inline-ismine branch from 348e5aa to e09407c Compare December 7, 2024 17:39

DrahtBot added the Needs rebase label Dec 9, 2024

test: Extra verification that migratewallet migrates

1eb23bc

achow101 force-pushed the migrate-inline-ismine branch from e09407c to 76f1d5c Compare December 9, 2024 21:31

DrahtBot removed the Needs rebase label Dec 9, 2024

theStack reviewed Dec 10, 2024

View reviewed changes

achow101 added 3 commits December 10, 2024 13:33

tests: Test migration of additional P2WSH scripts

dedb15c

wallet: Remove IsMine from migration

afb4514

As IsMine will be removed, the relevant components of IsMine are inlined into the migration functions.

Revert "wallet: Move LegacyScriptPubKeyMan::IsMine to LegacyDataSPKM"

d25c0fb

This reverts commit b231f4d.

achow101 force-pushed the migrate-inline-ismine branch from 76f1d5c to d25c0fb Compare December 10, 2024 18:36

theStack mentioned this pull request Dec 10, 2024

test: raise explicit error if any of the needed release binaries is missing #31462

Merged

achow101 mentioned this pull request Dec 13, 2024

wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases #31495

Merged

achow101 closed this Dec 13, 2024

	std::vector<std::vector<unsigned char>> keys(solutions.begin() + 1, solutions.begin() + solutions.size() - 1);
	Assert(solutions.size() >= 2);
	std::vector<std::vector<unsigned char>> keys(solutions.begin() + 1, solutions.begin() + solutions.size() - 1);

	if (HaveCScript(CScriptID(ms_wsh)) && all_keys_compressed(keys)) {
	if (HaveCScript(CScriptID(ms_wsh)) && is_valid_script(script, ScriptContext::P2WSH)) {

wallet: Remove IsMine from migration code #30328

wallet: Remove IsMine from migration code #30328

Uh oh!

Conversation

achow101 commented Jun 24, 2024

Uh oh!

DrahtBot commented Jun 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

achow101 commented Jul 11, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DrahtBot commented Jul 22, 2024

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

hodlinator 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!

DrahtBot commented Aug 3, 2024

Uh oh!

hodlinator commented Sep 12, 2024

Uh oh!

laanwj left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

furszy commented Nov 27, 2024

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

achow101 Dec 7, 2024 • 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!

Uh oh!

theStack 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!

sipa commented Dec 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

achow101 commented Dec 13, 2024

DrahtBot commented Jun 24, 2024 •

edited

Loading

hodlinator left a comment •

edited

Loading

achow101 Dec 7, 2024 •

edited

Loading

sipa commented Dec 12, 2024 •

edited

Loading