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.

Sign up for GitHub

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

Jump to bottom

[WIP] Spend unacked bsq #2244

Closed
wants to merge 6 commits into from
Closed

[WIP] Spend unacked bsq #2244

wants to merge 6 commits into from

Conversation

sqrrm
Copy link
Member

@sqrrm sqrrm commented Jan 11, 2019

This PR allows to spend pending BSQ. It doesn't check where the pending txs came from however so any incoming BSQ will be spendable immediately. It might be better to only allow to spend own unacked txs rather than all, not sure.

sqrrm added 6 commits January 10, 2019 14:30
@sqrrm
Initialize Set to avoid nullpointer access
83aa0d7
@sqrrm
Fix bisq-network#2123
09a0ec5
@sqrrm
Don't burn BSQ that were part of a failed issuance request
2c89c46
@sqrrm
Add extra info on unused BSQ addresses
d038fdf
@sqrrm
Add option to parse unconfirmed tx
644f559
@sqrrm
Allow spend of pending BSQ
b2978d3 
All pending txs are parsed by the TxParser but kept in UnconfirmedState.
The confirmed state and unconfirmed state are aggregated on demand when
getUnspentTxOutputMap is called.
@sqrrm sqrrm mentioned this pull request Jan 11, 2019
Closed
@sqrrm
Copy link
Member Author

sqrrm commented Jan 11, 2019

Depends on #2242, that should be merged first

@ManfredKarrer ManfredKarrer removed the request for review from ripcurlx January 11, 2019 14:20
@ManfredKarrer
Copy link
Contributor

Here a few general comments:

  • I would prefer to not add too much dependency from DAO to BitcoinJ. The btc package should ideally does not know about the DAO (I know there are some violations of that goal, but I prefer to not add new ones). So the TxParser should not be injected there. We could invert that so that BsqNode listens on BitcoinJ for new blocks and then passes those transactions to the parser.
  • The UnconfirmedState is not persisted. Could that mess up the state if you close the app and restart?
  • There is no guarantee of the order we receive the block from Bitcoin Core and the tx from BitcoinJ. It can be that we complete parsing but BitcoinJ has received the tx still as unconfirmed as well the other way round that BitcoinJ gets a tx which is condisdered confirmed but we have not got it from Bitcoin Core. Not sure if that can cause issues but I fear it can and it is difficult to test and simulate.
    Another approach might be to use Bitcoin Core to request the tx by getmempoolentry <txId> but of course that would only work for a full DAO node.

Lets discuss all that directly, it feels quite a complex and risky thing we want to achieve here.

@ManfredKarrer
Copy link
Contributor

Btw was looking into the other dao dependencies there. I think we should extract those to a custom class which is responsible to deal with both domains: bitcoinj and DAO.

@sqrrm sqrrm changed the title Spend unacked bsq [WIP] Spend unacked bsq Jan 15, 2019
@sqrrm sqrrm mentioned this pull request Feb 28, 2019
Closed
@ManfredKarrer
Copy link
Contributor

Superseded by #2482

@sqrrm sqrrm deleted the spend-unacked-bsq branch September 19, 2019 15:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ManfredKarrer ManfredKarrer Awaiting requested review from ManfredKarrer

Assignees
No one assigned
Labels
in:dao
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@sqrrm @ManfredKarrer