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.

Sign up for GitHub

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

Jump to bottom

Unlock transaction inputs if tx cannot be published #1404

Merged
merged 12 commits into from
May 5, 2020
Merged

Unlock transaction inputs if tx cannot be published #1404

merged 12 commits into from
May 5, 2020

Conversation

sstone
Copy link
Member

@sstone sstone commented May 4, 2020

In some cases, funding a tx will work but publishing it may fail (because mempool fees are not met for example). In that case we need to make sure that the tx inputs, which were locked by fundrawtransaction, are unlocked.

@sstone
Unlock transaction inputs if tx cannot be published
7c5baf4 
In some cases, funding a tx will work but publishing may fail (because mempool fees are not met for example).
In that case we need to make sure that the tx inputs are unlocked.
@pm47 pm47 self-requested a review May 4, 2020 14:47
@sstone
Address review comments
d7b40d4 
Reformat `unlockOutpoints`, and consider that if Bitcoin Core returned `expected locked output` then unlocking was successful.
sstone added 2 commits May 4, 2020 18:59
@sstone
Nit: update scaladoc [ci skip]
13e95bf
@sstone
Add unlock test
333fd92 
We check that unlocking outpoints that are not locked is considered successful
pm47
pm47 reviewed May 4, 2020
View reviewed changes
Copy link
Member

@pm47 pm47 left a comment

Choose a reason for hiding this comment

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

I would have done it as an even simpler test in ExtendedBitcoinClientSpec but why not.

eclair-core/src/test/scala/fr/acinq/eclair/blockchain/bitcoind/BitcoinCoreWalletSpec.scala Show resolved Hide resolved
eclair-core/src/test/scala/fr/acinq/eclair/blockchain/bitcoind/BitcoinCoreWalletSpec.scala Outdated Show resolved Hide resolved
eclair-core/src/test/scala/fr/acinq/eclair/blockchain/bitcoind/BitcoinCoreWalletSpec.scala Show resolved Hide resolved
eclair-core/src/test/scala/fr/acinq/eclair/blockchain/bitcoind/BitcoinCoreWalletSpec.scala Show resolved Hide resolved
eclair-core/src/test/scala/fr/acinq/eclair/blockchain/bitcoind/BitcoinCoreWalletSpec.scala Outdated Show resolved Hide resolved
eclair-core/src/test/scala/fr/acinq/eclair/blockchain/bitcoind/BitcoinCoreWalletSpec.scala Outdated Show resolved Hide resolved
sstone and others added 6 commits May 4, 2020 20:40
@sstone @pm47
Update eclair-core/src/test/scala/fr/acinq/eclair/blockchain/bitcoind…
7ac975c 
…/BitcoinCoreWalletSpec.scala

Co-authored-by: Pierre-Marie Padiou <pm47@users.noreply.github.com>
@sstone @pm47
Update eclair-core/src/test/scala/fr/acinq/eclair/blockchain/bitcoind…
9e477d4 
…/BitcoinCoreWalletSpec.scala

Co-authored-by: Pierre-Marie Padiou <pm47@users.noreply.github.com>
@sstone @pm47
Update eclair-core/src/test/scala/fr/acinq/eclair/blockchain/bitcoind…
9cc524c 
…/BitcoinCoreWalletSpec.scala

Co-authored-by: Pierre-Marie Padiou <pm47@users.noreply.github.com>
@sstone @pm47
Update eclair-core/src/test/scala/fr/acinq/eclair/blockchain/bitcoind…
c121880 
…/BitcoinCoreWalletSpec.scala

Co-authored-by: Pierre-Marie Padiou <pm47@users.noreply.github.com>
@sstone @pm47
Update eclair-core/src/test/scala/fr/acinq/eclair/blockchain/bitcoind…
43751c7 
…/BitcoinCoreWalletSpec.scala

Co-authored-by: Pierre-Marie Padiou <pm47@users.noreply.github.com>
@sstone
Nit: update obsolete comment [ci skip]
5b7b417
pm47
pm47 previously approved these changes May 4, 2020
View reviewed changes
Copy link
Member

@pm47 pm47 left a comment

Choose a reason for hiding this comment

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

LGTM. I verified that when the special case for the "expected locked output" is commented out, the test indeed fails.

@sstone sstone merged commit 874bd4b into master May 5, 2020
@sstone sstone deleted the rollback-if-commit-fails branch May 5, 2020 09:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pm47 pm47 pm47 approved these changes

@t-bast t-bast t-bast approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sstone @pm47 @t-bast