1 2015-01-27 00:00:18 <Luke-Jr> SignatureChecker(CMutableTransaction, int) results in a segfault without any compiler warnings
2 2015-01-27 00:00:56 <Luke-Jr> because it implicitly converts CMutableTransaction to CTransaction, and SignatureChecker makes a reference to that clone, which is then deleted as soon as the statement finishes
3 2015-01-27 00:01:08 <Luke-Jr> is this a C++ bug? O.o
4 2015-01-27 00:03:05 <jrick> you can't pass in a temporary and keep around a reference
5 2015-01-27 00:03:09 <jrick> that's undefined behavior
6 2015-01-27 00:03:39 <jrick> so you probably have to allocate a CTransaction on the stack and pass that in by reference
7 2015-01-27 00:03:51 <jrick> or heap if it needs to live longer (not sure)
8 2015-01-27 00:07:36 <Luke-Jr> jrick: GCC is passing in the temporary implicitly
9 2015-01-27 00:09:29 <jrick> right
10 2015-01-27 00:09:44 <jrick> and it's destroyed after the expression is evaluated
11 2015-01-27 00:10:34 <Luke-Jr> yeah, my thought is that GCC shouldn't do the implied cast when it knows we are keeping a reference
12 2015-01-27 00:10:58 <cfields> Luke-Jr: CMutableTransaction doesn't inherit from CTransaction... ?
13 2015-01-27 00:11:05 <jrick> the compiler is allowed to do anything and everything in the presence of undefined behavior
14 2015-01-27 00:11:33 <cfields> oh i see, there's a ctor for it
15 2015-01-27 00:16:15 <Luke-Jr> maybe it should
16 2015-01-27 00:19:37 <cfields> Luke-Jr: i'm not sure why it holds a reference there rather than a copy? If it's just to save an include, that's silly and it should be changed. If it's for perf reasons, it still seems like a risky optim
17 2015-01-27 00:20:36 <cfields> i assume changing that fixes the problem?
18 2015-01-27 00:22:52 <Luke-Jr> I presume it would
19 2015-01-27 00:22:59 <Luke-Jr> holy crap, we are actually using this in code
20 2015-01-27 00:23:13 <Luke-Jr> does C++ guarantee temporaries survive until the entire statement finishes?
21 2015-01-27 00:23:35 <Luke-Jr> not sure if this is safe: if (!VerifyScript(txin.scriptSig, prevPubKey, STANDARD_SCRIPT_VERIFY_FLAGS, SignatureChecker(mergedTx, i)))
22 2015-01-27 00:23:36 <cfields> Luke-Jr: yes, but the problem is that we're storing the reference as a member var
23 2015-01-27 00:24:46 <cfields> (that's my guess, anyway)
24 2015-01-27 00:25:54 <Luke-Jr> dunno, that seems somewhat reasonable
25 2015-01-27 00:26:10 <Luke-Jr> moreso than being unable to pass CMutableTransaction without converting
26 2015-01-27 00:26:16 <cfields> the reference is dead as soon as the ctor finishes
27 2015-01-27 00:26:32 <Luke-Jr> then this is a bug
28 2015-01-27 00:27:01 <cfields> so any function used from the class after construction, if constructed with a temp CTransaction, should blow up
29 2015-01-27 00:27:40 <cfields> Luke-Jr: to be clear, this is what i'm suggesting: http://pastebin.com/raw.php?i=v6bZMfWb
30 2015-01-27 00:28:01 <Luke-Jr> cfields: yes, not sure it's the best solution though. but maybe it's worth the overhead to be safe
31 2015-01-27 00:28:40 <Luke-Jr> does C++ have a keyword to forbid temporaries in the constructor?
32 2015-01-27 00:28:45 <cfields> Luke-Jr: storing references as member vars is very bad practice imo
33 2015-01-27 00:29:12 <Luke-Jr> cfields: but this is likely hot code
34 2015-01-27 00:29:25 <cfields> then s/hot/explosive/ :)
35 2015-01-27 00:29:43 <Luke-Jr> I mean used a lot during IBD
36 2015-01-27 00:31:48 <Luke-Jr> brb (with a proposed alternative fix)
37 2015-01-27 00:54:04 <volante> hi, i'm trying to understand the format of the .dat files that store blocks. there seems to be 8 bytes at the start of the file before the actual raw block starts. are there docs on this file format?
38 2015-01-27 01:00:14 <sipa> volante: 4 bytes network magic, 4 bytes little-endian length descriptor
39 2015-01-27 01:00:54 <sipa> and then the block in network serialized form
40 2015-01-27 01:01:17 <sipa> also, no guarantee that there are no gaps, no guarantee all blocks are valid, no guarantee that they are in order
41 2015-01-27 01:15:31 <Luke-Jr> and no guarantee the format doesn't change
42 2015-01-27 01:20:08 <volante> what do you mean by gaps? as in, out of order, or there could be some other data between blocks?
43 2015-01-27 01:20:39 <volante> and no blocks from the main chain should be missing from those files if my node has fully synced right?
44 2015-01-27 01:25:36 <Luke-Jr> cfields: I can't make my solution clean, so maybe yours is better unless someone measures a relevant performance hit
45 2015-01-27 01:25:55 <cfields> Luke-Jr: it's verified fixed that way?
46 2015-01-27 01:26:11 <Luke-Jr> cfields: hard to see how it wouldn't be fixed that way..
47 2015-01-27 01:26:31 <cfields> Luke-Jr: well, it'd have to be used outside of the ctor for it to blow up
48 2015-01-27 01:26:36 <Luke-Jr> which it often is
49 2015-01-27 01:26:42 <cfields> if only the ctor is used and it's still busted, my change won't help
50 2015-01-27 01:26:57 <Luke-Jr> it won't blow up anyway
51 2015-01-27 01:27:00 <cfields> Luke-Jr: i'm asking about your specific case that triggers a crash. It fixes that crash?
52 2015-01-27 01:27:07 <Luke-Jr> I can test, sec
53 2015-01-27 01:27:47 <Luke-Jr> your patch fails due to incomplete type
54 2015-01-27 01:28:21 <Luke-Jr> #include "primitives/transaction.h"
55 2015-01-27 01:28:42 <cfields> yea, forgot to include that
56 2015-01-27 01:30:05 <Luke-Jr> note there is no reliable test for this, since it only blows up if the stack gets clobbered
57 2015-01-27 01:30:13 <Luke-Jr> sufficiently clobbered*
58 2015-01-27 01:31:43 <Luke-Jr> in fact, I'm unsure I can reproduce it
59 2015-01-27 01:38:04 <cfields> Luke-Jr: you should be able to fake it, i'd think, by zeroing the old ref directly after creating the object
60 2015-01-27 01:38:53 <jrick> link to patch? was messing with my router so I missed it
61 2015-01-27 02:47:44 <Luke-Jr> cfields: it's actually not that simple :|
62 2015-01-27 02:49:58 <Luke-Jr> cfields: I cannot reproduce the crash I had, so .. :/
63 2015-01-27 02:50:36 <Luke-Jr> cfields: either way, it's broken and should be fixed, so please PR it
64 2015-01-27 02:50:53 <cfields> ok
65 2015-01-27 03:24:13 <unicodesnowman> Luke-Jr, someone named "Luke_Jr" is insulting everyone. I think it's an impersonator.
66 2015-01-27 03:32:05 <Luke-Jr> unicodesnowman: that's old news
67 2015-01-27 03:32:09 <Luke-Jr> and off-topic
68 2015-01-27 04:29:18 <michagogo> 3:20:09 <volante> what do you mean by gaps? as in, out of order, or there could be some other data between blocks? <-- yes
69 2015-01-27 04:29:38 <michagogo> jrick: there are logs, in the topic
70 2015-01-27 04:30:31 <jrick> oh thanks, for some reason that didn't occur to me
71 2015-01-27 05:01:33 <jrick> Luke-Jr, cfields: so dumb question, but why not just pass around CTransaction by const ref when you don't want to mutate it, and then you can only call the member functions that are declared const?
72 2015-01-27 05:02:07 <Luke-Jr> jrick: CTransaction has some optimisations that are incompatible with modification
73 2015-01-27 05:02:28 <Luke-Jr> jrick: eg, the hash is just a 32-byte vector, rather than calculating it
74 2015-01-27 05:02:52 <jrick> ah
75 2015-01-27 05:03:10 <sipa> yeah, CTRansaction is immutable so it can have its hash precomputed; if you need modification, there is CMutableTransaction, and conversions in both directions
76 2015-01-27 05:03:56 <sipa> the validation/consensus code is more efficient and simpler for not needing to care about the possibility of mutation, or risking recomputing hashes over and over
77 2015-01-27 05:04:05 <jrick> well it's the (implicit) conversions that create these lifetime issues
78 2015-01-27 05:04:19 <sipa> agree, we should get rid of those
79 2015-01-27 05:06:06 <jrick> can you derive from a class, and inherit only the const member functions?
80 2015-01-27 05:06:35 <Luke-Jr> sipa: any reason we shouldn't make CMutableTransaction a subclass of CTransaction? just to avoid making GetHash etc virtuals?
81 2015-01-27 05:06:41 <jrick> actually scratch that idea
82 2015-01-27 05:07:33 <sipa> Luke-Jr: maybe that can be done, but it would mean wasting 32 bytes for that cached hash
83 2015-01-27 05:09:00 <sipa> maybe i haven't followed up, but where is it being proposed to make GetHash a virtual? i would really prefer avoiding dynamic binding in primitive data structures
84 2015-01-27 05:11:35 <Luke-Jr> sipa: hm, a common CTransactionBase then?
85 2015-01-27 05:11:56 <sipa> perhaps that shoudl be the primitive type, and have one with a cached hash inside validation logic
86 2015-01-27 05:12:00 <Luke-Jr> sipa: well, if CMutableTransaction were a subclass of CTransaction, GetHash would need to be a virtual to return the correct value
87 2015-01-27 05:12:19 <sipa> right; i would really avoid that...
88 2015-01-27 05:12:59 <Luke-Jr> sipa: any thoughts on cfields's solution btw? Is copying txIn going to kill performance?
89 2015-01-27 05:13:18 <warren> ummm
90 2015-01-27 05:13:35 <warren> Coin Control Dialog, is the first column title supposed to be just "1"? What is that supposed to mean?
91 2015-01-27 05:14:26 <sipa> Luke-Jr: performance of CMutableTransaction is not relevant
92 2015-01-27 05:14:35 <sipa> it's only for signing and tests
93 2015-01-27 05:14:40 <Luke-Jr> sipa: this is performance of SignatureChecker
94 2015-01-27 05:14:56 <Luke-Jr> warren: odd, IMO it shouldn't be
95 2015-01-27 05:14:58 <sipa> oh?
96 2015-01-27 05:15:09 <sipa> Luke-Jr: i haven't looked at pull requests for a few dats
97 2015-01-27 05:15:11 <sipa> *days
98 2015-01-27 05:15:15 <Luke-Jr> sipa: SignatureChecker(CMutableTransaction, unsigned int) is broken
99 2015-01-27 05:15:20 <Luke-Jr> sipa: affecting 0.10 code
100 2015-01-27 05:15:23 <Luke-Jr> potentially*
101 2015-01-27 05:15:30 <Luke-Jr> there are no related PRs yet
102 2015-01-27 05:15:34 <sipa> ah
103 2015-01-27 05:15:47 <sipa> well you need to convert to CTransaction first...
104 2015-01-27 05:15:56 <Luke-Jr> sipa: SignatureChecker(CMutableTransaction, unsigned int) implicitly makes a temporary CTransaction, which SignatureChecker stores a reference to, and is then deleted..
105 2015-01-27 05:16:03 <sipa> yes, i know
106 2015-01-27 05:16:06 <Luke-Jr> sipa: there are at least 3 cases of this in the code today
107 2015-01-27 05:16:35 <warren> Luke-Jr: it is in 0.10rc3
108 2015-01-27 05:16:49 <Luke-Jr> warren: bleh :<
109 2015-01-27 05:18:00 <sipa> Luke-Jr: i would suggest a SignatureChecker wrapper that accepts a CMutableTransaction, and stores the CTransaction converted from it inside the checker object
110 2015-01-27 05:20:00 <Luke-Jr> that's uglier than what I had thought of :/
111 2015-01-27 05:20:28 <Luke-Jr> - BOOST_CHECK(!VerifyScript(s, a_or_b, flags, SignatureChecker(txTo[1], 0), &err));
112 2015-01-27 05:20:29 <Luke-Jr> + BOOST_CHECK(!VerifyScript<SignatureChecker>(s, a_or_b, flags, txTo[1], 0, &err));
113 2015-01-27 05:26:26 <sipa> why is VerifyScript templated?
114 2015-01-27 05:27:19 <Luke-Jr> in case someone wants a different checker type?
115 2015-01-27 05:27:21 <sipa> and no, you can't do that; that makes VerifyScript aware of what arguments to pass to the checker
116 2015-01-27 05:27:32 <sipa> the whole point of introducing that class was to make that independnet
117 2015-01-27 05:28:39 <Luke-Jr> how about this? http://codepad.org/g2aMuTYN
118 2015-01-27 05:29:14 <sipa> NAK; signaturecherks shoudln't depend on mutabletransaction
119 2015-01-27 05:29:26 <sipa> signaturecheker is consensus code, mutabletransaction is utility code
120 2015-01-27 05:30:38 <Luke-Jr> hm
121 2015-01-27 05:30:46 <sipa> what's so bad about VerifyScript(s, a_or_b, flags, MutableSignatureChecker(txTo[1], 0), &err) ?
122 2015-01-27 05:30:57 <Luke-Jr> easier to mess up.
123 2015-01-27 05:31:20 <sipa> it would allow us to remove tothe automatic conversion from mutable to non-mutable
124 2015-01-27 05:31:34 <sipa> then it's very hard to mess up
125 2015-01-27 05:31:36 <Luke-Jr> would it? I guess that's okay then
126 2015-01-27 05:32:02 <jrick> why don't you make the mutable version the base class, and the one which caches the hash derives from it, and you always use that as const?
127 2015-01-27 05:32:43 <Luke-Jr> jrick: the non-mutable one cannot depend on the mutable one
128 2015-01-27 05:33:04 <sipa> jrick: that makes sense actually
129 2015-01-27 05:33:12 <sipa> if the mutable one has no utility code in it
130 2015-01-27 05:33:22 <warren> hmm, is the testnet android app working for you when syncing with 0.10?
131 2015-01-27 05:33:24 <Luke-Jr> hm
132 2015-01-27 05:33:24 <warren> it's stuck
133 2015-01-27 05:33:54 <Luke-Jr> sipa: but to cache the hash, GetHash needs to become virtual..
134 2015-01-27 05:34:08 <sipa> Luke-Jr: the base class simply would not have a GetHash at all
135 2015-01-27 05:34:10 <jrick> I'm not sure there would be too many places where you wanted to use both the mutable and non-mutable ones in the same container
136 2015-01-27 05:34:21 <Luke-Jr> sipa: then it doesn't help..?
137 2015-01-27 05:34:30 <sipa> ?
138 2015-01-27 05:34:44 <Luke-Jr> sipa: if the base class has no GetHash, then we can't use it in SignatureChecker, can we?
139 2015-01-27 05:34:49 <sipa> i'll try writing something; i may be missing part
140 2015-01-27 05:34:58 <sipa> Luke-Jr: ha, duh
141 2015-01-27 05:35:08 <sipa> well, there could be two SignatureCheckers still
142 2015-01-27 05:35:35 <Luke-Jr> or maybe make it templated
143 2015-01-27 05:35:39 <sipa> one which uses SerializeHash on the input, and one 'optimized' version which uses the cached hash
144 2015-01-27 05:35:49 <sipa> please, no templates in consensus code :S
145 2015-01-27 05:35:54 <Luke-Jr> sorry >_<
146 2015-01-27 05:36:24 <sipa> sorry, this is not the right time for me to be arguing
147 2015-01-27 05:36:30 <sipa> i need sleep, ignore me :)
148 2015-01-27 05:36:37 <Luke-Jr> no, it's a reasonable concern
149 2015-01-27 05:36:44 <Luke-Jr> but get some sleep
150 2015-01-27 05:36:47 <Luke-Jr> no rush afaik
151 2015-01-27 05:37:05 <Luke-Jr> I'll just open an issue so we don't forget before 0.10
152 2015-01-27 05:45:30 <Luke-Jr> https://github.com/bitcoin/bitcoin/issues/5715
153 2015-01-27 05:45:38 <Luke-Jr> someone flag for 0.10?
154 2015-01-27 05:47:00 <wumpus> done
155 2015-01-27 05:47:12 <warren> I found a bug in coin control, trying to confirm a fix but I need testnet to mine...
156 2015-01-27 05:47:20 <warren> I suppose I could use regtest
157 2015-01-27 05:48:26 <warren> and it seems the android testnet app is busted
158 2015-01-27 05:48:32 <warren> gets stuck at height 319746
159 2015-01-27 05:48:42 <wumpus> Luke-Jr: this is pretty ugly, for the reason of unintuitive with temporaries I've argued before against keeping references inside a class. At least this pointers this will never (automatically) happen
160 2015-01-27 05:49:18 <warren> anyone have testnet coins?
161 2015-01-27 05:49:27 <warren> I need ~10 inputs to play with
162 2015-01-27 05:50:09 <wumpus> e.g. you plan to save a pointer to an object, have it passed as a pointer
163 2015-01-27 05:51:13 <Luke-Jr> warren: address(es)?
164 2015-01-27 05:54:47 <Luke-Jr> warren: getdifficulty is the difficulty of the last block, not the current difficulty.
165 2015-01-27 05:56:06 <warren> Luke-Jr: testnet resets difficulty if there is no block for 10 minutes
166 2015-01-27 05:56:27 <Luke-Jr> warren: I know
167 2015-01-27 05:57:03 <warren> switched to pooler's cpuminer, which seems to be 700% faster than the internal miner
168 2015-01-27 05:58:00 <jrick> Luke-Jr: so forgive my c++ (I know this isn't BC style and I just threw this together) but this is what I sort of had in mind http://sprunge.us/KNDY
169 2015-01-27 05:58:55 <Luke-Jr> jrick: the problem is we need a common GetHash method
170 2015-01-27 05:59:30 <jrick> hmmm
171 2015-01-27 06:00:39 <wumpus> Luke-Jr: I'm not sure I see the problem yet; where does VerifyScript store a reference to the SignatureChecker beyond the immediate call?
172 2015-01-27 06:01:07 <jrick> actually that's not even a great solution either because it will copy the mutable transaction when creating the one which caches the hash
173 2015-01-27 06:01:23 <wumpus> Luke-Jr: temporaries are defined to have the lifetime of a statement, so the temporary transaction will live until VerifyScript ended execution
174 2015-01-27 06:01:33 <wumpus> Luke-Jr: is it still kept around after that?
175 2015-01-27 06:03:08 <wumpus> Luke-Jr: this is why e.g. printf("%s\n", obj.ToString().c_str()); works; a temporary string is created, and lives until the printf() statement finishes
176 2015-01-27 06:03:38 <Luke-Jr> wumpus: SignatureChecker stores it
177 2015-01-27 06:03:57 <Luke-Jr> wumpus: cfields tells me the temporary lifetime is not the statement, but the use
178 2015-01-27 06:04:11 <wumpus> Luke-Jr: yes, but only for the lifetime of the of the VerifyScript statement
179 2015-01-27 06:04:15 <wumpus> Luke-Jr: I think he's wrong
180 2015-01-27 06:04:24 <Luke-Jr> I was unsure, which is why I asked
181 2015-01-27 06:05:17 <cfields> well the lifetime ends as soon as that function returns. but until then, all called functions see a valid reference
182 2015-01-27 06:05:28 <cfields> otherwise you couldn't do something like foofunc(barclass())
183 2015-01-27 06:06:05 <warren> oh... nothing wrong with the android app
184 2015-01-27 06:06:16 <wumpus> unless the object is stored for later (e.g. for handling in a thread) I don't think that code is problematic
185 2015-01-27 06:06:34 <cfields> wumpus: it is stored for later
186 2015-01-27 06:06:44 <wumpus> cfields: where?
187 2015-01-27 06:06:45 <cfields> if we're speaking about the same code
188 2015-01-27 06:06:58 <wumpus> I'm looking at the code of VerifyScript
189 2015-01-27 06:07:17 <cfields> https://github.com/bitcoin/bitcoin/blob/master/src/script/interpreter.h#L96
190 2015-01-27 06:09:02 <cfields> so SignatureChecker() is fine, but foo = SignatureChecker(); foo.CheckSig() can blow up if the ctor was passed a temporary
191 2015-01-27 06:09:04 <wumpus> ok. the simple solution would just be to make the conversion explicit, and scope the CTransaction
192 2015-01-27 06:09:34 <wumpus> e.g. CTransaction tmp(mutabletx); foo=SignatureChecker(tmp); foo.checksig();
193 2015-01-27 06:09:42 <Luke-Jr> that results in a lot of ugliness
194 2015-01-27 06:10:05 <wumpus> correctness and clarity is much more important here
195 2015-01-27 06:10:49 <Luke-Jr> sipa's MutableSignatureChecker is pretty clear IMO
196 2015-01-27 06:10:50 <wumpus> also that reference should be a pointer to avoid this. Don't save a reference.
197 2015-01-27 06:11:27 <wumpus> if you have to take an explicit pointer, it forces the programmer to think about object lifetime
198 2015-01-27 06:11:39 <cfields> +1. that's really dangerous for exactly this reason
199 2015-01-27 06:12:27 <wumpus> this is a bug caused by the intricacies of 'nice looking' c++ code, I'd prefer a solution that is ugly and straightforward
200 2015-01-27 06:12:53 <wumpus> it had me fooled too, that shouldn't happen
201 2015-01-27 06:13:11 <Luke-Jr> wumpus: it turns 1 line into 4+ lines in every occurance
202 2015-01-27 06:13:21 <wumpus> Luke-Jr: I don't care
203 2015-01-27 06:13:25 <cfields> wumpus: why not just pass a copy here? that seems by far the most straightforward to me
204 2015-01-27 06:13:34 <wumpus> cfields: fine with me too
205 2015-01-27 06:13:49 <wumpus> what I don't want is: introduce templates, introduce virtuals
206 2015-01-27 06:14:13 <wumpus> or introduce all kinds of extra helper classes to cover the bug's ass and make it fragilely working again
207 2015-01-27 06:15:14 <wumpus> (until the next person using the interface underestimates the subtleties and introduces a bug again)
208 2015-01-27 06:15:15 <Luke-Jr> definitely don't want it fragile where this bug is easily possible
209 2015-01-27 06:15:21 <wumpus> rihgt.
210 2015-01-27 06:21:24 <wumpus> so as I see it there's two solutions to the reference trap: a) change the argument to a pointer instead of a reference, and manually manage lifetime or b) make a copy
211 2015-01-27 06:21:57 <wumpus> (b) is a one-line change
212 2015-01-27 06:23:44 <wumpus> okay, two lines, it needs a #include "primitives/transaction.h" in interpreter.h too :)
213 2015-01-27 06:24:16 <wumpus> so yes, that has my preference, certainly for 0.10
214 2015-01-27 06:27:38 <Luke-Jr> wumpus: might have a big performance hit, possibly
215 2015-01-27 06:27:46 <wumpus> well prove it
216 2015-01-27 06:28:23 <Luke-Jr> can't, IBD sucks for me already
217 2015-01-27 06:28:28 <Luke-Jr> took almost 2 days with 0.10
218 2015-01-27 06:28:41 <wumpus> *mumbles something about premature optimizations and roots of all evil*
219 2015-01-27 06:38:37 <warren> https://github.com/bitcoin/bitcoin/pull/5718 I have another commit that's somewhat related but relies on this fix to test properly. Should I push the second commit in another PR?
220 2015-01-27 06:38:53 <wumpus> Luke-Jr: you're the first to report *worse* IDB behavior for 0.10?
221 2015-01-27 06:39:00 <wumpus> Luke-Jr: are you sure it's not due to your local changes?
222 2015-01-27 06:39:16 <Luke-Jr> wumpus: not worse - just not improved
223 2015-01-27 06:39:48 <Luke-Jr> wumpus: no, I can't be sure - though I don't know any that could impact it
224 2015-01-27 06:40:17 <wumpus> it seems strange; parallel block download makes it lots faster for most other users that tried it
225 2015-01-27 06:40:26 <wumpus> so unless there's stomething specific to your network setup
226 2015-01-27 06:41:03 <wumpus> warren: if it's related, I'd say push to the same PR
227 2015-01-27 06:48:09 <Luke-Jr> wumpus: I think txTo can remain const..
228 2015-01-27 06:48:31 <wumpus> Luke-Jr: good point
229 2015-01-27 06:50:02 <warren> updated #5718
230 2015-01-27 06:50:07 <warren> with screenshots
231 2015-01-27 06:51:15 <jrick> another version which avoids virtual funcs because I got bored http://sprunge.us/eMKV
232 2015-01-27 07:00:22 <wumpus> warren: thanks
233 2015-01-27 07:06:26 <warren> wumpus: question if this behavior is intended for 0.10 or not ... say you want to send a 1114 byte transaction. Is 0.10 supposed to calculate a fee based on 1114 bytes or 2000 bytes? It matters because 0.9 won't relay if the fee is based on 1114 bytes.
234 2015-01-27 07:07:19 <wumpus> warren: I'm not quite up to date on the rounding behavior
235 2015-01-27 07:08:16 <wumpus> but I'd say if 0.9 doesn't relay it otherwise, the rounding is necessary
236 2015-01-27 07:09:06 <warren> looking deeper
237 2015-01-27 07:12:16 <warren> if I'm understanding this right ... even 0.10 won't relay it
238 2015-01-27 07:16:09 <wumpus> at least CFeeRate::GetFee doesn't do any rounding
239 2015-01-27 07:16:18 <wumpus> and that's used to determine the minimum relay fee
240 2015-01-27 07:20:28 <warren> brb food
241 2015-01-27 08:15:49 <robbak> I see that openssl has moved on to 1.0.1l : is this still causing problems?
242 2015-01-27 08:18:29 <wumpus> only if you use bitcoin core <0.9.4 or <rc3 and dynamically link to openssl
243 2015-01-27 08:19:28 <robbak> Ah, I see 0.9.4 is out. I have never found an announce mailing list that lets me know about this!
244 2015-01-27 08:21:00 <wumpus> it's only an intermediate release for e.g. distros
245 2015-01-27 08:21:48 <wumpus> has no advantages compared to 0.9.3 when you're running the executables from bitcoin.org, so I don't think there was ever a real announcement
246 2015-01-27 08:23:48 <wumpus> I had expected 0.10.0 to be out sooner though
247 2015-01-27 08:29:10 <wumpus> so normally, version announcements go to bitcoin-dev as soon as gititan executables are built (as there is effectively no difference to 0.9.3, they were never built for 0.9.4)
248 2015-01-27 08:39:26 <jonasschnelli> needs GUI tag: https://github.com/bitcoin/bitcoin/pull/5718
249 2015-01-27 08:39:46 <jonasschnelli> same here: https://github.com/bitcoin/bitcoin/issues/5716
250 2015-01-27 08:43:05 <warren> wumpus: transactions from 0.10 can fill the "Continuously rate-limit free transactions" area or be rejected by 0.9
251 2015-01-27 09:05:58 <wumpus> warren: due to the rounding change?
252 2015-01-27 09:06:04 <wumpus> jonasschnelli: thanks
253 2015-01-27 09:07:11 <warren> wumpus: yes
254 2015-01-27 09:08:01 <warren> in practice though if people always use estimatefee 25 when sending they'll be fine with 0.9 relays
255 2015-01-27 09:10:44 <warren> mm... I'm actually confused how 0.9 doesn't outright reject fees below its nMinRelayTxFee
256 2015-01-27 09:34:35 <warren> wumpus: I think #5718 appropriate for 0.10 as the fee values are effectively lying without it.
257 2015-01-27 09:45:43 <jonasschnelli> would it make sense to move the miner also into the new wallet module=
258 2015-01-27 09:45:47 <jonasschnelli> s/=/?
259 2015-01-27 09:46:40 <warren> technically mining doesn't require a wallet, although the internal miner probably does
260 2015-01-27 09:52:06 <Luke-Jr> the internal miner should just get removed :/
261 2015-01-27 09:53:10 <jonasschnelli> Luke-Jr, i can't do all changes within the new wallet structure.. :) so i will extract wallet interaction in miner.cpp with signaling.
262 2015-01-27 09:53:20 <Luke-Jr> âº
263 2015-01-27 09:53:40 <Luke-Jr> wait, signalling what?
264 2015-01-27 09:53:54 <Luke-Jr> the only thing the internal miner interacts with the wallet, is for the generation address to use
265 2015-01-27 09:56:19 <jonasschnelli> Luke-Jr, currently the miner has the #ifdef ENABLE_WALLET wrapping. We could move the minder into the new wallet module or leave it outside and do the interaction (get key, mapRequestCount) with signaling
266 2015-01-27 09:58:07 <warren> jonasschnelli: moving it there would be even more misleading
267 2015-01-27 09:58:11 <warren> as mining does not require a wallet
268 2015-01-27 09:58:30 <jonasschnelli> warren, in general yes. But our internal miner requires a wallet
269 2015-01-27 09:58:45 <warren> the fact that the internal miner exists at all or requires a wallet is more neglect than design
270 2015-01-27 09:59:05 <Luke-Jr> jonasschnelli: you can't really get a key with signals.. that's what makes signals different from function calls
271 2015-01-27 10:00:27 <warren> ACTION sleep
272 2015-01-27 10:00:52 <jonasschnelli> Luke-Jr, yes. It would be a hack... but maybe something like walletSignals.getReserveKey(CReserveKey&)? But what if more than one wallet does listen to this signal event?!
273 2015-01-27 10:01:58 <Luke-Jr> jonasschnelli: the wallet interface is already abstracted, so I'm not sure what you're doing..
274 2015-01-27 10:03:35 <jonasschnelli> Luke-Jr, there is actually no walletInterface, there is a CValidationInterface, i leave that untouched for now and add a CWalletInterface (for wallet init, etc.)
275 2015-01-27 10:04:00 <Luke-Jr> jonasschnelli: CValidationInterface is a wallet interface; it's just usable for more than wallets..
276 2015-01-27 10:04:18 <Luke-Jr> you'll notice it was actually called CWalletInterface in older git..
277 2015-01-27 10:04:46 <jonasschnelli> Luke-Jr, hmm... yes. I saw that. Maybe i should extend this interface rather then create a new one...
278 2015-01-27 10:18:01 <jonasschnelli> Luke-Jr, hmm.. i think i add a CModuleInterface where module can register for some Init,Shutdown,RPC extending signals and keep CValidationInterface clean as possible.
279 2015-01-27 10:19:06 <Luke-Jr> Init/Shutdown can (should?) just use constructor/destructor
280 2015-01-27 10:19:14 <Luke-Jr> RPC probably could use some abstraction, though
281 2015-01-27 10:21:53 <jonasschnelli> Luke-Jr, i have to add some signals to keep the LegacyWallet as much as untouched as possible. Also some things need to be done before the module gets constructed (like config reading, data-dir stuff). But the module needs to be created then because we might add some line to the help text, etc.
282 2015-01-27 10:23:00 <Luke-Jr> jonasschnelli: I wonder if it would make sense to just implement the new wallet as a library, independent of the bitcoin repo.
283 2015-01-27 10:23:16 <Luke-Jr> rather than implementing it in the same repo and splitting it out later
284 2015-01-27 10:23:22 <jonasschnelli> Luke-Jr, yes. Could be once. But i still struggle with changing to much at once.
285 2015-01-27 10:23:30 <Luke-Jr> >_<
286 2015-01-27 10:23:53 <jonasschnelli> Luke-Jr, i start now by moving everything wallet relates to src/wallet and src/legacywallet
287 2015-01-27 10:24:13 <Luke-Jr> my point is, it may likely be easier (and cleaner) to just implement a library, then integrate it with Bitcoin Core when done
288 2015-01-27 10:24:14 <jonasschnelli> and create a module approach according to #3440
289 2015-01-27 10:25:54 <jonasschnelli> Luke-Jr, i would see the library as next step. But to not loose sight, i start with modularization. Maybe once a module (like the wallet) could be dynloaded.
290 2015-01-27 10:26:38 <Luke-Jr> the difference between a library and modularity, is like 5 lines of autotools code <.<
291 2015-01-27 10:27:17 <Luke-Jr> and starting as a library gives you a clean git history âº
292 2015-01-27 11:05:41 <wumpus> jonasschnelli: yes, a signal to request a key for mining from the wallet (if present) would make sense
293 2015-01-27 11:05:54 <wumpus> jonasschnelli: this could decouple the internal wallet from a specific wallet implementation
294 2015-01-27 11:06:04 <wumpus> s/internal wallet/internal miner/g
295 2015-01-27 11:07:07 <wumpus> for a signal you can define how the result is combined if there are multiple listeners
296 2015-01-27 11:07:36 <wumpus> e.g. pick the first, or return a list, or even return an error
297 2015-01-27 11:08:22 <wumpus> mind that the internal miner is only used for testing, so it should do the sensible thing there, but apart from that it doesn't matter too much
298 2015-01-27 11:09:27 <wumpus> if it fails with more than one wallet open, or mines into a random wallet in that case, that's acceptable
299 2015-01-27 11:39:44 <jonasschnelli> wumpus: okay. Thanks. Will do so.
300 2015-01-27 11:40:49 <jonasschnelli> wumpus, the question for me now is more, how to allow interaction between module. Some kind of dependence injection. The problem: the rpc server could also be a module. But how to add rpc commands from another module....
301 2015-01-27 11:41:06 <jonasschnelli> s/between module/between modules
302 2015-01-27 11:49:03 <Naphex> http://vimeo.com/116605178 Delve essays just keep getting better
303 2015-01-27 12:05:38 <wumpus> jonasschnelli: just make the wallet the only module for now
304 2015-01-27 12:06:34 <wumpus> jonasschnelli: let's assume there will always be at least the RPC command processor/dispatcher
305 2015-01-27 12:07:23 <wumpus> jonasschnelli: so e.g. the HTTP server could be a module at some point, or other RPC 'frontends', but it will dispatch commands via RPC, so that will be the central hub
306 2015-01-27 12:09:54 <wumpus> jonasschnelli: trying to take modularity too far brings you nowhere, assume for now that there is always a RPC server to register your commands with
307 2015-01-27 12:10:54 <wumpus> (in the worst case you'd end up with unreadable macaroni enterprise code, or a completely general program that does nothing :-)
308 2015-01-27 12:24:29 <jonasschnelli> wumpus, Yes. Thats good. Sometimes i think i already change to many things at once.
309 2015-01-27 12:35:47 <Luke-Jr> ACTION ponders if removing the internal miner would gain favour if he added a simple script to simulate the regtest setgenerate externally
310 2015-01-27 13:01:01 <op_mul> midly related, blockchain.info has added to their api the ability to search by master public key. please be sure to point out to anybody who mentions it how dangerous this is. we can't stop blockchain.info doing dumb ass shit, but we can educate people not to use that endpoint.
311 2015-01-27 13:02:09 <op_mul> MPK should be secret as well. there's no reason users should ever be pasting it into a fucking web form on a block explorer.
312 2015-01-27 13:10:29 <sipa> wumpus: 5717... that means copying the whole transaction for every signature check
313 2015-01-27 13:10:35 <jonasschnelli> can anyone explain me, why we have a LEAVE_CRITICAL_SECTION(pwalletMain->cs_wallet); and ENTER_CRITICAL_SECTION(pwalletMain->cs_wallet); in getblocktemplate?
314 2015-01-27 13:12:42 <jonasschnelli> nevermind, now i understand it
315 2015-01-27 13:33:17 <jonasschnelli> i assume the ENTER_CRITICAL_SECTION does not unlock when leaving the block? Where is the unlock counterpart of https://github.com/bitcoin/bitcoin/blob/master/src/rpcmining.cpp#L465?
316 2015-01-27 13:34:26 <sipa> jonasschnelli: above
317 2015-01-27 13:34:48 <sipa> the point is that the wallet is locked by default, but by using an explicit leave+enter, you can temporarily release it
318 2015-01-27 13:35:26 <jonasschnelli> aha... because of the threadSafe arg from the RPC dispatch table?
319 2015-01-27 13:36:00 <sipa> i assume so
320 2015-01-27 13:36:14 <jonasschnelli> sipa, thanks,...
321 2015-01-27 13:36:35 <hearn> jonasschnelli: i think the assumption is that it will let other threads make progress.
322 2015-01-27 13:36:44 <hearn> jonasschnelli: how true that is depends on a lot of things and it may not actually work on all platforms
323 2015-01-27 13:37:33 <sipa> indeed; it's just good practice to not hold locks while not using them
324 2015-01-27 13:37:52 <sipa> we violate that rule in many places though...
325 2015-01-27 13:38:30 <jonasschnelli> hearn, sipa thanks
326 2015-01-27 13:45:02 <wumpus> sipa: yeah...
327 2015-01-27 13:45:19 <sipa> wumpus: i'm coding a different solution, but you may not like it :)
328 2015-01-27 13:45:22 <wumpus> sipa: but keeping a reference in a structure that is really bad
329 2015-01-27 13:46:09 <wumpus> it should really be a pointer instead, but that change has to large impact pre-0.10
330 2015-01-27 13:46:26 <sipa> how does that help?
331 2015-01-27 13:46:36 <sipa> you can still take a pointer to a temporary object
332 2015-01-27 13:46:36 <wumpus> no automatic temporary
333 2015-01-27 13:46:44 <wumpus> yes, but you shoot yourself then
334 2015-01-27 13:46:53 <sipa> same with requiring explicit conversion
335 2015-01-27 13:47:13 <wumpus> that's much less clear
336 2015-01-27 13:47:14 <sipa> well, in this case
337 2015-01-27 13:47:16 <sipa> not generally
338 2015-01-27 13:47:21 <wumpus> really, you should *not* keep a reference in a structure
339 2015-01-27 13:47:28 <wumpus> it's dangerous
340 2015-01-27 13:47:56 <wumpus> sure, you can require explicit conversion for this type, but then someone will add a new type and forget that, and boom
341 2015-01-27 13:48:32 <sipa> it seems to me that the danger is due to passing a reference to constructor; not the fact the the sturcture stores it
342 2015-01-27 13:48:46 <wumpus> c++'s behavior here is really counter intuitive, and that's bad, and we shouldn't have that in our code
343 2015-01-27 13:48:52 <wumpus> it's a combination
344 2015-01-27 13:48:59 <wumpus> passing it is fine, if you copy it
345 2015-01-27 13:49:14 <sipa> we could require the constructor to pass a pointer, and keep the struct keep a reference to it (not a serious suggestion; just pointing out that it doesn't really change anything)
346 2015-01-27 13:49:21 <wumpus> storing a reference to a property of your own object... may be ok
347 2015-01-27 13:49:35 <wumpus> but storing a reference to something that is passed in is a nono
348 2015-01-27 13:50:16 <sipa> let me see if i can make an easy fix for that
349 2015-01-27 13:50:28 <sipa> the number of actual SignatureChecker invocations is really low
350 2015-01-27 13:50:45 <wumpus> yes, agreed, we could do that and that'd be fine too, you'd just be using the reference as a pointer... :P
351 2015-01-27 13:51:26 <sipa> well the thing it avoids is passing in a temporary
352 2015-01-27 13:51:26 <wumpus> (but we could also cast it to ptrdiff_t and store it as an integer, if you want to go there :p)
353 2015-01-27 13:51:31 <sipa> lol
354 2015-01-27 13:52:50 <wumpus> but yes, maybe another solution that doesn't need an intermediate reference to the tranaction at all makes more sense
355 2015-01-27 14:02:58 <sipa> wumpus: see 5719
356 2015-01-27 14:03:19 <sipa> it also uses a better name for the SignatureChecker implementations...
357 2015-01-27 14:05:10 <sipa> wumpus: i can make the TransactionSignatureChecker::txTo a pointer too, if that's clearer
358 2015-01-27 14:07:19 <wumpus> yes, let's do that
359 2015-01-27 14:08:23 <wumpus> LGTM apart from that
360 2015-01-27 14:08:43 <wumpus> most of the code impact seems to be in the tests, fortunately
361 2015-01-27 14:11:32 <sipa> wumpus: updated
362 2015-01-27 16:01:57 <jikoz> I am holding other peoples bitcoins on their behalf (think: like an exchange) and would like to prove I have their coins, without disclosing all the addresses that I control. Is such a scheme possible?
363 2015-01-27 16:07:05 <hearn> sipa: is there a way to see in getpeerinfo whether a node is downloading the chain from us?
364 2015-01-27 16:07:27 <sipa> it will have 'inflight' entries
365 2015-01-27 16:07:30 <sipa> ah, from
366 2015-01-27 16:07:32 <sipa> hmm, no
367 2015-01-27 16:07:40 <sipa> there's no state associated with that
368 2015-01-27 16:07:54 <sipa> except you will see rapidly increasind upload_bytes i guess
369 2015-01-27 16:08:20 <hearn> yeah
370 2015-01-27 16:08:25 <hearn> that's a good suggestion
371 2015-01-27 16:09:17 <hearn> yes looks like a peer/some peers are pulling the chain from one of my nodes and that's why it's so slow to do bloom filtering
372 2015-01-27 16:10:08 <hearn> it would be good if filtered traffic (latency/user sensitive) had priority over serving full blocks (user expects to wait hours no matter what)
373 2015-01-27 16:11:16 <hearn> (well this is just a guess)
374 2015-01-27 16:11:23 <hearn> ( i have no data to finger this as being the cause )
375 2015-01-27 16:12:29 <hearn> spv mode is getting pretty painful though
376 2015-01-27 16:12:46 <hearn> at least, at starbucks once they start throttling me :)
377 2015-01-27 16:13:19 <sipa> hearn: i agree; we're mixing two pieces of a protocol with different QoS intentions
378 2015-01-27 16:13:31 <sipa> different prioritization may be useful
379 2015-01-27 16:13:43 <sipa> ... or have separate protocols for bulk download and for sync
380 2015-01-27 16:13:56 <hearn> actually seems no matter what node i try i can't get above ~10 blocks/sec. i suspect i might be just be throttled to death by starbucks. will go home and try again.
381 2015-01-27 16:14:11 <hearn> anyway there are lots of low hanging optimisations that can be done before thinking about QoS inside core
382 2015-01-27 16:14:21 <sipa> are those in separate getdata requests?
383 2015-01-27 16:15:04 <hearn> no
384 2015-01-27 16:15:18 <maaku> jikoz: yes
385 2015-01-27 16:15:27 <jikoz> makku: Oh :o
386 2015-01-27 16:15:31 <sipa> maaku: how?
387 2015-01-27 16:15:37 <hearn> i need to spend some time analyzing and optimising, that's all. and finding someone to fund it :)
388 2015-01-27 16:15:43 <hearn> s/someone/some people/
389 2015-01-27 16:15:53 <sipa> you can do selective disclosing, to be able to answer to random sampling
390 2015-01-27 16:16:04 <hearn> but today .... i need to go home, where bandwidth is plentiful, as i have other tasks today
391 2015-01-27 16:16:09 <maaku> sipa: zkp over merkle tree of balances, give branches to individuals
392 2015-01-27 16:16:28 <sipa> maaku: that still needs selective disclosing to show you actually have the coins
393 2015-01-27 16:17:02 <sipa> unless you mean using a SNARK over the UTXO set :)
394 2015-01-27 16:17:10 <maaku> i interpreted the request as not disclosing globally
395 2015-01-27 16:17:20 <jikoz> That's right, I don't want to disclose globally
396 2015-01-27 16:17:28 <sipa> jikoz: are the coins shared in your wallet?
397 2015-01-27 16:17:35 <jikoz> yes
398 2015-01-27 16:17:37 <sipa> jikoz: or do you have separate address sets for each user?
399 2015-01-27 16:17:43 <jikoz> no, they're all shared
400 2015-01-27 16:17:53 <jikoz> Most of the time, they're in cold-storage addresses
401 2015-01-27 16:18:05 <sipa> in that case, you can't do selective disclosing, i think
402 2015-01-27 16:18:15 <jikoz> As all the accounts deposit address periodically get sweeped into cold storage
403 2015-01-27 16:18:28 <jikoz> (each time, a different cold storage address)
404 2015-01-27 16:18:44 <sipa> or you need to build a mapping between each user's balance and a UTXO to 'own' it (with some UTXOs shared among different users)
405 2015-01-27 16:18:54 <sipa> and then put those mappiungs in a merkle tree and selectively reveal
406 2015-01-27 16:19:21 <sipa> but if you have large single UTXO's that would be needed to back many user's assets, that doesn't gain you much
407 2015-01-27 16:19:25 <gmaxwell> sipa: are you counting normalizes to zero as an other-operation in secp256k1_gej_add_var?
408 2015-01-27 16:19:35 <sipa> no
409 2015-01-27 16:19:48 <sipa> but i probably should, as it's more expensive than add/negate/mul_int
410 2015-01-27 16:20:03 <sipa> but with 5 different version, it's hard to make assumptions about approximate timing
411 2015-01-27 16:20:09 <jikoz> When sweeping into cold storage, it's possible we could do it in such a way that each UTXO doesn't contain more than a single persons balance
412 2015-01-27 16:20:24 <jikoz> Seems kind of complicated though
413 2015-01-27 16:21:41 <jikoz> Maybe it could be done so that every 2 weeks we ensure a 1:1 mapping of users balances and UTXOs
414 2015-01-27 16:21:59 <jikoz> And prove solvency every 2 weeks
415 2015-01-27 16:48:25 <czzarr> Hi, I'm trying to extract information from the bitcoin core leveldb. In block values (keyed by b+hash), on testnet3 the first 3 bytes are always 84c03c, but I don't see how they can represent the block version in msb 128
416 2015-01-27 16:48:36 <czzarr> anyone knows what they mean?
417 2015-01-27 17:01:29 <ruukasu> testnet's difficulty is always 1, right?
418 2015-01-27 17:03:25 <czzarr> ruukasu: not technically
419 2015-01-27 17:03:34 <wumpus> ruukasu: no, not necessarily, only after 20 minutes without block
420 2015-01-27 17:04:21 <ruukasu> ah, makes sense. I was getting something in the 6 figures for a minute or so and then suddenly it was back to 1, was scared for a second
421 2015-01-27 17:04:58 <sl01_> http://www.openwall.com/lists/oss-security/2015/01/27/9 this affect bitcoin?
422 2015-01-27 17:06:05 <ruukasu> does the miner's node decide when it's been 20 minutes and they're out of luck if they submit a block with 1 difficulty and the network disagrees that it's been 20 minutes, or does it somehow check with the network first?
423 2015-01-27 17:10:21 <ruukasu> sl01_: that bug was fixed two years ago, it's only got a cve now because people just now realized it was a vulnerability
424 2015-01-27 17:25:27 <czzarr> Hi, I'm trying to extract information from the bitcoin core leveldb. In block values (keyed by b+hash), on testnet3 the first 3 bytes are always 84c03c, but I don't see how they can represent the block version in msb 128, anyone knows what they represent?