Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Trusted wallet peer enhancements #17872

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

spacefarmers
Copy link

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

@felixbrucker
Copy link
Contributor

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

@spacefarmers
Copy link
Author

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.

@Rigidity
Copy link
Contributor

Rigidity commented Apr 17, 2024

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)

@xearl4
Copy link
Contributor

xearl4 commented Apr 17, 2024

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 :)

@spacefarmers
Copy link
Author

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.

@Rigidity
Copy link
Contributor

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 Rigidity added Added Required label for PR that categorizes merge commit message as "Added" for changelog Logging Change output to .chia/NETWORK/log/debug.log labels Apr 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Added Required label for PR that categorizes merge commit message as "Added" for changelog community-pr Logging Change output to .chia/NETWORK/log/debug.log
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants