< bitcoin-git>
bitcoin/master db07f91 practicalswift: Assert that what might look like a possible division by zero is actually unreachable
< bitcoin-git>
bitcoin/master d32581c Wladimir J. van der Laan: Merge #9547: bench: Assert that division by zero is unreachable...
< bitcoin-git>
[bitcoin] laanwj closed pull request #9547: bench: Assert that division by zero is unreachable (master...avoid-potential-division-by-zero-in-benchmark-state-keeprunning) https://github.com/bitcoin/bitcoin/pull/9547
< wumpus>
I've noticed that there is a lot of overlap between dbwrapper.h and walletdb.h
< wumpus>
yeah
< jonasschnelli>
Yes. We need clean layering first to support multiple db backends
< wumpus>
I really like the dbwrapper.h. With that I've mangaed to replace the db backend multiple times. Implementing e.g. a walletdb.h for leveldb seemsto be more work
< wumpus>
(as it doesn't wrap some of the objects)
< * jonasschnelli>
looking closer at dbwrapper.h/cpp
< wumpus>
then again - a pure key/value store may not be what we eventually want for the wallet at all, maybe something like sqlite with indexes is better. But on the short term it'd be useful to switch database backends and just consider key/value stores as a given, abstracted.
< jonasschnelli>
Yes. Something like that for the wallet db would be great. I first though CDBEnv follows that approach... but not really
< wumpus>
I think they evolved from the same abstraction, but dbwrapper was updated time and time again to be a better abstraction, while walletdb did not :)
< jonasschnelli>
Sqlite could be useful. I though about it.... but not sure if we want private keys in there.. also not sure about the compatibility between different sqlite implementations..
< jonasschnelli>
I still like sipa logdb approach (full in-mem mapping, no on-disk database queries, simple append only log with on-going hash)
< wumpus>
sqlite actually encourages usage as an applicatiion data format in their faq
< wumpus>
so I *think* that means they make a commitment to on-disk compatibility. Also their databases are single files, unlike leveldb.
< wumpus>
yes sure I like that too
< wumpus>
in any case, for everything we need to improve the abstractions involved
< jonasschnelli>
For pure transactions and pubkey, I would probably prefere sqlite
< wumpus>
right - and with HD wallets we need a lot less key storage
< jonasschnelli>
Yes. Lets focus on abstraction and discuss the new persistence layer later.
< wumpus>
+private
< sipa>
the abstractions are complicated by the fact that BDB needs some things (flushing, locking, environment, ...) that nothing else needs
< jonasschnelli>
Yes. The abstraction needs to support that (at least for the next years)
< sipa>
no, only until we get rid of BDB
< sipa>
i hope that's less than a few years
< MarcoFalke>
Anyone want to review #9880?
< gribble>
https://github.com/bitcoin/bitcoin/issues/9880 | Verify Tree-SHA512s in merge commits, enforce sigs are not SHA1 by TheBlueMatt · Pull Request #9880 · bitcoin/bitcoin · GitHub
< jonasschnelli>
hmm.. if we offer a good migration option,... but the migration option needs BDB support... :)
< MarcoFalke>
Currently only wumpus can merge on master
< sipa>
jonasschnelli: that can just be a trivial "iterate the keys, convert" - not a full db abstraction
< jonasschnelli>
sipa: Indeed
< jonasschnelli>
wumpus, sipa: if you care for wallet db abstraction, this (#8574) could be reviewed:
< wumpus>
jonasschnelli: I'll take a look. On the short run I just need something that can replace berkeleydb with a different k/v store, no need for any backward compatiblity or migration
< jonasschnelli>
Thanks. Yes. That's a different path then...
< wumpus>
jonasschnelli: not using a database or a custom database would be fine too, but that seems like a lot more work :)
< wumpus>
and would require a huge patch set to maintain on top
< wumpus>
which would need to be updated for every upstream wallet change
< wumpus>
so yeah, switching the k/v store is probably the best option
< Victorsueca>
I just had a question this morning while at was at shower... Is it safe to defrag while bitcoin core is running?
< wumpus>
defrag? is that still a thing?
< BirneGetreide_>
Just don't defrag, ever. It is a waste of time
< jonasschnelli>
Oh... verify-commits.sh gave a "Segmentation fault: 11"! hah
< jonasschnelli>
I hope it's not coming from gnupg
< wumpus>
but no, we can't guarantee that that is safe. Moving filesystem blocks in the background could be safe or unsafe depending on whether the OS keeps track of things properly.
< Victorsueca>
it's windows so I doubt it does lol
< wumpus>
right, you didn't even need to ask
< jonasschnelli>
Hmm... I think the verify-commit.sh segfault 11 im getting is from /bin/sh
< jonasschnelli>
It happens when the script calls IS_SIGNED() (recursive call)
< jonasschnelli>
Happens on master was well...
< wumpus>
ugh :/
< jonasschnelli>
Can't even attach lldb to /bin/sh due to OSX's "integrity protection"... copied out sh and running now in lldb
< jonasschnelli>
Would it make sense to port verify-commits.sh to python? We already rely on python for the rest, right? ping BlueMatt
< jonasschnelli>
BlueMatt: it seems that the current amount of recursive calls on IS_SIGNED() seems to break some shell implementations (OSX 10.11 and I think also Ubuntu 14.04)
< wumpus>
can we somehow make verify-commits.sh simpler, at least temporary?
< wumpus>
e.g. do the most important check that the HEAD commit is signed, but no more, that shouldn't take any recursion
< wumpus>
then after the script is fixed we can go back to the more comphrehensive one
< jonasschnelli>
wumpus: Yes. That's probably a good idea... maybe we add an arg and by default, it just checks the head
< bitcoin-git>
[bitcoin] jonasschnelli opened pull request #9928: Allow verify-commit.sh to just verify the HEAD commit (Use non-recursive verification by default) (master...2017/03/vc_simple) https://github.com/bitcoin/bitcoin/pull/9928
< morcos>
Call for another set of eyes on #9602.. I think it has enough technical review now, but couldn't hurt to have some more people review the open questions in the PR message and make sure they are ok with the changes made.
< gmaxwell>
wumpus: for the unix domain socket bitcoin-cli, could you have the client behind an actual feature test? so that when the user's libevent is upgraded it starts working?
< wumpus>
gmaxwell: once my patch lands in mainline libevent, it can be enabled based on the libevent version
< wumpus>
currently there is no way to detect whether libevent can do it or not
< wumpus>
if it takes a long time to be included in libevent we could add the patch to depends so that the released binaries will have it
< wumpus>
can always decide on that when the 0.15 release gets close and it's still uncertain
< gmaxwell>
has the libevent people given any feedback suggesting if they're likely to accept the change or not?
< wumpus>
nope
< gmaxwell>
might be good to get that, if they wont we might want a different approach. :(
< gmaxwell>
(than potentially carrying a patch with release binaries forever)
< wumpus>
well it's still useful even without the client change
< wumpus>
and there is no other approach to support it in bitcoin-cli anyhow
< BlueMatt>
anyone able to reproduce the travis master fail?
< BlueMatt>
verify-commits is failing again but not locally anymore
< wumpus>
not that I know of, at least, maybe libevent people could suggest something, butI haven't had feedback on it at all
< gmaxwell>
BlueMatt: verify commits is failing for me on the 0.14 branch, I haven't checked master today.
< wumpus>
gmaxwell: wow that's depressing :/
< wumpus>
they've had, what, two days including a sunday to respond, I wouldn't worry about "carrying a patch forever" yet
< BlueMatt>
i didnt try the python code, but the bash-based snippet doesnt work here
< wumpus>
verify-commits.sh passes on master here, locally
< BlueMatt>
found the issue...needs gpg 2.1 to pass --weak-digest
< BlueMatt>
also the above tree-sha512 issue
< wumpus>
can't reproduce that one either,tree_sha512sum() gives dc5cf61d226a16a77f32fab8abc18fd469dfe8bda59a2cb8f9eca3a60e474f180dec4fa59bb3fee86efa0e123ba8c198c7efe76bfdaa2518f4169ab0849f6694
< BlueMatt>
yea, that looks like what i (and verify-commits) gets
< wumpus>
it's not causing verify-commits.sh to fail here though
< BlueMatt>
you have to run verify-commits with --tree-checks
< BlueMatt>
(because its slowwwww)
< BlueMatt>
next pr will fix all above issues, and also I think i should do --tree-checks but only for top commit
< BlueMatt>
so that travis will catch this
< BlueMatt>
but not for more because otherwise travis would take hours by the time we get to 0.15
< wumpus>
I agree, we should avoid making the check slower
< wumpus>
checking just the top commit or top two commits thoroughly should be enough, travis runs often enough
< BlueMatt>
yes
< BlueMatt>
it should run every commit, no?
< BlueMatt>
we might see it fail hours later, but at least it runs for each one
< wumpus>
I don't think that's guaranteed. It runs every time it sees a new push
< BlueMatt>
ahh, yes, race
< wumpus>
which usually means it runs for every top-level commit as the merge script pushes them one at a time, but still
< wumpus>
what is it with all those people reporting Ekiga build problems in our issue tracker
< sipa>
ekiga?
< wumpus>
apparently some VOIP program
< sipa>
and what are they reporting? i haven't seen that
< wumpus>
I first didn't notice they were talking about a different application
< gwillen>
well, one person happened to get a match on the text of the error message
< sipa>
hah
< BlueMatt>
wtf
< wumpus>
in any case, configure arguments suggestions port very badly between different applications
< gwillen>
then google indexed the word 'ekiga' when they said it, and now everyone's getting linked to this bug when they google 'ekiga' and the error message :-)
< wumpus>
lol that must be it
< gwillen>
indeed, I just checked, that bug is the first hit for the error, and the second hit for 'ekiga' plus the error text.
< BlueMatt>
wumpus: i cant verify your latest sha512 either
< wumpus>
sigh :/
< wumpus>
is it possible that the order is not deterministic?
< BlueMatt>
hmmm, now I'm confused
< BlueMatt>
i doubt it?
< wumpus>
is it hashing any files which are not part of git, but linger around in the directory?
< BlueMatt>
hmm, maybe I'm wrong, maybe your hash is right
< BlueMatt>
cant tell from terminal history anymore :(
< wumpus>
I'll check in a minute, after I push my tree
< wumpus>
for me it matches
< wumpus>
(using the python code, output is e55ce10bf7f2dc91de9797e60ab7767fb51f25255995d62ddf358c52b7aaa23c26fbfb522e1610ff950b86804ddbc38dc0d7708bfab2c4d33ad99a275d8c77db, which matches what is in the merge commit)
< wumpus>
"git ls-tree --full-tree -r --name-only HEAD | LANG=C sort | xargs -n 1 sha512sum | sha512sum" gives the same output
< BlueMatt>
yea, heisenbug
< BlueMatt>
travis is failing on it but i cant find it
< BlueMatt>
wumpus: yes, sipa meant to say LC_ALL when he said LANG (according to the sort man page)
< sipa>
i thought i meant to say C, but maybe i thought wrong about what i meant
< BlueMatt>
sipa: no, you have to set LC_ALL=C, not LANG=C
< bitcoin-git>
[bitcoin] TheBlueMatt opened pull request #9932: Fix verify-commits on travis and always check top commit's tree (master...2017-03-fix-verify-commits) https://github.com/bitcoin/bitcoin/pull/9932
< sipa>
BlueMatt: ahh!
< wumpus>
whoops, yes, stupid locales :/
< BlueMatt>
why travis has a different locale is beside me
< wumpus>
the python version in github-merge.py shouldn't be affected
< wumpus>
no idea what locale it sets, but it helped find this issue
< wumpus>
which I suppose is good
< BlueMatt>
fair
< bitcoin-git>
[bitcoin] jtimon closed pull request #9882: RPC: Introduce -rpcamountdecimals for the RPC to use other units than BTC (master...0.14.99-rpc-amounts) https://github.com/bitcoin/bitcoin/pull/9882