< GitHub117>
[bitcoin] nomnombtc opened pull request #8568: new var DIST_CONTRIB adds useful things for packagers from contrib (master...DIST_CONTRIB) https://github.com/bitcoin/bitcoin/pull/8568
< midnightmagic>
Lots of people getting into the gitian builds. Surprising number, really. A lot of people are taking action as a result of that warning.
< luke-jr>
midnightmagic: it makes me more confident Bitcoin in general can gain user awareness and security needed to succeed
< midnightmagic>
This is one of those things: lots of people want to help but either don't know how or can't think of what *they* can do to help.
< gmaxwell>
There are only 200 or so publically reachable nodes running 0.13. ... something else people can do. :) upgrade their nodes.
< luke-jr>
gmaxwell: I actually moved most of my hot wallet funds out today in preparation for upgrading mine (and hopefully being able to run it 24/7 again)
< GitHub121>
[bitcoin] rebroad opened pull request #8571: Don't disconnect just because a node's services have changed. (master...UnexpectedServicesNoDisconnect) https://github.com/bitcoin/bitcoin/pull/8571
< GitHub193>
[bitcoin] jonasschnelli opened pull request #8573: Set jonasschnellis dns-seeder filter flag (master...2016/08/filter_seed) https://github.com/bitcoin/bitcoin/pull/8573
< wumpus>
midnightmagic: yes, lots of gitian builders this time around, this is great
< wumpus>
sipa: though those accumulated over a longer time
< wumpus>
sipa: very likely 0.13.0 will still be a record
< sipa>
yeah
< jonasschnelli>
The CWallet/CWalletDB/CDB interleaving is really a mess. I fear nobody will review the just opened refactoring PR. They are much larger then I had originally expected.
< jonasschnelli>
But if we want to add support for a different wallet database format, we need better abstraction of wallet logic, wallet database logic and pure database storage logic
< jonasschnelli>
Somehow I think we need to do this before we add Multi Wallet Support,...
< wumpus>
agree that some refactoring of the wallet code is long due
< sipa>
jonasschnelli: will have a look
< wumpus>
but yes, assuring that it doesnt introduce nasty bugs or funcitonality people are relying on is hard
< wumpus>
+break
< wumpus>
which reminds me we still haven't add the label API to replace the accounts API
< jonasschnelli>
Yes. I try to move the BDB code carefully and keep its behavior (even if it silly sometimes).
< wumpus>
that makes sense for a refactor
< jonasschnelli>
wumpus: We should have a look at your PR
< jonasschnelli>
Will re-trigger travis on that PR
< wumpus>
yes, the code is pretty much outdated and useless at this point, but we should make sure at least the API is what we want
< gmaxwell>
FWIW, phantomcircuit spent basically months with a bunch of wallet refactors pipelined waiting for changes to go in.
< gmaxwell>
If people turn in broken up changes, they get randomly stuck waiting, presumably because few care to review things that don't seem important. If people submit large all at once changes, they get stuck because no one is able to review big changes.
< wumpus>
this is especially true for the wallet
< jonasschnelli>
Indeed. Maybe I should split up the PR in smaller chunks..
< wumpus>
and, perversely, the refactors would probably make it easier to review changes in the future
< wumpus>
but quite some people get a headache just for looking at the wallet code as it is now, which prevents them from reviewing it, and thus from those (necessary) changes from getting in
< gmaxwell>
what I'm saying is that phantomcircuit did split up his changes into chunks, submitted the first one, then it sat for ages.
< * jonasschnelli>
will have a look at phantomcircuit PRs
< wumpus>
yes, that's how it has always gone with wallet changes, even CodeShark's multiwallet PR back in the day sat for ages, without useful review or testing
< gmaxwell>
He had more pipelined, but I don't know if he gave up, or if its just because he's been sick that he hasn't PRed more of them since the longest standing went in just recently.
< gmaxwell>
probably the latter, he's not prone to giving up.
< gmaxwell>
:)
< wumpus>
it's just difficult to do, I don't have any useful advice for anyone trying unfortunately, as you say both breaking the change up as well as doing a lot at the same time run against barreirs
< wumpus>
changing the consensus code seems easier :)
< GitHub147>
bitcoin/master e86eb71 Patrick Strateman: Move CWallet::setKeyPool to private section of CWallet
< GitHub147>
bitcoin/master 8680d3a Patrick Strateman: Move wallet initialization logic from AppInit2 to CWallet::InitLoadWallet
< GitHub147>
bitcoin/master f916700 Wladimir J. van der Laan: Merge #8445: Move CWallet::setKeyPool to private section of CWallet....
< GitHub83>
[bitcoin] laanwj closed pull request #8445: Move CWallet::setKeyPool to private section of CWallet. (master...2016-07-01-cwallet-api-cleanup) https://github.com/bitcoin/bitcoin/pull/8445
< jonasschnelli>
I'm happy to co-maintain the wallet. I think I can now understand most of its code (also the silly one)
< wumpus>
thanks, you're very welcome to participate in wallet changes, but I think it'd be good to bring someone else on board
< wumpus>
otherwise you end up with everything on your plate
< jonasschnelli>
Yes. Agree.
< murch>
What responsibilities does being the wallet maintainer entail?
< wumpus>
work on the wallet, do a lot of work on the wallet, and review other people's work on the wallet
< jonasschnelli>
Review PRs, make sure we make progress with the wallet
< wumpus>
and have a very strong clue about the crypto and how the parts interact
< wumpus>
and doesn't get dizzyness and/or headaches or bouts of panic while reading the current wallet code, or changes to it
< sipa>
i'd love to spend time on making bitcoin core function as an SPV client, and make the wallet changes that are necessary
< wumpus>
(as I do)
< sipa>
but i think i have a bit too many other things to do first
< wumpus>
yes, I think you have enough on your plate too
< murch>
Unfortunately, I'm not really familiar with the crypto used in the wallet. I mean, I know in broad strokes what it is supposed to do, but I'd be lying if I said I have a strong clue.
< GitHub15>
bitcoin/master 7bd5ff4 Christian Barcenas: Trivial: Fix two VarInt examples in serialize.h
< GitHub15>
bitcoin/master f12d2b5 Pieter Wuille: Merge #8560: Trivial: Fix two VarInt examples in serialize.h...
< sipa>
yes, i'm not volunteering for the wallet maintainer position... just saying i'd like to work more on it :)
< GitHub188>
[bitcoin] sipa closed pull request #8560: Trivial: Fix two VarInt examples in serialize.h (master...fix_varint_examples) https://github.com/bitcoin/bitcoin/pull/8560
< jonasschnelli>
I guess there are plenty of node signaling NODE_BLOOM, though not sure if they are really offering the service
< sipa>
can i have ACK on 8451?
< jonasschnelli>
sipa: Not sure if the SPV mode would be utterly complex.
< sipa>
jonasschnelli: it won't be, but there are a few subtle changes to make
< jonasschnelli>
A first step would be to allow the wallet to run in mode without mempool knownlage
< jonasschnelli>
Then a way how the wallet can "ask for blocks",... main.cpp needs to download them, check headers (SPV pure PoW check) and passes the txes through the SyncWithWallet signal
< wumpus>
I don't like #8451 very much, it seems a step the wrong way around
< gmaxwell>
using undefined behavior isn't acceptable however.
< jonasschnelli>
sipa: what are the benefits of 8451?
< wumpus>
no, it's not, but that dosn't mean I have to like the solution
< sipa>
jonasschnelli: fixing our code, which is currently undefined (as in: the compiler is allowed to make it fire all the nukes)
< sipa>
wumpus: understood, and agree
< wumpus>
I really like the idea of having a transaction-set-ins-stone object, as well as a transaction builder (ImmutableTransaction). We should have that for more things, such as Script
< sipa>
but i think a better solution is too far off (modifying the serializing framework to support constructing-while-deserializing)
< sipa>
or encapsulating all fields in CTransaction
< wumpus>
it separates building and checking at aconceptual level
< gmaxwell>
wumpus: by builder, do you imagine that a transaction would always be built in one atomic step?
< sipa>
hmm, maybe there is a possibility to change all the places where CTransaction is deserialized to first deserialize into a CMutableTransaction, and then convert to a CTransaction?
< sipa>
there may not be very many of those
< wumpus>
gmaxwell: no. Not necessarily, but it is a "MutableTransaction" scaffold until it no longer needs to be one
< gmaxwell>
gotcha.
< sipa>
let's try.
< gmaxwell>
Thats really what I thought the intent was there in the past.
< wumpus>
sipa: sounds quite sensible
< wumpus>
the serialization framework *needs* a mutable object,so give it one
< gmaxwell>
(in particular making CTransaction always fully immutable allows changing its representation to an efficient one)
< wumpus>
gmaxwell: exactly!
< sipa>
gmaxwell: it requires more than that though
< sipa>
(also encapsulating its fields)
< gmaxwell>
I know.
< wumpus>
sipa: sure, it's only one step
< gmaxwell>
(as in one malloc per CTransaction, and only one-- oh to dream)
< wumpus>
that's what I meant with that making Transaction mutable is a step in the wrong direction
< wumpus>
just a step, not armageddon
< gmaxwell>
well, I think in C++ we shouldn't equat const/non-constness with immutability. They're partially orthorgonal concepts.
< gmaxwell>
er equate*
< wumpus>
I agree, though const helps enforce that at the compiler level
< sipa>
ugh.
< gmaxwell>
oh no, sipa found something awful, I can tell.
< sipa>
no more CTransaction::operator= or CTransaction::swap
< wumpus>
uh oh
< wumpus>
hmm. atomic replacement is ungood too?
< wumpus>
when you regard CTransaction as a reference to a transaction, those may be acceptable
< wumpus>
otherwise, yes, it's mutating
< wumpus>
though the future 'CTransaction needs one malloc' would imply it has one pointer, which could trivially be swapped with another one
< wumpus>
assignment would still need copying, or a shared pointer
< gmaxwell>
wumpus: well no, because a single malloced chunk could have pointers internally... so you can't say it implies it alone
< gmaxwell>
though obviously it would be better if it didn't. :)
< wumpus>
gmaxwell: oh, sure, then you'd need to do some more accounting
< wumpus>
I don't mean 'atomic' in the threading sense
< gmaxwell>
I was reading 'trivially swapped' meaning not needing any deep operations.
< wumpus>
it doesn't need any deep operations, just swap the surface pointers
< wumpus>
if there are more, pointing inside the structure, well swap all of them
< wumpus>
but if you have all pointers pointing inside the stucture as part of the malloc'ed structure itself you can easily keep it to one pointer
< wumpus>
depends on how much indirection is a performance bottleneck I guess...
< gmaxwell>
wumpus: fyi there was a thread on bitcointalk recently where someone profiled bitcoin core, and was asking why we weren't using faster sha2 because it was 35% in their benchmarks. I pointed them to your work with the SIMD sha2. They seemed to think that there would be 10x speedups, so I was sad to have to disappoint them. :)
< wumpus>
gmaxwell: yeah it was disappointing :( would still make some sense to integrate that, for a bit of speedup, but the overall gains were so little it demotivated me at least
< gmaxwell>
they were clearly measurable at least.
< wumpus>
but if anyone wants to continue that work they're welcome to, of course
< gmaxwell>
Thats something to be proud of, even if they aren't huge. They'll become more important as everything else gets optimized.
< wumpus>
true
< wumpus>
which reminds me of #8524
< gmaxwell>
he also commented on the leveldb crc
< wumpus>
hah :)
< MarcoFalke>
jonasschnelli: Can you explain?
< MarcoFalke>
we don't have auto stretch in master
< MarcoFalke>
I think
< jonasschnelli>
MarcoFalke: let me look at the code..
< jonasschnelli>
It just looks strange (part of the row don't use full width)
< wumpus>
and it works, I verified it's the same ariant of CRC (crc32c) and has the same output
< wumpus>
what is lacking/to be done: CPU detection at startup, replace those functions by function pointers, based on the best implementation for detected CPU
< wumpus>
in the case of leveldb maybe trying to upstream it
< jonasschnelli>
wumpus: nice work!
< wumpus>
thanks :)
< jonasschnelli>
Not sure hoe active upstream is
< * wumpus>
should finish more projects instead of starting more
< wumpus>
agreed, that's why that's a 'maybe', normally it'd be a requirement
< jonasschnelli>
Indeed. :-) 3xpings from paveljanik
< jonasschnelli>
But UniValue upstream is now bitcoin-core/univalue, right?
< wumpus>
though I think all of recent leveldb changes have been OS-support changes, not so much consequential database performance changes, but I haven't looked very recently
< MarcoFalke>
No, I think we keep bitcoin-core/stuff only for bitcoin-core related patches
< wumpus>
(or correctness fixes for that matter)
< wumpus>
jonasschnelli: well, effectively, for bitcoin it is. I think we're the only client for univalue anyhow.
< jonasschnelli>
Digital Bitbox also depends on UniValue. But thats it I guess.
< jonasschnelli>
IMO if jeff is not maintaining it, we should take care about bitcoin-core/UniValue
< paveljanik>
first, we should ask jeff.
< wumpus>
I tend to agree, though I can't spend much time being maintainer of yet another project
< MarcoFalke>
pff, it is always a mess to have several repos on github and no one knows whcih one is maintained
< wumpus>
bitcoin-core's is maintained, in the sense that it will always be the best version for our use
< wumpus>
well I know only one thing for sure: Jeff's isn't
< wumpus>
I'll merge patches when necessary for bitcoin or general performance/correctness
< wumpus>
which reminds me I still need to cean up my univalue fuzzing framework some time
< gmaxwell>
I don't know if andytoshi published his test harness. He rewrote a functional equivilent to univale in rust, then wrote a test harness that compared the two against each other and found a number of bugs in univale (though I believe they were all already bugs wumpus had found).
< * sipa>
tries compiling after making CTransaction deserializable-at-construct-time
< gmaxwell>
I think thats a really really useful testing technique for a parser.
< wumpus>
jonasschnelli: in any case univalue is a good choice when dealing with bitcoind client-side from C or C++, I don't think using it is a bad chocie, maybe it just needs very little maintenance
< wumpus>
gmaxwell: yes having two implementations and comparing them is a good way of testing
< wumpus>
gmaxwell: boost is so difficult to debug, with all the c++ template magic
< wumpus>
gruesomely overdesigned, even for something like time getting/conversion
< wumpus>
paveljanik: I think it is spurious, don't take my word for it though
< jonasschnelli>
paveljanik: agree with wumpus
< jonasschnelli>
How could such code end up here in the first place. :-)
< paveljanik>
I think it was cut&pasted...
< paveljanik>
the same line is present 6 times in our sources...
< wumpus>
it makes sense to have (de)serialization code there, as those entries are (de)serialized
< wumpus>
paveljanik: but the big question is, does gcc even generate code for them :D
< wumpus>
it would only make sense if there is a local veriable nVersion
< GitHub158>
[bitcoin] ajtowns opened pull request #8575: leveldb: generate lib independent of locale sort (master...leveldb-locale-reproducible) https://github.com/bitcoin/bitcoin/pull/8575
< wumpus>
which there isn't, so it'sa self-assign
< wumpus>
EH. Well there is, one of the arguments
< paveljanik>
yes 8)
< wumpus>
so take care that if nVersion is used after that line, you can't remove the line (without changing the use of nVersion to this->nVersion)
< paveljanik>
and this is where I'm lost ;-)
< wumpus>
having the structure field have the same name as a parameter is unadvisable here, as it causes confustion
< wumpus>
confusion*
< wumpus>
I think I get it
< paveljanik>
nVersion is not used at all after this line in all 6 cases.
< wumpus>
the same function is used for reading and writing
< wumpus>
on writing, this->nVersion is written to the serialization
< wumpus>
on reading, this->nVersion is read from the serialization, then assigned to nVersion
< wumpus>
but why? nVersion argument to Serialization is of a different sort than nVersion used internally
< wumpus>
.?!?!?? core dump
< wumpus>
I know I just copied that code, I can't have written it :)
< MarcoFalke>
The code is copied from the initial git commit
< paveljanik>
we have to ask the "TwoSpaces" man ;-)
< MarcoFalke>
ask satoshi or something
< wumpus>
I'd be very careful with touching it though or the whole pyramid may come down
< wumpus>
hah yes
< paveljanik>
yes, definitely
< wumpus>
this is why I wondered whether gcc generates code for it in the first place
< wumpus>
"-*- sipa tries compiling after making CTransaction deserializable-at-construct-time", that was his last comment and then he went silent, ominous
< gmaxwell>
haha
< phantomcircuit>
gmaxwell: im waiting on 8450 before making more changes
< phantomcircuit>
the next one is to change the accounting tests, but that needs to actually change the tests themselves
< wumpus>
are you waiting on that one? let's merge it, it's a test change and two acks already
< phantomcircuit>
so it's a bit more involved
< phantomcircuit>
(basically im going to be removing like 50% of the tests since they're nonsensical)
< GitHub65>
bitcoin/master 9578333 Patrick Strateman: Remove rpc_wallet_tests.cpp
< GitHub65>
bitcoin/master 21857d2 Wladimir J. van der Laan: Merge #8450: [Test] Replace rpc_wallet_tests.cpp with python RPC unit tests...
< phantomcircuit>
wumpus: yeah i was waiting
< GitHub112>
[bitcoin] laanwj closed pull request #8450: [Test] Replace rpc_wallet_tests.cpp with python RPC unit tests (master...2016-08-03-remove-rpc-wallet-tests) https://github.com/bitcoin/bitcoin/pull/8450
< phantomcircuit>
great there's now only a single thing external to CWallet using CWalletDB which is the accounting_tests.cpp
< phantomcircuit>
which mostly make no sense
< wumpus>
why does it use CWalletDB?
< wumpus>
or maybe better not to ask, sorry
< wumpus>
I'm sure accounting tests can be done with just a CWallet object
< phantomcircuit>
indeed better not to ask (it's adding the accounting entries directly...)
< phantomcircuit>
walletdb.ListAccountCreditDebit is the culprit really
< wumpus>
oh, crap related to the account sytem
< sipa>
accounting details are the only thing from wallet db we don't load into memory
< wumpus>
if we only removed that functionality already, we could have effortlessly gotten rid of this
< paveljanik>
wumpus, qt_libbitcoinqt_a-recentrequeststablemodel.o is different when this line is commented, so it is used...
< gmaxwell>
wumpus: https://bitcoin.org/laanwj-releases.asc should probably come with the signature of that key by your personal key. Keyserver retention of keys that are leafs isn't always all that good.
< paveljanik>
but I think this line has no meaning in the read case as the value is assigned to the shadowed nVersion, argument one, and discarded at the end...
< wumpus>
gmaxwell: yes, good idea
< wumpus>
gmaxwell: do I need to do anything special for that? I just did gpg --export --armor 0x90C8019E36C2E964 > ../websites/bitcoin.org/laanwj-releases.asc and git sees no difference
< gmaxwell>
hm. maybe I'm wrong and it has it already, let me check.
< gmaxwell>
it does, I'm sorry to have wasted your time.
< wumpus>
ok, well good to know
< gmaxwell>
someone in #bitcoin said it didn't, which primed me to conclude it didn't when I looked. :)
< wumpus>
should probably update my key at bitcoin.org with all the new signatures though
< arubi>
MarcoFalke, <3 "swinging the (block)chain.."
< sipa>
MarcoFalke: it was painful!
< MarcoFalke>
There is more pain to come
< MarcoFalke>
I suggest you don't look at the travi result
< sipa>
already on it
< cfields_>
wumpus: yes, boost::chrono is very much in my sights. Got to dump boost::condition_variable first, though.
< cfields_>
wumpus: I'm making headway on killing off the interruptible threads. I slashed a bunch of them last week, but got distracted with something else.
< kanzure>
wumpus: is there an issue or pull request for http/2 things?
< kanzure>
wumpus: specifically i am wondering what you meant by "http streaming"