Only subscribe to inner wallet puzzle hashes

[Quexington]

(recreation of #14345 for simplicity in fixing the immediate issue)

Dependent on:

• Subscribe to the change children of CATs #15176
• Hint to CAT change #15179

Purpose:

Currently, we will subscribe to full puzzle hashes for each type of asset in the wallet. I believe this logic is leftover from when we did not have hints. With hints, every coin will hint to a standard XCH address and will be picked up that way. Once it's been picked up, it gets sorted to the appropriate wallet. This means we only need to subscribe to puzzle hashes for innermost puzzles of which we only have one type right now: p2_delegated_or_hidden_puzzle.

Testing Notes:

Make sure you still receive and sync all CATs, DIDs, DL singletons etc. This was made in response to an error subscribing to too many puzzle hashes so maybe make a bunch of standard puzzle hashes and then add a bunch of wallets and see if you get that error again.

[xdustinface]

This currently only does what you want to during puzzle hash creation i think? We still subscribe to all puzzle hashes stored in the puzzle store (which include all the ones you want to exclude with this PR) during normal sync operations:

chia-blockchain/chia/wallet/wallet_node.py

Lines 749 to 759 in 4e34ff7

	all_puzzle_hashes = await self.get_puzzle_hashes_to_subscribe()
	not_checked_puzzle_hashes = set(all_puzzle_hashes) - already_checked_ph
	if not_checked_puzzle_hashes == set():
	break
	for chunk in chunks(list(not_checked_puzzle_hashes), 1000):
	ph_update_res: List[CoinState] = await subscribe_to_phs(chunk, full_node, 0)
	ph_update_res = list(filter(is_new_state_update, ph_update_res))
	if not await self.receive_state_from_peer(ph_update_res, full_node):
	# If something goes wrong, abort sync
	return
	already_checked_ph.update(not_checked_puzzle_hashes)

Also left some more comments.

[xdustinface]

So this PR seems to try to solve four independent things from my understanding, right? Like:

1. Only subscribe to inner wallet puzzle hashes which is what the PR should be about only from the title.
2. Add some new table to make adding unacknowledged CATs more reliable.
3. Fix some place where we still forgot to hint some CAT output when a new CAT gets minted. Which i btw, also figured to be an issue over in wallet: Fix and improve hint parsing #15274.
4. In offer code you added some extra coin_id subscriptions for non offer related coins and you added additions/removals to some TransactionRecord.

I think it would be easier to understand and discuss whats happening and what belongs to what if you would just create separate PRs for 2-4. Still, some thoughts about them for now:

2. Wouldn't it be safer to just subscribe to the puzzle hashes again when adding a unacknowledged CAT since with this table its possible to attack (fill up the disk) for free (same goes for the retry store probably), i think? While its possible to dust a wallet too we at least have a dust filter now.. Maybe its not so critical, didn't do any math or think a lot about it yet but still i want to raise the concern :)
3. Seems reasonable but for me it feels like this is so far still a blocker for this PR and we need to handle this backwards compatible somehow like the to offer a way to retrospectively hint unhinted coins because now, while it's fixed by the changes in here for newly minted CATs there are still unhinted ones out in the wild which can not be synced/processed during initial sync because determine_coin_type will fail?
4. Im not so sure whats going on here and why it is required here or in general.

[Quexington]

So this PR seems to try to solve four independent things from my understanding, right?

Not really, we're changing one thing and then fixing all of the pieces of code that were relying on the old way of doing things. This PR makes sense as a single PR because it's "one core change, and here's everything it affects".

Wouldn't it be safer to just subscribe to the puzzle hashes again when adding a unacknowledged CAT since with this table its possible to attack (fill up the disk) for free (same goes for the retry store probably), i think? While its possible to dust a wallet too we at least have a dust filter now.. Maybe its not so critical, didn't do any math or think a lot about it yet but still i want to raise the concern :)

Perhaps, but it's a bit less of an issue than dust used to be. We can't subscribe because we'll just end up with the same problem this PR is attempting to fix. The dust filter doesn't stop us from syncing anything, it just stops us from putting info in the DB about it and leaving it available for coin selection and display to the user. Obviously there's no coin selection or user display for these unacknowledged CATs so it shouldn't affect normal wallet operations, just a little bit of DB bloat. The "attack" isn't really free either with blockchain fees taken into account. The retry store is designed to only hold flaky states so it's not susceptible to this.

Seems reasonable but for me it feels like this is so far still a blocker for this PR and we need to handle this backwards compatible somehow like the to offer a way to retrospectively hint unhinted coins because now, while it's fixed by the changes in here for newly minted CATs there are still unhinted ones out in the wild which can not be synced/processed during initial sync because determine_coin_type will fail

The function that's fixed here is a test function and is not a supported use case in the wild. Unhinted CATs that are minted would already have failed on resync without the changes in this PR, the reason our tests weren't susceptible to that is that we don't ever resync in the tests.

[xdustinface]

Not really, we're changing one thing and then fixing all of the pieces of code that were relying on the old way of doing things. This PR makes sense as a single PR because it's "one core change, and here's everything it affects".

Well thats the whole point of splitting up changes. To merge "everything it affects" independently because its not only an issue within this PR but also on current main even though this PR kind of uncovered some of the issues. Which then makes finding issues in the commit history easier if not a bunch of changes are squashed into one thing, gets certain stuff merged faster than others which might unblock other PRs, avoids discussions about different topics in PRs and what else i don't recall right now. Its always "one thing" you want to implement but still there are always other things which are affected and its good practice (not only in my mind) to get this other affected or refactored things merged in independently and upfront to avoid what we have here now.. mixed changes and discussions about other things in a PR which is supposed to be about "inner puzzle hash subscriptions". I tried many times already to communicate this to you and while this is not a big PR and its not super important its also not a big deal to just do it and start to follow good practices to get used to it. But i will resign here for now..

