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

wallet/walletrpc.c: txprepared transactions now use current tip blockheight by default. #3797

Merged
merged 1 commit into from
Jul 1, 2020

Conversation

ZmnSCPxj
Copy link
Collaborator

@ZmnSCPxj ZmnSCPxj commented Jun 29, 2020

Changelog-Changed: txprepare now prepares transactions whose nLockTime is set to the tip blockheight, instead of using 0. fundchannel will use nLockTime set to the tip blockheight as well.

@ZmnSCPxj ZmnSCPxj requested a review from cdecker as a code owner June 29, 2020 04:13
Copy link
Member

@cdecker cdecker left a comment

Choose a reason for hiding this comment

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

Do we really want yet another parameter, that confuses users?

Wouldn't it be more sensible adding the default to current blockheight in, and not exposing the option until people ask for it? Of course if you have a concrete use-case for this already I'll eat my words 😉

wallet/walletrpc.c Outdated Show resolved Hide resolved
wallet/walletrpc.c Outdated Show resolved Hide resolved
@niftynei
Copy link
Collaborator

If there's not a reason for wanting control over setting it explicitly, I'd suggest re-using the logic we use for withdraw, which mimic's the bitcoind logic also.

https://github.com/ElementsProject/lightning/blob/master/wallet/walletrpc.c#L331-L334

@ZmnSCPxj
Copy link
Collaborator Author

Well, we could use locktime and set it to 499999999 when doing the initial txprepare (the one that gets txdiscarded later) in fundchannel/multifundchannel, as a belt-and-suspenders protection to prevent accidentally txsending the initial txprepared transaction. Though input reservation would do just as well (if only there was a way to atomically get reserved UTXOs into a txprepare....)

@niftynei yes, that code just gets shifted around in this PR.

@ZmnSCPxj
Copy link
Collaborator Author

Removed locktime argument.

…ckheight by default.

Changelog-Changed: `txprepare` now prepares transactions whose `nLockTime` is set to the tip blockheight, instead of using 0. `fundchannel` will use `nLockTime` set to the tip blockheight as well.
@cdecker
Copy link
Member

cdecker commented Jun 30, 2020

Need to kick Travis a bit more, but LGTM

ACK 51c40f9

@ZmnSCPxj ZmnSCPxj merged commit d0c8503 into ElementsProject:master Jul 1, 2020
@ZmnSCPxj ZmnSCPxj deleted the txprepare-tip branch July 1, 2020 15:18
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.

None yet

3 participants