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

Support bitcoin-0.19.1 #1380

Merged
merged 2 commits into from
Apr 27, 2020
Merged

Support bitcoin-0.19.1 #1380

merged 2 commits into from
Apr 27, 2020

Conversation

araspitzu
Copy link
Contributor

Update our regtests to use bitcoin-0.19.1, the most notable changes in this release are about fees (especially the CPFP-carve out rule for the dual-anchor commitment format). There are also some RPC changes: the fundrawtransaction RPC does not fail when providing a too little feerate but instead bitcoin will fail when broadcasting the transaction with sendrawtransaction, the error message is also changed.

I replaced the URLs for our bitcoin-core test instance with bitcoincore.org which is quite faster than bitcoin.org

@araspitzu araspitzu marked this pull request as ready for review April 16, 2020 09:51
@sstone
Copy link
Member

sstone commented Apr 20, 2020

I think that the impact of recent fee policy changes should be documented in this PR: do we need to change how we create funding transactions ? can funding transactions be created but not published ? Do users need to take any action ?

@araspitzu
Copy link
Contributor Author

Right, so here is the gist of the fee policy changes in the new bitcoin release:

  • CPFP carve-out: allows for one extra ancestor in the CPFP "package" of transactions, this relaxes the mempool acceptance constraints to allow the dual-anchor design. We don't have to take action for this.
  • -maxtxfee parameter in fundrawtransaction: this is a hard limit set by default to 0.1BTC to the maximum fee payable by a transaction created via RPC, we don't have to take action for this as our transactions are (hopefully) paying less than 0.1BTC, although it's worth to bear this parameter in mind.

One notable change for eclair are the RPCs fundrawtransction and sendrawtransaction, it's a bit undocumented in the release notes but their behavior has slightly changed. fundrawtransaction does not fail anymore if the feerate is too low, instead bitcoin-core will fail to broadcast it (with sendrawtransaction) if it doesn't meet the criteria to enter the mempool, this means that transactions can be created but not published (the min-fee parameters are unchanged in this release and it's still 253 sat/kw). Users don't have to take any action nor we need to change the way the transactions are created because we correctly handle failure to publish transactions.

@pm47 pm47 requested a review from sstone April 23, 2020 08:40
sstone
sstone previously approved these changes Apr 23, 2020
@t-bast
Copy link
Member

t-bast commented Apr 23, 2020

Our README still mentions that we need at least bitcoin core 0.17.1, do we want to update that to a more recent version? And our wiki?

@pm47
Copy link
Member

pm47 commented Apr 23, 2020

@sstone please confirm that you have checked the hashes + sigs.

@pm47
Copy link
Member

pm47 commented Apr 23, 2020

Our README still mentions that we need at least bitcoin core 0.17.1, do we want to update that to a more recent version? And our wiki?

Good point, we should recommend what we test.

@araspitzu
Copy link
Contributor Author

Our README still mentions that we need at least bitcoin core 0.17.1, do we want to update that to a more recent version? And our wiki?

Good point, we should recommend what we test.

Do we want to drop support to 0.17.1? I've been using 0.18 for quite some time in my personal instance, i suggest to update the README saying we support 0.18 and 0.19 but we recommend 0.18, WDYT?

@pm47
Copy link
Member

pm47 commented Apr 23, 2020

Do we want to drop support to 0.17.1? I've been using 0.18 for quite some time in my personal instance, i suggest to update the README saying we support 0.18 and 0.19 but we recommend 0.18, WDYT?

But we could be breaking compatibility with 0.17.1 and 0.18 tomorrow without noticing, because we are not testing them. I have no strong opinion on this though.

@sstone
Copy link
Member

sstone commented Apr 23, 2020

@pm47 Yes I've checked all hashes
Good point about the readme which I think should be updated to 0.18.1 and higher (I'm in favor of dropping support for older versions)

@araspitzu
Copy link
Contributor Author

I updated the README to support explicitly 0.18.1 and 0.19.1, so when 0.20 is out we don't get caught off-guard :), both supported versions are actively tested either by running eclair on mainnet with 0.18.1 or using the 0.19.1 it in the tests.

@codecov-io
Copy link

Codecov Report

Merging #1380 into master will increase coverage by 0.29%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1380      +/-   ##
==========================================
+ Coverage   86.45%   86.75%   +0.29%     
==========================================
  Files         123      123              
  Lines        9357     9427      +70     
  Branches      405      403       -2     
==========================================
+ Hits         8090     8178      +88     
+ Misses       1267     1249      -18     
Impacted Files Coverage Δ
...eclair/blockchain/bitcoind/BitcoinCoreWallet.scala 87.50% <0.00%> (-1.39%) ⬇️
...la/fr/acinq/eclair/transactions/Transactions.scala 96.38% <0.00%> (-0.51%) ⬇️
...c/main/scala/fr/acinq/eclair/channel/Helpers.scala 95.84% <0.00%> (-0.47%) ⬇️
...main/scala/fr/acinq/eclair/io/PeerConnection.scala 83.48% <0.00%> (-0.31%) ⬇️
...e/src/main/scala/fr/acinq/eclair/router/Sync.scala 98.38% <0.00%> (+0.10%) ⬆️
...src/main/scala/fr/acinq/eclair/router/Router.scala 94.30% <0.00%> (+0.29%) ⬆️
...c/main/scala/fr/acinq/eclair/channel/Channel.scala 85.90% <0.00%> (+0.60%) ⬆️
...nq/eclair/blockchain/electrum/ElectrumClient.scala 74.55% <0.00%> (+0.71%) ⬆️
...q/eclair/blockchain/electrum/ElectrumWatcher.scala 55.20% <0.00%> (+1.60%) ⬆️
...main/scala/fr/acinq/eclair/router/Validation.scala 92.57% <0.00%> (+1.63%) ⬆️
... and 3 more

@araspitzu araspitzu merged commit 22d4767 into master Apr 27, 2020
@araspitzu araspitzu deleted the support_bitcoin_0.19.x branch April 27, 2020 14:10
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.

5 participants