Skip to content

json_withdraw: fix change_outputs showing forever as unconfirmed in listfunds - #2310

Merged
cdecker merged 1 commit into
masterfrom
unknown repository
Feb 4, 2019
Merged

json_withdraw: fix change_outputs showing forever as unconfirmed in listfunds#2310
cdecker merged 1 commit into
masterfrom
unknown repository

Conversation

@ghost

@ghost ghost commented Feb 1, 2019

Copy link
Copy Markdown

Add the change output to owned_txfilter so its entry in db will
get a confirmation_height when detected in a block by filter_block_txs

before this commit, after a 'withdraw' command, 'listfunds' would
not show our change outputs as confirmed. This was mentioned in #2300

Modified the log message in wallet_extract_owned_outputs to
append 'CONFIRMED' when it is called with a blockheight arg.
To make distinction between (1st call) when adding owned output to the
db and (2th call) when confirmed in block.

Add the change output to owned_txfilter so its entry in db will
get a confirmation_height when detected in a block by filter_block_txs

before this commit, after a 'withdraw' command, 'listfunds' would
not show our change outputs as confirmed

Modified the log message in wallet_extract_owned_outputs to
append 'CONFIRMED' when it is called with a blockheight arg.
To make distinction between (1st call) when adding owned output to the
db and (2th call) when confirmed in block.
@ghost
ghost requested a review from cdecker as a code owner February 1, 2019 10:03
@ghost

ghost commented Feb 1, 2019

Copy link
Copy Markdown
Author

To comment to myself: I am working/thinking about the semantics of commands like withdraw and fundchannel that try to broadcast and how to make it more user friendly in case of broadcast failure.

The withdraw command currently only adds/updates outputs in db when broadcast succeeds, but manual broadcasts are not detected.

My idea now is to handle withdraw and fundchannel in a similar way to take into account that users may manually rebroadcast failed tx and, in the meanwhile, prevent reusing outputs or perhaps even lock the wallet until bitcoin-cli problem is fixed. So we don't end up with stuck channels etc. that try to spend change outputs of failed broadcasts.

Will likely create separate (combined) PR for that and further discussion.

@cdecker cdecker left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ACK 0a445ed

Comment thread wallet/wallet.c
Comment thread wallet/wallet.c
@cdecker

cdecker commented Feb 4, 2019

Copy link
Copy Markdown
Member

I'll add the space issue into a cleanup commit 😉

@cdecker
cdecker merged commit 35545f7 into ElementsProject:master Feb 4, 2019
@ghost
ghost deleted the 31jan_withdraw_cmd_fix_confirmation branch February 4, 2019 15:48
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.

1 participant