< cfields>
sipa: benchmark shows copying is ~100x faster. moving is ~1000x (as there were no moves before, and moves are little more than pointer swapping)
< cfields>
ofc those are suspiciously high. curious to see if others get anywhere near the same numbers
< phantomcircuit>
cfields: how did you benchmark that?
< cfields>
phantomcircuit: benchmark is included
< phantomcircuit>
oh that reminds me
< phantomcircuit>
cfields, can you look at doing a fuzzing framework?
< phantomcircuit>
literally just needs to be an executable that takes input on stdin
< cfields>
phantomcircuit: i thought you were working on one?
< phantomcircuit>
my modifications to the build process to do this didn't seem to work right consistently
< phantomcircuit>
yeah i was and it worked for me but it broke the build process in weird ways i dont understand because autotools
< cfields>
phantomcircuit: oh, you just need the build changes skeleton?
< phantomcircuit>
yeah
< phantomcircuit>
the fuzzing executable itself is pretty trivial
< phantomcircuit>
and i suspect the build changes are trivial too but not for me :)
< cfields>
phantomcircuit: sure, though, you should basically be able to copy Makefile.bench.include
< phantomcircuit>
i tried that first and somehow screwed it up so i ended up copying bitcoin-tx stuff but that's not great cause i actually do want multiple executables
< cfields>
phantomcircuit: you're going to have to lay out exactly what you want. multiple executables gets more complicated
< phantomcircuit>
i could also do it the way i was doing it before which is to make the first few bytes of the fuzzing input select which test to run
< phantomcircuit>
i guess it could also be an executable with cli flags to select which test to run
< phantomcircuit>
having them all be in a single executable isn't ideal for the fuzzer
< phantomcircuit>
gmaxwell, ^ opinions?
< sipa>
cfields: concept ack, i'll review in detail later
< sipa>
on the copy/move
< cfields>
sipa: thanks. if you'd prefer to make it class-safe, i can work on that instead
< cfields>
sipa: but if so, i think it's worth having 2 versions. the POD implementation is much quicker and would suffer if lumped into per-element copies/moves
< sipa>
cfields: i'm impressed by how much faster it id
< sipa>
*it os
< sipa>
ah, but that's move compared to the old placement new based approach?
< cfields>
sipa: you reproduced? or going by my numbers?
< sipa>
going by your numbers
< cfields>
sipa: for move, if malloc'd, all you need to do is copy the pointer. so it's extremely light compared to the previous deep copy
< sipa>
cfields: also, is the script/transaction move constructor/assignment actually invoked anywhere?
< cfields>
sipa: unsure, but I suspect that there are plenty of places that pass in rvalues ala: myfunction(CScript())
< sipa>
cfields: well, easy enough to create an explocit move constructor and putting a printf or gdb breakpoint in it
< gmaxwell>
phantomcircuit: A single executable can be done, with top level branches. Because of aliasing it may be somewhat less effective than seperate, but I don't think it makes a big deal. Biggest downside is that you can't easily focus further testing.
< cfields>
sipa: oh, if you're asking if the moves are actually resolved, yes, i tested that. in the prevector bench, I added a quick version with the exact same operations. CScript was sped up exactly the same amount.
< cfields>
sipa: more precisely, I just stuck an assert(0) into the CTransaction move assignment oper, and test_bitcoin aborts right away
< GitHub118>
[bitcoin] NicolasDorier opened pull request #8339: Consensuslib: Block Verify / Transaction Verify [Work in progress] (master...blkconsensus) https://github.com/bitcoin/bitcoin/pull/8339
< jtimon>
jonasschnelli: what do you think I should do with the consensus PRs ?
< jonasschnelli>
jtimon: I think you should do one small step after another...
< jtimon>
those are 3 small steps one after the other
< jonasschnelli>
But they do include the same commits.
< jtimon>
imagine someone reviewed more than 2 of those little steps
< jonasschnelli>
I think you should open only one PR ...
< jtimon>
yeah, that's the "one after the other" part
< jonasschnelli>
IMO the past has shown that we have not enough reviwers for the consensus parts..
< jonasschnelli>
This is why I think one PR at the time would be more efficient.
< sipa>
i'm fine with the pure move consensus code PRs, after 0.13 branches off
< jtimon>
jonasschnelli : so we should just give up?
< jonasschnelli>
jtimon: no... sure not!
< sipa>
for everything else (encapsulation, API)... *please* first document first
< jonasschnelli>
I just think you will have more reviwers if there are clear distortable/small PRs
< jonasschnelli>
jtimon: to have that said: I'm a big fan of the consensus encapsulation and I appreciate your work!
< jonasschnelli>
I just try to optimize the workflow on that part
< jtimon>
jonasschnelli: you can review one PR at a time, I was just hoping that some people would show more interest than that, they're actually pretty trivial
< sipa>
same goes for NicolasDorier by the way
< sipa>
these are big architectural changes, and i think everyone needs to be on board before going one way or another
< jonasschnelli>
jtimon: I just think you will have more reviewers if you just focus on one PR
< jtimon>
none of the open PRs are big architectural changes, one it's renaming files and the other 2 are moveonly
< afk11>
I can't wait for there to be more code to write bindings to. jtimon please keep going, I see what you're doing :)
< jtimon>
afk11: thak you that's encouraging, but some review on #8337 (which contains #8329 (which contains #8328 )) would be much more encouraging :p
< jtimon>
in my plans I was thinking #8328 much later, but people keep f##$%^ing creating consensus files out of the consensus dir
< paveljanik>
jtimon, just nACK them...
< sipa>
jtimon: it's not always obvious
< sipa>
jtimon: should prevector be in consensus, for example?
< sipa>
consensus logic depends on it, but it's implementing a standard interface, and is more generically useful
< jtimon>
sipa I would say prevector and versionbits were pretty obvious, you even asked me for versionbits, you even had it in the consensus dir and then for some reason you moved it out
< jtimon>
prevector is a dependency of uint256, no?
< sipa>
no
< sipa>
it is of CScript
< jtimon>
oh, yeah
< sipa>
but even that... i'm not sure CScript storage in the long term belongs in consensus
< jtimon>
I mean, prevector is currently needed in libconsensus for verifyScript
< sipa>
ok, so your view that all dependencies for libconsensus (currently or in the near future) belong in the consensus/ directory?
< jtimon>
well, let's start with the short term and complete libconsensus, no? in the long term I would change it from C++ to C...
< sipa>
i'm just trying to understand your view
< sipa>
i think you've thought this true, and have a good picture in your head of what belongs where
< sipa>
but it's not obvious to me
< sipa>
should everything that libconsensus depends on be in the consensus/ directory?
< jtimon>
sipa: yes, and in the consensus packages only if they only depend on other things from consensus, crypto or libsecp256k1 (for example, pow is not ready for the consensus package because it still depends on chain.h)
< sipa>
jtimon: so script/script.h, script/interpreter, primitives/*, ... move to consensus?
< jtimon>
well, right now it's just a few that I'm renaming that are not required by current libconsensus (block, pow, utilmoneystr...)
< sipa>
jtimon: i am not talking about your current PRs...
< jtimon>
everything that's going to be needed for a complete libconsensus exposing verifyblock
< sipa>
ok, thank you
< jtimon>
sipa: that's fine, but there's a list right there
< sipa>
does that include primitives?
< jtimon>
yes, of course
< sipa>
ok
< sipa>
so if in a later stage i want to be able to build an SPV wallet from the codebase, and don't want to include the full consensus logic
< sipa>
the wallet would still depend on the primitives inside the consensus directory?
< jtimon>
in fact I forgot sigache in that PR, that will be needed too I guess
< jtimon>
sipa: yeah at some point if we want to completely separate libconsensus, after bitcoin core itself calls its API, I see no other option than to duplicate some of the code in bitcoin core, but seems too far away in the future to be a big concern
< sipa>
fair enough
< jtimon>
first step is completing libconsensus while allowing bitcoin core to keep using its internals
< sipa>
my expectation (but i don't care strongly) was that we'd have more than 1 layer... primitives being the lowest one with just serialization/data structures, and then consensus being allowed to depend on primitives but nothing else
< jtimon>
I mean, the renaming (moving to the consensus dir) can be done at any time, but I think it makes things clearer
< sipa>
sure
< jtimon>
well, my expectation is that consensus is the lowest layer, it exposes function of different layers I guess (ie script tx header block)
< jtimon>
I mean, the lowest layer is really the crypto dir and libsecp
< sipa>
do those also move to consensus/ ?
< sipa>
not all of crypto/ is needed in consensus
< jtimon>
only one hash function wasn't last time I checked IIRC
< jtimon>
but well, they're in the crypto dir already so...
< sipa>
hmac, rfc6979, aes, sha512
< sipa>
those are not needed in consensus
< jtimon>
the whole folder is currently being built for libconsensus
< sipa>
i'm aware
< sipa>
(but i'm not sure that's a good thing)
< jtimon>
yeah I have my doubts there too, it's not clear to me what do do with that when/if we want to make libconsensus a subtree or something
< jtimon>
so I wouldn't oppose to take hmac, rfc6979, aes, sha512 out of libconsensus, but I'm not sure it's worth the effort
< jtimon>
it's only touching the makefile anyway
< sipa>
well, we'd be breaking apart script/ too
< jtimon>
well, yeah, script is already being built in different packages
< jtimon>
some are not consensus code like sign and standard
< sipa>
indeed
< jtimon>
what do you think about utilmoneystr ?
< sipa>
i don't think string conversions belong in consensus
< sipa>
ideally consensus logic doesn't need to contain any error strings
< sipa>
but the alternative is defining huge enums of error constants
< jtimon>
I'm fine with having tinyformat for now, it's tiny, it says so in the name
< jtimon>
but yeah, I guess the enum is what errorScript was about
< jeremyrubin>
cfields: I was profiling prevector the other day, and was just thinking about switching it to move for CScript -- beat me to it, nice work :)
< cfields>
jeremyrubin: heh, thanks. yea, profiling showed it to be very slow for copy/move. I'd be curious to see if you see similar results in the bench
< cfields>
moves don't happen much in the current code (but will later), but lots of copies.
< jeremyrubin>
yeah will profile it later today
< cfields>
jeremyrubin: there's a bench in that commit. just 'git checkout HEAD~1 -- prevector.h' and rebuild to get the before numbers.
< cool_guy>
ASTOUNDING! I have discovered blockchain exploding technology. Send me your bitcoin and I will return MUCH more back to you, INSTANTLY. This is totally legitimate & vouched by the OPS of this channel. PM me to begin!
< morcos>
cfields: i couldn't get your copy-move branch to compile.
< morcos>
./prevector.h:253:23: error: 'is_trivially_copyable' is not a member of 'std'
< jeremyrubin>
cfields: maybe add these ifdef's for compat
< cfields>
morcos: blah, just comment them out for now. they don't do anything other than throw an error if you try to (for ex) prevector<28, std::string>
< cfields>
morcos: i'll try to find a trait implemented earlier
< cfields>
looks like std::is_pod should work, need to double-check its guarantees though
<@sipa>
when i want information about some standard library function, i usually just type it into my url bar
< sipa>
unfortunately, the browser interprets std:: as an address scheme
< cfields>
heh
<@wumpus>
putting " around it usually helps
< bsm2357>
So I have some confusion about OP_CODESEPARATOR and segwit. I don't see anywhere that the OP_CODESEPARATOR manipulations are actually performed when creating a SignatureHash. Is this just the responsibility of the caller to know what to do with OP_CODESEPARATOR?
< sipa>
the placement of codeseparators influences what is passed as scriptCode to signaturehash
< sipa>
note that codeseparators are assumed to be useless in their current form
< bsm2357>
Heh
< bsm2357>
Ok then client code. (I'm copying test cases from BIP143)
< sipa>
but you should already be doing that
< sipa>
bip143 does not affect the behaviour of code separators
< bsm2357>
python-bitcoinlib is still annoying low-level. AFAICT it doesn't have any "sign transaction" function that would do this. It's up to you to manually call SignatureHash.
< kanzure>
SignatureHash isn't so bad
< kanzure>
(besides the strange name; i appreciate jl2012's signature digest algorithm name idea)
< jtimon>
meeting is now or in an hour?
< eragmus>
jtimon: 49 min.
< jtimon>
eragmus: yeah, thanks, it definitely didn't look like it was now ;)
< morcos>
sipa: all the "inexpensive" checks in CheckInputs and CheckTxInputs are not parallelized. but with a signature cache and libsecp, those "inexpensive" checks are taking up almost all the time... would it make sense to bring all input level checks into what is now CScriptCheck?
< sipa>
morcos: i always assumed the inexpensive checks are actually not expensive
< sipa>
only fetching of the inputs is
< sipa>
am i wrong
< sipa>
?
< morcos>
sipa: i'm about to try to separately bench UpdateCoins
< morcos>
but do you think the fetching is expensive if we are caching well?
< morcos>
i mean i'm testing this with a huge cache, but it would be nice to know if we can increase our cache hit rate with a smarter hot caches, how much it can benefit us
< morcos>
i'll do some benching to see, but just wondering if there was a more fundamental reason not to go in this direction if it helps
< morcos>
sipa: yeah i think with a good dbcache then UpdateCoins is a small fraction of the time spent in " - Connect %u transactions". Like 15% of the time.
< morcos>
When I get a chance, I'll test out moving many more of the input checks into the parallelized part.
< bsm2357>
I'm still having a bit of trouble with segwit transactions, I'm getting error: 64: non-mandatory-script-verify-flag (Script evaluated without error but finished with a false/empty top stack element). sipa or somebody, would you be so kind as to take a look at this tx and tell me if you see anything wrong? https://www.zerobin.net/?94c4f0fb71e982a1#Uc3l1MLgEB+W0izaZXvb7BohI6rMBxxuK6CzTWHh7fo=
< bsm2357>
It's a P2WPKH so I'm confused, the script is implicit
< sipa>
bsm2357: make bitcoind print out the signaturehash it is checking
< sipa>
and compare that to the value you're signing
< bsm2357>
Ah nm, need to pass SIGVERSION_WITNESS_V0 to *use* the new SignatureHash...
< bsm2357>
Thanks sdaftuar I've been reviewing that for days ;-)
< sdaftuar>
haha okay :)
< morcos>
sipa: as you may have realized, i was only measuring UpdateCoins, when of course HaveInputs is the big issue, thats another 40% of the time.. Still, room to improve, and I want to see how much HaveInputs can come down with more perfect caching.
<@wumpus>
meeting time?
< jl2012>
bsm2357: there are some test vectors in BIP143
< bsm2357>
I know, I wrote tests for them! They all pass. :-P
< bsm2357>
Thanks for that ;-)
< sipa>
wumpus: ack
<@wumpus>
#startmeeting
< lightningbot>
Meeting started Thu Jul 14 19:00:52 2016 UTC. The chair is wumpus. Information about MeetBot at http://wiki.debian.org/MeetBot.
<@wumpus>
has ACK by sipa and is being tested by gmaxwell
< sdaftuar>
i think sipa acked an earlier version of the code
< bsm2357>
pong
< instagibbs>
jeremyrubin *ping*
<@wumpus>
cfields: has your comment there been addressed sufficiently?
< sdaftuar>
oh, thanks sipa :)
< cfields>
wumpus: yes, i'm happy with the slow DOS trickle
< jeremyrubin>
instagibbs: yes?
<@wumpus>
as for #8295, mining changes are always harder to get people to review/test, I'd encourage people to get involved, though I see sipa ACKed that one too
< sipa>
yeah, the reason i didn't include telhr 8295 approach myself was because i incorectly assumed it would require extra counters
<@wumpus>
jonasschnelli: #8323 still has some open comments
< sipa>
and the existing tests cover it
<@wumpus>
trivial things though
< MarcoFalke>
no blockers
< jonasschnelli>
Yes. paveljanik just added some... will fix the trivials together (once there are some)
< morcos>
sipa: are you referring to mining unit tests?
< sipa>
morcos: yes
< sipa>
(well, and any rpc test that mines)
< morcos>
sipa: those aren't worth much, they've been in need of redoing for some time
< sipa>
hmm, ok
<@wumpus>
jonasschnelli: I'll also review and test it soon
< jonasschnelli>
thanks
< morcos>
not that i'm objecting to 8295 though
< sipa>
morcos: i'll test by running old and new in parallel and comparing then
< luke-jr>
a quick glance at 8323 suggests it won't make problems with key origin stuff, rihgt?
<@wumpus>
sdaftuar: any blockers remaining there?
<@wumpus>
(after all currenly 0.13-tagged PRs are merged)
<@wumpus>
luke-jr: let's discuss that outside the meeting please
< luke-jr>
….
< sdaftuar>
wumpus: i was waiting for #8295 to be merged before doing the release notes writeups
< sdaftuar>
oh, should we talk about -blockmaxcost briefly?
<@wumpus>
sdaftuar: oh release notes writeups aren't urgent, they need to be done for final but not rc1
< luke-jr>
(what's the point of a meeting if not to discuss the topics raised?)
< sdaftuar>
one of the to do items was better documentation for that option, but as has been pointed out, it's an awful name :)
< sipa>
sdaftuar: suggested replacement?
<@wumpus>
yes, the 'max cost' confuses people, they think it's about monetary cost
< petertodd>
wumpus: +1
<@wumpus>
(and that's also how translators translate it)
< luke-jr>
>_<
<@wumpus>
(and it's a public command line option so the help gets translated)
< sipa>
we could have maxblockvsize instead
< petertodd>
blockmaxcost is still just a size limit isn't it?
< gmaxwell>
Cost has been a repeated source of confusion.
<@wumpus>
but that means the help needs to be improved, not necessarily the option renamed
< luke-jr>
well, it *is* indirectly monetary
< jeremyrubin>
maybe utilization
< sipa>
petertodd: it's the 'cost' as defined in the BIP, which is vsize*4
< gmaxwell>
Score/points/weight/usage/ouchie/bitgo would all be better words for it.
< luke-jr>
"network resource usage"?
< sdaftuar>
sipa: i think my best idea was to change the term in the BIP. i think we use "block cost" in the BIP. if we change that to something more descriptive, "block size validation cost" or something, and reference that in the documentation for the option, then i think that'd be good enough maybe
<@wumpus>
it's an abstract cost, and CS students understand that, but for most users it's confusing :)
< sipa>
sdaftuar: yes, agree, let's makr it consistent, whatever we agree upon
< petertodd>
sdaftuar: yeah, I think changing the BIP's term is a good idea too - it's still a size, just a redefined size
< CodeShark>
#agree
<@wumpus>
block size validation cost would already be better help than there is now
< luke-jr>
petertodd: it's not a size.
< petertodd>
notably, if we had referred to block size in the BIP, that'd help discourage the trolls...
< gmaxwell>
"externality"
< gmaxwell>
luke-jr: it is a size, in cost-space.
< sipa>
well, any reason not use vsize?
< petertodd>
segwit *is* a blocksize increase, so just continue to call it a size
< petertodd>
sipa: vsize is fine
<@wumpus>
yes vsize is fine
< btcdrak>
yes
< petertodd>
sipa: (so long as the help text explains what vsize is)
< luke-jr>
vsize is inaccurate
< gmaxwell>
V means validation?
< sipa>
virtual
< luke-jr>
going to *4 the meaning, so people specify x.25 now?
< btcdrak>
v for vendetta
< sdaftuar>
hm, we have been using vsize to refer to the scaled down value, not the *4 value?
< gmaxwell>
ugh, there is nothing virtual about it.
< petertodd>
gmaxwell: +1
< luke-jr>
fractional values for consensus code?
< petertodd>
sdaftuar: that's a mistake then
< sipa>
sdaftuar: yes, cost == vsize * 4
< gmaxwell>
re fractional values, haven't we learned from difficulty?
< sipa>
vsize is cost, but scaled down for compatibility with old code
< sipa>
so they don't start sending 4x fees etc
< gmaxwell>
if we use fractional values for display friendlyness, other people will use them in consensus code and cause faults.
< * luke-jr>
grabs a thesaurus
< gmaxwell>
sipa: ::sigh:: point.
< btcdrak>
agreed
< jeremyrubin>
I like weight the best
<@wumpus>
agree jeremyrubin
< jeremyrubin>
because usually you would say a weighted sum
< sipa>
jeremyrubin: nice idea
<@wumpus>
weight is a pretty good term for it
< CodeShark>
#agree
< btcdrak>
good call
<@wumpus>
and it's not used anywhere yet in bitcoin
< jeremyrubin>
(gmaxwell: mentioned first)
< luke-jr>
outlay? :/
< luke-jr>
weight sounds fine
< jtimon>
yeah, there's two sizes with their weights, it's a cost heuristic
<@wumpus>
so: rename blockmaxcost to blockmaxweight?
< instagibbs>
"weight" also carries a nice intuitive notion
< sipa>
yes, and let's report weight for transactions too
< jeremyrubin>
maxblockweightcostheuristic
< gmaxwell>
I read that as block mascot.
<@wumpus>
hehe
< petertodd>
OTOH, if we just call the command line stuff a block size, we help educate people on the fact that segwit is a blocksize increase - some miners will leave it at 1000000, but that's a temporary problem
< jeremyrubin>
virtualmaxblockweightcostheuristic
< jtimon>
sorry, I missed the first 15 mins or so, but why do we need to rename blockmaxcost ?
< luke-jr>
petertodd: size is something else
< gmaxwell>
jtimon: because the word cost is confused as fee.
< sdaftuar>
i think "block weight" and -blockmaxweight [
< sdaftuar>
both work okay
< btcdrak>
ack
<@wumpus>
jtimon: well it started as that the help for that option was confusing, then people realized the option was named terribly too, but please just read back :)
< jtimon>
mhmm, no, it's costs for the miners, although that's also why they charge you fees
< luke-jr>
weight also should be fine for GBT; nothing is released using the old cost stuff yet
< luke-jr>
jtimon: is there a problem with "weight"?
< gmaxwell>
in any case, I support the great cost to weight renaming.
< sipa>
petertodd: that's what i did originally
<@wumpus>
#action rename blockmaxcost to blockmaxweight
< jtimon>
yeah, sorry I'll wait for botbot.me to update
< instagibbs>
We can chew it over offline
< petertodd>
sipa: well, ack your original idea :)
< sipa>
petertodd: but then people started complaining that there had to be a way to limit the serialized size too
< jeremyrubin>
jtimon: but that's not the units of the cost, otherwise you would denote it into BTC or something...
< btcdrak>
jtimon: you can see.logs in slack
< petertodd>
jeremyrubin: blockbytescost :)
< jtimon>
jeremyrubin: the cost for miners is in size, or in "costs relative to the maximum limit" or whatever, but sorry, let's move on
<@wumpus>
other topics?
< sipa>
segwit backport
<@wumpus>
ah yes
<@wumpus>
#topic segwit backport
< jtimon>
btcdrak: I didn't knew, never used the bitcoin slack
< sipa>
wumpus: some people have raised concerns about backporting segwit to 0.12
<@wumpus>
concerns about doing it at all?
< jtimon>
what's the concern?
< luke-jr>
^
<@wumpus>
well, next chance is 0.13.1
< sipa>
morcos, btcdrak: ?
< morcos>
:), yes i was arguing that it would be a mistake
< jl2012>
me too
< instagibbs>
could you reiterate for the audience
< jtimon>
if the backport doesn't get enough review and testing there's no new 0.12 release, simple, no?
< morcos>
I dont' think that it's worth it. I'm not sure there are are benefits that outweight hte cost (weight) in terms of developer time and increased risk for bugs in the backport
< sipa>
one concern i have is that it would likely be the first segwit code that gets actually deployed on the network, and that it is hard to give it the same amount of testing and review as 0.13 did
<@wumpus>
I think it would easier for miners if it was backported to 0.12, but if it turns out to be too much technical difficulty/risk, well too bad I suppose...
< maaku>
if that were the case then I would have to argue that we wait until 0.14.1 .. so strong argument is needed
< luke-jr>
jtimon: backports never do, unless they're the tip release
< morcos>
jtimon: agree that its that simple, but woudl be a shame for some people to sacrafice time backporting and reviewing if its not going to pass the bar
< btcdrak>
yes. I share morcos concerns about backporting
< luke-jr>
wumpus: "too bad" may result in delayed deployment maybe
< jtimon>
luke-jr: so all previous backported softforks were unsafe ?
< luke-jr>
jtimon: such as?
<@wumpus>
luke-jr: yes, but delay is better than messing up
< sipa>
about this topic
< sipa>
in the past we've seen miners often run even pre-release code
< jtimon>
luke-jr: I don't know if any of them was unsafe, I'm asking
< sipa>
if that is going to happen, my concerns about a backport go down
<@wumpus>
I hate delays, but minimizing risk is the highest priority
< sipa>
but so does its usefulness
< jl2012>
have anyone asked miners that they really need a backport?
< maaku>
i'm strongly against "too bad" -- we need to take our support guarantees seriously
< gmaxwell>
the concern is mostly that we don't want to force people to quickly adopt 0.13 derrived code just to catch up with segwit, otherwise 0.13.1 would be fine.
< morcos>
maaku: we already didn't backport CSV
<@wumpus>
maaku: there are no guarantees
< maaku>
people are running businesses assuming we support "current plus prior" not "current plus prior, except when it's inconvenient"
< gmaxwell>
maaku: doing segwit in 0.13.1 isn't a violation of the process, that still remains no softforks in major feature releases.
<@wumpus>
maaku: from the begining we've said that the plan would be 0.12.1, but if it would somehow lapse it would be 0.13.1
< maaku>
gmaxwell: it would mean you could no longer mine on 0.12
< sipa>
jl2012: that's the most important question
<@wumpus>
this is not about 'inconvenient', this is about risk
< morcos>
gmaxwell: are you more worried about miners or users?
< gmaxwell>
maaku: with things like CPFP I'm not sure that anyone will be mining on 0.12 in short order. But thats something we could inquire about.
< gmaxwell>
morcos: users.
< morcos>
i just think it's very likely to be safer consensus wise to upgrade to 0.13.1 from 0.12.1 than to upgrade to 0.12.2 (segwit backport) which is more likely to have bugs
<@wumpus>
we wouldn't want a DAO scale disaster over over-hurried release of hardly-reviewed and tested code
< CodeShark>
wumpus +1
< petertodd>
miners that truly need v0.12 might be better off mining with a v0.12 node behind a segwit 0.13 node...
<@wumpus>
so if it needs to be 0.13.1, it will be 0.13.1
< gmaxwell>
Sure.
< morcos>
wumpus: i thought it was definitely going to be 0.13.1, the question was whether we'd also make a 0.12.2?
< luke-jr>
maaku: we need to stop guaranteeing things we can't providr
< achow101>
how many miners even use backports?
<@wumpus>
morcos: no - if it would be in 0.12.2, it could make it into 0.13.0 too
< gmaxwell>
So one risk we have right now is that 0.13.x would end up releasing concurrently or likely ahead of 0.12.2, which would be bad for network consistency in general.
<@wumpus>
morcos: assuming 0.12.2 is released before 0.13.0 of course
< luke-jr>
achow101: all of htem right now I think
< jtimon>
morcos: maybe rebasing my code to 0.13.1 is not easier than rebasing it to 0.12.2
<@wumpus>
morcos: then again that ship sailed already
< morcos>
ok, well i don't have a strong opinion on release numbers or timing. my objection is to spreading ourselves too thing by having to thoroughly review a very substantial set of changes in 2 code bases and then putting the network at risk
< gmaxwell>
morcos: oh no, plan was always 0.12.2, but I agree it might be reasonable to not do that based on current timing.
< achow101>
what about releasing 0.13.1 and then 0.12.2?
< morcos>
i think segwit in master is now relatively well reviewed... it will be very difficult to get that level of review in the 0.12 branch, which woud make me uncomfortable releasing that
< morcos>
on top of that, its not something i'd want to spend my time on... i think all our time would be more wisely spent on moving forward with future work
<@wumpus>
achow101: well if a backport is released at all, why would it matter in what order?
< gmaxwell>
Wouldn't want to make a classic blunder, "never get involved in a land war in Asia"!
< morcos>
anyway, sorry, i have to catch a train, got to run!
< sipa>
assuming the demand for a backport of segwit is for users and not miners, releasing 0.12.2 with segwit backport after releasing it in 0.13 is not unreasonablr
< btcdrak>
It's also a lot cleaner releasing in 0.13.1 because 0.12 would not have compact blocks
< luke-jr>
wumpus: order matters a lot for testing
< petertodd>
sipa: sure, if it removed getblocktemplate that'd be an easy idea to support
< gmaxwell>
well right now 0.13.0 doesn't have CB for segwit, though I suppose 0.13.1 could.
< jtimon>
sipa: yeah, I assumed that was the plan
< achow101>
wumpus: for proper review of the backport without delaying deployment
<@wumpus>
I do wish we had realized this sooner, instead of seeming like a last minute decision
< gmaxwell>
wumpus: likewise.
<@wumpus>
achow101: even fewer people will care about 0.12.x if 0.13 is already out
< gmaxwell>
otoh, decisions to do _less_ at the last minute are better than decisions to do _more_. :)
< btcdrak>
gmaxwell: right, but if we released 0.12.2 then segwit would not have CB. If we release with 0.13.1 then we get CB at the get go.
< sipa>
i guess the question is what has priority: segwit backport or 0.13 release?
<@wumpus>
gmaxwell: hah, fully agreed
< petertodd>
sipa: 0.13 release I think
< gmaxwell>
We have real problems with virtually no one ever using backports.
<@wumpus>
for me the 0.13 release has priority
< helo>
+1 release
< jonasschnelli>
agree with wumpus
< sipa>
my assumption was that we'd do 0.13.0, then 0.12.2 backport with segwit, then 0.13.1 release with segwit active
< gmaxwell>
0.13 release has priority.
< sipa>
and if that is the case, a backport would be urgent
< jtimon>
waiting to release 0.13 to release 0.12.2 first sounds stupid to me
< sipa>
or 0.13.1 would suffer delays
< btcdrak>
sipa: getting enough review for backport to 0.12.2 would be questionable.
< gmaxwell>
sipa: yes, that was my expectation too, though the release of 0.13 would likely sabotage 0.12.2 deployment.
< luke-jr>
IMO we should remove any promise to maintain older branches once the newer one has a release, and provide backports only as a at-your-own-risk basis
< petertodd>
I'd suggest putting a "if you want 0.12.2 w/ segwit, please let us know" in the release nodes for v0.13.0
< jtimon>
petertodd: that's a good idea
< jonasschnelli>
Don't we take serious risks in BP SW in 0.12.2? Would it be evil to not BP SW to 0.12 and require 0.13 for SW? Would this delay supermajority activation significant?
< sipa>
if there are critical bugs in 0.12.1, we can always release 0.12.2 to fix them
< btcdrak>
we didnt backport CSV to 0.11 because the changes were too extensive - to be clear the backport was done, but we decided not to merge it (and there was not enough review either).
< petertodd>
jtimon: I think we've done it before too
< jtimon>
and depending on the response decide to the the backport or not
< sipa>
btcdrak: tbh, i think that the difficulty of a segwit backport is not that hard
< gmaxwell>
luke-jr: that would be unfortunate in that its the wrong direction for the industry. The lack of professionality in software lifecycle isn't something we should be cementing, though I'm not sure I see much choice when we don't actually get the review/testing.
< btcdrak>
sipa: you would say that though :)
< sipa>
btcdrak: the original segwit PR was made with mostly 0.12 code still
< gmaxwell>
luke-jr: maybe we should start making more really disruptive changes in major releases. :)
<@wumpus>
'maintain' older branches does not mean backport everything
< jonasschnelli>
agree
< luke-jr>
gmaxwell: I agree we should strive to support stable branches of course (as I've tried for years now), but the reality is we *can't* as things stand now
<@wumpus>
just that serious and critical issues are addressed
< sipa>
yes 0.12.2 can still happen if a serioud bug in 0.12.1 is founf
< luke-jr>
wumpus: not supporting a deployed softfork is a critical issue for a full node
< jtimon>
gmaxwell: I've been always in favor of doing disruptive refactors just before branching instead of right after
< sipa>
ok, i need to run
< jonasschnelli>
luke-jr: SW could be deployed, I guess the majoriry is already on 0.13 and there is no need to keep 0.12
< sipa>
i'll still work on a backport if there is demand
< luke-jr>
jonasschnelli: that's a point I suppose
< gmaxwell>
sipa: it might be a useful excercise from a review perspective in any case.
<@wumpus>
jtimon: that increases the pre-release pressure even more, and the potential number of problems what need to be solved before release ('disruptive refactors' have been known to introduce serious bugs in some cases)
< petertodd>
luke-jr: remember that you can always run a node not supporting a soft-fork behind one that does to get the same security
< luke-jr>
petertodd: true; we should make this more well-known IMO
< luke-jr>
not sure how many people realise it
< petertodd>
luke-jr: good thing for the release notes!
< gmaxwell>
I always point it out.
< jtimon>
wumpus: I mean the of the kind that present low risk (like file renaming on moveonly commits)
< gmaxwell>
but it's the sort of thing that needs a diagram. :)
< luke-jr>
gmaxwell: did you for Eligius? :P
< CodeShark>
petertodd +1
< maaku>
has anyone actually quantified what level of review is required for a backport?
< * luke-jr>
knows he overlooked it
< jtimon>
wumpus: but I guess you're right about the pre-release preassure
< maaku>
segwit was written against 0.12.
< luke-jr>
actually, I guess it might not entirely work for CSV + mining
<@wumpus>
jtimon: in any case I doubt raneming and moveonly commits are what really makes it difficult to backport things :)
<@wumpus>
jtimon: more things like the mempool changes
< jl2012>
petertodd: except you can't mine any segwit tx with a 0.12 behind 0.13
< btcdrak>
well I'd like to see a refactoring window right after branching, maybe 2 weeks
< jtimon>
but you don't need to backport segwit from the present, you backport it from when it was merged, no?
< gmaxwell>
maaku: it's not been done yet, so no-- but right now in general we're struggling to maintain the backport releases at all, because virtually no one uses them.
< jtimon>
how would any refactor now difficult backporting segwit?
< gmaxwell>
luke-jr: I don't consider that a great configuration for mining, but I think I'd mentioned it for a prior softfork.
<@wumpus>
we're spread really thin - agree fully on the 'don't get involved in a land war in asia' comment
< petertodd>
jl2012: "can't mine" is a smaller group of people than all full node users
< jtimon>
btcdrak: I would like to get refactor code reviewed during that "refactor window", just declaring the week of refactor doesn't make things get merged :p
< petertodd>
incidentally, in my consulting practice I've always advised clients to use architectures where the exact behavior of full node implementations isn't important, which makes upgrades much less risky
< btcdrak>
jtimon: ack
< gmaxwell>
petertodd: careful, though that thinking also drives the "I made my own 'node' implementation and have plugged it directly into the public p2p network! yippie!"
< gmaxwell>
in any case, short on time. Are we done on this subject?
< instagibbs>
8 minutes left
< petertodd>
gmaxwell: well, my advice is to either use a lite-client w/ up-to-date full node, or if you're using bitcoin core, plan to put it behind an up-to-date node
< gmaxwell>
petertodd: perhaps we should talk to harding and write a deployment guide that shows things like that layering and test infrastructure.
<@wumpus>
seems so
< petertodd>
gmaxwell: good idea
< gmaxwell>
Generally you should have a two later setup in any case, don't put your production node up exposed to the internet...
< petertodd>
gmaxwell: that too...
<@wumpus>
other topics? last-minute announcements?
< gmaxwell>
As I've lamented in the past, commercial users just generally don't report issues they encounter. I don't know how to improve that.
< jtimon>
can be after the meeting but...
<@wumpus>
well if bitcoin core's wallet is unusable for commercial use, I wonder what wallet is..
< bsm117532>
FWIW I've been encountering wallet issues, and am working on a patch that addresses some of them, especially WRT segwit.
< gmaxwell>
What I could extra there was related to wallet performance, which phantomcircuit has recently done some improvement (and has more in the pipeline)
< gmaxwell>
wumpus: people use centeralized api providers instead almost universally.
< bsm117532>
But I would like to see removal of "accounts". Is anyone working on that?
<@wumpus>
bsm117532: I tried to introduce a label API for 0.13 to replace it, then deprecate accounts in 0.14
< petertodd>
bsm117532: re: removing accounts, have you checked with joinmarket on what they'll do? they use accounts
<@wumpus>
bsm117532: but that didn't get enough review
< jtimon>
without having to ack or nack #8328 right now, general thoughts about moving consensus files to the consensus folder? I have heard different things, but if it's not clear, I will take it out of the base of my latest consensus branch
< jtimon>
sipa: yeah, I'm assuming after 0.13 branches
<@wumpus>
petertodd: that'd be really ill-adviced, given that we've been discouraging use of them for years, are you sure?
< gmaxwell>
ironically, one of the complaints I got there was that accounts are discouraged. ::shrugs::
< sipa>
though i prefer to leave some things as "base logic" like maaku suggests
< jtimon>
I mean, if people want before I'm all in, but I highly doubt it
< btcdrak>
we should ping belcher
< petertodd>
wumpus: yes, I'm 99% sure - I use joinmarket all the time
< bsm117532>
wumpus: I will review 7729 and try to offer some feedback
< btcdrak>
he's got some homework then...
< maaku>
as I wrote in the PR I think pushing everything used by consensus into `src/consensus` makes the source code less organized and less approachable by newbies
< luke-jr>
I need to get back on the road, bbl
< sipa>
maaku: agree
< jtimon>
sipa: mhmm, you mean making another "base logic" dir and package in the makefile ?
< sipa>
jtimon: well one such piece of base logic is primitives
< petertodd>
maaku: less approchable? I'd argue more - makes it easier to understand what consensus is and isn't
<@wumpus>
gmaxwell: it's also taking so insanely long to remove it
< jtimon>
maaku: I strongly disagree (but maybe not all people are mostly interested in the consensus code like me)
<@wumpus>
gmaxwell: same as I've seen with the label API pull - no one is really interested in it
< sipa>
jtimon: things like prevector and so seem also more broadly usable than consensus
< maaku>
petertodd: that's rarely someone's first question when approaching the code base
< afk11>
I think keeping some base logic outside may work, ultimately serialization libraries might be soft-forks wrt an older consensus implementation
< sipa>
time up
< btcdrak>
well I hate to say it, but its not like the code is that approachable as is...
< gmaxwell>
wumpus: is this the point where I hang my head in shame and point out that I've forgotten there was a label pull?
<@wumpus>
#endmeeting
< lightningbot>
Meeting ended Thu Jul 14 20:01:20 2016 UTC. Information about MeetBot at http://wiki.debian.org/MeetBot . (v 0.1.4)
< jtimon>
sipa: agreed, but libconsensus needs them if we're ever going to separate the library ala libsecp
< instagibbs>
btcdrak, heh
< maaku>
btcdrak: it is way better than when I started
< petertodd>
maaku: my usual advice to people is to follow the block acceptance logic and read the consensus code to understand how the bitcoin protocol works
< maaku>
so please don't make it worse
< sipa>
jtimon: there's no problem in dependong on things outside of consensus
< gmaxwell>
It's my guess that if the label stuff is done removing accounts is much easier, as that the only legit use.
< sipa>
jtimon: we also depend on libstdc++ and an OS, righ
< btcdrak>
maaku: beauty is in the eye of the beholder :-p
< petertodd>
maaku: that's how I learned how the bitcoin protocol works
< jtimon>
no, I mean, if at some point libconsensus becomes its own repo
< maaku>
petertodd: that doesn't require the files to literally be in the same directory.
< jonasschnelli>
Yes. We should reconsider wumpus 7729
< * btcdrak>
is just teasing
< maaku>
when you see #include <script/interpreter.h> you know to find it in the src/script folder
< sipa>
jtimon: ok, so there may be a 3rd repo/library with just data structured and serialization
<@wumpus>
gmaxwell: I proposed an API there; didn't write tests yet as I was just interested if that was what people had in mind, but discussion never really got started
< maaku>
i would very much like a libbitcoinserialization for other purposes
< sipa>
wumpus: agree, i forgot there was a pull
<@wumpus>
then people complain 'there are no tests'... well duh
< maaku>
(there's a lot of api changes to get there though)
< jtimon>
sipa: oh, I see we would separate some tools like prevector and serialization into a library that libconsensus would depend on?
< petertodd>
maaku: being in the same directory makes it very easy to tell people "here's the consensus code, read it" - took me personally awhile to figure out what was an wasn't consensus. Equally, once it gets split into a separate repo, that's basically mandatory.
< jtimon>
that would work
< instagibbs>
"No shit" - wumpus 2016
< petertodd>
instagibbs: ...for president!
< instagibbs>
i lol'd
< * petertodd>
ducks
< petertodd>
maaku: fwiw, I already do that kind of separation in python-bitcoinlib, with everything consensus under bitcoin.core
< bsm117532>
wumpus: FWIW I'm working on a commercial product that will include a bitcoin full node. It needs to fund transactions for many users, so the "label" idea sounds appropriate. But, I'll mostly be querying txids. (Which is one reason I've been working on getting segwit into python-bitcoinlib)
< btcdrak>
I'm going to talk with belcher about accounts see what his plans are
< petertodd>
wumpus: oh, re: joinmarket, I should point out that it uses accounts with watch-only addresses, which isn't the usual accounts usecase that we're concerned about...
<@wumpus>
bsm117532: right - the label functionality is the minimum functionality in that regard that we need to keep, it removes the bookkeeping stuff, but still allows grouping addresses under a name
< gmaxwell>
which, IMO, is the only sensible thing accounts actually does in any case.
< bsm117532>
That's what I gather, looks good. I'll test it.
< btcdrak>
petertodd: maybe they should use my addrindex patch.
< petertodd>
wumpus: yeah, I'll bet you joinmarket would be fine with just labeling functionality - I doubt it ever uses the bookkeeping of accounts
<@wumpus>
petertodd: oh good, in that case there is no problem at all
< gmaxwell>
does account bookkeeping even do anything with watch addresses?
< jtimon>
btw, I'm sure you guys could find things that you don't like in that branch already, please to leave it all for the refactor week
< bsm117532>
Hmmm I would need watchonly addresses attached to my labels.
< petertodd>
btcdrak: only as an option - way more convenient to not have to keep those huge indexes unless you really have too, and joinmarket (should) work just fine with a pruned node (haven't tested that myself yet)
< petertodd>
wumpus: yeah, I may be wrong, but it's probably not a major fix if there even is a problem - sorry for the false alarm!
<@wumpus>
petertodd: yes, joinmarket works with a pruned node
< gmaxwell>
I think joinmarket really wants multiwallet support in any case, what it wants is a watching wallet to track relevant JM transactions...
< jtimon>
what about using utilmoneystr in libconsensus or not? maaku and I are in disagreement here
< jtimon>
oh, damm, he left
< sipa>
jtimon: i say get rid of that formatting
< sipa>
it's debug output
< gmaxwell>
anything with "str" doesn't sound like it belongs in consensus code. :P
< sipa>
in the long term, concensus logic should not do error formatting
< petertodd>
gmaxwell: we need a semi-consensus library for stuff like that :)
< petertodd>
gmaxwell: maybe, like dante's rings of hell...
< jtimon>
yeah, me too, but he says showing satoshis would be breaking CAmount's encapsulation by treating it as an integer. I mean, I would be happy with not printing the values at all, but I guess that's worse
< jtimon>
gmaxwell: well, I wasn't planning on removing tinyformat, at least not yet, but this is one line