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

New deposit ignored by yield-generator #1021

Closed
BitcoinWukong opened this issue Sep 18, 2021 · 13 comments · May be fixed by #1042
Closed

New deposit ignored by yield-generator #1021

BitcoinWukong opened this issue Sep 18, 2021 · 13 comments · May be fixed by #1042

Comments

@BitcoinWukong
Copy link
Contributor

BitcoinWukong commented Sep 18, 2021

While yield-generator is running, if the maker deposited new fund into the wallet, it will be ignored by yield-generator and will not be selected to participate the coinjoin process (even though it does recognize that a new UTXO has been added to the wallet). The yield-generator has to be restarted in order for it to make use of those new deposits.

Ideally, the yield-generator could recognize the new deposit and use it in the coinjoin process without the need of relaunching it.

@BitcoinWukong BitcoinWukong changed the title New deposit ignored by yield-generator [Bug] New deposit ignored by yield-generator Sep 21, 2021
@AdamISZ
Copy link
Member

AdamISZ commented Oct 11, 2021

There is a confirm as well as an unconfirm callback.

So, the way it currently works is, the confirm callback is inside the client_protocol module, which is the one responsible for telling the daemon (the 'network/messaging daemon' perhaps best to call it, the thing responsible for talking to other bots over message channels, but knows nothing about bitcoin keys etc.) what messages to send out. Currently the JMAnnounceOffers command is only sent out at the start, and in the unconfirm and confirm callbacks. I think we just need to have another callback in client_protocol that can be triggered by something coming from the transaction_monitor loop in the wallet service. (Notice the register_callbacks function in the WalletService, and its documentation; I've already found that useful in other contexts, e.g. adding notifications to the websocket in #996 ).

@AdamISZ
Copy link
Member

AdamISZ commented Oct 11, 2021

Or actually I think it's even simpler.

See here:

So basically we already register that function confirm_callback as a callback whenever any transaction is confirmed; it's just that it chooses to ignore it, if the transaction was not related to our ongoing coinjoin(s). So just ditch that requirement and have it reannounce on any tx confirmation :)

I mean there will be some details but probably that's basically it.

@AdamISZ
Copy link
Member

AdamISZ commented Oct 11, 2021

OK, still reading the code and I spoke a bit too soon there.
Because see what happens next in that function: it calls the yield generator's on_tx_confirmed with that txinfo assuming that it was a transaction related to an offer. That'll definitely screw up somewhere.

You need a new confirmed callback function. It'll take a bit more thinking (although I think it will end being a fairly small code delta, still).

@AdamISZ
Copy link
Member

AdamISZ commented Oct 11, 2021

Derp, obviously you'll need a new confirm_callback quite separate from the above; because currently the callback is only added once a coinjoin has been negotiated (so it wouldn't even work if the deposit came in when there was no coinjoin in flight). But the line

self.client.wallet_service.register_callbacks([self.confirm_callback],
txinfo, "confirmed")
and the docstring of register_callbacks probably gives the general idea.

@BitcoinWukong
Copy link
Contributor Author

I have tried to fix and stated by adding TX monitor lines that add new UTXO from wallet path dynamically while they are unconfirmed. The YG can then select from those new UTXO for next CJ he takes part of, once the newly UTXO got confirmed in block and without need for restart.

I would love to see this fix merged even if an updated offer isn't announced after the confirmation. It would be a pure improvement upon the current status quo.

@AdamISZ
Copy link
Member

AdamISZ commented Oct 12, 2021

While yield-generator is running, if the maker deposited new fund into the wallet, it will be ignored by yield-generator and will not be selected to participate the coinjoin process (even though it does recognize that a new UTXO has been added to the wallet). The yield-generator has to be restarted in order for it to make use of those new deposits.

This isn't correct. It will be used, but the new balance will only be added to the published offer after another coinjoin transaction completes. Technically the new funds could actually be used in a coinjoin.

(That's why I was mildly confused this was labelled 'bug'; it's something at least I've known for years and explained this point to people many times on IRC, I thought there was at least one issue opened about it before, but perhaps not, as I can't find one. So I didn't see it as quite so pressing, since it self corrects after a while. See

mix_balance = self.get_available_mixdepths()

which gets the current real time balance. It's just only called, currently, when a coinjoin transaction confirms (and also on unconfirm, i.e. it gets triggered twice for each coinjoin).)

AdamISZ added a commit that referenced this issue Oct 12, 2021
Fixes #1021. Also applies to withdrawals.
This commit adds a registration of a callback for
all transactions, which then follows up with a callback
on the confirmation event, after which we send the
AnnounceOffers message to the daemon (after modifying
our orders in line with current balance).
Minor changes: WalletService.active_txids is a set, not
a list, to avoid reannouncements, and the unused argument
to on_tx_confirmed is removed.
@AdamISZ
Copy link
Member

AdamISZ commented Oct 12, 2021

It's not particularly trivial in terms of how much code, albeit it is trivial conceptually.
I'll add some notes to the PR for anyone who wants to understand a bit better.

@BitcoinWukong
Copy link
Contributor Author

While yield-generator is running, if the maker deposited new fund into the wallet, it will be ignored by yield-generator and will not be selected to participate the coinjoin process (even though it does recognize that a new UTXO has been added to the wallet). The yield-generator has to be restarted in order for it to make use of those new deposits.

This isn't correct. It will be used, but the new balance will only be added to the published offer after another coinjoin transaction completes. Technically the new funds could actually be used in a coinjoin.

Interesting... So in my own experience, after a new deposit was confirmed, even though there were several CoinJoin transactions happened afterwards, none of them utilized the new deposit. And after I restarted the maker, the very next Coinjoin transaction immediately utilized that new deposit.

It had been the same case multiple times, and I'd learned to always restart the maker if I wanted the new deposit to be used.

It is possible that had I waited long enough, the new deposits might eventually got utilized. Even though that doesn't seem to be the case from my own experience, but I have only limited data.

@BitcoinWukong BitcoinWukong changed the title [Bug] New deposit ignored by yield-generator New deposit ignored by yield-generator Oct 13, 2021
@AdamISZ
Copy link
Member

AdamISZ commented Oct 13, 2021

I mean the best way to test is on regtest. I can only say it makes no sense to me, since the call to get_balance_by_mixdepth is at the time of creating a new announcement (create_my_orders), and then in subsequent coinjoins the filling of the offer with actual utxos does select_utxos (in YieldGeneratorBasic._get_order_inputs) and they are both querying the utxo database directly.

@AdamISZ
Copy link
Member

AdamISZ commented Jun 2, 2022

Interesting... So in my own experience, after a new deposit was confirmed, even though there were several CoinJoin transactions happened afterwards, none of them utilized the new deposit.

It's possible that this is a bug that was fixed by #1278 ? Of course the general topic in this thread is only addressed by #1042 or similar, but wondering about that specific report.

@BitcoinWukong
Copy link
Contributor Author

Yes, confirmed that with #1278 , this issue is now fixed.

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 a pull request may close this issue.

3 participants
@AdamISZ @BitcoinWukong and others