We can't subscribe because we'll just end up with the same problem this PR is attempting to fix.

Why? We anyway need to subscribe, even after this PR or does this PR magically let us keep our wallet up to date without any subscription? In fact we anyway currently downloading all coin states from height 0 and drop all below our synced height every time we go into a long sync which basically happens for each new untrusted connection and also for each trusted connection so i think we could do better than just adding yet another table which doesn't even take reorgs into account and might provide us reorged coins later which then at the end get pushed into another table from where it gets "retried" and after it gets pushed into the final table if we succeed to process the coins after all, if its still valid.

The dust filter doesn't stop us from syncing anything, it just stops us from putting info in the DB

Yes but the unacknowledged CATs will still be put into the DB then with this change no matter what.

The function that's fixed here is a test function and is not a supported use case in the wild.

Isn't it used inside the create_new_wallet RPC command and with that exposed to whoever wants to use it?

[Quexington]

Why? We anyway need to subscribe, even after this PR or does this PR magically let us keep our wallet up to date without any subscription?

We're subscribing only to standard puzzle hashes, if we subscribe to all the cat puzzle hashes when a new unacknowledged CAT wallet is added, we end up with the multiplication factor (# of puzzles hashes x # of CAT wallets) that we're trying to get rid of.

[Quexington]

Isn't it used inside the create_new_wallet RPC command and with that exposed to whoever wants to use it?

We also have /farm_block which clearly isn't meant for general use. Removing that function from the RPC and codebase in general is a piece of tech debt that we have yet to pay off. The supported ways to mint CATs are well documented and do not refer people to this RPC.

[xdustinface]

We're subscribing only to standard puzzle hashes, if we subscribe to all the cat puzzle hashes when a new unacknowledged CAT wallet is added, we end up with the multiplication factor (# of puzzles hashes x # of CAT wallets) that we're trying to get rid of.

Why would we need to subscribe to all CAT puzzle hashes? Isn't that what hints are there for and what this PR is going to leverage for the CAT sync anyway or how is syncing "unackknowleged CATs" different?

Also, no thought about the reorg issue i mentioned above? And in general about this new "store" and the "retry store".. In my mind, this is rather something we should get rid of instead of giving it more attention because it certainly leads to coin states being processed out of order only if there was some "flake" somewhere in between and we depend on processing them in order so its basically something which inevitable leads to issues in certain cases.

[Quexington]

Why would we need to subscribe to all CAT puzzle hashes? Isn't that what hints are there for and what this PR is going to leverage for the CAT sync anyway or how is syncing "unackknowleged CATs" different?

I explained how they are different above. We've already "synced" unacknowledged CATs. The full node already gave us everything we subscribed to. Adding a new CAT wallet doesn't trigger us to reprocess every state we've ever seen. So if we don't store the ones we might want to process, we'll never add the coin states at all.

Also, no thought about the reorg issue i mentioned above?

So many things to respond to I guess it got lost :) It's a good catch. I added a rollback to the interested store and made a separate PR for doing the same to the retry store: #15303.

it certainly leads to coin states being processed out of order only if there was some "flake" somewhere in between and we depend on processing them in order so its basically something which inevitable leads to issues in certain cases

I've explained this on a separate PR, but the current wallet paradigm is supposed to assume that the order we get the coins in doesn't matter. Any wallet that does depend on the order is bugged, and should be fixed so that is not the case. We shouldn't compromise general flexibility for a few wallets that can't handle it, we should bring them in compliance with the standard.

[xdustinface]

I explained how they are different above. We've already "synced" unacknowledged CATs. The full node already gave us everything we subscribed to. Adding a new CAT wallet doesn't trigger us to reprocess every state we've ever seen. So if we don't store the ones we might want to process, we'll never add the coin states at all.

Of course right now adding a new CAT wallet doesn't trigger us reprocessing every state thats why i brought up just subscribing again to all puzzle hashes (what we anyway already do permanently) and filter/process the ones we need once a unknown CAT gets acknowledged which would avoid us storing things we don't care about and not opening up the ability for someone to blow up the DB with unknown CATs which sounds risky to me. But seems like you don't share the same concerns or just don't want to change things again..

So, if we really go with this table you could maybe at least add unit tests for the new table's methods and ideally also a tests where you retrospectively acknowledge a CAT to at least make sure the things work as expected.

I added a rollback to the interested store and made a separate PR for doing the same to the retry store: #15303.

Thanks!

Any wallet that does depend on the order is bugged

Why is wallet is bugged if it expects to receive coins in order? Sounds actually reasonable to me. In general it's the reality a coin creation exists before a spend of the same coin so shouldn't we also expect and process it this way?

[Quexington]

Why is wallet is bugged if it expects to receive coins in order? Sounds actually reasonable to me. In general it's the reality a coin creation exists before a spend of the same coin so shouldn't we also expect and process it this way?

It's a dependency that we don't need, it is entirely possible to sync every wallet with out of order coins, and since this is better and puts less pressure on the wallet protocol/node to get us everything in order, I think we should preserve that.

[xdustinface]

Its obviously covered somehow, but i think there is no tests which specifically tests this new "retrospectively add CAT states" feature, would have been nice to see this too :)

[Quexington]

Its obviously covered somehow, but i think there is no tests which specifically tests this new "retrospectively add CAT states" feature, would have been nice to see this too :)

chia-blockchain/tests/wallet/cat_wallet/test_cat_wallet.py

Line 737 in 9e8d6eb

async def test_cat_hint(self, self_hostname, two_wallet_nodes, trusted, autodiscovery):

<- tests autodiscovery including after you have already received a CAT. This test failing is what prompted the need for the unacknowledged states table.