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

Validate deposit prior adding to the wallet #1040

Merged
merged 2 commits into from
May 3, 2019

Conversation

frolosofsky
Copy link
Member

At the moment, when a user prepares deposit, the node creates a transaction and adds it to the
wallet. It, in turn, tries to accept it to the memory pool and could fail when the deposit is
invalid (for example validator is blacklisted). That leads to disappearing of the deposit from the
balance.

This commit fixes that by validating deposit before adding transaction to the wallet.

Fixes #996.

At the moment, when a user prepares deposit, the node creates a transaction and adds it to the
wallet.  It, in turn, tries to accept it to the memory pool and could fail when the deposit is
invalid (for example validator is blacklisted). That leads to disappearing of the deposit from the
balance.

This commit fixes that by validating deposit before adding transaction to the wallet.

Fixes dtr-org#996.

Signed-off-by: Stanislav Frolov <stanislav@thirdhash.com>
@frolosofsky frolosofsky requested review from scravy, kostyantyn, AM5800 and a team May 2, 2019 10:10
@frolosofsky frolosofsky self-assigned this May 2, 2019
@frolosofsky frolosofsky added bug A problem of existing functionality wallet labels May 2, 2019
@frolosofsky frolosofsky added this to the 0.2 milestone May 2, 2019
Copy link
Member

@kostyantyn kostyantyn left a comment

Choose a reason for hiding this comment

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

utACK 2ee0f9c

@scravy
Copy link
Member

scravy commented May 2, 2019

This code change looks alright to me, but I am wondering:

The whole balance thing is a tricky thing, once I spend some UTXO they are deduced from my balance. Yet they might not be included in the blockchain actually yet, so they are not really spend. So I am wondering it there should be some sort of "in flight balance", like something which one owns but which has not been included in the blockchain yet.

The thoughts that led me here are: It strikes me as odd that anything which has not been confirmed yet (included in the blockchain, possibly even finalized) alters any numbers; but of course it makes sense for the wallet as a user should not create a transaction form balance which she already attempted to spend, true. So maybe some "there but not really there" would be appropriate.

Copy link
Member

@Gnappuraz Gnappuraz left a comment

Choose a reason for hiding this comment

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

utACK 2ee0f9c

@Gnappuraz
Copy link
Member

Gnappuraz commented May 3, 2019

This code change looks alright to me, but I am wondering:

The whole balance thing is a tricky thing, once I spend some UTXO they are deduced from my balance. Yet they might not be included in the blockchain actually yet, so they are not really spend. So I am wondering it there should be some sort of "in flight balance", like something which one owns but which has not been included in the blockchain yet.

The thoughts that led me here are: It strikes me as odd that anything which has not been confirmed yet (included in the blockchain, possibly even finalized) alters any numbers; but of course it makes sense for the wallet as a user should not create a transaction form balance which she already attempted to spend, true. So maybe some "there but not really there" would be appropriate.

There is a getunconfirmedbalance rpc that I think does what you ask, returns the balance that is coming from txs with 0 confs or in the mempool.

@frolosofsky frolosofsky merged commit 699d618 into dtr-org:master May 3, 2019
@frolosofsky frolosofsky deleted the salvage-deposit branch May 3, 2019 11:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A problem of existing functionality wallet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Balance disappears when sending deposit to non whitelisted pubkey
4 participants