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

Adding Moneypot simulation #15

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Adding Moneypot simulation #15

wants to merge 8 commits into from

Conversation

karelbilek
Copy link
Contributor

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 :(

@karelbilek
Copy link
Contributor Author

karelbilek commented Jul 12, 2017

The standard test error is some npm craziness

standard/standard#949
npm/npm#17717

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')
Copy link
Contributor

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?

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 wanted to name it like this so more data can be added eventually

@dcousens
Copy link
Contributor

LGTM, will merge after nit then play with it locally

@dcousens
Copy link
Contributor

should I do that in this PR or separately

ignore for now, I'll find out post-merge

}
let txo = {
address: 'A',
value: Math.abs(value)
Copy link
Contributor Author

@karelbilek karelbilek Jul 14, 2017

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

Copy link
Contributor Author

@karelbilek karelbilek Jul 14, 2017

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

Copy link
Contributor Author

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 )

Copy link
Contributor

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.

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 prefer development in PRs too

@karelbilek
Copy link
Contributor Author

karelbilek commented Jul 15, 2017

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.

@dcousens
Copy link
Contributor

dcousens commented Jul 16, 2017

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.

...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
@karelbilek
Copy link
Contributor Author

karelbilek commented Jul 17, 2017

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!

@dcousens
Copy link
Contributor

dcousens commented Sep 7, 2018

@karel-3d shall I rebase/merge this?

@dcousens
Copy link
Contributor

dcousens commented Sep 7, 2018

What are the benefits of this PR?

@karelbilek
Copy link
Contributor Author

More data for the simulations. It's from an actual big hot wallet of some online betting game

@dcousens
Copy link
Contributor

dcousens commented Sep 7, 2018

@karel-3d the data you bring in, probably needs to be attributed/linked back to https://github.com/Xekyo/CoinSelectionSimulator in some way? (@xekyo)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants