-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Trusted wallet peer enhancements #17872
Conversation
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. |
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) |
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 :) |
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. |
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. |
I do think the trusted is a reasonable addition if you want to dust this off |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me
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. |
Coverage exemption |
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.