< phantomcircuit>
is there some reason im not aware of that the wallet functions aren't acquiring the lock themselves?
< phantomcircuit>
instead of relying on the caller to have acquired the lock?
< phantomcircuit>
luke-jr: ^
< phantomcircuit>
it *should* be safe for me to simpyl add LOCK(cs_wallet); calls to all methods which assume a lock? (which is basically all of them)
< luke-jr>
phantomcircuit: my guess would be so they could be used recursively
< Atomic_zEU0b>
hey
< phantomcircuit>
gmaxwell: the LOCK construct is recursive right?
< luke-jr>
pretty sure it isn't. or at least can't be relied on to be.
< wumpus>
phantomcircuit: yes, the default mutex used in bitcoin core is a recursive one
< wumpus>
we'd rather have it otherwise, there are a few solid software engineering reasons to avoid recursive mutexes, but that's part of our inheritance and it's terribly difficult to change (and be sure of the result)
< wumpus>
note that new code in general tries to use normal, single-use mutexes
< luke-jr>
wumpus: pretty sure I hit a deadlock from locking the same mutex twice in a thread, fwiw
< wumpus>
that is very possible, but not with the main, wallet locks and such as they are recursive mutexes
< wumpus>
you can try locking them 100 times in a loop in the same thread, they just keep track of a counter, so as long as you unlock the same number of times you will be fine
< jonasschnelli>
I take the time and height directly from pindexBestHeader
< wumpus>
why does github review suck? Ithink combining multple comments into one review is pretty useful
< jonasschnelli>
(each time the singal fires)
< MarcoFalke>
It shows the changes as approved by me, even though new commits were pushed
< sipa>
wumpus: agree
< sipa>
MarcoFalke: it does tell you what changes the review was for, no?
< MarcoFalke>
wumpus: But it does not hide the comments when they are solved
< jonasschnelli>
MarcoFalke: but the "timeline" implies that you have reviewd on older state?
< jonasschnelli>
thats indeed bad
< jonasschnelli>
Do they go away if a commit gets added?
< MarcoFalke>
Yes "timeline". But it is possible to push commits such that they appear in the past
< jonasschnelli>
I really dislike the non-removing comments on the source-code...
< wumpus>
MarcoFalke: that's the same with an ACK, really, the reason for speicfying the commit it was against is to be able to check what exactly you reviewed later, not to de-approve on changes
< jonasschnelli>
They stay also after a force push or commit that removes that code part
< wumpus>
and you can still mention the commit in review comments if you want
< wumpus>
jonasschnelli: that's slightly annoying, yes, otoh it makes the review history clearer
< wumpus>
so people don't, say, suggest the opposite thing when something has been discussed already
< wumpus>
hiding can be pretty annoying in that regard
< jonasschnelli>
Yes. That indeed true..
< sipa>
i hate it when review commemts automatically disappear
< sipa>
i want to judge whether a comment still applies
< wumpus>
yes
< jonasschnelli>
I just fear very large pages with endless amount of comments/reviews
< wumpus>
indeed, github's judgement is not very good there
< sipa>
you can explicitly mark as resolved, no?
< jonasschnelli>
But right... it makes stuff more obvious for new reviewers
< jonasschnelli>
I guess no
< luke-jr>
sipa: that'd be nice, but I've never seen it
< wumpus>
it just looks if the line changes and then considered the comment invalid
< jonasschnelli>
wumpus: I think for review-comments, they stay even if the lines gets removed afterwards
< sipa>
and it used to differ based on whether it was a commit comment or pr comment
< MarcoFalke>
So use the review thing when you want the comments to stay. And use the old comment thing when the feedback should vanish after being addressed.
< luke-jr>
MarcoFalke: and then GitHub will change the behaviour under you :P
< sipa>
MarcoFalke: just a rebase can make comments disappear
< MarcoFalke>
Well, I use comments to indicate merge conflicts sometimes. I expect them to disappear after
< wumpus>
MarcoFalke: you can still add 'single comments'
< wumpus>
I'd expect them to still be treated the same as before on changes, unsure though
< wumpus>
'disappear' is not the right word, I think they never fully disappear, just get collapsed?
< wumpus>
'this comment is on a previous version of ...'
< MarcoFalke>
Jup, single comments are the old fashioned comments
< MarcoFalke>
wumpus: Should I add the DEFAULT_DISABLEWALLET=false in this pull?
< wumpus>
MarcoFalke: would be nice to do that in one go, yes
< jonasschnelli>
I think we need to go in small step PRs
< luke-jr>
ok, I'll open a new PR with just some refactoring
< MarcoFalke>
Also, I like the approach of defaultWallet()
< luke-jr>
unless you think I should do each refactor separately.. ._.
< wumpus>
I mean there's obviously a compromise here, try to change the world in one PR and no one has the energy to review it, change too little and it's death by a thousand cuts as people don't see the story anymore
< jonasschnelli>
Luke-Jr: Yes. And sorry for "shooting" in the same direction with https://github.com/bitcoin/bitcoin/pull/8764 I like your mutliwallet PR and really like to boost getting this in
< wumpus>
lol I had an adverse reaction on seeing the word 'boost' even though it's used in a completely different context
< luke-jr>
XD
< jonasschnelli>
hehe
< * wumpus>
reads too much code
< jonasschnelli>
Luke-Jr: What do you think about exposing the addWallet stuff to RPC?
< jonasschnelli>
the -wallet= approach of adding wallets is kinda limited on runtime
< luke-jr>
jonasschnelli: intentionally so
< wumpus>
jonasschnelli: can be sone later
< luke-jr>
for the "too much in one PR" reason
< jonasschnelli>
Yes. Thats true
< wumpus>
runtime wallet loading and unloading is super-spiffy,but harder to get right
< luke-jr>
changing wallets at runtime makes this a lot more complex
< wumpus>
yes,that
< jonasschnelli>
Okay. I see. Agree on that.
< luke-jr>
(especially closing)
< wumpus>
same as with the wallet HD support, it's better to do small (but significant) steps at a time
< jonasschnelli>
Luke-Jr: In my tests, the Qt drop-down stays there even with a single wallet
< luke-jr>
jonasschnelli: hmm, that's a bug then
< luke-jr>
IMO
< luke-jr>
(at least in the current stage, I don't think normal users should be exposed to the feature)
< luke-jr>
oh, the *debug* dropdown should stay with only one wallet though
< luke-jr>
because it has a (none) option
< jonasschnelli>
Okay. I'll test some more...
< luke-jr>
actually tempting to make that the default, so users have yet one more step to compromise their wallet..
< wumpus>
how does this pushing-to-others-branches work? say I want to remove the double space in #8769 without that user openeing yet anouther pull request because he can't squash
< MarcoFalke>
jup
< luke-jr>
wumpus: never tried, but I speculate you'd do something like the origin-pull remote trick
< MarcoFalke>
I tried it
< wumpus>
"Add more commits by pushing to the patch-4 branch on unsystemizer/bitcoin." ah
< wumpus>
let's just try that
< MarcoFalke>
git add remote unsys ...
< luke-jr>
oh, it actually gives you access to his personal repo? :o
< MarcoFalke>
git push unsys patch-4 -f
< MarcoFalke>
luke-jr: Only this branch, IIRC
< wumpus>
MarcoFalke: yes I was thining to difficult, I thought I had to push to some special pulls branch on bitcoin/bitcoin
< luke-jr>
what happens if I open a PR for one of MarcoFalke's branches? <.<
< MarcoFalke>
Hmm, good question
< luke-jr>
MarcoFalke: got something to PR I can try it with? :D
< MarcoFalke>
One sec
< luke-jr>
dad932c needs to be fixed, if you want something trivial
< wumpus>
"git push -f git@github.com:unsystemizer/bitcoin.git HEAD:patch-4" seems to have worked
< wumpus>
feels a bit like intruding to push to someone elses repository, but okay, it's very practical in this case
< GitHub92>
[bitcoin] luke-jr opened pull request #8771: CONTRIBUTING: Mention not to open several pulls (master...Mf1609-ContributeDoc) https://github.com/bitcoin/bitcoin/pull/8771
< GitHub137>
[bitcoin] luke-jr opened pull request #8773: Trivial Bugfix: doc/gitian-building.md: Link to release-process needs to be updated (master...bugfix_link_20160921) https://github.com/bitcoin/bitcoin/pull/8773
< MarcoFalke>
wumpus: Can I get permission to merge doc changes (comments, markdown, ...)?
< MarcoFalke>
Otherwise I will just ping you when I feel soemthing trivial can be merged
< GitHub124>
[bitcoin] luke-jr opened pull request #8774: Qt refactors to better abstract wallet access (master...multiwallet_prefactor_qt) https://github.com/bitcoin/bitcoin/pull/8774
< GitHub30>
[bitcoin] luke-jr opened pull request #8775: RPC refactoring: Never access wallet directly, only via new CRPCRequestInfo (master...multiwallet_prefactor_rpc) https://github.com/bitcoin/bitcoin/pull/8775
< GitHub181>
[bitcoin] luke-jr opened pull request #8776: Wallet refactoring leading up to multiwallet (master...multiwallet_prefactor_wallet) https://github.com/bitcoin/bitcoin/pull/8776
< luke-jr>
there we go, I think that's as far as I can go with split PRs until they get merged :p
< GitHub109>
bitcoin/master 6f933c6 Luke Dashjr: Trivial Bugfix: doc/gitian-building.md: Link to release-process needs to be updated...
< GitHub109>
bitcoin/master 52b5a87 MarcoFalke: Merge #8773: Trivial Bugfix: doc/gitian-building.md: Link to release-process needs to be updated...
< GitHub143>
[bitcoin] MarcoFalke closed pull request #8773: Trivial Bugfix: doc/gitian-building.md: Link to release-process needs to be updated (master...bugfix_link_20160921) https://github.com/bitcoin/bitcoin/pull/8773
< wumpus>
I think it's even more important than multiwallet
< wumpus>
well it's pretty much orthagonal but okay
< wumpus>
as we can't get around allowing users to import things into the wallet, finally having a consistent, one-stop call to import things into the wallet would help
< sipa>
wumpus: thabks for the reminder, eill review
< sipa>
*will
< sipa>
*thanks
< wumpus>
thanks
< MarcoFalke>
Sry, I think qt no longer compiles on master.
< sipa>
uh-oh
< MarcoFalke>
Forgot to #include wallet.h
< MarcoFalke>
the "introduce DEFAULT_DISABLE_WALLET" is more involved
< MarcoFalke>
in qt
< MarcoFalke>
than search and replace
< MarcoFalke>
might want to properly fix it, now that multiwallet is the goal
< wumpus>
let's first get it to compile again
< sipa>
*commits a #if 0 .. #endif around all .cpp files* - what did you do?! - it compiles again!
< wumpus>
hehe
< wumpus>
it's just that 'properly fix it' sounds like something that takes significant time, and I'm held up by this, maybe we should just revert that commit for now? @MarcoFalke
< wumpus>
then do it the proper way some other time
< wumpus>
re-do
< MarcoFalke>
wumpus: Will create a pull
< wumpus>
but if it involves multiwallet changes may be better to revert the current naive commit for now?
< MarcoFalke>
should work without multiwallet changes
< MarcoFalke>
Is this "Method names start with upper case" a style thing?
< wumpus>
e.g. just fa58edb not the other commitin that pull
< wumpus>
MarcoFalke: dunno, seems another satoshi-ism
< MarcoFalke>
I think we have half of the names start upper case and the other half don't
< wumpus>
qt code uses lowerCamelCase at least
< MarcoFalke>
Never sure what to use in fresh code
< MarcoFalke>
ok
< wumpus>
nah if you sak me for something new and independent I'd suggest using lowerCamelCase, that's a more common C++ naming convention
< wumpus>
if you change existing code just go with the style of that code
< sipa>
hmm, nearly all of the existing core code uses capitalized class and function names
< sipa>
so i'd rather not introduce something else
< sipa>
the developer notes used to mention this
< wumpus>
I really have no strong opinion on it
< wumpus>
I'm fine with either, but don't mix them in one class/unit
< wumpus>
especially with code I may use for other projects (such as the locked memory manager) I really prefer not to use this weird method capitalization style
< wumpus>
even c++11 has some assumptions about method and function names starting with lower-case, e.g. range-base for loops expect begin/end() not Begin/End()
< wumpus>
capitalizing class names, on the other hand, makes sense to keep them apart
< sipa>
all stl functions are lower_separated_by_underscore()
< wumpus>
yes, that's a valid choice too
< wumpus>
snake_case :)
< sipa>
i perdonally find lowerCaseFollowedByUpper very strange, but it's all highly subjective
< wumpus>
many projects use that for variables and member variables, that's reasonable sane
< wumpus>
for method names too, but using it for everything including class names well it's consistent but makes it hard to tell things apart
< wumpus>
unless you have an editor such as Qt Creator which highlights class names differently from methods differently from class variables, but I don't have that luxury since I switched to vim
< wumpus>
having a separate naming convention for classes also avoids creating a class with the same name as a function
< wumpus>
in any case I have no problems with stl-style either, except that it reminds me of boost :-)
< sipa>
wumpus: i guess i'm biased because i learned c++ from bitcoin's codebase :)
< sipa>
well, i knew c++ from university, but that was purely theory without ajy software development experience
< wumpus>
yes, that can twist your mind :)
< wumpus>
MarcoFalke: I'm confused by #8777, why do we have a wallet model if the wallet is disabled
< wumpus>
did I really make it that way?
< MarcoFalke>
My method is static
< wumpus>
oh, okay, yes, that can work
< MarcoFalke>
I think there is no walletModel in case of -disablewallet=1
< wumpus>
static methods cover no instances, so can apply to all instances
< wumpus>
yes
< wumpus>
phew
< wumpus>
sipa: I think you learned c++ very well considering what you learned it from. Then again, I learned c++ from the MSVC6 manual and MS MFC stuff, so my initial influence is ever so malformed too.
< sipa>
wumpus: i did know C pretty well beforehand, maybe that helped :)
< sipa>
also, satoshi's c++ had a weird style, but it also did many things well, i think - hardly any manual memory management, no overuse of class hierarchies, some templating but only where it really avoids a ton of code, ...
< wumpus>
yes that's true, it has its positive sides. It's just that he was bad at isolating aspects from each other, and code organization.
< MarcoFalke>
So let's fetch some tea until travis clears the backlog
< sipa>
wumpus: /me still recalls the call to mark a key as used from inside the script validation code, wallet being in main.cpp, and direct calls into the wx code from inside block validation :)
< wumpus>
sipa: hah!
< wumpus>
MarcoFalke: we could cancel all the preceeding builds as they're going to fail anyway
< MarcoFalke>
done. Should pick up the pull now
< MarcoFalke>
reminds me that no entry in the travis matrix has qt builds enabled
< MarcoFalke>
IIRC osx cross build does it (silently)
< wumpus>
that was disabled for *somereason cfields probably knows*
< MarcoFalke>
timeouts?
< wumpus>
possibly timeout-related back when there were problems with caching after the travis trusty switch
< wumpus>
yes
< MarcoFalke>
Also, I thought about switching to docker within travis, but then the travis script is already complicated and it won't get better/easier, as there are no dockerfiles for native osx and native win
< wumpus>
yea docker solves a problem we don't have, here
< MarcoFalke>
Was on an unrelated note ;)
< MarcoFalke>
(travis has no plans to switch to xenial soon)
< wumpus>
testing native osx/win would be interesting, but I guess that runs into license issues
< MarcoFalke>
Oh, travis has native osx?
< wumpus>
and in the case of windows multi-stage issues: we want to build on trusty, then run the tests on an actualwindows machine
< wumpus>
though WINE is really convenient here and I have never seen any issues where wine didn't detect a problem that real windows would. WINE seems to be *stricter* in API correctness
< MarcoFalke>
(Right now we are bound to whatever ubuntu 14.04 provides us. Though, deterministic builds of the toolchain would help to untangle this)
< wumpus>
switching to xenial would be a disaster right now, esp for windows build
< wumpus>
I should likely put https://github.com/bitcoin/bitcoin/pull/8249 on ice; having less effective ASLR is great compared to the alternative of crashing with stack protectors :)
< MarcoFalke>
No one detected this. We should have run tests on xenial at least for the release
< MarcoFalke>
But then it is unfeasible to try every os and combination of toolchains
< wumpus>
well this is a cross build issue that didn't get detected
< wumpus>
we can't test every toolchain in existence
< wumpus>
right
< wumpus>
unfortunately, there are plenty of mingw toolchains that create absolutely broken windows code
< MarcoFalke>
So cfields work sounds like the right step
< wumpus>
we used to have that issue, and now it's back
< MarcoFalke>
Looks like travis likes my pull.
< * MarcoFalke>
off
< wumpus>
I wonder if switching to clang would help there. From what I've heard from some infosec related people the clang windows support is getting pretty good, and MSFT is even contributing to it. And Mozilla uses it to build Rust binaries (unsure if they use it for building firfox for windows)
< wumpus>
thanks ,going to merge it
< wumpus>
cfields: ^^ re clang and windows, should we consider that?
< wumpus>
would be worth an experiment at least I guess
< GitHub78>
[bitcoin] MarcoFalke closed pull request #8536: [qa] Adjust poll interval for micro-optimization of run time (master...Mf1608-qaOptSync) https://github.com/bitcoin/bitcoin/pull/8536
< jonasschnelli>
wumpus: Yes. Your listed... need to remove the *_tmp.html files!
< jonasschnelli>
I updated the page right after you submitted your form. :)
< jonasschnelli>
Ah. instagibbs just submitted the form as well...
< instagibbs>
i saw it being discussed here
< * jonasschnelli>
didn't know that wumpus is in the plubming business. .. :)
< jonasschnelli>
*plumbing
< MarcoFalke>
jonasschnelli: I think you forgot to merge the overlay pull this morning :)
< jonasschnelli>
MarcoFalke: I was hoping someone did another review,.. but will merge in the next couple of hours.
< jonasschnelli>
*someone will do
< BlueMatt>
luke-jr: they seem to have partially fixed the email issues
< BlueMatt>
github, that is
< BlueMatt>
as of yesterday i was still getting duplicate emails from them, but at least they're properly threaded
< jonasschnelli>
BlueMatt: while your here.. :), do you use the contrib/debian packaging dir for your bitcoin PPA?
< BlueMatt>
i forked off of it a while ago, but anything something gets updated in contrib/debian i re-sync it
< BlueMatt>
iirc the only real difference is package description (the new one is pretty terrible, iirc) and changelog
< BlueMatt>
new as of like 3 versions ago, that is
< jonasschnelli>
BlueMatt: I guess we should add zmq to the dependencies
< jonasschnelli>
which probably should result compiling with zmq
< BlueMatt>
oh, thats another difference - upnp is not built-in
< BlueMatt>
because of the security issues a while back, and i never re-added it
< jonasschnelli>
otherwise our "official" binary comes with zmq while the PPA does not
< BlueMatt>
but no one has complained, so whatever
< BlueMatt>
yea, could add zmq, though also security-swiss-cheese
< BlueMatt>
but maybe i should....
< jonasschnelli>
I would argue that if you like to use ZMQ, you should self-compile... but, yeah...
< BlueMatt>
i mean ive never heard any complaints about that or upnp
< wumpus>
zmq is only used for notification, and disabled by default
< BlueMatt>
(not that that indicates much, people like to keep complaints to themselves, mostly)
< BlueMatt>
wumpus: I'm aware, but so is upnp in bitcoind, and its still gross :p
< BlueMatt>
though i did actually get like 2/3 emails about the 12.04 empty-package update, which i found impressive
< wumpus>
yes, it's up to you as maintainer, please do explain that in the issue
< BlueMatt>
hmm? was an issue opened for this? sorry, I havent checked github emails in a day, I'll get caught up on github when I'm done with breakfast
< wumpus>
people really shouldn't complain to us about the PPAs, though i guess the ppa doesn't have its own issue tracker
< wumpus>
yes there's an issue, let em see
< sipa>
BlueMatt: a few people in #bitcoin were confused by bitcoind disappearing
< jonasschnelli>
we have contrib/debian though
< BlueMatt>
oh, well if someone complained I can add it
< BlueMatt>
sipa: yea, if i get emails then #bitcoin is generally guaranteed to complain
< BlueMatt>
not much i could do, though :/
< sipa>
BlueMatt: maybe a #!/bin/sh echo "The Bitcoin Core PPA no longer supports Ubuntu 12.04" would be better than just empty package?
< BlueMatt>
i was later informed that you can make the updater pop up the package changelog during the update process so people have to read it, though i assume the gui tools might hide it
< wumpus>
BlueMatt: yes, I've seen that only once, the GUI tools do show it
< wumpus>
it's meant for really critical notifications regarding a package, I guess this qualifies
< BlueMatt>
indeed
< wumpus>
btw re: github mails I found out that it's possible to filter mails where you're personally notified from the rest of the bulk by filtering for "to:mention@noreply.github.com", may be useful
< * BlueMatt>
ponders maildrop rule therefor
< wumpus>
I started receiving so many github mails that that's the only ones I read at at the moment
< wumpus>
(of course I tend to read the rest, but on github.com, not in my mailbox)
< wumpus>
in any case please comment on #8759. I don't mind personally whether you want to support zmq or not, but the decision should be documented
< wumpus>
jonasschnelli: at least the manpages will be gone from there now
< BlueMatt>
wumpus: yep, still eating breakfast, give me a few :p
< wumpus>
BlueMatt: yes I don't mean to hurry :p
< wumpus>
re: the new github review system, is it possible to un-approve a pull?
< xinxi>
Hi guys, are any documents on how to add a new type of address?
< xinxi>
For example, if I want to use public key directly as the address using of using hash160, what should I do given that we don't care about quantum resistance?
< instagibbs>
murch: I'd be interested to see what effect, if any, witness discounting would have on effective paid fees per algorithm
< murch>
instagibbs: Lemme have a look at the results
< murch>
instagibbs: Fees are almost exactly halved through the bench.
< murch>
instagibbs: I've uploaded the whole output table here ^
< sipa>
murch: do any of the algorithms behave significantly better/worse (in terms of average number of transaction inputs) with and without segwit?
< sipa>
or number of utxos held
< murch>
sipa: Good question. Actually, Core seems to have a significantly increased average UtxoPool size. With P2WPKH it's 70% bigger (×1.7). Only Core significantly changes though.
< murch>
Input set size has also a much bigger deviation for Core under P2WPKH. I'm weirded out by that though. I'll have to check whether that is reproducable.
< instagibbs>
murch: are total fees in satoshis or?
< murch>
instagibbs: Yes.
< murch>
instagibbs: All bitcoin values are in satoshi.
< skyraider>
hi, i'm getting some odd bitcoind errors on startup - the tor disconnect callback seems to be firing, but i have nothing about tor in my config file
< sipa>
it's enabled by default, but doesn't do anything if you don't have tor running
< skyraider>
sipa: ah, so it's harmless?
< sipa>
yes
< skyraider>
thanks
< sipa>
that PR just prevents it from spamming your debug.log all the time
< sipa>
now it just mentions it onc
< sipa>
*once
< sipa>
you can disable it by setting listenonion=0 in bitcoin.conf
< sipa>
or running with -nolistenonion
< skyraider>
hmm, i received several warnings... 6, it seems
< sipa>
that's unexpected
< skyraider>
listenonion=0 is effective, though. no warning after applying that config setting.
< gmaxwell>
murch: is your 'corewallet' with or without the extra input removal?
< murch>
gmaxwell: Without pruning
< murch>
i.e. extra input removal
< gmaxwell>
okay
< murch>
gmaxwell: That's actually the only difference between BreadWallet and Mycelium: Both do FIFO and Mycelium additionally minimizes the input set by removing extra inputs. As you see, it has a huge impact on the mean Utxo set size.
< gmaxwell>
how do the wallets doing fifo handle it if they get dustspammed? (someone pays them more tiny inputs than they can fit in a single txn?)
< BlueMatt>
murch: in your analysis, I'm assuming AndroidWallet is just the old bitcoinj code I wrote many years ago and not something AndroidWallet specific?
< murch>
BlueMatt: Yes, it's the coin selector from BitcoinJ.
< gmaxwell>
BlueMatt: the paper says it uses "value * inputAge"
< BlueMatt>
gmaxwell: it was written when using priority as selection wasnt insane :p
< BlueMatt>
murch: is your "fee spent" thing just tx size * fee something, or does it actually analyze the code's fee selection behavior?
< BlueMatt>
murch: have you tested the bitcoinj thing with a coinselector which does something smarter (like picking smallest-bigger-than-desired) even if its dead-simple?
< murch>
gmaxwell: In their selector I didn't see any special checks for small inputs, they may have checks in their utxo pool management though. I didn't check that.
< murch>
BlueMatt: It uses a flat fee of 10 satoshi per byte.
< BlueMatt>
murch: ok, cool, yea, so it generates the smallest total tx bytes
< murch>
BlueMatt: I added block height to the UTXO in order to give the BitcoinJ coin selector a better chance (instead of just selecting the largest always). My time model is very simple though and just jumps Gaussian time steps between transactions.
< murch>
BlueMatt: Unfortunately, my scenario data does not contain actual time information.
< BlueMatt>
murch: i mean the bitcoinj thing is now braindead as fuck...it made sense way back when it was written but the fact that it hasnt been updated since is surpsiing to me, because thats insane
< murch>
BlueMatt: I have not checked just selecting "smallest bigger" yet. I just replicated what I found in the code.
< gmaxwell>
BlueMatt: you mean making the final utxo set 126 times larger than bitcoin core isn't good?
< BlueMatt>
murch: wait, did you take the like overcomplicated thing in the wallet that tries to figure out if change is reasonable, or just do the priority-based selection
< murch>
BlueMatt: Perhaps there is a misunderstanding here: I have just replicated the coin selection behavior. I have also replicated fee estimation to the point that some wallets will always assume a change output and pay for it. I have not replicated how fee levels are estimated for each wallet.
< BlueMatt>
gmaxwell: i mean now its essentially just randomly selecting shit...like ignore that its making the utxo set larger its just like selecting things almost randomly because it hasnt been updated to realize that priority hasnt been a thing for a year or two now
< BlueMatt>
murch: did you emulate public FeeCalculation calculateFee(SendRequest req, Coin value, List<TransactionInput> originalInputs,
< BlueMatt>
I mean because that is, essentially, the actual coin selection algorithm, not just CoinSelector
< murch>
BlueMatt: Having a look, sorry it's been a few weeks
< BlueMatt>
yea, np
< BlueMatt>
god this code is soooo old
< BlueMatt>
wow, does no one maintain bitcoinj at all? how was this not replaced
< instagibbs>
murch: i think you should try to control for estimated fees as much as possible. Hard to disentagle fees vs utxos with current data
< gmaxwell>
murch: so is your codebase in a position to try more hypothetical algorithims?
< murch>
BlueMatt: I had missed "calculateFee", I had only discovered the CoinSelector.
< BlueMatt>
murch: oh shit, that will probably significantly effect your simulation
< murch>
gmaxwell: I was planning to provide it around Scaling Bitcoin. There was a few more things I wanted to do before publishing it. Trying other strategies is a case of inheritance and overriding a single function though.
< murch>
BlueMatt: I should have written to the mailing list earlier. (._.)
< BlueMatt>
murch: well either way you re-confirmed what we all knew: bitcoinj is unmaintained and its use should be significantly discouraged
< murch>
BlueMatt: Is it as complex as it looks. Could you walk me through how it influences Coin Selection from the top of your head, if it isn't too complicated?
< murch>
instagibbs: I was actually just looking at Coin Selection strategies and had in the beginning completely avoided fee estimation. I added fees after I realized how deeply ingrained fee estimation was in Core's coin selection.
< murch>
Seems like I'm misrepresenting BitcoinJ here then.
< murch>
perhaps also the other wallets.
< BlueMatt>
murch: hmmm, i cant say i recall exactly what the behavior there is...its also very much based on what bitcoin core did at the time (and its non-fee anti-spam measures)
< BlueMatt>
murch: is your simulator in java? that code is very standalone
< sipa>
i don't understand why fee estimation matters
< murch>
BlueMatt: Scala actually.
< BlueMatt>
sipa: its not really fee estimation, its the thing that selects between coin selection options and change
< BlueMatt>
ie whether to throw away change
< sipa>
for a comparable simulation across clients you should assume a constant approximate feerate on the network
< BlueMatt>
murch: so you can just drop that function in, mostly? I actually have no idea about scala tooling
< murch>
sipa: E.g. Bitcoin Core uses an initial guess of the fee to restrict solutions in the selection pass through. It will only update the fee guess after the selection attempt if it didn't produce a satisfying result.
< BlueMatt>
sipa: he does that
< sipa>
and use the same fees in multiple tools
< sipa>
murch: ah! you mean fee estimation of the transaction you are creating
< sipa>
not feerate estimation on the network
< murch>
sipa: I did assume a constant fee rate of 10 satoshi per byte. There are some minor differences how wallets apply it though. (E.g. some always pay for a change output.)
< murch>
BlueMatt: Scala can call any Java class. But I'm a bit confused at this point: Does the calculateFee code just check whether to drop the change or create it, or does it change the actual selection?
< BlueMatt>
murch: it takes the CoinSelector and will call it three times, and select between the options based on what the resulting change is
< murch>
BlueMatt: o.0 I had the impression that the CoinSelector is deterministic?!
< BlueMatt>
i dont recall 100% how it works, iirc its something like "first call with the amount you need, then, if you got a change value which you were gonna throw away, call it again with enough additional to not throw away the change, and then there is a third case that does X"
< murch>
sipa: I should probably stress that my simulation does not cover fee levels. I've just applied a common fee estimation to all wallets that prescibes 10 satoshi per byte. The figures for the fees in my results are therefore essentially an abstraction of "inputs spent", "outputs created" and "changes created".
< sipa>
murch: that seems desirable to me
< murch>
BlueMatt: Ah, thanks. Now I get what you meant. I should probably be able to replicate that no problem.
< sipa>
(the way you do it)
< sipa>
otherwise there is the added variance of how wallets determine fees
< murch>
sipa: Yeah, and it would have also blown the overhead for my simulation out of proportion. I'll add it to "Future Work". ;)
< murch>
gmaxwell: Did you have something in particular in mind to try? We (sipa, you, instagibbs, me) did discuss this a few months ago already though. And I've also read your Bitcoin Wiki page on Coin Selection. :)
< * murch>
will be back after dinner
< gmaxwell>
murch: 10sat/byte or per weight? (how does the segwit analysis change it?)
< gmaxwell>
murch: so are you not modling the behavior where very small change out is converted into fee to avoid creating change?
< xinxi>
gmaxwell: CT uses ECDH, which is not quantum resistant. Why not use a quantum resistant key exchange crypto algorithm?
< instagibbs>
xinxi: maybe #bitcoin-wizards ?
< xinxi>
instagibbs: what's that for?
< instagibbs>
this channel is for near-term bitcoin core development, your question is about an implementation of elements sidechain and/or future quantum advances
< gmaxwell>
murch: as far as changes, there are a number of simple strategies I'd try. E.g. try to form a zero change transaction, converting up to X to extra fees, failing that, add a change output, and spend all coins paid to a selected address when any are spent. If change >2x the payment, create two change outputs, randomly selecting splitting the change in half or making one equal to the payment.
< gmaxwell>
MarcoFalke: please include more explination in your commit messages (/pull requs); e.g. the getinfo deprecate should give the short reason for it (replaced by rpc x/y/z)
< gmaxwell>
jonasschnelli: on the sweep thing (#8763); I agree the functionality would be useful. But I'm somewhat torn-- we really should be avoiding introducing pruning incompatible functionality, and right now recan is so slow as to be more or less useless.
< gmaxwell>
I suppose a sweep could instead work from the utxo set without loss.
< BlueMatt>
<instagibbs> pfrom is referring to the remote node's flags, I hope
< BlueMatt>
...
< BlueMatt>
<BlueMatt> I think nLocalServices refers to YOUR services
< instagibbs>
apparently not we learned, which confused the hell out of me
< BlueMatt>
<BlueMatt> and you store it in every CNode object
< BlueMatt>
is that true?
< BlueMatt>
that seems...fun?
< instagibbs>
nServices is the remote's services
< sipa>
will check
< instagibbs>
but both are stored on CNode
< BlueMatt>
seems like an easy optimization for cfields to fix?
< instagibbs>
well it could be a way to remember which services you promised without reconnecting?
< instagibbs>
ie remembering what other node will have for nExpectedServices
< luke-jr>
irssi exploit: https://irssi.org/security/irssi_sa_2016.txt (seems like a repeat of CVE-2003-1020, so may wish to consider whether there might be more unfound exploits..)
< sipa>
BlueMatt: i believe it was done because nLocalServices can change - by copying it into the CNode we have an immutable value that never changes, and also tells us what local services we announced to that peer
< BlueMatt>
ahh, i guess it doesnt change now, but we might decide to change the way we handle things in the future and then it could change?
< BlueMatt>
(I'm not aware of any way it could change in the current code?)
< BlueMatt>
eg pruning/bloom/etc cannot be turned on and off at runtime
< sipa>
agree, it's always set at startup
< instagibbs>
mind if i put a comment on that field to make that explicit? Maybe fewer tears in future
< BlueMatt>
please do
< sipa>
i'm making hypotheses here, cfields should probably comment
< BlueMatt>
i dont think its new/changed in CConMan
< instagibbs>
that was mine as well, but I'll wait for "official" call
< sipa>
BlueMatt: before connman nLocalServices was a global
< instagibbs>
I have a feeling reviewing cb pull will make a lot more sense now
< murch1>
gmaxwell: 10 sat per byte, and for P2WPKH 10 byte per weight (if that's bytes + byte/4 for the witness part)
< murch1>
gmaxwell: I have a MIN_CHANGE parameter, which for some wallets is the limit for conversion to fee and for other wallets (such as Core) added to the target before selection.
< sipa>
murch1: sounds right
< gmaxwell>
murch1: does your model for core convert dust to fees?
< murch1>
gmaxwell: Concerning the strategy, what is X? What would X be? The fee cost of the change output? My simulation doesn't model "addresses", so I can't model issues concerning address reuse. Address reuse is already heavily discouraged and I've decided to omit that. Change splitting is clear.
< gmaxwell>
::sigh::
< gmaxwell>
murch1: okay, well your efforts then are moving things into a realm of academic novelty rather than anything pratical then.
< sipa>
gmaxwell, murch1: i believe you are misunderstanding each other
< gmaxwell>
because address reuse is _very_ common on the network, and agressively consuming inputs is probably a good strategy-- but it's potentially devistating to privacy, but not if it's limited to same-address ... in which case it's probably helpful for privacy.
< sipa>
how is the rule to convert dust to fees at all related to address reuse?
< gmaxwell>
sipa: murch1 was responding to my earlier comment, not the most recent question about dust->fees..
< sipa>
ah
< sipa>
gmaxwell: he is currently moddelling how actual wallets work... that seems very practically useful and not an academic novelty
< sipa>
sure, if we're trying to find an optimal strategy, it may be worth taking reuse into account
< murch1>
gmaxwell: dust to fees: Yes it does, but only if it cannot create a direct match or a change of at least 0.01 BTC. In my simulation the smallest change Core created was 0.01 BTC.
< sipa>
i believe that's correct
< sipa>
(though we may want to reduce that value - there has been talk about making it based on the dust rule, derived from the mempool feerate or estimated feerate, but the current code is still 0.01 BTC i think)
< gmaxwell>
I'd be interested in knowing how the results change if it will send dust to fees more agressively.
< sipa>
yup
< sipa>
it's already pretty aggressive
< gmaxwell>
e.g. try a direct match, send dust change to fees. Only try targeting 0.01 btc change if that fails.
< murch1>
gmaxwell: Here is a question. For a very long time I thought that spending several outputs from the same address required only one signature. I recently saw an answer on Bitcoin.SE that said that spending transactions from the same address still uses a full signature each. Is the only concern privacy then?
< gmaxwell>
Sorry for sounding harsh above, it's demoralizing though to hear that you've done all this modling work and it can't model the behavior I suggested on the input pruning PR.
< murch1>
sipa: It seems to me that the MIN_CHANGE of 0.01 BTC is the one thing that keeps the UTXO pool of Core wallets so small.
< gmaxwell>
murch1: making larger change may result in more optimial selection, without regard to privacy. But agressively doing that in general would be very bad for privacy.
< murch1>
I'm not sure it would make sense to make it smaller. I recently ran a series of tests with the RandomWallet with different levels of MIN_CHANGE. ……… Searching………
< gmaxwell>
If it always spends all the coins paid to a paritcular address, that would not have that downside-- and would even be good for privacy, since they wouldn't get pulled into other transactions where they're cosslinked.
< murch1>
gmaxwell: I model almost the complete transaction process, e.g. UTXO are unique, have value and confirmation height. Adding an Address is not that much work. The issue is with modelling address reuse behavior. I'm already making generous assumptions about transaction sizes by working off of only one real-world scenario data set, I'm also guessing at intervals between transactions. Pulling Address behavior from thin air seemed another
< gmaxwell>
yea, well I saw you were working off real data, so I had my hopes up. :P
< sipa>
murch1: another idea may be to tie the minimum change to a percentage of the sent amount
< murch1>
gmaxwell: The dataset is only positive and negative integers. Not even the order is actually really correct.
< murch1>
because outgoing payments get recorded when requested and incoming only after confirmed.
< murch1>
I have no clue when the transactions occurred or anything about addresses. :(
< gmaxwell>
well, sensible wallets will not spend unconfirmed coins from third parties, so that ordering sounds basically correct.
< kanzure>
murch1: i think you are experiencing message cutoff. there are various ways to force your irc client to split your text into multiple messages.
< luke-jr>
what is the purpose of share/qt/protobuf.pri?
< kanzure>
gmaxwell: when i have looked at coin selection (for some reason i reimplemented the stochastic approximation approach?), my thinking was "gee it would be hard to express complex preference profiles for how i would like a good coin selection method to do my bidding for me". there are a bunch of different knobs to tweak- probably it will end up being wallet developers that pick some "sane choices" instead of exposing this to users.. dunno.
< gmaxwell>
other possible data sources would be taking 'wallets' extracted from the blockchain via taint analysis cross linking. E.g. pick a random transaction, add all addresses that its inputs were paid too. Then find all transactions that spend coins paid to those addresses, add all their inputs, and repeat until no more is added.
< murch1>
sipa: Concerning Size of the MIN_CHANGE: I've run 10 iteratios of RandomWallet with MIN_CHANGE 10 sats, 100 sats, 1000 sats,…,1BTC. There is a tiny bit of a dip on 1mBTC, the first palpable drop in average UTXO pool size is at 0.01BTC, it really goes down on 0.1 BTC.
< sipa>
murch1: but that does depend on the order of magnitude of the amounts you are dealing with
< murch1>
true
< sipa>
someone who regularly receives and sends small amounts may want a smaller setting for that
< murch1>
I'm still working of the same MoneyPot dataset.
< gmaxwell>
kanzure: I am doubtful users can usefully chose on this... but there are choices on the efficient frontier. E.g. I cannot think of _any_ downside to adding all inputs paying to an address you're spending (up to some maximum). I think it's strictly superior to not doing so.
< murch1>
kanzure: Thanks
< murch1>
completely off-topic, but Pidgin sucks for IRC • I used to have a decent client when I still used Linux, but I've only started using IRC for Bitcoin again recently. Any recommendation for a decent IRC client?
< murch1>
on Linux?
< murch1>
You probably meant the message to gmaxwell before?
< murch1>
sipa: That's basically what "DoubleWallet" does. It sets MIN_CHANGE to the sent amount (i.e. the percentage is 100%). It has consistently a much larger UTXO pool than Core on the P2PKH sim.
< sipa>
murch1: irssi
< murch1>
gmaxwell: If the message was cut off before.
< murch1>
gmaxwell: I model almost the complete transaction process, e.g. UTXO are unique, have value and confirmation height. Adding an Address is not that much work. The issue is with modelling address reuse behavior.
< sipa>
used it exclusively for over 10 years now
< murch1>
gmaxwell: I'm already making generous assumptions about transaction sizes by working off of only one real-world scenario data set, I'm also guessing at intervals between transactions. Pulling Address behavior from thin air seemed another huge endeavor.
< kanzure>
gmaxwell: i think change creation can be informed by known long-term behavior of your wallet-- perhaps you know you're going to be making payments in a very specific way, even long-term pre-planned payments. OTOH, it's probably the folks that /don't/ have this foresight who would benefit the most from better privacy in coin selection.
< kanzure>
((well also the general benefit that everyone receives from good coin selection everywhere.))
< murch_>
sipa: Tried that, but it took me forever to figure out how to get anywhere. I'll try again maybe, when I have a few days to just play around with it. Got something else now, I think.
< murch_>
sipa: did you see that about DoubleWallet?
< luke-jr>
ugh, we added unlicensed-or-GPL code to master (and soon 0.13..)