< NicolasDorier>
spudowiar: Awesome. I think you should share that as well with Ledger (btchip), they would probably review and make a plugin. Also it might move forward the conversation about HW standard
< spudowiar>
NicolasDorier: Will do. I think I'll make a new RPC method (signhwwtransaction) first (because this isn't entirely compatible with signrawtransaction) and then ask him to review it
< bitcoin-git>
bitcoin/0.14 21e1ed4 Wladimir J. van der Laan: doc: Preliminary release notes 0.14.2
< NicolasDorier>
spudowiar: what? signrawtransaction should just work. I mean the wallet already have all the info needed for a hardware wallet to sign
< NicolasDorier>
so why a new rpc method needed ? oO
< NicolasDorier>
the cool thing if we can reuse signrawtransaction is that moving from software hotwallet to hardware wallet is a configuration change, not a code change for users
< spudowiar>
I called the method signrawtransaction, but it's not compatible (as it sends hdKeypaths instead of privkeys)
< spudowiar>
Also, I want to add an identifier, so we can support multiple wallets attached to the computer
< spudowiar>
e.g. a serial number
< bitcoin-git>
[bitcoin] pavlosantoniou opened pull request #10530: Fix possibly unsafe accesses of array in class base_uint<BITS>. (master...master) https://github.com/bitcoin/bitcoin/pull/10530
< spudowiar>
I'm also going to add a listhwwdevices which could be used on the first run to list all the devices so Bitcoin Core can ask the user to select one
< spudowiar>
And a gethwwinfo to get the xpub for the selected device
< spudowiar>
Then on first run, Bitcoin Core could allow you to select a vendor and then a device
< spudowiar>
And store this in the wallet
< NicolasDorier>
spudowiar: for multiple wallet, you don't need serial number. There is the multi wallet PR which use RPC User/Password as a way to selection a wallet
< NicolasDorier>
and the wallet already know the keypath given the address
< NicolasDorier>
so you don't need to pass it to the RPC method
< spudowiar>
NicolasDorier: How does the wallet plugin know which hardware device to connect to?
< spudowiar>
NicolasDorier: You might be misunderstanding
< spudowiar>
I'm not talking about the RPC interface Bitcoin Core normally uses
< spudowiar>
I'm talking about JSON-RPC that is sent over stdin to the hardware wallet plugin
< NicolasDorier>
so it will be possible to use BTC signrawtransaction as if it was keys in software ?
< spudowiar>
Oh yeah
< spudowiar>
The normal RPC interface won't change
< NicolasDorier>
ha awesome
< spudowiar>
Although I might make it compatible as a hardware wallet plugin
< NicolasDorier>
that is what I meant
< spudowiar>
So you could connect one instance to another :)
< spudowiar>
And it thinks the other instance is a hardware wallet :)
< NicolasDorier>
spudowiar: very cool. Would you mind once you tested a bit to ACK my PR about hdwatchonly ? This would make your PR a bit smaller if you plan to include what you are doing into core
< NicolasDorier>
my PR is hanging around for a while, I am using it in production. But was lacking reviews
< spudowiar>
If you check that branch, the first few commits are your PR
< spudowiar>
I think I did ACK it, if not I'll do that when I can
< NicolasDorier>
yes I saw. Just a way to make your PR smaller, so it will be easier to merge yours later
< spudowiar>
The other PR we need is making signing asynchronous in Bitcoin Qt
< spudowiar>
At the moment, while it is talking to the hardware wallet the GUI freezes
< spudowiar>
Because the io_service is running on the UI thread
< NicolasDorier>
rebasing my PR.
< spudowiar>
Thanks, I'll ACK and rebase ASAP
< NicolasDorier>
actually you already rebased right ?
< NicolasDorier>
I can just take your branch up to my commits I think
< spudowiar>
Yeah, sure but you might want to check (I had to make a few changes for merge conflicts)
< NicolasDorier>
ok
< spudowiar>
A three way diff should let you see what I had to change
< spudowiar>
Do you think I should add a scan in the PATH for plugins? Or a scan in a folder?
< spudowiar>
e.g. bitcoin-hww-*
< spudowiar>
Then, on startup, the GUI wallet could display all the vendors
< NicolasDorier>
spudowiar: unsure, I like your current approach to just delegate to a child process specified by command line.
< wumpus>
I also like the external process approach
< wumpus>
loading plugins into bitcoind/-qt 's address space seem like the wrong way, for me, and is hard harmonize with the static linking we do for distributed executables
< spudowiar>
sipa__: Is there some good reading about the polymod function Bech32 uses?
< BlueMatt>
;;later tell sipa sipa__ is it just my C++11-lack-of-sanity, or am I suppose to find https://github.com/bitcoin/bitcoin/blob/master/src/coins.cpp#L99 gross as fuck? std::move(it->second.coin) then a few lines later it->second.coin.function_call()?
< gribble>
The operation succeeded.
< cfields>
BlueMatt: after a move, contents are usually undefined, but valid
< cfields>
BlueMatt: a clear after a move makes them defined and valid :)
< BlueMatt>
cfields: ugh, that just feels super gros
< BlueMatt>
s
< BlueMatt>
since that line depends on the exact behavior of CScript::Clear(), prevector, and CTxOut
< cfields>
BlueMatt: the alternative would be defining an explicit move ctor which does the clear and leaves it in a valid state
< cfields>
(or, i suppose, you'd rather just have it not used after the move)
< BlueMatt>
yea, I mean I generally read std::move(X) as "X will not be touched again"
< BlueMatt>
maybe thats just my naivete on C++11
< cfields>
BlueMatt: well, that's not really feasible with a container :\
< cfields>
std::move(foo[0]), foo still exists just fine.
< cfields>
In many cases, this one included i think, it's the only way to avoid incurring the copy when moving the contents of a member
< BlueMatt>
cfields: well obv destructor must run fine after std::move(), but to me I read that as, "I wont touch the 0th element of foo anymore after this, except to destruct"
< sipa__>
BlueMatt: typically the definition os that you're allowed to do anything that foesn't depend on otd prior state
< sipa__>
so you can assign, clear, ...
< sipa__>
in a vector, a move-from + .clear() is well specified
< sipa__>
doesn't
< MarcoFalke>
btcdrak: done. Looks like they hit the 50 min timeout :(
< BlueMatt>
sipa__: yea, but coin.Clear() is super strange, it calls CScript::clear() which doesnot prevector::clear(), instead it prevector().swap(*this), essentially
< BlueMatt>
which is...wat
< BlueMatt>
sipa: can we make it it->second.coin = CCoin() instead?
< BlueMatt>
should be +/- identical performance, but without feeling so yucky
< sipa>
i don't see the difference
< sipa>
both the Clear method and assignment operator can be user defined
< BlueMatt>
sure, swap just feels worse to me...I'll shut up if no one else thinks it feels strange, though :)
< cfields>
BlueMatt: the swap idiom comes from pre-c++11 when there was no shrink_to_fit().
< cfields>
imo the clear shouldn't be forcing mem dealloc, it should just be clearing, and we should be using shrink_to_fit() more liberally at higher layers.
< sipa>
right
< BlueMatt>
yes, prevector clear() doesnt, script clear() does
< BlueMatt>
could make script more vector-like by pushing it up to CCoin Clear()
< BlueMatt>
or CTxOut
< sipa>
script clear could be changed into prevector clear + shrink to fit
< sipa>
though i think prevector clear automatocally shrinks
< bitcoin-git>
[bitcoin] achow101 opened pull request #10533: [tests] Use cookie auth instead of rpcuser and rpcpassword (master...tests-use-cookie-auth) https://github.com/bitcoin/bitcoin/pull/10533
< BlueMatt>
sipa: ah, indeed, no bug in prevector, I was misreading the meaning of _size
< spudowiar>
I added some stuff to the Intro Qt form for hardware wallet support... Qt Designer made me cry :(
< jtimon>
cfields: awesome, I'll have a look, thanks
< BlueMatt>
sipa: would you be upset if I PRd making AccessCoin return a CoinAccessor which just asserts if you modify the coinscache while holding a reference? I know its not illegal
< BlueMatt>
cause map, but I'm afraid jeremy or I will at some point suggest using a different map
< BlueMatt>
and then introduce a bug
< BlueMatt>
should be +/- free performance wise
< sipa>
BlueMatt: that seems more something for a sanity checker
< sipa>
why specifically for CCoinsViewCache and not every other map?
< BlueMatt>
sipa: cause I have a strong suspicion over the next release or two someone is gonna realize std::unordered_map is not the ideal map and decide to suggest a hand-written one
< sipa>
yes, so?
< BlueMatt>
and then suddenly using the std sanity-check mode wont help?
< sipa>
yes, so?
< sipa>
i mean things like valgrind exist to find bugs like that
< sipa>
i plan to work on replacing the std::unordered_map myself there, btw
< BlueMatt>
cause I'd be not at all surprised if it were very difficult to tickle that issue during valgrind
< jeremyrubin>
Can also use a weak reference or something
< BlueMatt>
lol, figured someone would
< sipa>
i don't understand why you're specifically worried about this map
< BlueMatt>
jeremyrubin: that may be incompatible with a smarter map, sadly :(
< jeremyrubin>
Can just add an "updates" counter to the map
< jeremyrubin>
and the weak ref is invalid if counter != weakref.counter
< sipa>
it's a big and important one, but hardly the only complex data structure around
< gmaxwell>
BlueMatt: please rebase #10192; review #10148
< BlueMatt>
sipa: well we hold references into that map in rpc functions...I'm not aware of any other map where we do anything like that?
< BlueMatt>
gmaxwell: yesyes, I'm working on a few small things i wanted in per-utxo first
< jeremyrubin>
anyways, I'm generally sympathetic to Matt's request; long held iterators to maps -> bugs
< BlueMatt>
well, luckily for now we have no "long held" ones
< BlueMatt>
but we do have ones in RPC
< sipa>
jtimon: not long held iterators; references to elements
< sipa>
eh, jeremyrubin
< sipa>
iterators are encapsulated
< BlueMatt>
sipa: AccessCoin is called in rpc/rawtransaction
< BlueMatt>
in several places
< sipa>
in many places we also hold an iterator that comes out of mapBlockIndex.find
< BlueMatt>
sipa: I'm sure this comes back to your "objects are transparent holders of data" vs my "objects should be opaque, where possible"
< gmaxwell>
BlueMatt: k. other people's review is blocked on that rebase.
< BlueMatt>
gmaxwell: hey, you're the one who got me to focus on fibre for two weeks :p
< BlueMatt>
gmaxwell: but will try to circle around today
< BlueMatt>
sipa: hmm, somehow i feel better about that cause its clear its a map, unlike CCoinsViewCache, where its an encapsulated datastructure that is actually private
< sipa>
BlueMatt: it does return a reference, which is always a worry
< gmaxwell>
BlueMatt: I know, I know.
< jeremyrubin>
BlueMatt: at a higher level, I see what you're trying to do, trying to get me to write a better map and I'm maybe falling for it
< BlueMatt>
jeremyrubin: sipa already said he was working on it :p
< sipa>
if you're worried about particular call sites, you can always use Coin coin = view.Accessor(outpoint) ... instead of Coin& coin = ...
< BlueMatt>
sipa: our disagreement is clearly a disagreement about encapsulation
< sipa>
BlueMatt: i'm just so glad i got rid of that CCoinsModifier... and now you want to introduce it again :)
< sipa>
the interface got so much easier
< BlueMatt>
sipa: CCoinsViewModifier *did* things, there I agree that is gross
< BlueMatt>
I'm proposing something who's only value is to assert with litte/no performance impact
< BlueMatt>
so as to act as a sanity checker on callsites to a function I cant say I'm sold on
< gmaxwell>
BlueMatt: what sanity check will it do?
< sipa>
gmaxwell: check that no references are held while a modify happens
< BlueMatt>
gmaxwell: technically assert something that is currently allowed, but something I assume will be come unallowed when sipa (or someone) writes a custom map for mapCoins
< gmaxwell>
that doesn't sound free to verify.
< sipa>
BlueMatt: also, i don't think it's technically illegal to hold a reference to a deleted object
< BlueMatt>
hmm? just a boolean/int to set when you AccessCoins and assert() on in function calls
< sipa>
only using it is
< jeremyrubin>
what if you have a templated thing that doesn't check that property if the type of map used is the current one ;p
< BlueMatt>
gmaxwell: compared to looking up in the map it is free :)
< gmaxwell>
BlueMatt: not necessarily compared to an alternative data structure (also, I doubt it's free even compared to a map)
< BlueMatt>
sipa: sure, but cutting hairs, I'm asking if you're ok adding a class which is more strict than it needs to be, cause its easy to write and, imo, a useful sanity check
< ryanofsky>
sipa, not directly related but #9384 also adds back a version of the modifier
< BlueMatt>
gmaxwell: that sounds like something that the cpu will run without any attached preconditions and will easily run free since you're informing the branch predictor what is unlikely
< sipa>
ryanofsky: i like that 9384 reduces logic duplication, i dislike that it does not actually reduce the amount of code :)
< ryanofsky>
i think that could be because i wrote it in an intentionally verbose style with lots of comments and named variables
< sipa>
ryanofsky: yeah, will review
< sipa>
that was just a quick comment
< sipa>
BlueMatt: so, an accessor object may have another advantage at some point... if we'd want to make CCoinsViewCache have split storage for txids and CCoins
< sipa>
eh, Coins
< sipa>
i don't know if that's a good idea or not, but it may reduce memory usage a fair bit (or not)
< BlueMatt>
you mean to hold a ptr to uint256?
< sipa>
well the point is that encapsulating it may give freedom to change the representation more liberally
< BlueMatt>
sipa: if you really dont want an accessor object, I'ma add a nice long comment to AccessCoin describing the preciditions...which I'm ok with, but not as much a fan
< BlueMatt>
hence the q
< BlueMatt>
fair
< sipa>
so how about we benchmark whether it has an effect?
< BlueMatt>
do we have a decent way to benchmark?
< BlueMatt>
i mean could reindex
< sipa>
reindex chainstate on a -connect=0 node with fixed CPU clockrate
< BlueMatt>
but that nearly definitely wont show it
< sipa>
works very accurately
< BlueMatt>
I so dont want to do that....I can haz ssh?
< sipa>
i can set things up
< BlueMatt>
sipa: well, went and did it and no clear way to enforce semantics cause you can take reference and then drop the accessor too easily when writing cleints. it is nice cause it forces back to a ptr and restores a bunch of effectively-assert-cause-nullptrs that you removed in per-utxo
< BlueMatt>
maybe I'll just re-add asserts and a long comment
< bitcoin-git>
[bitcoin] practicalswift closed pull request #10527: Use parentheses to clarify intended precedence when using bitwise operations (master...clarify-precedence) https://github.com/bitcoin/bitcoin/pull/10527
< Chris_Stewart_5>
FYI: You can install bitcoind as a dependency now on travis ci, previously it had been black listed.
< jtimon>
cfields: thanks a lot! with your suggestion and another small change it seems the problem is gone
< cfields>
jtimon: err, you need to use the reverser thing everywhere, not just in tests
< jtimon>
maybe now it doesn't make sense for them to be separated, but maybe if I hadn't separated them I would still be stuck, so no regrets
< jtimon>
oh, I see
< jtimon>
I didn't run the python tests, but they should fail in that case, I'll wait for travis to fail
< jtimon>
then edit the scripted-diff commit to do the same thing everywhere
< jtimon>
although prevector tests will still need something different for the const reverse loop
< jtimon>
or I think so
< jtimon>
thanks for the heads up, I really thought it was just there because of how templates are used there
< cfields>
np. no, it's because of how a range-based-for binds to the expression
< jtimon>
then the new class sucks a little bit, perhaps I can do the same with auto and rbegin rend inlined
< cfields>
though I can't say that I 100% understand the semantics
< jtimon>
without needing a new class
< jtimon>
alright, let's wait for travis
< jtimon>
if it passes and you are still right maybe we can spot some missing tests
< jtimon>
I can run the python tests locally too, I'll do that
< jtimon>
jnewbery: what do you think about changing /tmp/bitcoin_test_runner_20170605_235927 to /tmp/bitcoin_test_runner/20170605_235927 ?
< jtimon>
not a big deal, just slightly easier to remove the whole folder, kudos for the clearer prefix
< jnewbery>
mildly against. Why add an extra directory layer? It just means I need to press the tab key one more time :)
< jnewbery>
why easier to remove the whole folder?
< jtimon>
fair enough, that was the question
< jtimon>
it wasn't unlikely that it was only more convenient for me
< jtimon>
the current prefix is good enough anyway
< jtimon>
cfields: I don't know about travis, but this passes python3 ./test/functional/test_runner.py -j56 --extended -x pruning,smartfees,maxuploadtarget
< jnewbery>
jtimon: cool. Thanks
< cfields>
jtimon: I think you need a new set of tests for reverse_iterator. I don't trust it with the simple for construct :(
< jtimon>
cfields: yep, more reason to simply rewrite the scripted-diff into something that uses rbegin/rend direclty instead of adding the new template
< cfields>
agreed
< jtimon>
but, still, if this shouldn't pass tests and it passes, perhaps we should open an issue asking for more tests
< cfields>
agree with that too :)
< jtimon>
I will try the version without the new class either way, it'll probably look smaller, just a more complex script to read if you use scripted-diff for review
< jtimon>
but the diff should look very simple as well
< midnightmagic>
wait was there an rc that I missed a gitian for..?
< BlueMatt>
14.2? maybe?
< midnightmagic>
argh
< achow101>
midnightmagic: might as well not build it now since an rc2 is guaranteed and the rc1 codesigned bins won't be made
< achow101>
the version number wasn't bumped
< midnightmagic>
achow101: I have the time, and I am one of those completionists you see sometimes who opens every crate in the game just because there might be a coupla gold in there..
< luke-jr>
but there is no chance of gold in this case? :p
< midnightmagic>
The gold is figurative!
< luke-jr>
midnightmagic: what does it represent in this case?
< bitcoin-git>
[bitcoin] practicalswift opened pull request #10536: Remove unreachable or otherwise redundant code (master...unreachable) https://github.com/bitcoin/bitcoin/pull/10536
< midnightmagic>
luke-jr: The reward of having opened every case. :) Also, I can say to people that I've built almost every gitian and therefore personally verified that almost every build is represented by source code as it is in the repo.