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

Reduce the risk of change chaining transactions #2380

Merged
merged 5 commits into from Feb 22, 2019

Conversation

Projects
None yet
3 participants
@cdecker
Copy link
Member

cdecker commented Feb 21, 2019

We had quite a few cases in which one failed transaction (potentially due to insufficient fees) was causing all following transactions to remain in limbo as well.

This PR adds a minconf argument inspired by bitcoind that allows us to determine how many confirmations an output must have before being considered for coin selection. The default of 1 confirmation mitigates the risk of chainge chaining, with the drawback of failing a few more transactions at selection than strictly necessary.

cdecker added some commits Feb 21, 2019

wallet: Allow limiting the selection by confirmation height
This allows us to specify that an output must have been confirmed before the
given maximum height. This allows us to specify a minimum number of
confirmations for an output to be selected.

Signed-off-by: Christian Decker <decker.christian@gmail.com>
jsonrpc: Expose the minconf parameter for fundchannel and withdraw
Signed-off-by: Christian Decker <decker.christian@gmail.com>
pylightning: Expose minconf in fundchannel and withdraw
Signed-off-by: Christian Decker <decker.christian@gmail.com>
jsonrpc: Arm the minconf=1 parameter and deal with the fallout
We want to disallow using unconfirmed outputs by default, so making the
default 1 confirmation seems a good idea. This also matches `bitcoind`s
minimum confirmation requirement.

Arming however breaks some of our tests, so I used `minconf=0` for the
breaking tests and added a new test specifically for the `minconf` parameter
for `fundchannel`.

Signed-off-by: Christian Decker <decker.christian@gmail.com>

@cdecker cdecker requested review from rustyrussell and niftynei Feb 21, 2019

@cdecker cdecker self-assigned this Feb 21, 2019

@cdecker cdecker added this to the v0.7 milestone Feb 21, 2019

@@ -283,6 +284,13 @@ static const struct utxo **wallet_select(const tal_t *ctx, struct wallet *w,
struct amount_sat needed;
struct utxo *u = tal_steal(utxos, available[i]);

/* If we require confirmations check that we have a
* confirmation height and that it is below the required
* maxheight (current_height - minconf */

This comment has been minimized.

@niftynei

niftynei Feb 21, 2019

Collaborator

ubernit: missing )

* confirmation height and that it is below the required
* maxheight (current_height - minconf */
if (maxheight != 0 &&
(!u->blockheight || *u->blockheight > maxheight))

This comment has been minimized.

@niftynei

niftynei Feb 21, 2019

Collaborator

tbh i find maxheight not intuitive wrt to what you're checking for here. maybe something like gated_confirmation_height or req_conf_height ... neither of these seems exactly right.

This comment has been minimized.

@cdecker

cdecker Feb 21, 2019

Author Member

Yeah, I've been going back and forth with the name, it's the maximum acceptable confirmation_height really, and being maybe the special value 0 is what makes it confusing.

This comment has been minimized.

@rustyrussell

rustyrussell Feb 22, 2019

Contributor

Yeah, 0-as-special here is OK. Sometimes I gratuitously use a pointer so NULL can mean "don't care"...

@niftynei

This comment has been minimized.

Copy link
Collaborator

niftynei commented Feb 21, 2019

ACK 0defb33

@rustyrussell
Copy link
Contributor

rustyrussell left a comment

Ack 0defb33

* confirmation height and that it is below the required
* maxheight (current_height - minconf */
if (maxheight != 0 &&
(!u->blockheight || *u->blockheight > maxheight))

This comment has been minimized.

@rustyrussell

rustyrussell Feb 22, 2019

Contributor

Yeah, 0-as-special here is OK. Sometimes I gratuitously use a pointer so NULL can mean "don't care"...

@rustyrussell rustyrussell merged commit 8fb2e6b into master Feb 22, 2019

3 checks passed

ackbot PR ack'd by niftynei, rustyrussell
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.