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

Late locking #530

Merged
merged 5 commits into from
Nov 19, 2020
Merged

Late locking #530

merged 5 commits into from
Nov 19, 2020

Conversation

jaspervdm
Copy link
Contributor

@jaspervdm jaspervdm commented Nov 11, 2020

Adding experimental late locking feature to the wallet. If this option is set during tx creation, no UTXOs will be locked on the wallet. Instead, they will only be locked as late as possible, namely when the transaction is finalized.

This feature is considered experimental. If during finalization we realize that the initial fee is not sufficient to pay for the transaction anymore, we simply return an error. This scenario will probably be rare for regular users, but wallets that have many transactions outstanding simultaneously might encounter this. The solution to this would be to add another kernel that makes up the fee difference. This will be addressed in a future PR.

Thanks to @yeastplume for helping with this new feature.

TODOs:

  • Remove the 3*fee and output a nice warning when the final fee does not match the initial fee
  • Add CLI flag to enable late locking

@Paouky
Copy link

Paouky commented Nov 11, 2020

Great!
Do you think it should be set as the default option once these 2 items on the TODO list are crossed?

@antiochp
Copy link
Member

antiochp commented Nov 12, 2020

Great!
Do you think it should be set as the default option once these 2 items on the TODO list are crossed?

Eventually yes, but there is a fair amount of testing and simply trying this out in various scenarios and under various conditions before we would consider making it the default.

This is still very much "experimental" and will likely remain so for a while.

Copy link
Member

@antiochp antiochp left a comment

Choose a reason for hiding this comment

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

Still reviewing.
Commented on a couple of very tiny cleanups in comments etc.

Looks 👍 so far.

libwallet/src/api_impl/owner.rs Outdated Show resolved Hide resolved
libwallet/src/api_impl/owner.rs Outdated Show resolved Hide resolved
libwallet/src/api_impl/owner.rs Outdated Show resolved Hide resolved
controller/tests/late_lock.rs Outdated Show resolved Hide resolved
@jaspervdm
Copy link
Contributor Author

jaspervdm commented Nov 12, 2020

Resolved the TODO items. Currently the finalization step errors if the intial fee is different from the final fee. Technically we could handle the case where the initial fee is higher than the final fee with relative ease. It means we slightly overpay for the tx but it would be better than the tx failing to finalize.

@antiochp
Copy link
Member

Currently the finalization step errors if the intial fee is different from the final fee.

This is probably fine to go with for now? I'm assuming this would be a minor change that affects only a single wallet (the one finalizing the tx) so rolling out an improvement for this would be a minor task.

Copy link
Member

@antiochp antiochp left a comment

Choose a reason for hiding this comment

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

👍
This is cool.
Tested locally generating a bunch of "competing" slatepacks with different sends and able to choose any one of them to actually receive and finalize.

No locked outputs.
No unconfirmed change outputs.
Nothing to cleanup afterwards.

@antiochp
Copy link
Member

antiochp commented Nov 12, 2020

"Coin control" gets interesting with "late locking" (see #529).

We need to specify a large enough fee up front, but we do not need to select explicit outputs to spend until the finalize step.

I'm actually kind of excited about how "coin control" would look with "late locking". You have a lot of freedom to pick and choose outputs in any way you want once you come to finalize it. Selection strategies can be very flexible this way.

@jaspervdm jaspervdm merged commit a83f92d into mimblewimble:master Nov 19, 2020
@antiochp antiochp changed the title Late locking (experimental) Late locking Nov 25, 2020
bayk added a commit to mwcproject/mwc-wallet that referenced this pull request Aug 2, 2024
…e#530)

MWC Note: Those change are already here. In this commit the  test is fixed and activated.
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