1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
179
180
181
182
183
184
185
186
187
188
189
190
191
192
193
194
195
196
197
198
199
200
201
202
203
204
205
206
207
208
209
210
211
212
213
214
215
216
217
218
219
220
221
222
223
224
225
226
227
228
229
230
231
232
233
234
235
236
237
238
239
240
241
242
243
244
245
246
247
248
249
250
251
252
253
254
255
256
257
258
259
260
261
262
263
264
265
266
267
268
269
270
271
272
273
274
275
276
277
278
279
280
281
282
283
284
285
286
287
288
289
290
291
292
293
294
295
296
297
298
299
300
301
302
303
304
305
306
307
308
|
Received: from sog-mx-3.v43.ch3.sourceforge.com ([172.29.43.193]
helo=mx.sourceforge.net)
by sfs-ml-2.v29.ch3.sourceforge.com with esmtp (Exim 4.76)
(envelope-from <btcdrak@gmail.com>) id 1Y0X5b-0000P0-0n
for bitcoin-development@lists.sourceforge.net;
Mon, 15 Dec 2014 14:57:35 +0000
Received-SPF: pass (sog-mx-3.v43.ch3.sourceforge.com: domain of gmail.com
designates 209.85.212.176 as permitted sender)
client-ip=209.85.212.176; envelope-from=btcdrak@gmail.com;
helo=mail-wi0-f176.google.com;
Received: from mail-wi0-f176.google.com ([209.85.212.176])
by sog-mx-3.v43.ch3.sourceforge.com with esmtps (TLSv1:RC4-SHA:128)
(Exim 4.76) id 1Y0X5Z-0001Za-Dz
for bitcoin-development@lists.sourceforge.net;
Mon, 15 Dec 2014 14:57:34 +0000
Received: by mail-wi0-f176.google.com with SMTP id ex7so9127337wid.9
for <bitcoin-development@lists.sourceforge.net>;
Mon, 15 Dec 2014 06:57:27 -0800 (PST)
X-Received: by 10.180.88.33 with SMTP id bd1mr31900989wib.10.1418655447268;
Mon, 15 Dec 2014 06:57:27 -0800 (PST)
MIME-Version: 1.0
Received: by 10.194.25.130 with HTTP; Mon, 15 Dec 2014 06:57:07 -0800 (PST)
In-Reply-To: <20141215124730.GA8321@savin.petertodd.org>
References: <20141215124730.GA8321@savin.petertodd.org>
From: Btc Drak <btcdrak@gmail.com>
Date: Mon, 15 Dec 2014 14:57:07 +0000
Message-ID: <CADJgMztRdNWyogPCjv64qpUHcvGDiOZDVAkv5Ra8hn25Z9xa8w@mail.gmail.com>
To: Peter Todd <pete@petertodd.org>
Content-Type: multipart/alternative; boundary=f46d0444e8fdfe44dc050a427551
X-Spam-Score: 1.0 (+)
X-Spam-Report: Spam Filtering performed by mx.sourceforge.net.
See http://spamassassin.org/tag/ for more details.
1.0 HK_RANDOM_FROM From username looks random
-1.5 SPF_CHECK_PASS SPF reports sender host as permitted sender for
sender-domain
0.6 HK_RANDOM_ENVFROM Envelope sender username looks random
0.0 FREEMAIL_FROM Sender email is commonly abused enduser mail provider
(btcdrak[at]gmail.com)
-0.0 SPF_PASS SPF: sender matches SPF record
1.0 HTML_MESSAGE BODY: HTML included in message
-0.1 DKIM_VALID_AU Message has a valid DKIM or DK signature from
author's domain
0.1 DKIM_SIGNED Message has a DKIM or DK signature,
not necessarily valid
-0.1 DKIM_VALID Message has at least one valid DKIM or DK signature
X-Headers-End: 1Y0X5Z-0001Za-Dz
Cc: Bitcoin Dev <bitcoin-development@lists.sourceforge.net>
Subject: Re: [Bitcoin-development] Recent EvalScript() changes mean
CHECKLOCKTIMEVERIFY can't be merged
X-BeenThere: bitcoin-development@lists.sourceforge.net
X-Mailman-Version: 2.1.9
Precedence: list
List-Id: <bitcoin-development.lists.sourceforge.net>
List-Unsubscribe: <https://lists.sourceforge.net/lists/listinfo/bitcoin-development>,
<mailto:bitcoin-development-request@lists.sourceforge.net?subject=unsubscribe>
List-Archive: <http://sourceforge.net/mailarchive/forum.php?forum_name=bitcoin-development>
List-Post: <mailto:bitcoin-development@lists.sourceforge.net>
List-Help: <mailto:bitcoin-development-request@lists.sourceforge.net?subject=help>
List-Subscribe: <https://lists.sourceforge.net/lists/listinfo/bitcoin-development>,
<mailto:bitcoin-development-request@lists.sourceforge.net?subject=subscribe>
X-List-Received-Date: Mon, 15 Dec 2014 14:57:35 -0000
--f46d0444e8fdfe44dc050a427551
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
This is a pretty good example about refactoring discipline as well as
premature/over optimisation.
We all want to see more modular code, but the first steps should just be to
relocate blocks of code so everything is more logically organised in
smaller files (especially for consensus critical code). Refactoring should
come in a second wave preferably after a stable release. Refactoring should
be in the pure sense, optimising code with absolutely no change in
behaviour.
When it comes to actual API changes, I think we need to be a lot more
careful and should be considered feature requests and get a lot more
scrutiny as we are essentially breaking backwards compatibility. #4890 was
pretty much merged with no discussion or thought yet other really simple
and uncontroversial PRs remain unmerged for months. A key question in the
case of EvalScript() would have been, "why are we passing txTo and nIn
here, and are there any future use cases that might require them? Why
should this be removed from the API and the entire method signature
changed?". BC breaks always need strong justification.
So I've expressed my concern a few times about the speed and frequency of
refactoring and also the way it's being done. I am not alone, as others not
directly connected with the Bitcoin Core project have also expressed
concerns about the number of refactorings "for the sake of refactoring",
especially of consensus critical code. Careful as we may be, we know from
history that small edge case bugs can creep in very easily and cause a lot
of unforeseen problems.
BtcDrak
On Mon, Dec 15, 2014 at 12:47 PM, Peter Todd <pete@petertodd.org> wrote:
>
> BtcDrak was working on rebasing my CHECKLOCKTIMEVERIFY=C2=B9 patch to mas=
ter a
> few
> days ago and found a fairly large design change that makes merging it
> currently
> impossible. Pull-req #4890=C2=B2, specifically commit c7829ea7, changed t=
he
> EvalScript() function to take an abstract SignatureChecker object,
> removing the
> txTo and nIn arguments that used to contain the transaction the script wa=
s
> in
> and the txin # respectively. CHECKLOCKTIMEVERIFY needs txTo to obtain the
> nLockTime field of the transaction, and it needs nIn to obtain the
> nSequence of
> the txin.
>
> We need to fix this if CHECKLOCKTIMEVERIFY is to be merged.
>
> Secondly, that this change was made, and the manner in which is was made,
> is I
> think indicative of a development process that has been taking significan=
t
> risks with regard to refactoring the consensus critical codebase. I know =
I
> personally have had a hard time keeping up with the very large volume of
> code
> being moved and changed for the v0.10 release, and I know BtcDrak - who i=
s
> keeping Viacoin up to date with v0.10 - has also had a hard time giving t=
he
> changes reasonable review. The #4890 pull-req in question had no ACKs at
> all,
> and only two untested utACKS, which I find worrying for something that ma=
de
> significant consensus critical code changes.
>
> While it would be nice to have a library encapsulating the consensus code=
,
> this
> shouldn't come at the cost of safety, especially when the actual users of
> that
> library or their needs is still uncertain. This is after all a
> multi-billion
> project where a simple fork will cost miners alone tens of thousands of
> dollars
> an hour; easily much more if it results in users being defrauded. That's
> also
> not taking into account the significant negative PR impact and loss of
> trust. I
> personally would recommend *not* upgrading to v0.10 due to these issues.
>
> A much safer approach would be to keep the code changes required for a
> consensus library to only simple movements of code for this release, acce=
pt
> that the interface to that library won't be ideal, and wait until we have
> feedback from multiple opensource projects with publicly evaluatable code
> on
> where to go next with the API.
>
> 1) https://github.com/bitcoin/bips/blob/master/bip-0065.mediawiki
> 2) https://github.com/bitcoin/bitcoin/pull/4890
>
> --
> 'peter'[:-1]@petertodd.org
> 00000000000000001b18a596ecadd07c0e49620fb71b16f9e41131df9fc52fa6
>
>
> -------------------------------------------------------------------------=
-----
> Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT Server
> from Actuate! Instantly Supercharge Your Business Reports and Dashboards
> with Interactivity, Sharing, Native Excel Exports, App Integration & more
> Get technology previously reserved for billion-dollar corporations, FREE
>
> http://pubads.g.doubleclick.net/gampad/clk?id=3D164703151&iu=3D/4140/ostg=
.clktrk
> _______________________________________________
> Bitcoin-development mailing list
> Bitcoin-development@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/bitcoin-development
>
>
--f46d0444e8fdfe44dc050a427551
Content-Type: text/html; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
<div dir=3D"ltr">This is a pretty good example about refactoring discipline=
as well as premature/over optimisation.<div><br></div><div>We all want to =
see more modular code, but the first steps should just be to relocate block=
s of code so everything is more logically organised in smaller files (espec=
ially for consensus critical code). Refactoring should come in a second wav=
e preferably after a stable release. Refactoring should be in the pure sens=
e, optimising code with absolutely no change in behaviour.</div><div><br></=
div><div>When it comes to actual API changes, I think we need to be a lot m=
ore careful and should be considered feature requests and get a lot more sc=
rutiny as we are essentially breaking backwards compatibility. #4890 was pr=
etty much merged with no discussion or thought yet other really simple and =
uncontroversial PRs remain unmerged for months. A key question in the case =
of EvalScript() would have been, "why are we passing txTo and nIn here=
, and are there any future use cases that might require them? Why should th=
is be removed from the API and the entire method signature changed?". =
BC breaks always need strong justification.<div><br></div><div>So I've =
expressed my concern a few times about the speed and frequency of refactori=
ng and also the way it's being done. I am not alone, as others not dire=
ctly connected with the Bitcoin Core project have also expressed concerns a=
bout the number of refactorings "for the sake of refactoring", es=
pecially of consensus critical code. Careful as we may be, we know from his=
tory that small edge case bugs can creep in very easily and cause a lot of =
unforeseen problems.<br></div><div><div><br></div><div>BtcDrak<br></div></d=
iv></div><div><br></div></div><div class=3D"gmail_extra"><br><div class=3D"=
gmail_quote">On Mon, Dec 15, 2014 at 12:47 PM, Peter Todd <span dir=3D"ltr"=
><<a href=3D"mailto:pete@petertodd.org" target=3D"_blank">pete@petertodd=
.org</a>></span> wrote:<blockquote class=3D"gmail_quote" style=3D"margin=
:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">BtcDrak was workin=
g on rebasing my CHECKLOCKTIMEVERIFY=C2=B9 patch to master a few<br>
days ago and found a fairly large design change that makes merging it curre=
ntly<br>
impossible. Pull-req #4890=C2=B2, specifically commit c7829ea7, changed the=
<br>
EvalScript() function to take an abstract SignatureChecker object, removing=
the<br>
txTo and nIn arguments that used to contain the transaction the script was =
in<br>
and the txin # respectively. CHECKLOCKTIMEVERIFY needs txTo to obtain the<b=
r>
nLockTime field of the transaction, and it needs nIn to obtain the nSequenc=
e of<br>
the txin.<br>
<br>
We need to fix this if CHECKLOCKTIMEVERIFY is to be merged.<br>
<br>
Secondly, that this change was made, and the manner in which is was made, i=
s I<br>
think indicative of a development process that has been taking significant<=
br>
risks with regard to refactoring the consensus critical codebase. I know I<=
br>
personally have had a hard time keeping up with the very large volume of co=
de<br>
being moved and changed for the v0.10 release, and I know BtcDrak - who is<=
br>
keeping Viacoin up to date with v0.10 - has also had a hard time giving the=
<br>
changes reasonable review. The #4890 pull-req in question had no ACKs at al=
l,<br>
and only two untested utACKS, which I find worrying for something that made=
<br>
significant consensus critical code changes.<br>
<br>
While it would be nice to have a library encapsulating the consensus code, =
this<br>
shouldn't come at the cost of safety, especially when the actual users =
of that<br>
library or their needs is still uncertain. This is after all a multi-billio=
n<br>
project where a simple fork will cost miners alone tens of thousands of dol=
lars<br>
an hour; easily much more if it results in users being defrauded. That'=
s also<br>
not taking into account the significant negative PR impact and loss of trus=
t. I<br>
personally would recommend *not* upgrading to v0.10 due to these issues.<br=
>
<br>
A much safer approach would be to keep the code changes required for a<br>
consensus library to only simple movements of code for this release, accept=
<br>
that the interface to that library won't be ideal, and wait until we ha=
ve<br>
feedback from multiple opensource projects with publicly evaluatable code o=
n<br>
where to go next with the API.<br>
<br>
1) <a href=3D"https://github.com/bitcoin/bips/blob/master/bip-0065.mediawik=
i" target=3D"_blank">https://github.com/bitcoin/bips/blob/master/bip-0065.m=
ediawiki</a><br>
2) <a href=3D"https://github.com/bitcoin/bitcoin/pull/4890" target=3D"_blan=
k">https://github.com/bitcoin/bitcoin/pull/4890</a><br>
<span class=3D"HOEnZb"><font color=3D"#888888"><br>
--<br>
'peter'[:-1]@<a href=3D"http://petertodd.org" target=3D"_blank">pet=
ertodd.org</a><br>
00000000000000001b18a596ecadd07c0e49620fb71b16f9e41131df9fc52fa6<br>
</font></span><br>---------------------------------------------------------=
---------------------<br>
Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT Server<br>
from Actuate! Instantly Supercharge Your Business Reports and Dashboards<br=
>
with Interactivity, Sharing, Native Excel Exports, App Integration & mo=
re<br>
Get technology previously reserved for billion-dollar corporations, FREE<br=
>
<a href=3D"http://pubads.g.doubleclick.net/gampad/clk?id=3D164703151&iu=
=3D/4140/ostg.clktrk" target=3D"_blank">http://pubads.g.doubleclick.net/gam=
pad/clk?id=3D164703151&iu=3D/4140/ostg.clktrk</a><br>__________________=
_____________________________<br>
Bitcoin-development mailing list<br>
<a href=3D"mailto:Bitcoin-development@lists.sourceforge.net">Bitcoin-develo=
pment@lists.sourceforge.net</a><br>
<a href=3D"https://lists.sourceforge.net/lists/listinfo/bitcoin-development=
" target=3D"_blank">https://lists.sourceforge.net/lists/listinfo/bitcoin-de=
velopment</a><br>
<br></blockquote></div></div>
--f46d0444e8fdfe44dc050a427551--
|
