-
Notifications
You must be signed in to change notification settings - Fork 102
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
Adding Moneypot simulation #15
base: master
Are you sure you want to change the base?
Conversation
The standard test error is some npm craziness standard/standard#949 works with yarn though :) I can fix the npm issue with an edited travis.yml; should I do that in this PR or separately |
let feeRate = 56 * 100 | ||
let results = [] | ||
|
||
let list = require('./moneypot-hotwallet.json') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we rename this to data
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to name it like this so more data can be added eventually
LGTM, will merge after nit then play with it locally |
ignore for now, I'll find out post-merge |
} | ||
let txo = { | ||
address: 'A', | ||
value: Math.abs(value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please don't merge this yet. There is a bug; because this has no script, simulation.js thinks this is a change output (if it's being sent), so all utxos are always returned back to the "wallet". solved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Frankly - I am a little stuck on the {script: {length}}
now.
It is used both in utxos and txos in simulation.js; however, it is also used in utils.outputbytes for both inputs and outputs! So both input script size and output script sizes are the same in the simulations, which makes the results wrong :)
solved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(OK, I thought coin selection simulation will be a quick task, and it takes me quite a while. :D Luckily, I have two different implementations, one here and one in scala, and I am looking at where they differ :D )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries.
As for the merge, I don't mind master being a development branch here.
Happy to keep development in the PR if you are.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer development in PRs too
Hm, I finally found out why I have different results from murch's simulation. It seems in his simulation, if a wallet/strategy cannot make a tx because it has too small "balance", the outgoing transaction is waiting until the "balance" is high enough. It's actually more complex - if he is comparing more wallets/strategies and any of them don't have enough money, all of them are waiting (that is, receiving new utxos) until all of them have enough balance. (And "enough balance" is actually "all outgoing outputs + some artificial constant." :)) In this repo, the transaction just won't happen (and is counted in the DNF percentage). So the ending "total costs" are not directly comparable. I kinda like the idea of "waiting until there is enough balance", if not exactly how the other repo is doing it, since that simulates user's behavior more. |
Agreed, DNF is still a possibility, but, certainly less likely. I wonder how the "wait" approach might impact results though, since you are allowing some algorithms to wait for more optimum UTXOs, which may disadvantage others unfairly. |
Depends on #18
...so that the results are the same as in the Murch's PDF :)
Seems like I misunderstood data in the scala repo; this is using the correct moneypot data
OK, I have added fixes for the script size, plus correct MP data, plus waiting for the queue (needs #18 first) I do this torrent of commits, since I finally made the two repos behave exactly the same to the satoshi, so I needed to make a lot of changes :) this should be all though! |
@karel-3d shall I rebase/merge this? |
What are the benefits of this PR? |
More data for the simulations. It's from an actual big hot wallet of some online betting game |
@karel-3d the data you bring in, probably needs to be attributed/linked back to https://github.com/Xekyo/CoinSelectionSimulator in some way? (@xekyo) |
Adding data from Moneypot simulation from Murch, from https://github.com/Xekyo/CoinSelectionSimulator
The data are described on pages 43/44 (in PDF, on pages 53/54) in the thesis http://murch.one/wp-content/uploads/2016/11/erhardt2016coinselection.pdf - this is the "derived analysis".
Note that some of the analysis are extremely slow; for my own experiments, I needed to comment out
bestof
since it took just too much time. I have also added time to the printed table; but I was not sure about the unit - since both simulations use the same "table" logic, but the time is different in orders of magnitude. (The same actually applies to the other values.)The table starts to also be so big it doesn't fit on my terminal anymore :(