< paveljanik>
kanzure, how many parallel discussion are you able to type? Thanks for the Zurich log! I'll read it all weekend ;-)
< sipa>
paveljanik: on tuesday we were separated into groups, and kanzure was only in one at a time
< sipa>
eh, on saturday
< sipa>
otherwise, i think he transcribed most
< paveljanik>
anyway, unbelievable :-)
< sipa>
and he types faster than most people can speak
< sipa>
happy that the transcript is useful
< sipa>
i've wanted to go through it and annotate with context and clarifications
< sipa>
but it's so huge...
< paveljanik>
13 printed pages of 4 A4 on 1 A4...
< sipa>
feel free to ask questions though
< sipa>
it's sometimes hard to follow
< sipa>
spoken word is not always very consistent :)
< paveljanik>
thanks. I'll read it in the silent evening, in the wild, without connectivity 8)
< cfields_>
jonasschnelli: ping
< jonasschnelli>
cfields_: pong
< cfields_>
jonasschnelli: ooh, that was quick :)
< jonasschnelli>
latency?
< jonasschnelli>
:-)
< cfields_>
jonasschnelli: i'd be grateful if i could get a quick opinion on the qt part of the net refactor, when you have a few min to spare
< jonasschnelli>
cfields_: sure... is there a PR or a branch I can look at?
< cfields_>
jonasschnelli: for now, qt is using one global everywhere. That works for now, but it's not ideal. I'm not sure where to start with passing it around properly
< sipa>
cfields_: clientmodel ?
< sipa>
and perhaps walletmodel
< cfields_>
sipa: that seemed too easy, i was hoping that'd be the answer :)
< cfields_>
(that's no ideal, i discussed some potential future work with wumpus. I think you've previously discussed more of a pull model for broadcasting txs, this would fit in nicely with that)
< jonasschnelli>
cfields_: So, CConnman is the p2p abstraction from the wallet perspective?
< jonasschnelli>
Ideally I'd like to see a node abstraction from the wallet perspective.
< cfields_>
jonasschnelli: yes
< jonasschnelli>
like node->estimatefee, node->broadcastsomething, etc.
< jonasschnelli>
but your step seems right.
< * jonasschnelli>
is looking how we interact with that in the GUI part
< cfields_>
jonasschnelli: well, this takes care of the 2nd, but not the first. For ex, you may be running without networking, but still want to commit a tx for later broadcasting
< cfields_>
jonasschnelli: sec for an example there
< cfields_>
sipa: btw, I know I owe you some reviews/acks/PRs. Not forgotten.
< jonasschnelli>
cfields_: the GUI also uses wallet->CommitTransaction directly
< cfields_>
jonasschnelli: for now, I'm just using a global there because I'm not sure how to pass the instance properly. Should it be as sipa suggested, in clientmodel?
< jonasschnelli>
cfields_: looks good!
< sipa>
cfields_: none of the gui code should know anything core related, ideally
< sipa>
so certainly not about connman
< jonasschnelli>
I think walletmodel is fine...
< gmaxwell>
"developers adding conman to bitcoin core"
< sipa>
i think the connman connnnman
< gmaxwell>
:P
< cfields_>
sipa: hmm, why not? It's cleanly separated from the rest of bitcoin, and knows nothing of transactions/blocks/etc. Seems abstract enough for the gui to deal with directly?
< jonasschnelli>
cfields_: so right? the wallet does not keep track of the conman?
< cfields_>
sipa: or do you have in mind an abstraction layer on top of that?
< jonasschnelli>
the wallet object itself is not capable of broadcasting a transaction?
< cfields_>
jonasschnelli: for now, the wallet is passed an instance of connman. Ideally in the next refactor round, we'd switch to a pull model, where the application layer requests tx's to be broadcast, and sends them to its connman instance
< jonasschnelli>
What about having some glue-code that abstracts the "node". Chain could be <core>-<connman>-<node-api>-<GUI/clientmodel>-<GUI/gui>?
< cfields_>
cfields_: maybe the pull model ^^ is what you had in mind?
< jonasschnelli>
cfields_: g_connman is a global singleton?
< cfields_>
jonasschnelli: by node, you mean a remote peer like CNode? I'm not sure what you mean by that chain, really
< gmaxwell>
ConnmanWrapperFactorySingleton.
< cfields_>
jonasschnelli: It's a global now simply for convenience. The global needs to go away though, that's the part I'm working on
< jonasschnelli>
cfields_: Sorry. Confusing. With "node glue code" I was not thinking of a CNode object,... with "node glue code" I was more thinking of a full-node abstraction that could expose "boardcast", "estimatefee", "syncblocks", etc.
< jonasschnelli>
cfields_: maybe for now you access g_connman from clientmodel.cpp and broadcast from there?
< cfields_>
jonasschnelli: the global is used only in the qt code and rpc, simply because I haven't done the work yet to pass the instance properly. It's a temporary hack.
< jonasschnelli>
Or an alternative solution would be to use g_connman from walletmodel.cpp
< jonasschnelli>
Ideally the wallet should only share base classes and only talk to the core code over a single api (node-glue-code).
< cfields_>
ok, thanks. I'll have a look at that.
< jonasschnelli>
the api/glue-code could then once be replaced with RPC/ZMQ or similar.
< cfields_>
jonasschnelli: yes, I see what you're getting at
< sipa>
cfields_: why does the gui itself need to know anything about connman?
< jonasschnelli>
lol
< sipa>
cfields_: all interactions with it should already be going through clientmodel
< jonasschnelli>
sipa: because there is no glue-code. :)
< cfields_>
sipa: i'm pretty sure the problem is simply that I have no idea how the gui code is structured
< jonasschnelli>
cfields_: the idea is that clientmodel "talks" to the core objects.
< jonasschnelli>
But IIRC there are many places where the GUI talks directly with a core object/instance.
< cfields_>
i see. So rather than using g_connman to ban a peer, it should be requesting that the clientmodel bans on its behalf?
< jonasschnelli>
cfields_: Yes!
< cfields_>
jonasschnelli: light bulb! That makes perfect sense. I should just be extending that, then.
< sipa>
cfields_: yes!
< jonasschnelli>
If all core interaction goes over clientmodel.cpp, we have great readability and good base for detaching possibilities.
< cfields_>
sipa: when you said "clientmodel", i agreed because my plan was just to stuff a connman instance in there and call it as needed. I see now what you really meant.
< cfields_>
jonasschnelli: roger.
< cfields_>
sipa / jonasschnelli: thanks a bunch, that was very helpful.
< jonasschnelli>
np, thanks for asking
< cfields_>
jonasschnelli: could I convince you to allow the g_connman hack for now, with a plan to fix it up in a follow-up PR? I'm afraid it'll never get merged if I try to get it all in the first go.
< cfields_>
(rpc needs to be dealt with similarly, and that will be less fun)
< sipa>
i think keeping a global initially is fine
< jonasschnelli>
Yes. IMO there are no multiple connman's possible for now? If so, then I don't see a reason to make it _not_ global.
< cfields_>
jonasschnelli: yes, it was written with the intention of using multiple connmans in the future
< cfields_>
though obviously we only have 1 now
< cfields_>
s/using/being able to use/
< jonasschnelli>
cfields_: maybe later with have a global conmanMan. :)
< cfields_>
haha
< sipa>
abstractconmanfactory
< gmaxwell>
BBC, is that you?
< sipa>
?
< instagibbs>
sipa doesn't catch anything but pure tech puns, sorry
< luke-jr>
ugh @ calling part of Core by a well-known OS component?
< luke-jr>
more importantly: I can still build without glib, right? <.<
< cfields_>
heh, yes
< ebfull>
sipa: at the moment we're stuck with old libsecp256k1 code (from before 0.12 where it was updated from upstream and enabled for verification)
< ebfull>
did that code support verification of compact signatures?
< jonasschnelli>
ebfull: yes. it does
< jonasschnelli>
ebfull: use secp256k1_ecdsa_verify
< jonasschnelli>
ebfull: the signatures are in a struct called secp256k1_ecdsa_signature
< jonasschnelli>
you can "fill it up" with a compact signature over secp256k1_ecdsa_signature_parse_compact
< ebfull>
this was before `secp256k1_ecdsa_signature` was introduced
< ebfull>
as far as i can tell
< jonasschnelli>
or with a def: secp256k1_ecdsa_signature_parse_der
< ebfull>
at least in the exposed api
< jonasschnelli>
s/def/DER
< jonasschnelli>
Not sure what version you use... but it is like this since ~6month.
< ebfull>
yeah, from before that :)
< ebfull>
secp256k1_ecdsa_verify doesn't appear to parse the compact signatures unless my code is wrong
< ebfull>
or maybe it requires a version byte at the beginning :)
< sipa>
ebfull: of course it does support it
< sipa>
ebfull: it's used for signature verification
< ebfull>
sipa: maybe i'm using the wrong terminology. in the old code, secp256k1_ecdsa_sign_compact produces a 64-byte (r, s), which secp256k1_ecdsa_sig_parse (as used by secp256k1_ecdsa_verify) does not appear to parse
< ebfull>
i can see how to use the new api to do it, but not the old api
< sipa>
ebfull: 0.12 used libsecp for message signature validation, which used compact format
< sipa>
so just look up how that worked
< ebfull>
we're using the libsecp from before that :(
< sipa>
oh
< sipa>
well, update it
< sipa>
we fixed bugs
< ebfull>
i'll have to explore the feasibility of that
< ebfull>
last time i looked into it i had a rough time following the trail of github UI bugs in the pull requests involved
< Chris_Stewart_5>
sipa: After generating script_tests.json.gen do you need to manually copy it over to data/script_tests.json?
< sipa>
yup
< sipa>
ebfull: meh, just copy it over
< ebfull>
at that point i think it's probably way easier for us to use ed25519 for what we're implementing anyway
< BlueMatt>
lol bitcoin core under valgrind is an absolute shitshow now
< BlueMatt>
I turn around and two minutes later valgrind is all like "More than 1000 different errors detected. Go fix your program!"
< sipa>
ouch :(
< sipa>
i haven't run valgrind in a while
< Lightsword>
memory leaks everywhere?
< BlueMatt>
uninitialized values everywhere
< BlueMatt>
its actually probably just C++11 confusing valgrind
< MarcoFalke>
I tried pre-cpp11 and had a similar number of complaints
< BlueMatt>
ouch
< BlueMatt>
sipa: also lots from secp
< BlueMatt>
lolnvm
< BlueMatt>
just make random not use openssl and literally every single warning goes away
< cfields_>
BlueMatt: there's a build-switch for openssl
< cfields_>
BlueMatt: it seeds with uninit data by default
< BlueMatt>
cfields_: yes, I would have to rebuild openssl for that
< BlueMatt>
cfields_: its impressive how far some of the errors go, though....everything anywhere that is seeded with random values, so you get lots of shit in ccoins/mempool/bloom/etc
< cfields_>
heh
< cfields_>
boost tends to piss off sanitizing tools as well
< BlueMatt>
havent seen anything blow up except on shutdown yet