Chia-Network / chia-blockchain

Chia blockchain python implementation (full node, farmer, harvester, timelord, and wallet)
Apache License 2.0
10.82k stars 2.03k forks source link

Should max_subscribe_items check be bypassed if is_in_network? #14142

Closed joshpainter closed 1 year ago

joshpainter commented 1 year ago

I spent the last few days troubleshooting a broken wallet. The derivation index is over 20,000 and it has 7 CAT wallets, so we are well over the default max_subscribe_items limit according to this issue:

https://github.com/Chia-Network/chia-blockchain/issues/14002#issuecomment-1344654073

I noticed all kinds of weird behavior, especially offer files with multiple CATs. Some times some of the offer transactions would show up, sometimes not. Sometimes they would confirm, sometimes they'd be "stuck" forever. Sometimes restarting the wallet would work, sometimes I'd have to delete transactions and try again. There was nothing in the logs about going past this limit, but I methodically tried different settings in the config.yaml file (I thought it might be timeouts at first) until I figured out that this is the one that did it. By changing it from the default of 200000 to 2000000, my wallet can sync from empty and new offers/transactions all show up correctly as expected.

I wonder if these issues are related:

13963

14002

After digging into the code a bit more, I have some suggestions. First, I think this error should be logged. It doesn't seem to get logged or throw an exception, which means the client has no idea what is happening and we get the weird behavior as described above.

https://github.com/Chia-Network/chia-blockchain/blob/dfd888dc58e86f2a9b482912b2795ba813235e0f/chia/full_node/full_node_api.py#L1501

Secondly: I don't necessarily think that the default should be increased, as we don't want our poor full-nodes getting DOS'd with external wallet subscriptions BUT I think it is fair to assume that a local wallet running against the local trusted full node should not be subject to this max, similar to the is_in_network & is_localhost check done for max connections here:

https://github.com/Chia-Network/chia-blockchain/blob/dfd888dc58e86f2a9b482912b2795ba813235e0f/chia/server/server.py#L331

This would mean that the rare large wallets wouldn't be limited here by default when talking to their own full node, but untrusted wallets that connect from outside would still be unable to DOS us.

I am not confident enough yet in my python fu to know if this is the correct approach, or if I'm missing something else entirely. Thanks!

joshpainter commented 1 year ago

![Снимок экрана 2022-12-13 093004]Hello, I see you are very good at programming. Can you help me? When you try to open the NFT section, such an error occurs. Version 1.6.1.But with any version, such an error, and only on one account, on others everything is fine.

Hi @Svetyr, I don't think that error is related to this issue and I'm not sure what would cause it. You can create a new issue by clicking on the 3 dots on your reply and then "Reference in new issue" so somebody else might be able to help.

emlowe commented 1 year ago

It's a good idea to make a different limit for the local node. We can add logging to the node, but in the non-local case, this will just cause log entries that the node user can't do anything about. It would make some sense to reply back to the wallet with some sort of error code.

MumfMeisterT commented 1 year ago

Need to restrict this to local host. -- Add trusted wallet config

joshpainter commented 1 year ago

About the subscription data itself, there might be a more optimized way to store this.

Currently, we must register a subscription for each puzzlehash and CAT combo, which results in (DerivationIndex * CATs) subscriptions that Full Node must maintain for each connected wallet.

Instead, we could subscribe to a puzzlehash and a "TAIL filter" which would be a bitwise AND of all the TAILs in which we are interested. Now when Full Node finds a new CAT coin with our puzzlehash, it will bitwise OR it with the TAIL filter to determine if it should be forwarded to the wallet.

I'm not advanced enough to be able to prove it mathematically, but I think this would result in huge savings around subscription records for the most common wallet use-case: XCH plus a handful of CATs.

almogdepaz commented 1 year ago

It's a good idea to make a different limit for the local node. We can add logging to the node, but in the non-local case, this will just cause log entries that the node user can't do anything about. It would make some sense to reply back to the wallet with some sort of error code.

actually if i understand correctly we have no way of knowing if a full node omits updates due to whatever reason.

we can add a replay message that states that we exceeded subscriptions but we will need to add a new capability for that (will require a new message format for the replay) so not sure what version we can get that in.

almogdepaz commented 1 year ago

adding different limit config for trusted wallets https://github.com/Chia-Network/chia-blockchain/pull/14188

nqzhang commented 1 year ago

mark

emlowe commented 1 year ago

Closing issue - we added a new entry for local nodes in #14188