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

improves the drain-accounts script so that it funds more accounts #1988

Closed
wants to merge 2 commits into from
Closed

improves the drain-accounts script so that it funds more accounts #1988

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2d49e13
improves the drain-accounts script so that it funds more accounts
cbeck88 May 17, 2022
99b2dd8
fix variable names, fix bug
cbeck88 May 17, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 15 additions & 10 deletions mobilecoind/strategies/drain-accounts.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,10 +59,10 @@ def parse_args() -> argparse.ArgumentParser:
type=int,
default=20,
help="Amount less than the balance that we attempt to send")
parser.add_argument("--max-accounts",
parser.add_argument("--num-src-accounts",
type=int,
default=-1,
help="Number of accounts to pull from the account keys / destination accounts keys folders")
default=0,
help="Number of accounts to pull from the source account keys dir (note: we always pull all the destination keys)")
parser.add_argument("--token-id",
type=int,
default=0,
Expand Down Expand Up @@ -141,20 +141,25 @@ def filename_key(filename):
for b58 in dest_b58addresses
]

# Go through each account and have all their friends transact to them
for i, (src_account, dest) in enumerate(zip(source_accounts, dest_addresses)):
# If we have already done max_accounts many accounts, then stop
if i == args.max_accounts:
break;
# Go through each destination account and pay it once from a source account,
# going round-robin through the source accounts (only first args.num_src_accounts source accounts)
num_dest = len(dest_addresses)
num_src = args.num_src_accounts or len(source_accounts)
for i, dest in enumerate(dest_addresses):
src_account = source_accounts[i % num_src]
Comment on lines +146 to +149
Copy link
Contributor

Choose a reason for hiding this comment

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

Re:

This is working but it is very slow, it seems like it's going to take an hour

In addition to Eran's comment of leveraging up to 16 outputs. Some of this could be done asyncronously - even though the code is fundamentally synchronous you can still use async collaborative multi-tasking with sync code with this: https://docs.python.org/3/library/asyncio-eventloop.html#asyncio.loop.run_in_executor

we could also actually mint to 500 accounts, but mobilecoind gets noticeably slower as we add more accounts to monitor,

At what point does that start happening? If just a few more source accounts doesn't slow it down. We could go through this with a queue of like 5 accounts sending & awaiting and probably have a significant speedup.

Copy link
Contributor

Choose a reason for hiding this comment

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

If it interests you - I can give that a shot.

Copy link
Contributor

Choose a reason for hiding this comment

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

This example seems sufficient and simpler: https://docs.python.org/3/library/concurrent.futures.html#threadpoolexecutor-example

Copy link
Contributor

Choose a reason for hiding this comment

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

Either way should achieve greater concurrency

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the problem is that, there is only one thread in mobilecoind, so even if we make more requests to it in parallel they are all going to be serialized

Copy link
Contributor

Choose a reason for hiding this comment

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

i think this line means there is exactly one thread in the grpcio thread-pool for handling rpc requests

Yeah that's their "completion queue" object which does indeed wait for calls to return before returning the data. @remoun since you're with gRPC internals - what do you think the consequences of extending the CQ thread count might be?

I don't think there's a set versioning for McD right like there is for full service? Exchanges track main? If that's the okay It might be possible to maybe create an unstable branch which mainly tracks main but has a few unstable, but extremely useful features we can use in cases like this. That's maybe a bit cursed of a strategy but would also open up a good deal of flexibility for us.

How much work would #4 be do you think? It may be something do-able for 1.3.0 - like we just go with Eran's suggestion for this round and get ahead with FOG distro for the next release/intermediate testing.

Copy link
Contributor

Choose a reason for hiding this comment

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

i think this line means there is exactly one thread in the grpcio thread-pool for handling rpc requests

Yeah that's their "completion queue" object which does indeed wait for calls to return before returning the data. @remoun since you're with gRPC internals - what do you think the consequences of extending the CQ thread count might be?

Like you mentioned, it creates additional gRPC worker threads. While that would definitely improve throughput, it could also reveal data races or other multi-threading issues, so we should approach that carefully and take the time to test it thoroughly.

I don't think there's a set versioning for McD right like there is for full service? Exchanges track main? If that's the okay It might be possible to maybe create an unstable branch which mainly tracks main but has a few unstable, but extremely useful features we can use in cases like this. That's maybe a bit cursed of a strategy but would also open up a good deal of flexibility for us.

I don't see how that branch strategy would work, while we develop against master. Am I missing something?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see how that branch strategy would work, while we develop against master. Am I missing something?

I'm not sure what version of MobilecoinD exchanges still using it use since it doesn't seemed to be versioned like Full Service. Maybe they just track the overall releases like 1.2.0/1.3.0 etc?

The branch strategy would be one alternative to alternative to enable potentially unsafe features we only need for testing. The branch would consistently track master, but be able to contain a few extra, potentially unsafe features we need for testing like increasing the cq count.

Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't seem like reasonable strategy to me. In my opinion, we either have faith in the code we build or we don't. Having a branch that has stuff in it that seems to work but we don't trust it enough to actually stand behind it will likely result in a stale branch that no-one remembers to update until they are forced to, and then they have to dig into whatever potential issues lives there. We should strive to to reduce the amounts of hacks and workarounds that we do, even for internal testing.

I think the main issue with increasing the RPC worker threads for mobilecoind would show up with the endpoints that do UTXO selection as that could result in the same UTXOs being selected for multiple transactions. For users that explicitly manage their UTXOs and feed in the ones they want to spend explicitly, I think it should be fine, but like @remoun said we should approach this carefully.

I think trying out the multiple-outputs txs as well as evolving fog-distro are viable paths forward. I do think that we should start treating fog-distro like a first class citizen in our codebase, since realistically if it breaks our ability to make progress slows down or halts.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd also vote for fog-distro as a first class citizen. Testing with mobilecoind has been a hacky (albeit necessary) process which has lead to "tradeoff hell" more than once it seems. Making decisions under release pressure about how to change mobilecoind doesn't lead to the best design/maintenance decisions for it.

Fog-distro updates promise to result in a much cleaner process and allow us to add testing features strictly in the domain of exercising integration tests.

What would we need to change about FOG distro to make it suit our needs?


wait_for_accounts_sync(stub, [src_account.monitor_id], 3)
# Get starting balance
resp = stub.GetBalance(
mobilecoind_api_pb2.GetBalanceRequest(monitor_id=src_account.monitor_id, token_id=args.token_id))
balance = resp.balance
logging.info("Starting balance for account %s : %s", i, resp)
logging.info("Token-id %s balance for account %s : %s", args.token_id, i, resp)

amount = balance - args.fee
# This source account will be chosen for this many future iterations of the loop
num_payments_remaining = int((num_dest - i) / num_src)
# Divide the remaining balance evenly across this payment and the remaining future payments
# but also subtract the fee we have to pay
amount = int(balance / (num_payments_remaining + 1)) - args.fee

# Create a pool of transfers to all other accounts
logging.info("Transferring %s to %s", amount, dest)
Expand Down