mimblewimble / grin

Minimal implementation of the Mimblewimble protocol.
https://grin.mw/
Apache License 2.0
5.04k stars 989 forks source link

Wallet is stuck when creating a transaction with 'smallest' strategy #981

Closed jaspervdm closed 6 years ago

jaspervdm commented 6 years ago

For testing purposes I have been sending transactions around with a couple of wallets. I have noticed that at some point, a wallet gets stuck indefinitely when trying to create a transaction when using the 'smallest' strategy:

$ grin wallet send -f -d DEST -s smallest 0.1
Apr 18 21:27:18.643 INFO This is Grin version 0.2.0 (git testnet2-64-g323480a), built for x86_64-unknown-linux-gnu by rustc 1.25.0 (84203cac6 2018-03-25).
Apr 18 21:27:18.643 DEBG Built with profile "debug", features "" on Tue, 17 Apr 2018 14:55:39 GMT.
Apr 18 21:27:18.643 DEBG Using wallet seed file at: ./wallet.seed
Apr 18 21:27:18.722 DEBG Refreshing wallet outputs
Apr 18 21:27:19.004 INFO Acquiring wallet lock ...
Apr 18 21:27:19.101 INFO ... released wallet lock

After that last line it just waits forever. On the server side it looks like this (I have removed the middle part)

Apr 18 21:29:33.126 DEBG outputs_by_ids: ["09cc058b8598e07877b3784d933932d715528f3c0c755c0b3c298d6b85962cf51d", "09ea353622cd5500071bdb00457d6727c63f4df7d1070132da678244abe5725d5c", ..., "0878ad63cbe5af18833084a156f09b0523a7acc6193694ecc71b23f46d8d8493f4", "0907aebe3d636c1cb35107010ac9489ca1328d0f1f0f68551eb9450e9d97fac68b"]

The server appears to keep function normally after that. There is no error of any kind on the wallet or server side.

I asked @quentinlesceller to verify independently and he was able to reproduce the situation with the specific seed. He also noticed that the wallet gets "unstuck" by using the default strategy. If you want to see it yourself, you can restore from the seed 0ed9be2bfccddb04f0bc938196a48b697ac2bda8332d19a42b41fc680ae7e593. Please use the same command as I did, otherwise the wallet might get unstuck and I have to try to create another seed with the same error.

jaspervdm commented 6 years ago

I think I've created another seed that has the same problem: 78214b9afd92b5d5a4654181f588f781d851bc07cf377accc5cde8db015878b6

quentinlesceller commented 6 years ago

Okay just found that the wallet is stuck in a loop here https://github.com/mimblewimble/grin/blob/master/wallet/src/sender.rs#L272-L292.

quentinlesceller commented 6 years ago

This bug happens when the input amount used is exactly the same as the output (e.g. here I want to send 0.1 which is with fee 0.107 and I have an output with exactly the same amount) hence there is no change outputs. Looking at a way to fix it now. Also the calculation of the fee always start with 2 outputs by default.

quentinlesceller commented 6 years ago

So the problem is the following: 1 - We try to spend 0.1 grins with the smallest strategy 2 - We compute the fee for this transaction with two outputs. The fee is 0.007 3 - So in total we will spend 0.107 grins 3 - We have an output which is exactly 0.107 grins. 4 - Right now we are stuck in a loop since we try to get more coins because of the <= conditions in sender.rs:272. Right now, I see two ways to fix it: 1 - In a case of the output is an exact match we should build the transaction with exactly one output. Currently this transaction with be rejected since there is fee mismatch between sender and receiver (fees are computed for a two outputs transaction and if we compute the fee for one output we a change...). So maybe, after allowing no change output transaction remove the fee dispute part, since the sender now pay the fee? 2 - Allows empty change outputs ? 3 - None of the solution above are perfect so there is probably something I did not think about (@antiochp) ?

ignopeverell commented 6 years ago

We should start with the fee setup for a single output and update it for 2 outputs in the loop if the total exceeds the amount with fee (we found change was needed).

quentinlesceller commented 6 years ago

Okay agree, I'm currently working on it. However in this case, it's kind of special. Once we add the second outputs, the total match exactly the inputs hence the change is exactly zero. If we try to bypass this by removing the change we have a fee dispute and if we don't the node reject the transaction as malformed. Should we "force add" an input so that the change is different than zero? Again this is a very special case here.

antiochp commented 6 years ago

Oh so there is change initially based on the single output tx. But then we add the change output and this adjusts the fee which then cancels out the change output...

lol

antiochp commented 6 years ago

This change output is small (as it matches the fee adjustment for that one single output). So can we just remove the output and adjust the fee slightly to compensate for this?

We check for "fee dispute" by looking for an exact match (both sender and receiver need to calculate the exact same fee) - but maybe we can relax this so there is only a dispute if the sender has underpaid in terms of fees (sender fee must be >= receiver calculated fee)? Just thinking out loud here - not sure this is even possible given how we interactively construct the tx.