< gribble>
https://github.com/bitcoin/bitcoin/issues/9465 | [Wallet] Do not perform ECDSA signing in the fee calculation inner loop. by gmaxwell · Pull Request #9465 · bitcoin/bitcoin · GitHub
< gmaxwell>
morcos: will #9404 increase the number of cases where you get change back when you were really trying to send all your funds? hm. I guess not, if you're doing a subtract fee from amount the second pass will subtract less.
< btcdrak>
what happens if you quote ticket A, which references B and in the title of B reference A? would gribble flood the channel?
< sipa>
btcdrak: i don't think it triggers based on things it says itself
< morcos>
gmaxwell: yep, i was going to do that, 9465 (just leave off the # if you don't want the bot) looks good to me, was just going to let it sit for a day before i rebased
< morcos>
9404 shouldn't have any affect when you were trying to send all your funds.. it only kicks in if you already had change and that change was big enough to absorb fees
< morcos>
i also have #9343 which makes a change to cases when you are subtractFeeFromAmount'ing which i think will make future improvements easier if we're willing to accept the change in behavior
< morcos>
in any case, i think we should evaluate these as to whether they are better behavior than existing but not hold them to too high a standard b/c i think only a reasonable simple change has a chance for 0.14.. after that we can try to do something more complete
< gmaxwell>
morcos: I agree that we need to focus on small improvements that make things simply better at the moment, and that a big redesign is out of scope at the moment.
< jonasschnelli>
We had 15208 github comments in 2016, from 517 users. That's an avg of ~41.5 comments per day.
< bitcoin-git>
bitcoin/master ce370c1 Pieter Wuille: Mark the minconf parameter to move as ignored
< bitcoin-git>
bitcoin/master 48d7e0d Wladimir J. van der Laan: Merge #9474: Mark the minconf parameter to move as ignored...
< bitcoin-git>
[bitcoin] laanwj closed pull request #9474: Mark the minconf parameter to move as ignored (master...stale_minconf_parameter) https://github.com/bitcoin/bitcoin/pull/9474
< kallewoof>
I am noticing that listsinceblock is sometimes including and sometimes not including a transaction that was double-spent and discarded by the network. Is this expected?
< kallewoof>
Should've asked on #bitcoin-dev. Migrating, sorry.
< kallewoof>
It is an automated test. Only thing I can think of is some weird timing, but I'm being very generous with letting things sync so not sure what's going on.
< wumpus>
I wonder, does listing transactions not in blocks make sense at all for "listsinceblock"
< wumpus>
apparently it's a difference with regard to what unconfirmed/conflicted transactions are listed
< kallewoof>
My assumption was that it would list anything affecting me given the last state of the block chain from the hash I give. I.e. if a tx was dropped it would be present.
< kallewoof>
Specifically an invoice system would do listsinceblock periodically to ensure payments did not get double spent or similar. As it is, you have to do extra work to detect those.
< wumpus>
in that case not showing "orphaned" transactions would be better
< wumpus>
but a difference like that could indeed be due to a timing or race condition, e.g. block not yet processed by the wallet
< kallewoof>
What is the proper way to detect a double spend, in that case? I thought listsinceblock seemed ideal. And it is, at least sometimes.
< fanquake>
wumpus Spoke with theuni about some depends changes yesterday, so waiting on some feedback from him about most of those prs.
< fanquake>
re libevent, I think potential code improvements should be in a followup PR? Such as removing work arounds, any no-longer needed version checks etc?
< wumpus>
fanquake: I think the libevent change is ok, not sure about the remark about that the hash is potentially not stable
< wumpus>
fanquake: well we're far from being able to remove those workaround
< wumpus>
fanquake: it should stil support building with stable libevent :)
< wumpus>
just with new libevent it can use the new features
< wumpus>
(which it already does, in some places)
< fanquake>
wumpus Yea I would have thought it would be stable also. However, there could be a "more stable" URL before 0.14.0
< fanquake>
Might even be a proper release!
< wumpus>
the only option for us I see there would be to copy the file somewhere and host it there
< fanquake>
Yes we can do that also, we already host some files as a backup on bitcoincore.org.
< fanquake>
I'd like to put Boost on there so we can remove our last use of SourceForge.
< wumpus>
should ask cfields for that though, I don't have access to that server AFAIK
< fanquake>
Yes I'll bring it up with him later on.
< fanquake>
Does #9475 mean a 0.13.3 release quite soon? Or waiting a while longer for more bug reports.
< bitcoin-git>
bitcoin/master d29505d jonnynewbs: Fix transaction size comments. Size now refers to virtual size as defined in BIP141.
< bitcoin-git>
bitcoin/master ce43630 Wladimir J. van der Laan: Merge #8747: [rpc] Fix transaction size comments and RPC help text....
< bitcoin-git>
[bitcoin] laanwj closed pull request #8747: [rpc] Fix transaction size comments and RPC help text. (master...rpc_comments) https://github.com/bitcoin/bitcoin/pull/8747
< bitcoin-git>
[bitcoin] MarcoFalke closed pull request #9473: doc: Parameterize rpcauth help text for translation (master...Mf1701-transPara) https://github.com/bitcoin/bitcoin/pull/9473
< MarcoFalke>
wumpus: The rpcauth still needs a `make translate` to show up properly for translators in transifex.
< MarcoFalke>
Not super important, but letting you know.
< MarcoFalke>
Would you mind to create a pull request for `make translate` updates whenever this hasn't been done in months?
< MarcoFalke>
Thus, we could catch failures early
< MarcoFalke>
(Not after they have been propagated to transifex)
< Alsazia>
Premium link generator (30+ host supported) ---> www.galileo.pw
< phantomcircuit>
wumpus, listsinceblock should almost certainly not list things which aren't actually in a block
< wumpus>
MarcoFalke: the reason for having only a relatively small translation window is that it doesn't matter whether "make translate" is before that in the meantime
< wumpus>
MarcoFalke: during the translation window (e.g. starting this month for 0.14) we have to make sure it is up to date though
< wumpus>
(well that's one of the reasons; the other is to prevent translators from wasting time translating messages in the meantime that go away before the release anyway)
< fanquake>
Pretty sure theuni has a patch to fix the weird 9472 error. However he's on a plane I think.
< sipa>
kallewoof: is it about double spends tgat your node has seen? they're not usually relayed by the network
< sipa>
fanquake: he's in NY
< fanquake>
Just chased that code in 9451 back to "First commit"
< wumpus>
fanquake: hah, nice catch
< phantomcircuit>
fanquake, i believe i was right there?
< phantomcircuit>
seems to be about multi byte op codes
< sipa>
even then it was redundant
< sipa>
but it did make more sense
< bitcoin-git>
[bitcoin] kallewoof opened pull request #9478: Trivial refactor: BOOST_FOREACH(a, b) -> for (a : b) (master...replace-boostforeach) https://github.com/bitcoin/bitcoin/pull/9478
< phantomcircuit>
i can fuzz test 9451 but not at the moment
< fanquake>
kallewoof See the discussion in #8718 re bulk BOOST_FOREACH changes. Have a feel opinion may still be as it was a few months ago.
< gmaxwell>
These are reindex times on a dbcache=2000 host from the time it hit block one till it hit tip, the first is normal, the second is with all signature checks enabled.
< gmaxwell>
so it took eight and a half minutes longer, or a 4% slowdown.
< sipa>
gmaxwell: i made a graph of progress (according to my pr) vs time
< sipa>
on my laptoo
< sipa>
*laptoo
< sipa>
*laptoP
< sipa>
and it shows a very clear change in slope at 295000
< gmaxwell>
I'm doing a par4 run, to get more figures, in case verification parallelism beyond 4 actually works now. :P
< gmaxwell>
But there being a change in slope doesn't refute my figures. sync is very fast at 295k and the utxo set is small.
< gmaxwell>
so it makes a difference there, sure.. but it's only a small part of the chain.
< morcos>
gmaxwell: it works! at least to 16
< morcos>
and beyond with some minor'ish tweaks that might be overfitting to checkqueue
< BlueMatt>
note: meeting in 10 minutes...obviously a major point of discussion will be the huge volume of outstanding things for 0.14
< gmaxwell>
morcos: even without the checkqueue changes?
< BlueMatt>
if y'all get the time prior to the meeting, please think a bit about what you want for 0.14, and what you dont think needs to make it
< gmaxwell>
BlueMatt: you mean the huge volume of things that are going to miss 0.14? :(
< BlueMatt>
gmaxwell: yea, probably :(
< morcos>
yeah the cuckoocache was the immediate bottle neck, so with that, we've definitely got improvement , but from 8 -> 16 isn't that great
< BlueMatt>
need to be able to prioritize review for what can make it in a week, and what wouldnt without an extra week :/
< gmaxwell>
morcos: if it really was we could have gotten a similar speedup by bypassing the cache in IBD. :P oh well. :)
< morcos>
gmaxwell: ah yes i guess i wasn't talking about IBD... well i don't remember now what my IBD results were, but i'd be surprised if you couldn't go past 4 previously... actually having to do the signtures makes the parrellism more valuable
< gmaxwell>
sipa: oh I didn't know one was needed there.
< morcos>
sipa: you didn't specify what you changed the last time, i looked and it seemed just the lock inversion fix?
< sipa>
morcos: yes, just that
< sipa>
morcos: i mean compared to before implementing your approach
< gmaxwell>
oh now I kinda wish I didn't load that PR...
< BlueMatt>
sipa: done (I recalled that I had already reviewed half of it yesterday...)
< morcos>
if you guys trust my rebases, just merge #9138 already... please? (next time in exchange for more willing reviewers of fee estimation, i will break into smaller PR's)
< wumpus>
haha yes concept ack. Haven't looked at the code yet.
< sipa>
i haven't had time to look at the details... 0.14 is getting close
< jl2012>
but this is more than just fixing it
< BlueMatt>
but we've fucked that up several times already, so needs careful review, I think
< instagibbs_>
here
< sipa>
jl2012: elaborate?
< jl2012>
it also stores invalid headers
< morcos>
jl2012: are you trying to say you think it NEEDS to be in 0.14
< phantomcircuit>
huh what
< jtimon>
oh, meeting, wasn't planning on attending today, but really nothing to do until one hour from now...
< phantomcircuit>
oh
< jl2012>
specifically, it stores valid headers under an invalid block
< kanzure>
hi.
< jl2012>
also, headers with invalid nVersion are stored
< sipa>
jl2012: but those do have valid PoW?
< jl2012>
yes
< jl2012>
and valid nTime
< sipa>
iirc that is our only invariant for the storage of headers
< BlueMatt>
jl2012: do we need to store them or can we just store them in memory?
< BlueMatt>
but, I'm fine either way
< BlueMatt>
looking at invalid headers with valid pow for user-warnings sounds good to me
< wumpus>
stored headers are forever, both on disk and in memory, unfortunately
< sipa>
there may be some DoS avenues that open up due to it, if they get accepted for chains that fork off early on
< luke-jr>
jl2012: does this do anything to address that nodes sending us such chains will be DoS banned
< luke-jr>
?
< jl2012>
won't be banned
< sipa>
(but i shouldn't comment on that before looking at code)
< gmaxwell>
I feel pretty blah about the utlity of that warning system, and warnings in general. I think we've burned people out with false warnings, and they weren't used usefully by most to begin with. :(
< wumpus>
the alternative to fixing it would be to just disable the system, at least for 0.14
< BlueMatt>
gmaxwell: having reliable warning messages is the first step towards fixing that trust :)
< luke-jr>
jl2012: so this removes the DoS banning for invalid blocks?
< jl2012>
luke-jr: just for valid header
< wumpus>
shipping it broken, especially with risk of false positives, indeed isn't going to do anything good
< jl2012>
if the block content is invalid (e.g. invalid script), still banned
< gmaxwell>
There is currently no false positive risk from it AFAIK.
< luke-jr>
jl2012: which will lead to you not getting future headers
< petertodd>
jl2012: to be clear, if the block is too large it will be banned as well?
< wumpus>
having to store more data forever just to be able to warn seems a bit inefficient to me though
< wumpus>
the block headers are never pruned, and all loaded into memory at start
< BlueMatt>
petertodd: i dont think it changes anything wrt consensus code? the way I read it
< gmaxwell>
petertodd: we can't even deseralize it if its too large so how would we even know if the contet were invalid.
< jl2012>
petertodd: should be, I don't change that part
< BlueMatt>
petertodd: /any/ consensus logic
< sipa>
i wonder if we need a detection of internal consensus errors (database corruption, CPU overheating, ...)
< petertodd>
gmaxwell: we can deserialize if it's only a little bit too large though IIRC
< sipa>
because 99.999% of all fork warning that are seen in practice as just broken nodes
< gmaxwell>
An invarient we have right now is that we depnd on banning to make sure all our connection slots are not consumed by peers that are on an incompatible blockchain.
< gmaxwell>
sipa: I think we do.
< gmaxwell>
sipa: which ultimately should be used to trigger recovery from chainstate backup automatically. (I think)
< sipa>
but i don't know how without having a thread in the background that computes Pi or so :p
< morcos>
jl2012: i'd like to understand the context of the discussion, is that about the change in general or whether it shoudl be in 0.14. Is the current status of master somehow worse than 13.1/2?
< wumpus>
detecting database corruption on disk is pretty easy as everything is CRC-ed, CPU overheating or memory corruption on the other hand...
< jl2012>
morcos: I think that has broken for a few versions
< gmaxwell>
Broken just means that it won't trigger in all cases it might trigger, right?
< wumpus>
good to know, yes, if it's been broken for a few versions there's no hurry to fix it for 0.14
< jl2012>
so you may consider it as a new feature
< jl2012>
gmaxwell: yes
< sipa>
well it can be considered for 0.14.1 or so
< sipa>
if it's a bugfix (which i think it is)
< wumpus>
I wonder if it can be done without storing more headers though
< luke-jr>
but it doesn't sounds like this PR will actually fix it, and fixing it may be more complicated than it seems due to the point gmaxwell raised
< gmaxwell>
My understanding is that there are cases where invalid blocks make us ignore later headers (normally a good thing) which will prevent them from triggering the warning.
< wumpus>
I really don't like storing otherwise unnecessary data forever
< BlueMatt>
wumpus: store only headers required to prove to yourself upon restart that you should display a warning, and prune the (separate) storage for that?
< sipa>
we could prune old header chains
< sipa>
(both from disk and memory)
< BlueMatt>
or that
< BlueMatt>
from memory....sounds hard
< wumpus>
we could, sure, but right now we don't, so should be careful not to store more than necessary
< BlueMatt>
on-disk/restart-only sounds doable
< petertodd>
wumpus: provided theres a reasonable minimum PoW to storing it, it'll never be all *that* much extra data given it's just headers
< jtimon>
re: invariant for storage of headers: remind you that matt moved the nTime check from CheckBlockHeader() to ContextualCheckBlockHeader()
< sipa>
yeah, on restart it trivial
< wumpus>
yes on startup would be good enough
< morcos>
I think it would be wise to use our limited time to concentrate on things people think are really important for 0.14, it doesn't sound like anyone is making that argument about this change?
< sipa>
agree
< jl2012>
ok, please move on
< gmaxwell>
I'd like improvements here, but I don't think it's 0.14 critical.
< wumpus>
other proposed topics?
< BlueMatt>
0.14 prs-to-review...
< luke-jr>
morcos: this change is mostly useful to plan for a future hardfork, but I don't think it's critical it's in 0.14
< gribble>
https://github.com/bitcoin/bitcoin/issues/7533 | RPC: sendrawtransaction: Allow the user to ignore/override specific rejections by luke-jr · Pull Request #7533 · bitcoin/bitcoin · GitHub
< jtimon>
custom chainparams really
< BlueMatt>
gmaxwell: it tries very hard to limit concurrency changes - its whitelisted on messages, plus other things
< wumpus>
gmaxwell: same
< morcos>
i'd like to see the improvements to block relay #9375, #9441, #9447 and possibly parallel ProcessMessages
< BlueMatt>
gmaxwell: no open prs of mine make any significant concurrency changes
< BlueMatt>
gmaxwell: or at least they try hard not to
< BlueMatt>
s/not//
< cfields>
wumpus: +1 for named arguments. Will re-review.
< gmaxwell>
BlueMatt: we really have inadequate testing there, as cfield's version assert has shown us.
< morcos>
+1 on named arguments as well... will amke future PR's easier/better
< BlueMatt>
gmaxwell: helgrind works well, it turns out :), but, agreed
< gmaxwell>
BlueMatt: only if you actually execute the codepaths.
< BlueMatt>
sure
< gmaxwell>
Where are we with the multiwallet support?
< cfields>
BlueMatt: maybe you could briefly describe what you're hoping to immediately improve?
< instagibbs_>
gmaxwell: there's one refactor outsanding I believe
< luke-jr>
gmaxwell: 3 PRs left, 9375 and 9441
< sipa>
9441? sure?
< luke-jr>
err
< instagibbs_>
8775
< luke-jr>
8775 8694*
< gmaxwell>
I had been hoping for that and the utxo scriptpubkey index (#8660) in 0.14, but it looks like the latter has been abandoned by its requester.
< sipa>
i'd like to see #9441 in to get the performance benefit
< BlueMatt>
cfields: specifically, because many miners rely on bitcoind submitblock -> p2p network latency to relay their blocks out, this ends up being pretty important for miners in several ways, so speeding up the relay (hopefully without introducing a ton of new concurrency outside of cmpctblock/getblocktxn/blocktxn message processing) would be a massive win for many miners
< BlueMatt>
yes, so multiwallet and at least the currently-open net prs are review focus
< gmaxwell>
wumpus: I think it's suffered less attention than it would have recieved because it's poorly labled and people keep thinking its a blockchain index.
< morcos>
ok well if i had to pick one, i think 9375 makes a huge difference
< wumpus>
gmaxwell: looks like droark might pick it up
< gmaxwell>
BlueMatt: well I had a PR that removed the biggest source of submitblock latency-- it's a couple lines changes, I assume its functionality has been duplicated in one of cfields overlapping PRs that I closed that one in favor of.
< morcos>
if nothing else the caching of the block/cmpctblock that you are about to send to all your peers reduces the time per per from 25ms to <1ms
< BlueMatt>
gmaxwell: yes, its in the "Net: Massive speedup" one, but the validation time cost (which the parallel getblocktxn/processmessages stuff + the fast relay stuff fixes)
< BlueMatt>
gmaxwell: for current-tip you really want both, but the net stuff you're referring to isnt the only thing here
< gmaxwell>
I am going to be irritated if 9441 misses 0.14. I had an alternative PR that resulted in the same speedup which I think was much less invasive, but I closed it because cfields prefered the other change because it serviced his overall refactor planning.
< BlueMatt>
I think that one is pretty far along, its gotten a lot of eyes even if not so much acks
< BlueMatt>
<BlueMatt> yes, so multiwallet and at least the currently-open net prs are review focus <-- any other things to add to that list
< BlueMatt>
?
< wumpus>
gmaxwell: let's focus on making that one get in then (I do think there's some regression risk, as it's a pretty invasive change to do last minute)
< sipa>
i'll focus on the net locks overhault, named args, early compact block relay
< morcos>
gmaxwell: so 9441 doesn't count as concurrency changes you are worried about merging now
< gmaxwell>
wumpus: that was my feeling too but yea.
< morcos>
+1 sipa's list
< gmaxwell>
morcos: right, I think it's invasive but it shouldn't be meaningfully creating new concurrency.
< wumpus>
sgtm
< wumpus>
#action focus on the net locks overhault, named args, early compact block relay
< BlueMatt>
sip, ok, named args it is, also multi-wallet?
< BlueMatt>
a
< gmaxwell>
The caching improvements as part of early compact block are nice. One option is if we feel uncomfortable about early compact blocks is that we disable that part and just take the caching.
< BlueMatt>
sipa
< instagibbs_>
luke-jr: might make sense to split out QT stuff for time
< luke-jr>
instagibbs_: planning to do so, once 8775 is merged
< wumpus>
multi-wallet may be still too many PRs away for 0.14 , dunno how realistic it is to get that in, though we certainly can make progress with the refactors
< morcos>
Obviously I think 9138 (improve fee estimation) needs to be merged, but i think its ACK'ed enough (excpet it had some rebases)
< BlueMatt>
gmaxwell: the fast-relay stuff there has been /super/ scaled back...at this point its only a) if its the first thing you're annoucing at that height, and b) if its built directly on your tip
< BlueMatt>
gmaxwell: so hopefully its easier to be comfortable with it
< gmaxwell>
On the remaining multiwallet, I felt one of the outstanding refactor PRs introduced a fair amount of not very C++ish code, but who am I to judge? and I didn't know what to recommend instead, so I asked sipa to look at it but I doubt he's had a chance yet.
< luke-jr>
instagibbs_: although Qt stuff ties in with RPC stuff for the debug window
< wumpus>
morcos: if something is ready for merge you should let me know :)
< morcos>
well at this point, my judgement isn't to be trusted.. :)
< gmaxwell>
BlueMatt: remaining concerns are things like "are we going to accidentally DOS attack our peers by announcing something and then reorging" and things like that.
< jonasschnelli>
Multiwallet: I think need to rebase this one: #8764
< gribble>
https://github.com/bitcoin/bitcoin/issues/8764 | [Wallet] get rid of pwalletMain, add simple CWallets infrastructure by jonasschnelli · Pull Request #8764 · bitcoin/bitcoin · GitHub
< BlueMatt>
gmaxwell: yes, hence scaling it back...been discussing it a bunch with folks in the past few days
< luke-jr>
jonasschnelli: that kinda conflicts with the current multiwallet stuff
< luke-jr>
might be more efficient to do 8764 after the basic parts are in
< gmaxwell>
For 0.14 I really want to see the second pass through createtransaction change from morcos in... fixes some concerning fee overpayment corner case.
< gmaxwell>
I don't know why #9312 isn't merged yet.
< gribble>
https://github.com/bitcoin/bitcoin/issues/9465 | [Wallet] Do not perform ECDSA signing in the fee calculation inner loop. by gmaxwell · Pull Request #9465 · bitcoin/bitcoin · GitHub
< sipa>
i read a few things about accidental extreme fee
< jtimon>
maybe related to estimate fee for the very next block now disabled? (no idea, just thinking out loud)
< sipa>
is that related to 9138 ?
< wumpus>
gmaxwell: same holds for you, if something is ready for merging you should let me know
< sipa>
or 9404?
< gmaxwell>
#9404 though I really want it in, I haven't actually reviewed the code becuase I know it'll be simpler after 9465. (the story there is a user reported an issue that might be that fee bug, I went go fix it, realized it would be easier to fix after factoring out the signing.. did that.. then realized your preexisting patch was already the fix I wanted. :P )
< wumpus>
I think I stopped paying attention there when a certain person started trolling then lost track of it.
< sipa>
can someone explain what causes this?
< sipa>
(i don't visit reddit)
< morcos>
yes
< BlueMatt>
^ that
< kanzure>
luke-jr: see 9404, i think
< gmaxwell>
luke-jr: I believe its fixed by 9404. Of course, I can't know for sure, not enough info.
< sipa>
oh, is it change unnecessarily converted to fee?
< instagibbs_>
gmaxwell: so it's wallet-related code, not estimation
< morcos>
the 9404 case is caused when you select coins to pay for a tx, calculate fee and realize you dont' have enough, so you have to go and reselect coins. you end up selecting many fewer for whatever reason, and now you have enough fee of course, and you end up paying the fee you calculated for the prior iteration
< luke-jr>
gmaxwell: well, OP's post there said he's using estimatefee :/
< morcos>
gmaxwell: 9404 is already rebased on 9465 as of this morning, so easy peasy to review now
< gmaxwell>
But the user seemed to be reporting that it payed several times the fee estimator figures (at least thats my read on it), which supports 9404 over 9138 though 9138 needs to go in too.
< gmaxwell>
morcos: oh missed that, will review.
< jonasschnelli>
#9294 should also go into 0.14 (needs overhaul, my turn) and some form of a HD rescan would be great.
< gmaxwell>
jonasschnelli: do we still have nothing that removes key from the keypool when they show up in transactions out of order, so that a rescan while unlocked would successfully find everything?
< gmaxwell>
luke-jr: no. unfortunately.
< gmaxwell>
well it didn't suggest that the known ways that estimatefee could be high weren't being hit.
< jonasschnelli>
gmaxwell: we don't. But I'm working on it. Got distracted with SPV/BFD and 2016 visualisation.
< gmaxwell>
jonasschnelli: okay, I'll do it if you don't have time; I just figured you are much more recently familar with that code than I. Sorry to nag.
< morcos>
sipa: its also possible that fee estimation could temporarily be very high.. was MUCH more likely for estimatefee 1 though, which has already been disabled
< sipa>
morcos: ok
< jonasschnelli>
gmaxwell: I'm happy if you do it.
< jonasschnelli>
The one above is an older try that could be useful for hd restore.
< jonasschnelli>
Fun topic: 2016 Git Visualisation: I'd created a draft video, need feedback to overhaul it and place it on the bitcoincore.org website.
< jonasschnelli>
(will be there for a couple of mins)
< jonasschnelli>
(sorry to spam the meeting)
< gmaxwell>
okay I concept acked, well I'll complete the review.
< luke-jr>
jonasschnelli: why password protect it and post the password in public? :P
< jonasschnelli>
Security by obscurity.
< petertodd>
luke-jr: MILITARY LEVEL BLOCKCHAIN SECURITY
< jonasschnelli>
heh
< wumpus>
hehe
< petertodd>
anyway, I think that constitutes an "effective access control" under the DMCA...
< gmaxwell>
Who called this meeting?
< sipa>
jonasschnelli: short comment: overuse of capitalization (why are Day and Commit capitalized) and dashes (Code-contributors should be written with a space in between)
< jonasschnelli>
sipa: Thanks, will adapt
< instagibbs_>
any other topics?
< wumpus>
apparently not!
< instagibbs_>
everyone's watching the video
< kanzure>
chainparams?
< wumpus>
looks like we'll close the first meeting of 2017 early
< kanzure>
8994
< wumpus>
what is there to discuss about chainparams?
< kanzure>
for 0.14, i think.
< BlueMatt>
final list of things to focus for review? multi-wallet, currently-open net prs, fee ones, what else?
< gmaxwell>
I think that is really a lot less important than everything else listed.
< gmaxwell>
(as people using that are also probably using git master)
< morcos>
are we really trying to get all these wallet changes in?
< wumpus>
yes it doesn't seem to make a lot of sense to focus last-minute review attention on that
< jtimon>
sure, feedback is still welcomed though
< morcos>
should we focus on some over others?
< BlueMatt>
morcos: yea, I think thats more than will make it, but that was the list
< gmaxwell>
(so merging it after 14 means just weeks of delay for someone who wants to use it)
< gmaxwell>
jtimon: fair enough.
< wumpus>
morcos: the fee fixes obviously have priority
< jtimon>
I doubt it will get to 0.14 that's why I asked "how realistic..."
< wumpus>
anything that can cause users to spend unnecessary coins should be first priority
< morcos>
well i guess i meant between hd-chain/split and multiwallet
< BlueMatt>
i think maybe multi-wallet is less likely so maybe less priority, but maybe others disagree since that would be a super nice user-facing feature
< instagibbs_>
I think multiwallet would be great... lots of demand for something like that
< gmaxwell>
I would prioritize fee fixes, net/relay things, hd/rescan improvements, multiwallet, the thousand other open PRs.
< instagibbs_>
but yes bigger changes
< morcos>
gmaxwell: in order?
< gmaxwell>
There is a lot of demand for multiwallet and I feel like if we don't get it done it'll continue to slip.
< morcos>
don't forget named args?
< gmaxwell>
morcos: yes that was an order.
< instagibbs_>
>thousand other open PRs
< jonasschnelli>
gmaxwell: Would multiwallet in 0.14 include the GUI?
< wumpus>
I think 0.14 multiwallet is too late - better to merge it as first thing for 0.15, then improve it in master
< jonasschnelli>
Have we already discussed how to select the wallet over RPC?
< luke-jr>
multiwallet is in Knots already, so may be less important in that sense (since users who want it can get it); stuff like HD split can't really go in Knots without being more final
< wumpus>
it will need a lot of last-minute fixes Im sure
< luke-jr>
jonasschnelli: a number of times :p
< wumpus>
a big feature like that is not done once it's merged
< luke-jr>
wumpus: it's not actually that big at this point
< gmaxwell>
At least on my earlier review it seemed like most of it was the refactors.
< gmaxwell>
Which helps.
< luke-jr>
99% of it is renaming pwalletMain to pwallet in rpc files
< jonasschnelli>
But we need to be careful. Running with many wallets needs some testing.
< morcos>
sorry i'm not trying to be a downer, but both 9375 (fast compact block relay) and 9441 (net speedup) are big heavy review changes, so we shouldn't spread ourselves too thin if we realyl want those in
< wumpus>
in any case there are plenty of PRs to focus on, as said before they can't make it all in
< jonasschnelli>
Even if it's code-wise trivial
< luke-jr>
overall, I think multiwallet can be delayed in Core if other things need time
< wumpus>
if we can't make any choices to postpone things, either 0.13 will slip incredibly (I'd hate that) or we'll have to randomly drop things last minute
< wumpus>
agree with morcos
< sipa>
0.13 slipping now would indeed be terrible!
< luke-jr>
heh
< sipa>
;)
< gmaxwell>
hah
< wumpus>
lol
< gmaxwell>
wumpus: if we slip it we slip it, but if people are active on review and testing I think we don't need to. I really wish the net changes were less invasive but thats water under the bridge now.
< gmaxwell>
I do believe the release will be delayed from fixes for just the things we already have in now.
< luke-jr>
am I likely to be of any help reviewing the net stuff if I'm not up to speed on the net refactoring so far?
< wumpus>
gmaxwell: I don't want to let it slip on features in any case, on bugfixes is more acceptable
< BlueMatt>
note: we have at least 4 regressions in master which are 0.14-blocking which do not yet have prs open to fix them
< wumpus>
so it's clear what to focus on then
< BlueMatt>
so....thats a thing
< gmaxwell>
wumpus: right, well ... you could just merge everything outstanding and then all slips are bugfix related! :P
< instagibbs_>
BlueMatt: there a list?
< BlueMatt>
oh, sorry, 1 has an open pr
< wumpus>
BlueMatt: are the issues tagged appropriately?
< BlueMatt>
yes, tagged 0.14, i believe
< wumpus>
ok
< gmaxwell>
AFAIK we are not actually waiting on the competion of any feature changes. (except perhaps some rescan improvements).. E.g. almost everything I think we might have in 0.14 has a PR already open.
< cfields>
to reviewers, don't let the amount of commits in 9441 scare you. Almost all of them are very tiny, explicitly to make review easier
< BlueMatt>
at least #9479, #9027, #9148 and #9212
< morcos>
oh shit, yeah sorry, one is mine.. , lets quickly decide which direction we want to take on that. Do we revert #9240 or do we not care about the notifications? I think #9371 is too late for 0.14
< gmaxwell>
jtimon: not going to have one either, not any time soon (1) it requires a consensus change; and I don't have the appetite to carry forward in the adversarial climate of the mailing list (which I am not on for a long time now), and (2) Sipa disagrees with my one strategy for removing the signature checking dependency, and petertodd disagrees with the other.
< wumpus>
Madars: what I don't like about 9371 is that is that it keeps the list of removed transactions on mempool, instead of an external object listening for signals
< wumpus>
sorry s/Madars/Morcos
< wumpus>
morcos: this means it can only support one client listening for removals at most
< gmaxwell>
(sipa disagrees that we can just check all signatures; petertodd disagrees with the proposal that we use burried by 30 days of work+other conditions)
< jtimon>
gmaxwell: mhmm, I didn't remember a consensus change, must be missing something, but sure, if it needs it, definitely no time to do it for 0.14
< wumpus>
morcos: for the rest I'm ok with it, and it doesn't conflict with 8549 too much
< sipa>
gmaxwell: what about the idea of once crossing a certain amount of work, requiring a higher minimum difficulty?
< gmaxwell>
jtimon: the part of it that didn't either need a consensus change (uppping the minimum difficulty) and didn't change the signature checking, has already been merged.
< morcos>
wumpus: i was trying to keep notifications from happening while we were locked in reorg.. couldn't we later make it so the mempoolremovaltracker could interface with multiple clients or something
< sipa>
ah, right, consensus change
< gmaxwell>
sipa: it's a consensus change, and I implemented it, and asked for review which jtimon gave some, but... consensus change.
< wumpus>
morcos: I just don't like making a notification mechanism stateful, but that may be a personal thing
< jtimon>
thanks for the info, wasn't up to date on the subject
< wumpus>
morcos: but ok maybe this is the only way to handle reorgs sanely
< gmaxwell>
but I really have no interest in writing a bit and getting treated like shit by zander and voskull or whatever other trolls inhabit the list today.
< morcos>
but isn't that what we've just done on purpose with ConnectTrace?
< morcos>
i guess not quite the same thing... but accomplishes same goal
< jtimon>
maybe in this 2 minutes...should I rebase the super-trivial #9279 ?
< gmaxwell>
sipa: presumably that explains the idea precisely enough. :)
< wumpus>
morcos: I guess your mechanism could be extended to multiple clients if the start/stop timespans for storing mempool removals are not determined by the client
< wumpus>
morcos: then the whole list of removed transactions could be passed to all listeners
< morcos>
yes i think thats what i meant
< wumpus>
morcos: and it's listener-agnostic without calling a signal for every transaction
< jtimon>
what about fetching the minimum difficulty from the 6 biggest mining pools?
< * jtimon>
hides
< gmaxwell>
/kb jtimon
< wumpus>
morcos: not sure what the batching granularity should be in that case though
< petertodd>
jtimon: so long as we have a service that defines which those 6 pools are
< morcos>
wumpus: well i batched it per lock of cs_main i think
< BlueMatt>
jonasschnelli: lol, I hope that was a joke too
< morcos>
which basically is ActivateBestChain(or Step can't remember) and per AcceptToMemoryPool
< jtimon>
jonasschnelli: right, sorry, should have said 5, didn't remember correctly :p
< jonasschnelli>
haha
< gmaxwell>
luke-jr: A bip for it would probably want to specify how the timestamps must be distorted if difficulty gets down to the limit x 4.
< gmaxwell>
luke-jr: which I've not bothered thinking through.
< luke-jr>
gmaxwell: I'd be inclined to suggest if we ever got to that point, a PoW change may be necessary just to avoid attacks from the ex-miners?
< jtimon>
anyway, should go back to vacations, we spaniards give the christmas presents tomorrow and stuff (yeah we like doing things late)
< gmaxwell>
luke-jr: I fully agree. but it would be a little crazy if the software just ... breaks. on its own (e.g. picks a difficulty whos blocks they won't accept).
< luke-jr>
jtimon: not just spaniards!
< jtimon>
who else?
< luke-jr>
gmaxwell: throw a JSON exception? ☺
< luke-jr>
jtimon: we also do gifts on the Epiphany here
< jtimon>
interesting, will ask more next time we meet, sorry everyone else for the offtopic
< gmaxwell>
luke-jr: it should be as simple as (if difficulty < 4 * limit, there is a maximum timestamp for exach block).
< luke-jr>
gmaxwell: yes, if it makes sense to continue that chain
< gmaxwell>
maybe four lines of code, just would take a little thinking to figure out the right four lines. doesn't even have to be a consensus rule, just a thing that createnetblock does.
< * luke-jr>
shrugs
< adiabat>
luke-jr: but what about if the hashrate is actually down 5X "naturally"; e.g. global thermonuclear war? :)
< gmaxwell>
adiabat: we're not talking about 5x. We're talking about 1/20000th.
< luke-jr>
^
< adiabat>
luke-jr: ahh, you mean 4X on min-difficulty
< gmaxwell>
luke-jr: it probably doesn't make sense to continue the chain, but it would be goofy if in tests the system fails on its own because of the rule.
< adiabat>
luke-jr: yeah not even thermonuclear war acheives that.
< gmaxwell>
maybe it does, but then who cares? y'all be dead in those cases.
< luke-jr>
XD
< kanzure>
death is merely a temporary inconvenience for thermonuclear scaling
< gmaxwell>
in any case, raising the minimum difficulty was one of the two remaining issues for checkpoint removal.
< gmaxwell>
The other is the signature checking bypass.
< luke-jr>
kanzure: I'm not sure it's scaling to decrease demand.
< gmaxwell>
I have two proposals for that: (1) we just get rid of it, sipa doesn't like this. (2) we replace it with a non-checkpoint based bypass but instead a proof of work based one, and petertodd opposed the best proposal thus far (and I assume otherws would too)
< luke-jr>
would it be so terrible to keep a single "checkpoint" just for sigcheck bypass?
< gmaxwell>
luke-jr: for the purpose of people misunderstanding the security model I think thats just as bad as what we have now.
< luke-jr>
what if it's a param the user can specify?
< luke-jr>
(with a sane default)
< gmaxwell>
luke-jr: the fact that you can -checkpoints=0 now doesn't prevent people from mistaking that for the origin of the security of the history.
< luke-jr>
:<
< gmaxwell>
I have been thinking that maybe a configurable known good value ANDed with the 9180 process might be good enough. But I'm not sure.
< gmaxwell>
and the continued obession with checkpoints by people (esp. academics) makes me want to not work on bitcoin anymore, so I might not be thinking about it objectively.
< luke-jr>
what if we have multiple such values, all but one of which are bogus valid chains? ☺
< gmaxwell>
from a security perspective if I was happy with PR9180 on its own, then I'd be happy with it anded with a restriction on what chain(s) it applies to. So I think thats fine, but the problem is that people focus on the freeking hardcoded value, so what I hoped to do was just eliminate it.
< gmaxwell>
luke-jr: in any case we are in an optimally bad situation now.
< gmaxwell>
because these hardcoded values pin the consensus we won't move them forward, and won't find out if making them do less would resolve the people confusing them for the security model, but because they (maybe) make a meaningful impact on IBD speed we won't take them out.
< gmaxwell>
and the alternative of using POW can be argued to give more control of the system to miners, so thats opposed too.
< gmaxwell>
(I don't share that belief because the threshold is so large that miners that could do that could simply confiscate all the coins anyways by replacing the chain back to 295k)
< gmaxwell>
(but I don't think the position is an absurd one)
< gmaxwell>
luke-jr: perhaps we can do this in the short term. Introduce a "all prior signatures known good" blockhash which is used for script checking but doesn't pin the chain. Ancestors of that block get scriptchecked.. and then we can set that to a current height. Then the chain pining checkpoints can be eliminated once we've done something about the header flooding... but regardless we can leave the
< gmaxwell>
m alone for now.
< sdaftuar>
gmaxwell: +1, seems reasonable to me.
< luke-jr>
Ancestors of that block get scriptchecked..?
< gmaxwell>
er don't get scriptchecked. :)
< luke-jr>
ok, that sounds pretty much like what I thought the near-term goal already was ☺
< gmaxwell>
okay, well I'll go PR the scriptcheck skipping change I guess.
< gmaxwell>
I guess I'll open this question up to more than sipa:
< gmaxwell>
22:49 <gmaxwell> (thats also the code I would have written, but I'd feel I was probably doing it wrong while doing it.)
< BlueMatt>
are we staunchly against having a "class CWallet" in !define WALLET?
< BlueMatt>
forward declaration, that is
< luke-jr>
that would make things so much simpler XD
< gmaxwell>
I think the problem is that it may not compile since disabling the wallet at compile time changes our dependencies.
< gmaxwell>
I suppose it could be a dummy class CWallet that exists in that case though.
< gmaxwell>
e.g. has no methods or fields.
< BlueMatt>
gmaxwell: not dummy, forward declartion
< BlueMatt>
gmaxwell: i think in this case it will still compile
< BlueMatt>
since there are no actual uses of it, just a pointer to it
< BlueMatt>
which the compiler knows how to do, even without knowing shit about the class
< gmaxwell>
Makes sense. Even to my C loving eyes, the code in 8775 struck me as pretty yucky. If it's forward declared and you try to use it when you shouldn't the result will fail to compile or link. So.. seems fine to me.
< gmaxwell>
but I'm not the person likely to have useful opinions on that.
< BlueMatt>
I kinda agree, i /think/ forward decl will work, easy to test at least
< sipa>
if it compiles with just a forward declaration, it should be fine
< sipa>
and i guess it should
< sipa>
i wouldn't want that in a longer-term solution, but that code will be in flux for a while
< luke-jr>
so I can use a fwd decl?
< BlueMatt>
luke-jr: try it, if it compiles, yes :)
< luke-jr>
pretty sure it will
< * sipa>
is strongly against code that does not compile
< gmaxwell>
If it works and someone thinks its somehow worse than functions with void pointers in their typedefs and typecasting all over the place I will argue with them. :P
< luke-jr>
sipa: but..!
< sipa>
luke-jr: i know, i know. bitcoin would be much more secure if it didn't compile
< gmaxwell>
sipa: code that does not compile never crashes.
< luke-jr>
lol
< BlueMatt>
it also doesn't have bugs
< BlueMatt>
(for some definition of bugs)
< BlueMatt>
then again nothing has bugs for some definition of bugs
< luke-jr>
but also everything has bugs for some definition of bugs <.<
< luke-jr>
btw, this doesn't look like it's going to change more than a few lines of code in this particular PR
< luke-jr>
and may have to still use the ugly cast in Qt code to pass through signals/slots, dunno
< instagibbs>
BlueMatt, if it doesn't compile there is no unexpected behavior :)
< gmaxwell>
luke-jr: fking signals/slots.
< gmaxwell>
morcos: do you think it would be reasonable for CreateTransaction or its kin to log the feerate its using for its construction? So that if there is further concern about strange fees there is log evidence if the strange fee was a result of the estimator vs something else?
< luke-jr>
a bigger concern I think is that logging is not enabled until it's too late
< luke-jr>
so unless you log it by default (which may be reasonable for manual activity?), it won't be there when you want it
< luke-jr>
maybe log it by default ASAP and we can quiet it in a few releases after things have been sufficiently resolved?