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

Paying through RPC/CLI could be 1 sat lower than the actual payment amount. #8274

Closed
jplix7k opened this issue Mar 22, 2023 · 7 comments
Closed

Comments

@jplix7k
Copy link

jplix7k commented Mar 22, 2023

For example if we set ["address", 0.00033389] in the params of paytomany method. It always results in 0.00033388 btc in transaction. Through investigation, looks like it is caused by type casting from Decimal to Int in satoshis function. (https://github.com/spesmilo/electrum/blob/master/electrum/commands.py#L86)

image

Test shows that rounding the amount before type casting can solve this issue. Could we please add a fix for it?
@ecdsa ecdsa added the bug 🐞 label Mar 22, 2023
@ecdsa
Copy link
Member

ecdsa commented Mar 22, 2023

I cannot reproduce this.
Here is the syntax:
./run_electrum -o paytomany "[[\"address\",0.00033389]]" --fee 0

@ecdsa
Copy link
Member

ecdsa commented Mar 22, 2023

rather than using round, I would suggest this:

index d7e1af79e..965cefed3 100644
--- a/electrum/commands.py
+++ b/electrum/commands.py
@@ -1456,7 +1456,7 @@ command_options = {
 
 # don't use floats because of rounding errors
 from .transaction import convert_raw_tx_to_hex
-json_loads = lambda x: json.loads(x, parse_float=lambda x: str(Decimal(x)))
+json_loads = lambda x: json.loads(x, parse_float=lambda x: str(Decimal(str(x))))
 arg_types = {
     'num': int,
     'nbits': int,

but again, I cannot reproduce your issue

@SomberNight
Copy link
Member

I suggest amounts to be strings already on the caller side.

For example if we set ["address", 0.00033389] in the params of paytomany method.

Just set ["address", "0.00033389"]
Otherwise, rounding might happen at the language level before we can do anything about it.

@SomberNight
Copy link
Member

I can reproduce via jsonrpc. It does not happen via the CLI.

$ ./run_electrum --testnet -w ~/.electrum/testnet/wallets/test_segwit_2 paytomany '[["tb1q6k5h4cz6ra8nzhg90xm9wldvadgh0fpttfthcg", 0.00033389]]' --fee 0
02000000000101b9e6018acb16952e3c9618b069df404dc85544eda8120e5f6e7cd7e94ce5ae8d0100000000fdffffff02fd8100000000000016001410c5b97085ec1637a9f702852f5a81f650fae1566d82000000000000160014d5a97ae05a1f4f315d0579b6577daceb5177a42b024730440220251d2ce83f6e69273de8e9be8602fbcf72b9157e1c0116161fa52f7e04db6e4302202d84045cc6b7056a215d1db3f59884e28dadd5257e1a3960068f90df90b452d1012102b0eff3bf364a2ab5effe952cba33521ebede81dac88c71951a5ed598cb48347b3a022500

$ curl --data-binary '{"id":"curltext","method":"paytomany","params":{"outputs":[["tb1q6k5h4cz6ra8nzhg90xm9wldvadgh0fpttfthcg", 0.00033389]], "fee": 0, "wallet": "/home/user/.electrum/testnet/wallets/test_segwit_2"}}' http://user:pass@127.0.0.1:7777
{"id": "curltext", "jsonrpc": "2.0", "result": "02000000000101b9e6018acb16952e3c9618b069df404dc85544eda8120e5f6e7cd7e94ce5ae8d0100000000fdffffff02fe8100000000000016001410c5b97085ec1637a9f702852f5a81f650fae1566c82000000000000160014d5a97ae05a1f4f315d0579b6577daceb5177a42b0247304402206ef66b845ca298c14dc6e8049cba9ed19db1671132194518ce5d521de6f5df8802205ca4b1aee703e3b98331fb9b88210917b385560020c8b2a8a88da38996b101c4012102b0eff3bf364a2ab5effe952cba33521ebede81dac88c71951a5ed598cb48347b39022500"}

@ecdsa is right, we should try to reuse:

def to_decimal(x: Union[str, float, int, Decimal]) -> Decimal:
# helper function mainly for float->Decimal conversion, i.e.:
# >>> Decimal(41754.681)
# Decimal('41754.680999999996856786310672760009765625')
# >>> Decimal("41754.681")
# Decimal('41754.681')
if isinstance(x, Decimal):
return x
return Decimal(str(x))

@SomberNight SomberNight self-assigned this Mar 22, 2023
@SomberNight
Copy link
Member

SomberNight commented Mar 22, 2023

Thanks for reporting this. Should be fixed on master now.

I still suggest using string amounts though :P (which you can also do on the latest release to work around this issue)

@jplix7k
Copy link
Author

jplix7k commented Mar 24, 2023

@ecdsa @SomberNight
Thank you for confirming and solving the issue. I'm very impressed by the efficiency. Could you please tell me roughly when the next version's release would be?

@ecdsa
Copy link
Member

ecdsa commented Mar 24, 2023

we are targeting next week for a release

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

No branches or pull requests

4 participants
@ecdsa @SomberNight @jplix7k and others