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

Fix availableForSend/Receive #1293

Merged
merged 2 commits into from
Jan 28, 2020
Merged

Fix availableForSend/Receive #1293

merged 2 commits into from
Jan 28, 2020

Conversation

t-bast
Copy link
Member

@t-bast t-bast commented Jan 27, 2020

There was a rounding issue with the availableForSend/Receive calculation.
Because CommitTx fee and Htlc fee were computed separately,
but each was individually rounded down to Satoshis, we could
end up with an off-by-one error.

This resulted in an incapacity to send/receive the maximum amount available.

We now allow computing fees in msat, which removes rounding issues.

There was a rounding issue with the availableForSend/Receive calculation.
Because CommitTx fee and Htlc fee were computed separately,
but each was individually rounded down to Satoshis, we could
end up with an off-by-one error.

This resulted in an incapacity to send/receive the maximum amount available.

We now allow computing fees in msat, which removes rounding issues.
@t-bast t-bast requested review from sstone and pm47 January 27, 2020 09:15
@t-bast
Copy link
Member Author

t-bast commented Jan 27, 2020

Great, it looks like the fuzz test has found something :)
I'm investigating that (it may be an issue with the test itself).

EDIT: it had run into this known issue: lightning/bolts#728
We can't really fix it for now, so we just make sure the test doesn't get into that state.

Otherwise we may run into the following issue:
lightning/bolts#728
@codecov-io
Copy link

codecov-io commented Jan 27, 2020

Codecov Report

Merging #1293 into master will increase coverage by 0.16%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1293      +/-   ##
==========================================
+ Coverage   77.18%   77.35%   +0.16%     
==========================================
  Files         143      143              
  Lines        9976     9978       +2     
  Branches      408      407       -1     
==========================================
+ Hits         7700     7718      +18     
+ Misses       2276     2260      -16
Impacted Files Coverage Δ
...in/scala/fr/acinq/eclair/channel/Commitments.scala 90.32% <100%> (ø) ⬆️
...la/fr/acinq/eclair/transactions/Transactions.scala 96.77% <100%> (+0.02%) ⬆️
...c/main/scala/fr/acinq/eclair/channel/Channel.scala 83.19% <0%> (-0.37%) ⬇️
...nq/eclair/blockchain/electrum/ElectrumWallet.scala 81% <0%> (+0.25%) ⬆️
...nq/eclair/blockchain/electrum/ElectrumClient.scala 73.64% <0%> (+0.36%) ⬆️
...src/main/scala/fr/acinq/eclair/router/Router.scala 92.97% <0%> (+0.68%) ⬆️
...q/eclair/blockchain/electrum/ElectrumWatcher.scala 52.13% <0%> (+0.85%) ⬆️
...c/main/scala/fr/acinq/eclair/crypto/ShaChain.scala 94.87% <0%> (+2.56%) ⬆️
...cinq/eclair/blockchain/bitcoind/zmq/ZMQActor.scala 94.87% <0%> (+5.12%) ⬆️
...clair/blockchain/electrum/ElectrumClientPool.scala 84.94% <0%> (+10.75%) ⬆️

@t-bast t-bast merged commit 0a66d3f into master Jan 28, 2020
@t-bast t-bast deleted the fix-available-for-send-rounding branch January 28, 2020 13:20
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.

3 participants