improves the drain-accounts script so that it funds more accounts #1988
Conversation
this was requested by the sdk teams to simplify testing we could also actually mint to 500 accounts, but mobilecoind gets noticeably slower as we add more accounts to monitor, so instead we went with this pay-round-robin approach
cbeck88
force-pushed
the
improve-drain-accounts
branch
from
ae54ed8
to
2d49e13
Compare
|
This is working but it is very slow, it seems like it's going to take an hour I wonder if there is some way to increase the parallelism here or something |
Maybe somehow leverage the fact that we can have up to 16 outputs? |
| 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] |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
If it interests you - I can give that a shot.
There was a problem hiding this comment.
This example seems sufficient and simpler: https://docs.python.org/3/library/concurrent.futures.html#threadpoolexecutor-example
There was a problem hiding this comment.
Either way should achieve greater concurrency
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
|
I think I'm going to close this in favor of: #2061 |
this was requested by the sdk teams to simplify testing
we could also actually mint to 500 accounts, but mobilecoind gets
noticeably slower as we add more accounts to monitor, so instead
we went with this pay-round-robin approach