chia-blockchain icon indicating copy to clipboard operation
chia-blockchain copied to clipboard

Trusted wallet peer enhancements

Open spacefarmers opened this issue 1 year ago • 7 comments

Purpose:

Recently, the SpaceFamers pool encountered significant difficulties in maintaining wallet integrity. Over time, it gradually lost track of coins. The wallet's peer ID had been incorporated into the node configuration, rendering it trusted. However, due to an unknown cause, the incorrect peer ID was configured. It took us a considerable amount of time to identify the root of the issue.

This pull request aims to enhance the experience for farmers who also operate wallets with substantial coin holdings while being limited by the node without clear indication.

Current Behavior:

Neither the wallet nor the node notifies the user when limits have been reached. Additionally, the wallet is not marked as trusted in the chia peer full_node -c CLI command. Consequently, there is currently no method to verify if a trusted peer has been configured accurately.

New Behavior:

This pull request indicates the peer as trusted, confirming the success of the configuration update. Additionally, it prompts a warning in the node log when subscription limits are exceeded. Perhaps there are better ways to warn the user (eg. in the wallet log), I'm looking forward to any suggestions.

Screenshot

spacefarmers avatar Apr 16 '24 13:04 spacefarmers

Is it possible to make the log warning only for trusted wallets? untrusted wallets from other users over the network might generate log spam. That being said it feels like that would be better placed in the wallets log instead of the full node log, yes

felixbrucker avatar Apr 16 '24 13:04 felixbrucker

Is it possible to make the log warning only for trusted wallets? untrusted wallets from other users over the network might generate log spam. That being said it feels like that would be better placed in the wallets log instead of the full node log, yes

@xearl4 mentioned that as well. If we're able to move it to the wallet logs with minimal code changes, I'd prefer that as well. I'll look into it later this week.

spacefarmers avatar Apr 16 '24 15:04 spacefarmers

I'd like to note that with CHIP-0026 once adopted, hitting the subscription limit will result in a rejection instead of silently failing. The wallet can then respond appropriately, such as subscribing via another node or warning the user. (Not saying this affects this PR really, just mentioning it since it's relevant)

Rigidity avatar Apr 17 '24 13:04 Rigidity

I'd like to note that with CHIP-0026 once adopted, hitting the subscription limit will result in a rejection instead of silently failing.

Thanks for bringing that up, that'll be a big improvement. I was looking into moving the warning to the wallet side, and concluded that it's not possible without protocol change. Happy to hear that CHIP-0026 will bring a related protocol change :)

xearl4 avatar Apr 17 '24 13:04 xearl4

I'd like to note that with CHIP-0026 once adopted, hitting the subscription limit will result in a rejection instead of silently failing. The wallet can then respond appropriately, such as subscribing via another node or warning the user. (Not saying this affects this PR really, just mentioning it since it's relevant)

That sounds much better! I'll revert the log warning code. Would the trusted label still be of any value? Otherwise the PR can be closed.

spacefarmers avatar Apr 17 '24 14:04 spacefarmers

We don't yet use CHIP-0026 in the wallet , though it will be in the full node in the upcoming release if things go according to plan. So for now this might still be useful to have.

Rigidity avatar Apr 17 '24 14:04 Rigidity

I do think the trusted is a reasonable addition if you want to dust this off

emlowe avatar May 16 '24 20:05 emlowe

This PR has been flagged as stale due to no activity for over 60 days. It will not be automatically closed, but it has been given a stale-pr label and should be manually reviewed by the relevant parties.

github-actions[bot] avatar Jul 05 '24 11:07 github-actions[bot]

Coverage exemption

emlowe avatar Jul 15 '24 18:07 emlowe