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

UTXO merging #1937

Merged
merged 6 commits into from
Aug 25, 2022
Merged

UTXO merging #1937

merged 6 commits into from
Aug 25, 2022

Conversation

smk762
Copy link
Collaborator

@smk762 smk762 commented Aug 15, 2022

Related to #1784

Currently this is only manually configurable by adding "utxo_merge": true to a coins config (e.g. in 0.5.6-coins-username.json)
It will only work for UTXO coins.

In future, we can allow this to be configured thru the gui settings.

@gcharang - hopefully this will make explaining how to recover an address from bulk mining utxos a little simpler to the non-technical users.

@smk762 smk762 requested review from a user, tonymorony, cipig, SirSevenG and Canialon August 15, 2022 08:01

if (coin_info.utxo_merge.value_or(false))
{
t_utxo_merge_params params{.merge_at = 300, .check_every = 60, .max_merge_at_once = 200};
Copy link
Member

Choose a reason for hiding this comment

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

we would have to test if it works with max_merge_at_once=200, i can imagine it would cause issues with some electrums, so maybe it's better to set max_merge_at_once to 100
check_every = 60 means it checks every minute... do we need to do this every minute? maybe better to do it every 10 minutes or so
any maybe also better to start earlier, not wait for 300 utxos
what do you guys think?

Copy link
Collaborator Author

@smk762 smk762 Aug 15, 2022

Choose a reason for hiding this comment

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

I"m happy to tweak the default params - just used those to drop around 3500 RICK utxos into 130 or so relatively quickly.

There is a user with 23000 utxos from mining tokel, which will take a while to consolidate either way.
23000 @ 200 utxos every 10 mins = around 19hrs to consolidate.

From memory, up to 800 or so utxos can be merged in a tx.
I"m not sure at what number utxos electrums begin to complain about "payload too large"
I think every 5 min is a balance between not too obtrusive, and not too slow. Every minute could cause clogging issues when blocks are slow.

Copy link
Member

Choose a reason for hiding this comment

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

OK, sounds like it will work... there is a difference though, RICK (and all other coins run by me) electrums have MAX_SEND = 2000000 set, the default is 1M

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

updated to use these values:
merge_at = 250
check_every = 300
max_merge_at_once = 125

@@ -38,7 +38,9 @@ namespace mm2::api
if (cfg.address_format.has_value()) {
j["address_format"] = cfg.address_format.value();
}
//SPDLOG_INFO("electrum: {}", j.dump());
if (cfg.merge_params.has_value()) {
Copy link

Choose a reason for hiding this comment

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

Allman style indentation

src/core/atomicdex/services/mm2/mm2.service.cpp Outdated Show resolved Hide resolved
@smk762 smk762 requested review from a user and cipig August 16, 2022 07:29
@tonymorony
Copy link

@smk762 it would be awesome do add text notification about merging event so user will see not just a notification about transaction (what might be scary/suspicious since user not initiated it) but also a message why this transaction happened

@smk762
Copy link
Collaborator Author

smk762 commented Aug 25, 2022

As it is right now, this merge wont happen unless user manually edits their coins file (e.g. at direction of support).
If we set it by default, I agree a message would be good - and a toggle setting would be better (we could add the warning when it toggles on). Going a step further, we could detect when a wallet has too many utxos and send a notification which links them to the toggle.

TKL and MCL are the most common coins with this issue, so if adding default without toggle, I would restrict it to these two. I'm wary to add it to all utxo coins - even tho chances are small someone gets automerged on BTC, I can see them complaining about fees.

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.

4 participants