< andytoshi>
does fundmany support a way to restrict the input set to only sufficiently confirmed outputs?
< andytoshi>
if i submit a PR to add this functionality (haven't scoped this out, idk if it's easy) would it be supported?
< andytoshi>
err, fundrawtransaction
< gmaxwell>
andytoshi: it uses the normal selection logic: only mature inputs unless they're from yourself.
< sipa>
you can globally disable -spendzeroconfchange=0 or so
< gmaxwell>
then only >=1 conf, unless it still can't be successful, then unconfirmed, unless -spendzeroconfchange=0
< sipa>
it first tries only 6 confirms; if that doesn't work it tries 1 confirm; if that does work and spednzeroconfchange is set it tries 0 confirmed change and 1 confirms for all the rest
< andytoshi>
sipa: gmaxwell: elements alpha uses this RPC to fund withdrawwatch transactions, which basically means the functionary policy is set by CAre
< andytoshi>
Core
< andytoshi>
well, in this specific are
< andytoshi>
area
< sipa>
andytoshi: good thing bitcoin core uses a reasonable policy :)
< andytoshi>
hehe : )
< dgenr8>
s/optimal/incentive-aligned/... relay policy can't be thought of as an optimization problem, too many independent actors involved.
< dgenr8>
Child-alone-pays-for-all-ancestors is simple and incentive-aligned. It should work for CNB too, though it may miss edge case opportunities.
< GitHub198>
bitcoin/master a6cbc02 Luke Dashjr: Bugfix: Default -uiplatform is not actually the platform this build was compiled on
< GitHub198>
bitcoin/master fa93174 Jonas Schnelli: Merge pull request #7127...
< GitHub118>
[bitcoin] jonasschnelli closed pull request #7127: Bugfix: Default -uiplatform is not actually the platform this build was compiled on (master...bugfix_uiplatform) https://github.com/bitcoin/bitcoin/pull/7127
< jouke>
gmaxwell: you are a hero for doing that.
< wumpus>
gmaxwell: yes thanks for doing that and providing some sanity in the always-hysterical reddit
< gmaxwell>
Thank you both for the thanks.
< btcdrak>
gmaxwell: yeah thanks from me too. I cant stress this enough though, I think you should start a blog or something. You could just use Github pages if you didnt want anything fancy. I fear so much of your writings just get buried.
< jonasschnelli>
sipa: vHashes seems to be and object of "if (!fInitialDownload) {"
< jonasschnelli>
not sure if i can expand the vHashes into the fInitialDownload region.
< wumpus>
gmaxwell: this is another example of how something only gains interest as soon as it is merged. RBF discussions have been going on for as long bitcoin exists, and still they manage to pose it as if this is something new and controversial instead of the compromise of years of argument
< gmaxwell>
If anyone picks up responding at all there, it's best whenever a new question is asked in a comment to break it out and answer it top level, instead of getting in a debate in the comments.
< gmaxwell>
wumpus: I think in this case the PR got a lot of attention first; and most on the noise now was started by an intentional effort to mislead people; there were huge numbers of threads (dozens) created by brand new accounts all repeating the same common misinformation.
< wumpus>
btcdrak: yes, summarizing/copying these things to a blog for future reference would be very useful
< gmaxwell>
That it totally breaks zero conf, that it is something being done by blockstream, etc.
< gmaxwell>
So I think poor Opt-in RBF is just being made a proxy in someone's battle for something else. :(
< wumpus>
btcdrak: the reddit format is good for interaction, but not for e.g. linking or referencing
< gmaxwell>
petertodd: sorry. :(
< wumpus>
gmaxwell: but I mean everyone has those worries and those have been considered from the very start
< gmaxwell>
with the _complete_ lack of complaint (even from the expected parties, hell dgenr8 ACKed) on the PR, I was not anticipating this. :(
< wumpus>
gmaxwell: some people suddenly awake 'oh no! zero-conf'.. if zero conf was not a concern the first iteration would have been merged
< btcdrak>
wumpus: Let's be honest here. Trouble makers are just waiting for topics that are easy to misunderstand and potentially emotive for general public, so they can jump on it and willfully misrepresent it.
< wumpus>
btcdrak: sure, that's true, it's kind of an attack I suppose
< btcdrak>
wumpus: also think about the timing. they want to drum up as much negative attention going into the conference as possible because this is literally their last stand.
< gmaxwell>
I think we haven't been really loud enough about zero conf not being a supported feature, its sort of shocking to hear people's expectations.
< wumpus>
people feeling like they need to raise a rabble about everything because they're suuch an anarchist
< tulip>
zero confirmation transactions are sort of unfortunate because there's no clean line of breakage, no matter how broken anybody can describe it, there's always a secret sauce solution which will "fix" it.
< wumpus>
gmaxwell: well WE have been loud enough about that
< wumpus>
it's just that no one really listens, unless there is a controversy
< wumpus>
(or perceived controversy)
< btcdrak>
gmaxwell: I thoroughly agree with you. In fact, we sort of need a PR side to act as a communication bridge with the general public. The weekly IRC meetings have been really well received, but because they are posted on social media they get buried. This is why I discussed with harding to make a new section on bitcoin.org so we can collate these kind of
< btcdrak>
things.
< wumpus>
tulip: yep
< gmaxwell>
tulip: "Homeopathic confirmation."
< btcdrak>
gmaxwell: LOL
< wumpus>
gmaxwell: that's a good comparison
< btcdrak>
gmaxwell: if only we could get homeopathic theory to work for us, the more the message gets diluted and agitated the more powerful it becomes.
< gmaxwell>
The problem is that the reasons that it doesn't provide the security people think it does are really unintutive to people; and so they stick to easy and wrong naratives instead. E.g. thinking that we're overobessing about perfect security, or saying that they can't use it in places where there is external trust or recourse, where they obviously have; or the latest because blockstream is behind i
< gmaxwell>
t (muhaha).
< gmaxwell>
And so you get this duality where people who grock distributed systems and security reasoning "Get It"; and keep posting "don't do it!" -- while everyone else tells themselves but it's fine and pats each other on the back about how smart they are. :(
< gmaxwell>
we can't save everyone; but its irritating if it gets in the way of progress. They're also attacking my mempool p2p message limiting PR, :(
< randy-waterhouse>
more and more mainstream, layman derpiness, afraid it might only keep tending that way
< btcdrak>
but like seriously gmaxwell please please collate your writings somewhere static. Better to cover a topic in your blog then paste it into reddit as replies. general consensus is your posts are extremely informative.
< wumpus>
so I agree we should have only one signal, but that means the wallet wil have to handle the notification differently first
< jonasschnelli>
the GetMainSignals().UpdatedBlockTip is unused by the wallet
< jonasschnelli>
it was added only for ZMQ IIRC
< gmaxwell>
sipa: if there is only one signal, then we must make sure all things that handle take no time.
< sipa>
jonasschnelli: pro tip: add ?w=1 after the url
< sipa>
gmaxwell: yes, that's the only sane design :)
< jonasschnelli>
sipa: Ah. Right. Much better...
< sipa>
gmaxwell: not saying that's viable now
< wumpus>
right, we can't block progress on the wallet getting fixed
< sipa>
but yes, signal handlers should never do work and never block
< jgarzik>
That's the standard signal processing ideal - do very little - just enough to trigger further processing - not blocking
< wumpus>
yes
< sipa>
we all agree :)
< CodeShark>
just have the signal handler all tasks into a queue and use separate threads for those :)
< sipa>
CodeShark: some things do have time comstraints
< jonasschnelli>
But could we not eliminate uiInterface.NotifyBlockTip and use GetMainSignals().UpdatedBlockTip instead?
< sipa>
but indeed, that's perfectly viable for a lot of thingfs
< jonasschnelli>
Or would this be hard for the UI?
< wumpus>
CodeShark: all but one of the handlers already take (almost) no time, and do their own "second half" handling, it's not necessary to use a solution like that globally
< sipa>
jonasschnelli: imho yes
< jonasschnelli>
But for the current PRs,... let's keep the signal. Combine and remove can be done later.
< sipa>
especially when it includes the initialsync bool
< wumpus>
CodeShark: in the case of e.g. the GUI a message is already sent in a queue, so adding another queue to insert into the second queue would just add latency
< CodeShark>
yes, of course - Qt already has its own messaging system for that
< wumpus>
also there is the problem of things blocking the dispatch thread then
< sipa>
i wish we could set w=1 on github by default for c++ files
< wumpus>
CodeShark: so does zmq, and so does blocknotify (which old-fashioned forks) :)
< wumpus>
it's just the wallet code that needs to do something smarter. Right now it is made sure that the wallet is always in sync with the chain, but this isn't necessary, it could catch up in its own thread
< CodeShark>
it's necessary for coin selection...but that's about it
< sipa>
... coin selection?
< CodeShark>
yes - selecting outputs to spend when creating a new transaction
< sipa>
there is no coin selection when updating the tip i hope!
< CodeShark>
well, it's not necessary if you only use coins that are sufficiently buried
< sipa>
we're talking about milliseconds difference
< sipa>
blocks already propagate in the order of seconds
< sipa>
you really don't need to inform the wallet within miliseconds
< wumpus>
well obviously you don't want the wallet to be many blocks behind, that'd also mess with fee computation
< sipa>
obviously
< wumpus>
but that's not what we're considering here
< sipa>
but async does not mean it has to lag behind
< wumpus>
sure, I agree
< sipa>
just that main can continue before the wallet is done handling things
< CodeShark>
yes, for sure
< wumpus>
could add a 'catch up fence' before doing coin selection / sending a transaction
< sipa>
sounds reasonable
< wumpus>
but that's mostly important if the wallet runs separately, e.g. if it can be stopped/started separately like monero's
< wumpus>
if it still runs in the same process it'll never get out of sync much
< gmaxwell>
in wizkids case, I don't think the node with the low notify had a wallet (other than maybe 100 addresses that had never been paid)
< sipa>
gmaxwell: i think i'm not fully understanding the problem there
< gmaxwell>
right now we trigger blocknotify last, after sending to peers. Wizkid noticed that rpc calls were often returning the new block before blocknotify fired, I suggested he move the notify up, he did. It was first every time.
< sipa>
oh, right
< sipa>
i'd say the right order is zmq,blocknotify,peers,wallet
< sipa>
hmm, but your notify may trigger wallet rpcs
< CodeShark>
wumpus: my stuff lets you do coin selection and create a transaction even if you're not connected to a peer...and peer sync is entirely asynchronous and can be started and stopped independently...but it's not very recommended that you try creating new transactions if you're not fully synched
< CodeShark>
:)
< CodeShark>
I guess I could make it smarter with conflict resolution as it syncs
< CodeShark>
i.e. replace inputs with new ones as they are seen spent during sync
< CodeShark>
and require resigning
< CodeShark>
but meh...probably not worth the effort :p
< wumpus>
...
< wumpus>
bitcoin core's wallet makes the explicit assumption that we're the only one with the keys in this wallet.dat, they cannot be shared, so that should never happen
< CodeShark>
right...that too
< sipa>
so if blocknotify runs before the wallet update, then the notified script can issue a wallet rpc and miss the uodate
< CodeShark>
my stuff does not make that assumption at all
< CodeShark>
and in fact, for many typical use cases it would be very bad to make that assumption
< sipa>
CodeShark: this is bitcoin core dev
< CodeShark>
yes, just acknowledging the design feature
< sipa>
i think it is completely reasonable that if you get informed about a new block, and as a result go query the wallet, you actually also see that update
< CodeShark>
so block all wallet calls during a tip update?
< sipa>
yuck
< wumpus>
maybe the wallet should have its own notification for new blocks
< sipa>
wumpus: i was just thinking that...
< wumpus>
'walletblockprocessed' or something like that
< wumpus>
there's a conceptual difference between 'new block came in' and 'new block processed by wallet'
< wumpus>
or to remain backward compatible: add a -blockearlynotify
< sipa>
... and the same in zmq?
< sipa>
or is zmq blocks only anyway already
< tulip>
zmq does transactions too
< wumpus>
zmq should be completely decoupled from the wallet already
< wumpus>
it's meant as a low-latency notification interface from the core, there is no wallet anything zmq
< sipa>
oh, it just notifies for all txn
< sipa>
if you subscribe?
< wumpus>
yes
< sipa>
so: zmq,blockearlynotify,peers,wallet,blocknotify
< wumpus>
ui should be notified as early as possible too
< tulip>
either transaction hashes or transaction binaries depending what you subscribe to.
< wumpus>
ui's notify just sends an async message to the UI thread to update the block counts etc, it does not make any assumptions about the wallet being updated or not
< sipa>
ok
< wumpus>
(there are CWallet signals that the UI subscribes to for wallet information, this is already handled properly)
< wumpus>
imo needs more code review: Add pre-allocated vector type and use it for CScript #6914
< wumpus>
(lots of concept ACK, but as this involves new data structures and pretty advanced c++, and affects consensus code, I think it needs more thorough review and testing)
< sipa>
wumpus: i think something like 7105 is needed now with mempool limiting, as we'll otherwise automatically respend evicted inputs
< sipa>
i'll help rebasing pr's tagged for 0.12 if that helps
< wumpus>
sipa: agree
< wumpus>
I'm not sure about #6898 (Rewrite CreateNewBlock), if it has not been tested for mainnet mining yet it may be too risky to include in a release
< sipa>
i understand the concern, but that'll likely be the case for any such change; people won't start using it before it's considered stable...
< GitHub77>
bitcoin/master 012fc91 Jonas Schnelli: NotifyBlockTip signal: switch from hash (uint256) to CBlockIndex*...
< GitHub77>
bitcoin/master e6d50fc Jonas Schnelli: [Qt] update block tip (height and date) without locking cs_main, update always (each block)
< GitHub77>
bitcoin/master 947d20b Jonas Schnelli: [Qt] reduce cs_main in getVerificationProgress()
< GitHub88>
[bitcoin] laanwj closed pull request #7112: [Qt] reduce cs_main locks during tip update, more fluently update UI (master...2015/11/qt_tipupdate) https://github.com/bitcoin/bitcoin/pull/7112
< wumpus>
sipa: but some people are mining with master, if it has been in master for a while before the release the concern is mostly gone
< wumpus>
sipa: my problem is not with merging it, but with merging it one day before a release branch-off
< sipa>
understood; i think the benefits are worth it though. the code produces the same blocks as the old code modulo a few rounding error (says morcos), so i'm not too worried about it accidentally producing blocks that are totally off in terms of what we expect to be in there
< sipa>
i will review the code again
< wumpus>
what I want to avoid is a 0.12 release where we have to spend a lot of time mopping up issues introduced last-minute before the release, whereas master as it is now is good as a release already. The goal shouldnt be to stash in everything last minute, at this point it's important to consider the risk not just the benefits
< GitHub20>
[bitcoin] sipa opened pull request #7133: Replace setInventoryKnown with a rolling bloom filter (rebase of #7100) (master...known_bloom) https://github.com/bitcoin/bitcoin/pull/7133
< morcos>
sipa: wumpus: i'm back now, so i can rebase 6898 (i'll do that right now) and starting this afternoon help with review
< morcos>
personally i think the most risky part about 6898 is not the code in the pull itself but the fact that it now relies on the mempool to correctly maintain some state such as number of sig ops, no orphans, etc ...
< sipa>
but we still run TBV afterwards, right?
< morcos>
correct
< morcos>
so then my question is , is that the best way to deal with a situation where that is broken at some point in the future
< morcos>
i'm relatively confident that its not broken for normal situations now
< sipa>
well that's what master and release candidates are for
< sipa>
though as wumpus notes, it is already quite late, and we don't have knowledge of people using this code in production
< sipa>
what about turning it into a separate createnewblockbeta RPC for now?
< morcos>
i generated a new block template and tested it on 1 out of every 128 transactions for 6 weeks in historical simulation, and of course they all passed
< sipa>
oh wow
< morcos>
i don't really have any objection to that if its preferred.. i think my concern was something about maintaining two sets of code last time it was mentioned
< morcos>
to be honest, i'm not sure too many people look at this stuff. that pull has been open for a full month and received almost no conversation on the PR
< sipa>
which is strange, because evidence shows that there are miners running master
< morcos>
i'm not really in a position to complain since i didn't review anyone elses PR's recently though...
< morcos>
Dec 1st just came too fast!
< sipa>
you'd think they'd also test PRs...
< sipa>
or show interest in things like this
< sipa>
so i'm afraid that the only way to actually test it is getting it in master
< sipa>
s/test/get production feedback/
< morcos>
well the nice thing is that its relatively easy to sub in and out
< morcos>
doesn't affect much more than 1 function
< sipa>
indeed, just looked
< sipa>
it doesn't seem hard
< morcos>
i think it would be slightly cleaner to just replace than maintain both, but either option is ok with me
< morcos>
let me rebase it really quick..
< sipa>
the question is mostly whether adding it as a separate RPC would actually make more people use it
< sipa>
wumpus: opinion?
< morcos>
actually wait
< morcos>
i know why i didn't like that idea
< morcos>
it basically makes it impossible to make forward progress for another release
< morcos>
well, yeah, depending on the answer to your question.. it would have to get a lot of uses as an alternate RPC
< morcos>
of if you have original and beta for 0.12
< morcos>
then what do we have for 0.13 ?
< sipa>
i'd say after branching off 0.12, beta goes away and becomes the only one
< sipa>
making this beta RPC a preview for an upcoming version, if no problems are found
< wumpus>
<morcos> Dec 1st just came too fast! <- I'm sure May 1st will come soon enough too, then :)
< morcos>
sipa: yes but my point is that it won't be the upcoming version
< wumpus>
morcos: I agree it's cleaner to just replace it, I'm just very afraid of breaking mining in some subtle way just before a release
< morcos>
i'd like to make more changes, to actually improve the algorithm, not just speed it up
< sipa>
right, that's where the real benefit lies, the fact that this new code can incorporate efficient CPFP etc
< sipa>
and that won't be in such a beta
< wumpus>
if you're fully confident that that cannot happen, then there's no reason to not just replace the old code, but the part about it not being tested on the main network at all scares me
< wumpus>
getting some ACKs from actual miners would be great
< morcos>
wumpus: i think sipa is right, lets put it in master. and announce we need actual miners to test it
< morcos>
its easy to sub out if we lose confidence before release, but i don't think we will
< wumpus>
then revert if there are none?
< wumpus>
ok
< morcos>
let me finish rebasing
< btcdrak>
we should ping wangchun_, his pool is pretty proactive in testing stuff out. also Luke-Jr of course.
< wumpus>
sipa: re: #7105, does listunspent need any change?
< wumpus>
(the default is already to only show outputs with at least one confirmation, but asking just in case)
< GitHub142>
bitcoin/master 4531fc4 Daniel Cousens: torcontrol: only output disconnect if -debug=tor
< GitHub142>
bitcoin/master 9b8fc6c Wladimir J. van der Laan: Merge pull request #7035...
< sipa>
wumpus: i have not checked, but i don't expect it does
< morcos>
wumpus: 7008 needs to be merged first. i just rebased that. or i need to rewrite 6898 without it
< wumpus>
7008 seems ready for merging
< sipa>
wumpus: at worst, it will show non-conflicted non-mempool things now
< sipa>
if you ask for unconfirmed results
< wumpus>
sipa: ok - maybe report 'trusted' in that case?
< wumpus>
not sure it's really relevant
< morcos>
wumpus: where are we on the deprecating priority question. i haven't looked at the changes needed yet, but i'm assuming my blockprioritysize=0 default PR breaks all the RPC tests.
< morcos>
it seems to me that we really should have that in for 0.12 if we're hoping to rip out priority for 0.13, and if so makes sense to put it in before the split
< morcos>
i'm happy to make it work right today if we agree, but otherwise i'll spend my time on something else
< sipa>
morcos: it's easy enough to keep the test and give them an explicit -blockprioritysize
< sipa>
those should keep working anyway
< wumpus>
yes, I'd say just make the tests pass in the right parameter, and add one or so test that tests the new state
< morcos>
sipa: yep, sure the fix isn't hard, but it might be a lot of RPC tests that need it, so we should fix make the changes pre split
< sipa>
agree
< morcos>
although i was thinking it might make sense (other than a test for that parameter) to make the tests just work right without needing free space, but maybe thats too tedious for now.. ok, so i'll do something anyway.
< morcos>
6898 is rebased as well now
< morcos>
i'll be back online in a couple hours
< sipa>
cool
< jonasschnelli>
sipa: any recommendation for decorating a "limbo"-transaction in the UI?
< jonasschnelli>
The "?" (questionmark) is already in use of unconfirmed transaction...
< jonasschnelli>
maybe a "?!"
< sipa>
jonasschnelli: ha!
< wumpus>
ah yes the ‽
< jonasschnelli>
haha
< wumpus>
I suppose using the same decoration as unconfirmed transaction makes some sense, it's a relatively rare occurenc
< jonasschnelli>
Okay. Then i'll add the "Trusted" (=in mempool) info to the transaction detail window only.
< wumpus>
don't think it warrants a new icon, isn't there some other way to distinguish it?
< wumpus>
hmm only in transaction detail window is the other extreme
< wumpus>
I'd like to avoid a christmas tree effect with too many implementation details in the UI - what kind of dysfunctional transaction of the day is this? - but it makes sense to be able to recognize them in some way I guess...
< jonasschnelli>
yes. Makes sense.
< wumpus>
hmm it determines whether we regard it as spendable or not, does it?
< sipa>
wumpus: indeed
< wumpus>
that's what the brackets around the balance are used for [...]
< sipa>
0 confirmations (only for transactions from youself) are spendable when in the mempool, unspendable otherwise
< sipa>
but the GUI afaik has no way of exposing the distinction between the two types of unconfirmed
< wumpus>
e.g. if it shows [123] it doesn't count to the balance, if it's 123 it does
< sipa>
if they receive an unconfirmed transaction from someone else it will always say untrusted
< sipa>
trusted is stronger than being in the mempool; it's also only true for your own transactions
< wumpus>
yes, if it's in transaction details you can give a longer description
< wumpus>
but trusted is very overused and ambigious
< wumpus>
let's not use that
< jonasschnelli>
"In mempool" | "Not in mempool"?
< sipa>
maybe use "0/unknown" if it's not in the mempool?
< wumpus>
nooo not unknown
< sipa>
that's probably even scarier :)
< wumpus>
yeah unknown is very, very scary :)
< wumpus>
'your wallet is glitching'
< sipa>
0/pray
< wumpus>
jonasschnelli: yes, let's just call it what it is, 'in memory pool', 'not in memory pool'
< jonasschnelli>
ok.
< wumpus>
heh
< sipa>
while we're at it: maybe also actually show the depth of the conflict for conflicted?
< jonasschnelli>
right... will do
< jonasschnelli>
sipa: "conflicted (-X)"?
< sipa>
sgtm
< wumpus>
or conflicted (at depth X) ?
< sipa>
sgtm
< sipa>
or "Conflicts with a transaction with X confirmations"
< wumpus>
that does get very long
< wumpus>
that's fine for the expanded transaction details, but probably too long for the onmouseover
< sipa>
ok; we have a GUI maintainer to decide such things :)
< wumpus>
yes
< wumpus>
:D
< jonasschnelli>
hah.
< jonasschnelli>
i'll check how it looks...
< sipa>
jonasschnelli: if you want to show "In mempool", you can't just use IsTrusted()
< jonasschnelli>
sipa: right.. need to refactor the mempool.exists check.
< jonasschnelli>
public expose "bool CWalletTx::InMempool()"?
< sipa>
sgtm
< wumpus>
if you do want to use IsTrusted, use "counts towards balance" instead of "in mempool"?
< wumpus>
otherwise it needs yet another flag on the ui's transaction structure, for kind of little gain
< wumpus>
also we don't expose "transaction in mempool" on RPC, should definitely do that if it's shown in the GUI...
< sipa>
agree
< jgarzik>
Double-check: is it correct that MTP is available via RPC, via getinfo.timeoffset? (curtime + timeoffset)
< sipa>
i would expect that to be about GetAdjustedTime
< gmaxwell>
it's in getblockchaininfo
< gmaxwell>
"mediantime"
< gmaxwell>
In master.
< jgarzik>
Yeah, the definition of GetAdjustedTime() is curtime + GetTimeOffset
< jgarzik>
but mediantime is better
< sipa>
jgarzik: do you mean getmediantimepast of the last block?
< sipa>
or the median time offset reported by peers?
< instagibbs>
wumpus, I think #7044 is merge-ready. Trying to slip it in for 0.12
< jgarzik>
sipa, A fair question - I'm trying to pick which is the best number to poll, for an external agent to trigger a "if $network_time > x, do y" action. MTP with a reorg safety margin seems most applicable.
< sipa>
jgarzik: agree
< GitHub22>
[bitcoin] sdaftuar opened pull request #7137: Tests: Explicitly set chain limits in replace-by-fee test (master...fix-rbf-test) https://github.com/bitcoin/bitcoin/pull/7137
< gmaxwell>
bluematt: apparently the 0.11.2 ppa is 'build failed' and not up yet?
< BlueMatt>
gmaxwell: yea, for 12.04 the build hasnt worked in like...6 months? I think I've received a total of about 10 complaints
< BlueMatt>
gmaxwell: it seems launchpad cant install a package which is in the repos according to ubuntu's package listing site
< BlueMatt>
gmaxwell: mostly, internet folks need to go complain to launchpad
< gmaxwell>
bleh.
< gmaxwell>
BlueMatt: Have you complained to ubuntu?
< BlueMatt>
next step for me would be to reproduce locally...its been really low on my priority list considering I've hard complaints about this only 5-10 times
< BlueMatt>
but I can go do that
< gmaxwell>
BlueMatt: I think its mildly important. My expirence is that you can break a piece of software in a way that impacts _millions_ of people, but if its broken in a really obvious way where they assume you know, no one will report it.
< gmaxwell>
I think on wikipedia when I introduced bugs that would throw consistent errors to readers I'd get about one bug report per million people who hit the error or something at that level.
< jgarzik>
Vaguely related - Fedora keeps inching along towards shipping bitcoin - I got a patch from Harald Hoyer to picocoin, adding bitcoin's curve to an otherwise functional openssl lib. Once libsecp256k1 is packaged into Fedora, that makes a nice end run around the patent/copyright annoyances blocking Fedora deployment.
< jgarzik>
(picocoin thing just an anecdote noting Fedora people working on the overall ecosystem; Harald also works on bitcoin)
< BlueMatt>
jgarzik: yea, warren has been pushing that one forward as well, iirc
< Luke-Jr>
sipa: what benefits? the only benefit is performance AFAIK, which isn't even a real concern for this; and CPFP went unmerged since 2011 despite lots of real-world testing.. (playing devil's advocate here - I'm fine with merging it if there are really no changes, including priority working correctly)
< sipa>
Luke-Jr: yes, it produces the same blocks as the old code, ignoring some rounding errors, afaik
< morcos>
Luke-Jr: in addition to the latency improvement, it also has the benefit of not blowing away your cache by loading up the inputs for every tx in your mempool
< morcos>
sipa: requires 6357 for the blocks to be the same, but they are quite close without that
< sipa>
right
< sdaftuar>
BlueMatt: I updated 6915, hopefully this version looks good to everyone... Can you take a look?
< sipa>
jonasschnelli: opinion about asking on bitcoin-qt first startup how much RAM the user wants to make available for chainstate?
< sipa>
the default (100 MiB) makes sense for smallish devices and VPS'es to not OOM all the time, but on modern desktop systems it can certainly go higher, and will significantly improve sync speed
< gmaxwell>
it would be useful to figure out where the point of diminishing returns is. E.g. perhaps it's 200MB. in which case it might makes sense to actually check how much the system has.
< gmaxwell>
and switch defaults based on the result.
< sipa>
now that the limit is actually honored accurately, maybe it's more useful to do that
< gmaxwell>
it's probably more useful to measure how much physical ram the system has, rather than free.. changing behavior randomly at startup would not be good.
< gmaxwell>
mempool limit could also change on that basis.
< morcos>
sipa: now that its honored?
< sipa>
morcos: -dbcache value is pretty accurately followed now
< morcos>
i think that the default of 100 MB is too small given the default mempool is 300MB and the rest of the random memory usage
< sipa>
agree
< morcos>
i thought that without 6872 you'd still blow through the limit?
< sipa>
not considering miners :)
< sipa>
but yes
< sipa>
and we should probably try to make checkmempool also not pull in the entire mempool into the cache
< morcos>
i thought there was still a problem of all the txs that coudl come in between blocks
< morcos>
especially rejected ones
< sipa>
hmm, didn't we fix that?
< sipa>
i can't remember how; only thinking about it
< sdaftuar>
that's what #6872 is (tagged for 0.12 but still not yet merged)
< morcos>
there is a check in init.cpp which prevents minrelaytxfee=0
< morcos>
it seems to me we should consider removing that check?
< morcos>
in a world without priority, it'll be impossible to mine 0 fee txs unless we do
< jtimon>
morcos: sounds reasonable to me
< gmaxwell>
I think we key othre things off the assumption that this can't be zero.
< morcos>
gmaxwell: first thing i found is that the absurdFee test would be a problem
< gmaxwell>
And it's a misconfiguration hazard, e.g. 0.0000001 is a lot more reasonable a setting than 0.
< gmaxwell>
Also perhaps dust limits?
< morcos>
well having the dust limit be 0 makes sense right
< morcos>
you might want to have that
< sipa>
and the minimum increase for mempool eviction
< morcos>
again
< morcos>
you might want that
< sipa>
indeed
< sipa>
not sure it'd be a good thing for the network, though
< morcos>
this came about for fixing rpc tests
< morcos>
i was going to make blockprioritysize=non-zero be a default for RPC tests
< gmaxwell>
"I support low fees!" which would drive the setting doesn't necessarily mean "I want my node to be trivialy vulnerable to DOS attack and a DOS amplifier towards other nodes" though.
< morcos>
but i realized that would break as soon as we remove priority for 0.13
< morcos>
gmaxwell: sure, but do you want to make it impossible to accept free txs
< gmaxwell>
morcos: then we must support priority.
< morcos>
there is not approriate emoji
< gmaxwell>
My understanding and my sadness at dropping priority is that it went hand in hand with not supporting free transactions.
< gmaxwell>
haha
< morcos>
yeah so i guess i'm distinguishing between supporting it as a normal mode of operation vs not allowing it period
< morcos>
for instance rpc tests, or mining some old tx authored a long time ago?
< morcos>
perhaps fee deltas is the solution to the second (and perhaps teh first)
< morcos>
but these issues are why we should rip priority out as soon as 0.12 is released, so we have time to get everythign working right
< sipa>
going to run master + memory-usage-impacting-PRs-i-expect-to-be-merged on bitcoin.sipa.be, with empty bitcoin.conf
< sipa>
and see what memory usage is
< jtimon>
rewarding the absurd fee check, see #7070
< morcos>
jtimon: ah ok nice... but i'll save trying to let minrelaytxfee=0 for 0.13
< jtimon>
morcos: ack, priority won't be removed for 0.12 anyway, no?
< morcos>
jtimon: correct, just updating the pull to set the default blockprioritysize=0
< BlueMatt>
sipa: I was waiting on 6936
< BlueMatt>
it looks like that replaces 6872? (morcos?)
< morcos>
BlueMatt: heh.. my ideal combination is some version of 6936 (warm cache) with a subset of 6872 (granular removals from the cache)
< morcos>
but i hadn't really put the effort into that, and was expecting 6936 to wait for 0.13
< BlueMatt>
oh? if you're gonna have a scoring metric for what to remove from mempool/cache, then we should only use that
< BlueMatt>
no need to also remove things manually
< sipa>
from morcos' benchmarks it seems that 6936 has a much less proven effect?
< morcos>
BlueMatt: well the problem was there was some unexplainable slowdown with frequent cache flushes which I believe was due to deallocation time.
< BlueMatt>
lol, fucking C++
< morcos>
so yeah i think 6936 is a net positive, but i don't think its ideally tune and i was kind of waiting for the prevector stuff
< morcos>
since that ought to change that whole equation
< BlueMatt>
hmm
< BlueMatt>
fucking dep tree
< sipa>
i think that we can do much better than prevector even for the coinscache if we write a custom memory-packed CCoins
< sipa>
so maybe we can try that for 0.13
< sipa>
but i think 6872 is necessary just for memory conservation
< BlueMatt>
at what point do we decide we've gone too far down the rabbit hole?
< morcos>
when we started working on bitcoin
< sipa>
when people stop being willing to review code :)
< morcos>
to be honest, i don't love 6872, but i reviewed it and test its performance, so i think if we want to address the potential memory growth problem between blocks, lets do 6872 now and we can always take some of it back out later
< BlueMatt>
morcos: yea, it was minimum-to-fix-the-bug
< sipa>
btw, 6872 rebase is trivial; already running it
< BlueMatt>
morcos: hence i stopped paying attention to it when you did 6936
< BlueMatt>
i figured you did that as a "oh, your hack? yea, i can do that better...."
< morcos>
ha, that was the original plan. :)
< morcos>
the other thing that makes 6936 not as good as it should be is stupid priority
< BlueMatt>
yup
< BlueMatt>
long overdue to be killed
< morcos>
have you guys thought about the performance hit to certain rejected txs from 6872 (see my comment)
< morcos>
i guess my vote is to go ahead and merge 6872, but be open to a better fix for 0.13?
< BlueMatt>
so libsecp256k1 has java bindings in-tree...what are people's opinions of doing something like that for libbitcoinconsensus?
< BlueMatt>
otherwise you end up with a thin wrapper library doing the jni stuff which then links libbitcoinconsensus
< BlueMatt>
very nice to have them in the same .so
< BlueMatt>
sipa: ?
< BlueMatt>
jtimon: ?
< jtimon>
BlueMatt: ?
< BlueMatt>
jtimon: java wrapper in bitcoin core for libbitcoinconsensus?
< sipa>
someone started updating them, but never got past the concept of read write locks, i think
< BlueMatt>
sipa: now? i have no idea, they used to
< sipa>
morcos: i think it's acceptable
< jtimon>
it should be included with tests that start failing if it becomes unmaintained
< BlueMatt>
i highly doubt anyone uses them, but I'm gonna try to push bitcoinj to remove its script engine entirely so I need some kind of replacement
< BlueMatt>
so people would actually use that one
< sipa>
jtimon: yup, that was my requirement; travis testable
< jtimon>
but more things that can potentially break if you somehow break the consensus encapsulation helps protect what's already encapsulated
< jtimon>
I mean, I don't plan to do it myself, but concept ack
< sipa>
jtimon: the wrapper should only use the already exposed C api
< sipa>
but yes, i'm certainly fine with that
< sipa>
not just for java
< jtimon>
sipa: yeah it's only going to be broken when the C api is changed, which shouldn't happen much once it's complete (and in the meantime)
< jtimon>
and of course not just java, although I think I would put these in the consensus dir directly in preparation for "subtreeing" rather than in src
< jtimon>
BlueMatt: ping #7091
< rook520>
Why is it wrong to get involved in the bitcoin trade?? Wouldnt it be smart to buy some shares and just let it sit and not touch it...and just let it evolve with bitcoin itself??
< rook520>
Andreas Antonopoulos....are there people in here smarter then him?? is he a joke?? Or does he know hes shit??
< jcorgan>
rook520: take it to #bitcoin
< rook520>
nvm i rather sit here quiet and watch you guys...Im learning in the process
< jtimon>
morcos: sipa: BlueMatt: can we decide on one of the two options in #7115 ?
< jtimon>
or another options, there's infinite ways to remove the new circular dependency, let's just chose one
< BlueMatt>
jtimon: yea, give me a day and I'll go review that one
< jtimon>
BlueMatt: awesome
< morcos>
jtimon: i'm confused. i thought you only presented 1 way of removing the circular dependency
< jtimon>
BlueMatt: there's ways inbetween (ie hide the global usage in main [only use it in GetMinFee] but still do it through a global instead of an attribute)
< morcos>
calling pool->getMinFee() from inside CTxMemPool::estimateSmartFee
< jtimon>
and as said I can produce an infinite amount of them with enough time
< jtimon>
but you guys are doing some incompatible requests
< sipa>
i'm staying out of this now, but morcos's suggestion seems reasonable
< jtimon>
for example, sipa says policy code shouldn't be created in the mempool, but then GetMinFee should be in the estimator instead of the mempool
< morcos>
sipa: but you still want to remove the circular dependency now?
< morcos>
jtimon: as for which method of removing circular dependency of the two you present, i like the one in the PR better
< morcos>
what i don't like in the PR is where you are setting that attribute
< morcos>
can you change it to be lastTrimmedSize and set it in TrimToSize?
< jtimon>
yeah in init is better like in the other option
< morcos>
its not necessarily a fixed value, its the value that was last trimmed to, it should be set from TrimToSize
< morcos>
if you want TrimToSize to call some global variable set in init, thats ok with me
< morcos>
but the mempool attribute should be set by TrimToSize
< jtimon>
but then I would prefer to use a global instead of an attribute (the global can be wherever we want, for example, policy/fee)
< morcos>
sigh.. why?
< morcos>
its really only localized to GetMinFee
< morcos>
why make it a globabl
< morcos>
you're talking about two separate things
< morcos>
the command line argument for maxmempool (fine make that a global)
< morcos>
and the high water mark for the decay in GetMinFee (which should not be a global or a fixed constant, it should be whatever number TrimToSize was called with)
< jtimon>
I haven't looked much yet, but how disruptive it seems to move GetMinFee to the estimator?
< jtimon>
then we can make it an attribute there
< morcos>
i haven't looked either but i also thought of that
< jtimon>
and the mempool can take the estimator as a parameter for its constructor instead of the minRelayfee
< morcos>
lets just concentrate on what MUST be done for 0.12.
< morcos>
having the mempool take the estimator as a parameter seems very weird to me
< morcos>
why does the circular dependency have to go right now?
< jtimon>
then we can call the estimator without necessarily going through the mempool (there's many facade methods in mempool) and start decoupling the mempool from the estimator
< jtimon>
morcos: again, because you introduced it right now
< morcos>
jtimon: agree with that last statement, but too much for 0.12
< morcos>
jtimon: but why is it a problem?
< sipa>
jtimon: it seems very natural for the estimator to deoend on the mempool; why does that have to go?
< jtimon>
if you hadn't introduced it unnecessarily there would be no hurry to remove it
< sipa>
we can of course disconnect everything, and have a ton of glue code to connect things together
< jtimon>
please let's discuss that properly when it's "needed" then, this is clearly not the case
< jtimon>
my plan, as said multiple times, is to make them both completely independent from each other (but acceptToMempool depends on both)
< sipa>
i dislike circular dependencies; they're a sign of badly separated code
< sipa>
what must be done: i don't think it's worth so much discussion
< jtimon>
if we introduce this unnecesssary dependency it will be harder to present my case later
< sipa>
maybe i'm fine with just having a plan to fix it
< morcos>
sipa: sure but if it makes sense for estimator to depend on mempool. then introducing that dependency isn't a regression even if mempool already depends on estimator, it just points out that at some point we should clean up the mempool depending on the estimator
< jtimon>
morcos: why make it dependent right now when it has been shown that is not necessary?
< sipa>
jtimon: no dependency is ever "necessary"
< sipa>
ever seen juice code?
< sipa>
all dependencies resolved at runtime
< morcos>
sipa: ok, i'm happy to make sure the dependency is at most 1 way, but i'd rather not worry about it before 0.12
< sipa>
that doesn't mean it's natural or readable
< jtimon>
sipa: exactly
< jtimon>
not seen juicy code but ack on no dependencies ever being necessary
< sipa>
jtimon: but it sometimes comes at great cost in needing glue to bind things together
< sipa>
the fact that a dependency is not necessary does not make it the best design
< jtimon>
so morcos and I agree that the mempool should not depend on the estimator but we disagree on whether or not the estimator should depend on the mempool or not, step one, whatever is more "natural" both are equally possible
< jtimon>
sipa: agreed
< jtimon>
so we disagree on what is the best design
< sipa>
it seems to me (but i'm not very familiar with the details) that having a mempool estimator depend on the mempool is perfectly natural... if the mempool changes, the estimator (or the glue code) will need to change too
< sipa>
anyway
< jtimon>
leaving the circular dependency there would be already deciding in his favor, since more couplings will rapidly follow
< morcos>
jtimon: but half of what we're arguing about is the stupid GetArg calls and has nothing to do with the circular dependency!
< sipa>
jtimon: if the coupling is natural, of course it will follow!
< jtimon>
morcos: no, from the PR: "But if people disagree, I can easily write a third patch that removes the circular dependency while maintaining all the GetArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE) * 1000000 verbosity."
< sipa>
i've said enough about this
< jtimon>
that's what sipa and matt are arguing about, it's the less prioritary thing for me
< morcos>
jtimon: ok well do you want to try that, remove the dependency by calling GetMinFee inside of the CTxMemPool function with the GetArg and then I'll stop objecting. If I need to introduce the dependency for something else we can argue then
< morcos>
leave all the GetArg's for now and we can celan those up separately
< jtimon>
morcos: exactly we can argue later about the dependency (while, I hope, collaborating n removing the inverse dependency we both dislike)
< morcos>
sounds like a plan
< morcos>
i have to run now
< jtimon>
ok, removing all the GetArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE) * 1000000 verbosity seemed like an obvious thing to do because I thought that was the general trend, I think we started arguing on where to put the global/attribute, but IMO the global is still better than argsMap (another global)
< sipa>
best is a global in init or mail, whose value is passed to the trim calls in ATMP :)
< sipa>
main
< jtimon>
yep main and initialization in init is what the great majority of globals currently do, I don't see why we should be inconsistent in this case
< jtimon>
anyway, I'll wait for BlueMatt to have an opinion, but the main thing is that morcos and I have agreed to disagree later
< jtimon>
well, I will prepare a 3-step thing for people to ack or nack the last 2 commits, the first one will be the minimal way of removing the circular dependency
< jtimon>
so, 1 minimal commit plus 2 squash-or-nack commits