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

TxConfirmationDialog: warn high fees #3897

Merged
merged 1 commit into from
Apr 28, 2022

Conversation

selsta
Copy link
Collaborator

@selsta selsta commented Apr 27, 2022

Screenshot 2022-04-27 at 17 42 26

Copy link
Contributor

@reemuru reemuru left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, compile tested and triggered high fee display

pr3897

@jeffro256
Copy link
Contributor

This shouldn't be the only long-term solution, but it's a nice quick fix for now!

@reemuru
Copy link
Contributor

reemuru commented Apr 27, 2022

This shouldn't be the only long-term solution, but it's a nice quick fix for now!

@jeffro256
agree, i can create an issue:

"Update Magic for high fee" ?

@selsta
Copy link
Collaborator Author

selsta commented Apr 27, 2022

The only long term solution will be to remove the remote node scanner from simple mode and use hardcoded trusted community nodes.

@jeffro256
Copy link
Contributor

jeffro256 commented Apr 27, 2022

@selsta Or have nodes provide some kind of blockchain proof that their recommended fee is reasonable and have the wallet double-check the blockchain state with other nodes. Really the wallet software needs to be more active in calculating/reviewing suggested fee amounts.

@SamsungGalaxyPlayer
Copy link

Apologies for being out of the loop here, but some sanity checks:

  1. Does the current code show this warning if fees are > 0.01 XMR?
  2. Does the current code check if the fee follows an appropriate multiplier set in the wallet defaults? We don't want wallets tricked to send unique fee values either.

@selsta

@selsta
Copy link
Collaborator Author

selsta commented Apr 27, 2022

Does the current code show this warning if fees are > 0.01 XMR?

Yes, I changed it for the screenshot to demonstrate it. I'm open for different values.

Does the current code check if the fee follows an appropriate multiplier set in the wallet defaults? We don't want wallets tricked to send unique fee values either.

As far as I understand it the wallet calculates the fee with data from the daemon. If the daemon tricks you like in this case I'm not sure what you want to do on wallet side.

@plxkax
Copy link

plxkax commented Apr 30, 2022

The only long term solution will be to remove the remote node scanner from simple mode and use hardcoded trusted community nodes.

It would be good to offer the choice between remote node or having the trusted community node. You don't want to make it feel like it's being forced, user should have the option to decide.

@selsta
Copy link
Collaborator Author

selsta commented Apr 30, 2022

You can either use simple mode, which automatically will connect you to a community node or you can use advanced mode which allows you to set your own remote node.

@plxkax
Copy link

plxkax commented Apr 30, 2022

Would there be any security compromises with automatically having all simple mode users onto community node only?

@selsta
Copy link
Collaborator Author

selsta commented Apr 30, 2022

Security compromises compared to now? No. Simple mode already connects to a random remote node.

@plxkax
Copy link

plxkax commented Apr 30, 2022

To confirm, Simple mode currently connects to a random remote node automatically but with this change Simple mode will connect to a community trusted node automatically with no security compromises compared to now?

@plxkax
Copy link

plxkax commented Apr 30, 2022

Do we have an any idea as to how big the community trusted node list will be?

@selsta
Copy link
Collaborator Author

selsta commented Apr 30, 2022

Correct.

@selsta
Copy link
Collaborator Author

selsta commented Apr 30, 2022

Do we have an any idea as to how big the community trusted node list will be?

Around 10 maybe? Don't know yet.

@nixt7
Copy link

nixt7 commented May 10, 2022

It's GUI discussed here. But what about wallet RPC? It needs protection from high fees too.

@selsta
Copy link
Collaborator Author

selsta commented May 10, 2022

The GUI simple mode currently connects you to a random remote node that's why it was important to add the warning here quickly. The RPC wallet doesn't have such a feature so it is way more unlikely that you run into the high fee issue in the first place, unless you manually connect to a malicious remote node.

We can add a warning to the other wallets too but you have to keep in mind that you should ideally use your own node or connect to a node from a person or community member that you trust.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants